Closed Bug 911117 Opened 8 years ago Closed 8 years ago

[flatfish] Headset icon is lost on status bar if booting up with headset plugged.

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:koi+, b2g-v1.2 fixed)

RESOLVED FIXED
blocking-b2g koi+
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: viralwang, Assigned: gduan)

References

Details

(Whiteboard: [Flatfish only])

Attachments

(2 files)

Plug in/out headset will not generate any event from input system.
It's randomly set plug in/out when boot up.
Audio path looks good in both ways (I can hear the sound from either speaker or headset when playing video)
It looks like the only problem is headset detection not working now.
Jeff, please check this issue. Thanks!
Assignee: nobody → jeff.cy.chuang
Blocks: flatfish
John, please help to check driver level.
Terry, please help to study Audio patch control
Assignee: jeff.cy.chuang → johnchang1972
I found UeventPoller failed to initialize itself because bind() returns -EADDRINUSE.  UeventPoller uses netlink socket to read uevents(same as Android) without the need of filling out address or port.  A possible EADDRINUSE error in netlink socket bind() is bind 2 or more netlink sockets with the same pid.  In the manpage of netlink(7), it says the following:

There are two ways to assign  nl_pid to a netlink socket.  If the application sets nl_pid before calling bind(2), then it is up to the application to make sure that nl_pid is unique.  If the application sets it to 0, the kernel takes care of assigning it.  The kernel assigns the process ID to the first netlink socket the process opens and assigns a unique nl_pid to every netlink socket that the process  subsequently creates.

This patch fix the bind() error in UeventPoller.cpp by setting nl_pid to 0 and add error handling after bind() fails.
Flags: needinfo?(ehung)
Another bug I'm working on:
When I boot up the device with headset inserted, the audio output is no problem(from headset), but status bar does not show headset icon.  I found a similar issue of bug 796723.

After debugging, I found that AudioManager's 1st mozChromeEvent broadcast happens before statusbar.js regisgers event listener and it seems no way for statusbar.js to check initial state of headphone plug status.

Related codes are as follows,
1. statusbar.js register mozChomeEvent
   https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/statusbar.js#L179
2. statusbar.js handles headphone change event
   https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/statusbar.js#L291
3. AudioManager broadcast 1st event in constructor
   http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/AudioManager.cpp#376
> Related codes are as follows,
> 1. statusbar.js register mozChomeEvent
>   
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/statusbar.
> js#L179
> 2. statusbar.js handles headphone change event
>   
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/statusbar.
> js#L291
> 3. AudioManager broadcast 1st event in constructor
>   
> http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/AudioManager.
> cpp#376

Seems a gaia bug.
Marco told me because of some Gecko change, the first headset change Chrome event fired from Gecko is earlier than shell.js been loaded, so System app can't register it in time. He also suggests to use `mozAudioChannelManager` API to detect and listen headset status.
Music app is using the API: https://github.com/mozilla-b2g/gaia/blob/master/apps/music/js/Player.js#L26
Component: Hardware → Gaia::System
Flags: needinfo?(ehung)
Summary: [flatfish] Headset detection can not work → [flatfish] Headset icon is lost on status bar if booting up with headset plugged.
blocking-b2g: --- → koi?
I don't think it should block but please ask for approval if you think it should be fixed in 1.2
blocking-b2g: koi? → -
(In reply to Terry Li from comment #3)
> Created attachment 811949 [details] [diff] [review]
> Fix bind() error in UeventPoller initialization
> 
> I found UeventPoller failed to initialize itself because bind() returns
> -EADDRINUSE.  UeventPoller uses netlink socket to read uevents(same as
> Android) without the need of filling out address or port.  A possible
> EADDRINUSE error in netlink socket bind() is bind 2 or more netlink sockets
> with the same pid.  In the manpage of netlink(7), it says the following:
> 

As you said, I also think it might be the problem that b2g process contains more than 1 netlink sockets in different thread. Because I didn't have Flatfish on my hand, can you please help me to modify the code like the below patch in UeventPoller.cpp? This statement can make sure saddr.nl_pid got the unique value. Thanks


   bzero(&saddr, sizeof(saddr));
   saddr.nl_family = AF_NETLINK;
   saddr.nl_groups = 1;
-  saddr.nl_pid = getpid();
+  //saddr.nl_pid = getpid();
+  saddr.nl_pid = pthread_self() << 16 | getpid();
(In reply to Gregor Wagner [:gwagner] from comment #6)
> I don't think it should block but please ask for approval if you think it
> should be fixed in 1.2

Hi,

I renom to koi? because if we don't fix bug here the user impact will be

  1. The audio will not route to headset device (big impact) on flatfish project.
  2. The status bar will not show the headset icon if headset was plugged during boot up. (small impact).

Thanks.
blocking-b2g: - → koi?
It's a must for flatfish. Marked as koi+ according to triage result.
blocking-b2g: koi? → koi+
Whiteboard: [Flatfish only]
Duplicate of this bug: 922966
(In reply to Vincent Liu[:vliu] from comment #7)
> 
> As you said, I also think it might be the problem that b2g process contains
> more than 1 netlink sockets in different thread. Because I didn't have
> Flatfish on my hand, can you please help me to modify the code like the
> below patch in UeventPoller.cpp? This statement can make sure saddr.nl_pid
> got the unique value. Thanks
> 
> 
>    bzero(&saddr, sizeof(saddr));
>    saddr.nl_family = AF_NETLINK;
>    saddr.nl_groups = 1;
> -  saddr.nl_pid = getpid();
> +  //saddr.nl_pid = getpid();
> +  saddr.nl_pid = pthread_self() << 16 | getpid();
This works as you expected.
By the way, when I print out the pid, it turns out to be the process of b2g.
(In reply to Terry Li from comment #11)
> (In reply to Vincent Liu[:vliu] from comment #7)
> > 
> > As you said, I also think it might be the problem that b2g process contains
> > more than 1 netlink sockets in different thread. Because I didn't have
> > Flatfish on my hand, can you please help me to modify the code like the
> > below patch in UeventPoller.cpp? This statement can make sure saddr.nl_pid
> > got the unique value. Thanks
> > 
> > 
> >    bzero(&saddr, sizeof(saddr));
> >    saddr.nl_family = AF_NETLINK;
> >    saddr.nl_groups = 1;
> > -  saddr.nl_pid = getpid();
> > +  //saddr.nl_pid = getpid();
> > +  saddr.nl_pid = pthread_self() << 16 | getpid();
> This works as you expected.
> By the way, when I print out the pid, it turns out to be the process of b2g.

NetlinkPoller::OpenSocket() runs in b2g process so it should be the expected value. From the result of Comment 11, it is better using |saddr.nl_pid = pthread_self() << 16 | getpid()| instead of |saddr.nl_pid = getpid()| to make sure the uniqueness of nl_pid. I will file another bug for checking.
(In reply to Vincent Liu[:vliu] from comment #12)
> NetlinkPoller::OpenSocket() runs in b2g process so it should be the expected
> value. From the result of Comment 11, it is better using |saddr.nl_pid =
> pthread_self() << 16 | getpid()| instead of |saddr.nl_pid = getpid()| to
> make sure the uniqueness of nl_pid. I will file another bug for checking.

nl_pid identifies a netlink socket, not a process(or thread). Using this way may have the risk to fail if 2 or more netlink sockets exist in the same thread. Using 0 to have kernel handle it, which is probably safer for both cases. What do you think?

Following is the related statement about nl_pid in netlink(7):

"If the application sets it to 0, the kernel takes care of assigning it.  The kernel assigns the process ID to the first netlink socket the process opens and assigns a unique nl_pid to every netlink socket that the process subsequently creates."
Depends on: 924729
Attached file PR to master
As stated in comment 4 and comment 5,
//AudioManager's 1st mozChromeEvent broadcast happens before statusbar.js regisgers event listener//
this patch will use navigator.mozAudioChannelManager.headphones to show/hide the headphone icon when firstly initialized.
Attachment #817668 - Flags: review?(alive)
Attachment #817668 - Flags: review?(alive) → review+
Thanks Alive.
Merge into master,
https://github.com/mozilla-b2g/gaia/commit/3d4f1107e6e91e5f5649edc0f2565ac837111d7d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Uplifted 3d4f1107e6e91e5f5649edc0f2565ac837111d7d to:
v1.2: d7d8b406b33ae0dbccf8fe03990a7053575b9ed5
Blocks: 934881
Assignee: johnchang1972 → gduan.moz
Assignee: gduan.moz → gduan
You need to log in before you can comment on or make changes to this bug.