745078 - Get switch device states at any time

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[StevenLee[:slee]]

When registering the switch observer, there might never be a uevent notifying a state change. We need a mechanism to know the current switch state at any time.

Comment 1

[StevenLee[:slee]]

Attachment: Implementation

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 628653 [details] [diff] [review]
Implementation

Hi Steven,

I may be missing something, but where do we read the current switch state in this patch?

Imagine a b2g phone boots up with a USB cable already plugged in.  How will our code be able to determine that the USB cable is plugged in?

If I'm missing something, please let me know! :)

Comment 3

[StevenLee[:slee]]

Attachment: Implementation V2

Hi Chris, 
I do some changes in this version.
1. It will always check the state of each uevent by opening the file descriptor in the first time.
2. The checking mechanism of headset is changed. I traced the code of android. They do not use switch name. They monitor the fd, |state|, in /devices/virtual/switch/h2w/. It will be more portable and could work on both SGS2 and otoro.

Comment 4

[StevenLee[:slee]]

Attachment: Implementation V3

Sorry for that I uploaded the wrong version. This one is the right patch.

Comment 5

[Dave Hylands [:dhylands]]

Comment on attachment 643757 [details] [diff] [review]
Implementation V3

Review of attachment 643757 [details] [diff] [review]:
-----------------------------------------------------------------

I think we need a bit more functionality here. Under GB, the USB Cable was implemented as a virtual switch and you can read sys/devices/virtual/switch/usb_configuration/state to get the state. Under ICS, it was no longer implemented as a virtual switch, and you need to read: /sys/devices/virtual/android_usb/android0/state

So back when I implemented the AutoMounter, we (Chris Jones and I) decided it would be best to emulate the new path as a switch. In order to make the code work on both GB and ICS, this means we can't just read a single location.

I'd be inclined to create a SwitchHandler class, whose default behaviour does whatever is required for the virtual switch, and then provide a derived class to allow the usb stuff to call down to the base class (for GB) or to emulate (for ICS).

The handler class should have a GetState method, a CheckEvent method, and the constructor should take the virtual switch path. You would have an array of pointers to instances of handler classes, some would be instances of the base class, and some might be instances of some derived class. Then whenever SwitchEventObserver::Notify gets a new NetlinkEvent, it would just walk the table and call CheckEvent to determine if this NetlinkEvent corresponds to the switch in question. If CheckEvent returns true, then it would call GetState and dispatch the event. We could also make CheckEvent fill in the state as a parameter so that we don't have to read anything (i.e. it gets the state from the NetlinkEvent).

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 643757 [details] [diff] [review]
Implementation V3

Clearing based on dhylands feedback.

Comment 7

[StevenLee[:slee]]

Attachment: Implementation V4

Apply Dave's comments.

Comment 8

[Dave Hylands [:dhylands]]

Comment on attachment 645240 [details] [diff] [review]
Implementation V4

Review of attachment 645240 [details] [diff] [review]:
-----------------------------------------------------------------

This patch needs to be rebased against mozilla-central. As given, it doesn't apply (the comment block at the beginning of the file appears to have changed).

::: hal/gonk/GonkSwitch.cpp
@@ +28,5 @@
>  
> +class SwitchHandlerBase : public RefCounted<SwitchHandlerBase>
> +{
> +public:
> +  SwitchHandlerBase(const char* path, SwitchDevice device) : 

nit: The colon should be on the next line.
nit: The arguments should be aPath and aDevice (a = argument and this comment applies to all of the arguments throughout the rest of the patch as well)

@@ +45,5 @@
> +  void GetInitialState() { 
> +    int fd = open(mStatePath, O_RDONLY);
> +    if (fd <= 0) {
> +      return;
> +    }

Use:   ScopedClose autoClose(fd);
here, then you don't have to worry about closing the file descriptor.

@@ +48,5 @@
> +      return;
> +    }
> +    
> +    char state[16];
> +    read(fd, state, sizeof(state));

Take a look at https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/AutoMounter.cpp#98 for a proper implementation.

You need to make sure that state is null terminated, and for convenience you should strip off any trailing \n, otherwise strcmp won't work.

@@ +60,3 @@
>  };
>  
> +class HeadsetSwitchHandler : public SwitchHandlerBase

So I think that this is more complicated than it needs to be.

I was envisoning that the SwitchHandler class would be able to deal with all of the virtual switch devices.

The uevent for a headset insertion looks like:

change@/devices/virtual/switch/h2w
  ACTION=change
  DEVPATH=/devices/virtual/switch/h2w
  SUBSYSTEM=switch
  SWITCH_NAME=h2w
  SWITCH_STATE=2
  SEQNUM=2581

The uevent delivered for the USB configuration under GB looks like:

change@/devices/virtual/switch/usb_configuration
  ACTION=change
  DEVPATH=/devices/virtual/switch/usb_configuration
  SUBSYSTEM=switch
  SWITCH_NAME=usb_configuration
  SWITCH_STATE=0
  SEQNUM=5038

and the uevent delivered for the USB configuration under ICS looks like:

change@/devices/virtual/android_usb/android0
  ACTION=change
  DEVPATH=/devices/virtual/android_usb/android0
  SUBSYSTEM=android_usb
  USB_STATE=CONFIGURED
  SEQNUM=1802

The only difference between the headset and the USB-GB is that the SWITCH_NAME and DEVPATH changes.

So you should be able to create a SwitchHandler class which deals with both of these cases by doing something like the following in your InitHandlers function:

sHandler.AppendElement(new SwitchHandler("h2w", SWITCH_HEADPHONES)); (use constant rather than hardcoded string)
sHandler.AppendElement(new UsbSwitchHandler());

The UsbSwitchHandler would be derived from the SwitchHandler class and would check for the android_usb subsystem, and if it doesn't match, call the base class (whose constructor would have been passed "usb_configuration")

@@ +71,5 @@
> +  ~HeadsetSwitchHandler() { }
> +  virtual bool CheckEvent(NetlinkEvent* event) { 
> +    const char* subSystem = event->getSubsystem();
> +    
> +    if (strcmp(subSystem, "switch") == 0) {

Use early return

@@ +76,5 @@
> +      const char *devPath;
> +      const char *switchState;
> +      devPath = event->findParam("DEVPATH");
> +      switchState = event->findParam("SWITCH_STATE");
> +      if (strcmp(devPath, HEADSET_SWITCH_DEV_PATH)) {

I'd be inclined to test SWITCH_NAME rather than DEV_PATH, but either should work.

@@ +81,5 @@
> +        return false;
> +      }
> +
> +      mState = ConvertState(switchState);
> +      return true;

The CheckEvent method should be genericized and folded into the base class.

@@ +104,5 @@
> +  virtual ~UsbSwitchHandlerGB() { }
> +  virtual bool CheckEvent(NetlinkEvent* event) { 
> +    const char* subSystem = event->getSubsystem();
> +    
> +    if (strcmp(subSystem, "usb_configuration") == 0) {

Use early return

@@ +116,5 @@
> +  SwitchState ConvertState(const char* state) {
> +    if (!state) {
> +      return SWITCH_STATE_UNKNOWN;
> +    }
> +    return strcmp(state, "CONFIGURED") == 0 ? SWITCH_STATE_ON : SWITCH_STATE_OFF;

This would be incorrect (as seen from the uevents delivered).

@@ +131,5 @@
> +
> +  virtual bool CheckEvent(NetlinkEvent* event) { 
> +    const char* subSystem = event->getSubsystem();
> +    
> +    if (strcmp(subSystem, "android_usb") == 0) {

Use early return

@@ +139,5 @@
> +    return false;
> +  }
> +};
> +
> +static nsTArray<RefPtr<SwitchHandlerBase>> sHandler;

nit: You need a space between the >> so that it doesn't get confused with the >> operator.

Declaring nsTArray as a static will cause the following warning on a debug build:

WARNING: XPCOM objects created/destroyed from static ctor/dtor

See bug 773792 and/or bug 773790 for further details.

I'd be inclined to make sHandler be a member variable of the SwitchEventObserver class, and move InitHandlers into the beginning of SwitchEventObservers::Init.

If the nsTArray is a member then you won't a static constructor to worry about.

@@ +183,5 @@
>      mEventInfo[aDevice].mEnable = false;
>      mEnableNum--;
>    }
>  
>    void Notify(const NetlinkEvent& event) {

nit: The opening curly brace for a function should always be on a new line (unlike the opening curly for if/while, etc).

The same applies to class declarations

class ClassName
{

It looks like this wasn't caught in the original review, so I'll live with it either way.

@@ +255,4 @@
>    }
>  };
>  
>  SwitchEventObserver* sSwitchObserver;

nit: Not part of your patch, but this should be a RefPtr rather than a bare pointer. Then you don't need the delete. Just assign sSwitchObserver to be NULL.

Comment 9

[StevenLee[:slee]]

(In reply to Dave Hylands [:dhylands] from comment #8)
> @@ +60,3 @@
> >  };
> >  
> > +class HeadsetSwitchHandler : public SwitchHandlerBase
> 
> So I think that this is more complicated than it needs to be.
> 
> I was envisoning that the SwitchHandler class would be able to deal with all
> of the virtual switch devices.
> 
> The uevent for a headset insertion looks like:
> 
> change@/devices/virtual/switch/h2w
>   ACTION=change
>   DEVPATH=/devices/virtual/switch/h2w
>   SUBSYSTEM=switch
>   SWITCH_NAME=h2w
>   SWITCH_STATE=2
>   SEQNUM=2581
> 
> The uevent delivered for the USB configuration under GB looks like:
> 
> change@/devices/virtual/switch/usb_configuration
>   ACTION=change
>   DEVPATH=/devices/virtual/switch/usb_configuration
>   SUBSYSTEM=switch
>   SWITCH_NAME=usb_configuration
>   SWITCH_STATE=0
>   SEQNUM=5038
> 
> and the uevent delivered for the USB configuration under ICS looks like:
> 
> change@/devices/virtual/android_usb/android0
>   ACTION=change
>   DEVPATH=/devices/virtual/android_usb/android0
>   SUBSYSTEM=android_usb
>   USB_STATE=CONFIGURED
>   SEQNUM=1802
> 
> The only difference between the headset and the USB-GB is that the
> SWITCH_NAME and DEVPATH changes.
> 
> So you should be able to create a SwitchHandler class which deals with both
> of these cases by doing something like the following in your InitHandlers
> function:
> 
> sHandler.AppendElement(new SwitchHandler("h2w", SWITCH_HEADPHONES)); (use
> constant rather than hardcoded string)
> sHandler.AppendElement(new UsbSwitchHandler());
> 
> The UsbSwitchHandler would be derived from the SwitchHandler class and would
> check for the android_usb subsystem, and if it doesn't match, call the base
> class (whose constructor would have been passed "usb_configuration")
> 
But the uevent on otoro is,
  change@/devices/virtual/switch/h2w
    ACTION=change
    DEVPATH=/devices/virtual/switch/h2w
    SUBSYSTEM=switch
    SWITCH_NAME=Headset
    SWITCH_STATE=1
    SEQNUM=1602
If we unplug headset, the SWITCH_NAME will be "No Device". I also trace the code
in Android and found that they use DEVPATH to check whether the uevent is headset.

For usb under GB and headset, we could put them in the same class, but the function
itself may be complicated. 

> @@ +81,5 @@
> > +        return false;
> > +      }
> > +
> > +      mState = ConvertState(switchState);
> > +      return true;
> 
> The CheckEvent method should be genericized and folded into the base class.
I will put this function in the base class.

> @@ +139,5 @@
> > +    return false;
> > +  }
> > +};
> > +
> > +static nsTArray<RefPtr<SwitchHandlerBase>> sHandler;
> 
> nit: You need a space between the >> so that it doesn't get confused with
> the >> operator.
> 
> Declaring nsTArray as a static will cause the following warning on a debug
> build:
> 
> WARNING: XPCOM objects created/destroyed from static ctor/dtor
> 
> See bug 773792 and/or bug 773790 for further details.
> 
> I'd be inclined to make sHandler be a member variable of the
> SwitchEventObserver class, and move InitHandlers into the beginning of
> SwitchEventObservers::Init.
> 
> If the nsTArray is a member then you won't a static constructor to worry
> about.
OK~ Thanks for your suggestions.

Comment 10

[Dave Hylands [:dhylands]]

(In reply to StevenLee from comment #9)
> (In reply to Dave Hylands [:dhylands] from comment #8)
> > @@ +60,3 @@
> > >  };
> > >  
> > > +class HeadsetSwitchHandler : public SwitchHandlerBase
> > 
> > So I think that this is more complicated than it needs to be.
> > 
> > I was envisoning that the SwitchHandler class would be able to deal with all
> > of the virtual switch devices.
> > 
> > The uevent for a headset insertion looks like:
> > 
> > change@/devices/virtual/switch/h2w
> >   ACTION=change
> >   DEVPATH=/devices/virtual/switch/h2w
> >   SUBSYSTEM=switch
> >   SWITCH_NAME=h2w
> >   SWITCH_STATE=2
> >   SEQNUM=2581
...snip...
> > The UsbSwitchHandler would be derived from the SwitchHandler class and would
> > check for the android_usb subsystem, and if it doesn't match, call the base
> > class (whose constructor would have been passed "usb_configuration")
> > 
> But the uevent on otoro is,
>   change@/devices/virtual/switch/h2w
>     ACTION=change
>     DEVPATH=/devices/virtual/switch/h2w
>     SUBSYSTEM=switch
>     SWITCH_NAME=Headset
>     SWITCH_STATE=1
>     SEQNUM=1602
> If we unplug headset, the SWITCH_NAME will be "No Device". I also trace the
> code
> in Android and found that they use DEVPATH to check whether the uevent is
> headset.

That's a pretty clear argument for using DEVPATH then :) (and probably worth mentioning in the code as a comment)

> For usb under GB and headset, we could put them in the same class, but the
> function
> itself may be complicated. 

Given the information that you've presented, what I'd like to see is that the DEVPATH and device be passed as arguments to the constructor. Then the code for headset and USB GB should be identical. They'll both check that subssystem == switch, that DEVPATH matches what was passed in the constructor. and will use SWITCH_STATE to determine the value.

The class that you wrote called HeadsetHandler is almost exactly what I was looking for as the base class. The only thing its missing is that it uses a hardcoded HEADSET_SWITCH_DEV_PATH, and I'd like it to use a member variable which was initialized from the constructor.

The state file, for reading the initial state can be constructed by using:

/sys + DEVPATH + /state

So you'd construct SwitchHandler(HEADSET_SWITCH_DEV_PATH, SWITCH_HEADPHONES) to deal with the headset and if we were just using GB, then you'd construct SwitchHandler(GB_USB_SWITCH_DEV_PATH, SWITCH_USB) (where GB_USB_SWITCH_DEV_PATH is "/devices/virtual/switch/usb_configuration").

And for consistency, since we're using SWITCH_USB and SWITCH_HEADSET, can we use:
SWITCH_USB_DEVPATH_GB and SWITCH_HEADSET_DEVPATH?

For the USB-ICS case, we'd use SWITCH_USB_DEVPATH_ICS

Comment 11

[StevenLee[:slee]]

Hi Dave, 
I found that "USB_STATE" on ICS has 3 possible values, 
DISCONNECTED, CONNECTED, and CONFIGURED. If the device is connected by cannot be configured, user cannot use it. So 
could I ignore CONNECTED status?

Comment 12

[Dave Hylands [:dhylands]]

Yeah - the code in the VolumeManager treats CONFIGURED as 1 (on), and anything else as 0 (off).

Comment 13

[StevenLee[:slee]]

Attachment: Implementation V5

Comment 14

[Dave Hylands [:dhylands]]

Comment on attachment 647111 [details] [diff] [review]
Implementation V5

Review of attachment 647111 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent. 

I think that the architecture is right now. Most of my remaining comments are nits, and some of them aren't even really part of your patch, but I think that they should probably be cleaned up.

So I think you just need a bit of cleanup and you'll be good to go.

::: hal/gonk/GonkSwitch.cpp
@@ +102,5 @@
> +
> +  void GetInitialState()
> +  {
> +    char statePath[128];
> +    if (snprintf(statePath, sizeof(statePath), "/sys%s/state", mDevPath) <= 0) {

You should use an nsPrintfCString here and then you don't need to worry about the buffer size.
https://mxr.mozilla.org/mozilla-central/source/xpcom/string/public/nsPrintfCString.h

@@ +169,5 @@
> +  SwitchState ConvertState(const char* aState)
> +  {
> +    if (!aState) {
> +      MOZ_ASSERT(aState);
> +      return SWITCH_STATE_UNKNOWN;

Why the difference between SwitchHandlerUsbIcs::ConvertState and SwitchHandler::ConvertState?

Shouldn't you just MOZ_ASSERT(aState) and not bother with returning SWITCH_STATE_UNKNOWN? It's a path that should never happen. If there is a specific reason for the difference, then it should be commented.

@@ +174,2 @@
>      }
> +    return strcmp(aState, "CONFIGURED") == 0 ? SWITCH_STATE_OFF : SWITCH_STATE_ON;

This is backwards. aState equal to "CONFIGURED" should correspond to SWITCH_STATE_ON

@@ +178,5 @@
>  
>  class SwitchEventRunnable : public nsRunnable
>  {
>  public:
>    SwitchEventRunnable(SwitchEvent& event) : mEvent(event) {}

nit: Not part of your patch, but this should be called aEvent rather than event.

@@ +204,3 @@
>  
>    int GetEnableCount() {
>      return mEnableNum;

nit: Not part of your patch. This should probably be called mEnableCount or mNumEnabledSwitches.

@@ +215,5 @@
>      mEventInfo[aDevice].mEnable = false;
>      mEnableNum--;
>    }
>  
>    void Notify(const NetlinkEvent& event) {

nit: This should be called aEvent as well.

@@ +249,2 @@
>  private:
>    class EventInfo {

nit: Also not part of your path, but the open curly for a class or function should go on the next line. I noticed this on alot of the inline functions.

@@ +256,2 @@
>      SwitchEvent mEvent;
>      bool mEnable;

nit: Also not part of your patch, but I'd call this mEnabled. Then the if statements read properly:

    if (info.mEnabled)

@@ +265,5 @@
> +    mHandler.AppendElement(new SwitchHandler(SWITCH_HEADSET_DEVPATH, SWITCH_HEADPHONES));
> +    mHandler.AppendElement(new SwitchHandler(SWITCH_USB_DEVPATH_GB, SWITCH_USB));
> +    mHandler.AppendElement(new SwitchHandlerUsbIcs(SWITCH_USB_DEVPATH_ICS));
> +    
> +    for (size_t i = 0; i < mHandler.Length(); i++) {

nit: The nsTArray class has some subtypes called index_type and size_type that should be used for declaring indicies and sizes.

So if you created a typedef

typedef nsTArray<RefPtr<SwitchHandler> > SwitchHandlerArray;

Then you can do:

for (SwitchHandlerArray::index_type i = 0; ...

I also prefer to create my own variable for the number of elements rather than calling Length() each time (in this case, calling Length() doesn't incur any real penalty, but in general it's a bad idea to be calling a function to get the length for each iteration through the array).

So I tend to do something like:

SwitchHandlerArray::size_type numHandlers = mHandler.Length();
SwitchHandlerArray::index_type handlerIndex;
for (handlerIndex = 0; handlerIndex < numHandlers; handlerIndex++)

@@ +278,4 @@
>      }
>    }
>  
> +  SwitchDevice GetEventInfo(const NetlinkEvent& event, SwitchState& state) {

nit: aEvent and aState

Comment 15

[StevenLee[:slee]]

Attachment: Implementation V6

Comment 16

[Dave Hylands [:dhylands]]

Comment on attachment 647468 [details] [diff] [review]
Implementation V6

Review of attachment 647468 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I was able to hook it up to the AutoMounter and verified that USB ICS stuff works as expected.

::: hal/gonk/GonkSwitch.cpp
@@ +93,5 @@
> +
> +  SwitchState GetState()
> +  { 
> +    return mState;
> +  }

So just for future reference, it's fine for these to look like:

SwitchState GetState() { return mState; }

or the way that you've shown it.

SwitchState GetState() { 
  return mState;
}

is the form I was looking to get fixed.

You don't need to change this back, I'm just pointing it out.

Comment 17

[StevenLee[:slee]]

Hi Dave, 
Got it. 
Thanks~

Comment 19

[Dave Hylands [:dhylands]]

I've already submitted a try run:
https://tbpl.mozilla.org/?tree=Try&rev=bb701bbf46ae

I'll land it tomorrow (once the try run has completed).

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c3606b8bbe01