958963 - FM App does not recognize wired headset for the first time after boot up

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[vasanth]

Attachment: Status bar shows headset is connected but FM app shows it doesn't

Steps to reproduce:
===================
1. Plug in wired Headset to device
2. Reboot the device.
3. After device is powered up, go to FM app.
4. FM  does not start and there is notification on device to plug in headset in order to make FM on. Headset has to be unplugged and plugged in to make FM on.

Initial analysis:
================
Headset is detected by the device, but only FM app does not detect the headset for the first time after reboot. Please see attached screenshot

Only when there is a change in headset state, FM app is notified [1].
When headset is connected before power on and when FM app is opened after that, now since there is no change in headset state, it doesn't know headset is connected

Not sure if there is any trivial fix?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/FMRadio.cpp#192

Comment 1

[Jason Smith [:jsmith]]

I've seen this bug as well. Probably a recent regression.

Comment 2

[Marco Chen [:mchen]]

Hi Pin,

Refer to the status bar, system app already knew headset plugged in.
So could you help to check what happened inside FMRadip app?

Thanks.

Comment 3

[J Zimbrick]

I have not been able to reproduce this issue on the latest mozilla 1.3 and 1.4 builds on a Buri device with the information provided. The phone recognizes the headphones on first boot every time. 

1.3 Environmental Variables:
Device: Buri v1.3 Mozilla RIL
uildID: 20140113004002
Gaia: b3fc4f712562ee92b0ed0bd17abc61be9a36a8da
Gecko: 5bb1837de7c0
Version: 28.0a2
Base Image: V1.2_20131115

1.4 Environmental Variables :
Device: Buri v1.4 Mozilla RIL
BuildID: 20140113040202
Gaia: fd3b9a97cb3c41cfa56be387b46a51db136b4422
Gecko: 12d3ba62a599
Version: 29.0a1
Base Image: V1.2_20131115



Could this issue be device (or even headphone) specific? 
What device, build, and base image was this issue seen on?
Was the reproduction rate on this issue 100%, or lower?

Comment 4

[Michael Vines [:m1] [:evilmachines]]

Vasanth, does this reproduce on the QRD 7x27a w/ v1.3 Gecko/Gaia?

Comment 5

[vasanth]

(In reply to J Zimbrick from comment #3)
> The phone recognizes the headphones on first boot every time. 

I will test in some other device and update here. From your comment, I guess you imply not only the phone, even the FM app recognized the headphone for the first time.

Comment 6

[J Zimbrick]

(In reply to vasanth from comment #5)
> I will test in some other device and update here. From your comment, I guess
> you imply not only the phone, even the FM app recognized the headphone for
> the first time.

Sorry. Yes, to clarfiy: both the phone and the FM app recognized the headphones on first boot each time.

Comment 7

[Pin Zhang [:pzhang] (inactive)]

Since JZimbrick can't reproduce, clear the needinfo request here.

Comment 8

[vasanth]

We know there are 2 ways to detect headset insert/remove,
1. Using Uevents
2. Using dev input jack events.
This issue is only in devices with #2 so not reproducible in 7x27a

FM app uses GetCurrentSwtichState() to get initial state of headset which internally uses |SwitchEventObserver:: mEventInfo| to get status

Implementation of #1 stores the event in mEventInfo and also notifies the observers [1]
Whereas implementation of #2 just notifies the observers [2]
So in devices with #2, GetCurrentSwtichState() will always return -1 (status unknown)

BTW this issue was covered before, since FM app didn’t check for status unknown
My previous fix uncovered this [3] :-(

[1] http://dxr.mozilla.org/mozilla-central/source/hal/gonk/GonkSwitch.cpp#262
[2] http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#441
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=953159

Comment 9

[Dylan Oliver [:doliver]]

Marco, we're still tracking this one for CS -- can we make progress now based on comment #8?

Comment 10

[Marco Chen [:mchen]]

Investigate it by myself first.

Comment 11

[Marco Chen [:mchen]]

Attachment: SendHeadsetStateFromInputDevToGonkSwitch-v1

Hi Vasanth,

Could you help to verify whether this patch can fix issue here because I have no such device? If it can then will start the review flow. Thanks.

Comment 12

[vasanth]

Attachment: fm-headset-patch-crash.txt

(In reply to Marco Chen [:mchen] from comment #11)
> Could you help to verify whether this patch can fix issue here because I
> have no such device? If it can then will start the review flow. Thanks.

Marco,

It crashes everytime, when b2g process starts itself. Attaching the bt from gdb
Seems UnregisterSwitchObserver frees some observers and NotifySwitchChange is trying to access them.

Comment 13

[vasanth]

Was able to root cause this.
SwitchEventObserver::mEventInfo.mEvent.device() is initialized only when status is not unknown [1]
Status will be unknown here in devices which doesn't use Uevent method and device value is still -1 which fails to get observers in NotifySwitchChange and crashes

Just initializing the mEvent.device before that check makes it work!

Also can't we just call hal::NotifySwitchStateFromInputDevice from UpdateHeadphoneSwitch instead of using SwitchEventRunnable there?
We already use a SwitchEventRunnable in GonkSwitch Notify()

Thanks Marco for the quick turnaround and patch.

[1] http://dxr.mozilla.org/mozilla-central/source/hal/gonk/GonkSwitch.cpp#323

Comment 14

[Marco Chen [:mchen]]

Hi Vasanth,

I guess that there is no node (ex: /sys/...) to get headset status actively by b2g process.
Could you confirm this?

Then I may add code for initializing the headset status to off when device didn't use UEVENT.
And wait for any notification from input device.

Comment 15

[Marco Chen [:mchen]]

(In reply to vasanth from comment #13)
> Also can't we just call hal::NotifySwitchStateFromInputDevice from
> UpdateHeadphoneSwitch instead of using SwitchEventRunnable there?
> We already use a SwitchEventRunnable in GonkSwitch Notify()

As I know that the thread calling HAL functions can be main thread only, that's the reason input reader thread needs a runnable to main thread. And another runnable in gonkswitch is for getting status on I/O thread bug notifying on main thread.

Comment 16

[Michael Vines [:m1] [:evilmachines]]

(In reply to Marco Chen [:mchen] from comment #14)
> I guess that there is no node (ex: /sys/...) to get headset status actively
> by b2g process.
> Could you confirm this?

Yep that is the case.  There's only the input switch event.

Comment 17

[Marco Chen [:mchen]]

Attachment: SendHeadsetStateFromInputDevToGonkSwitch-v2

Hi Dave,

Could you help to review this patch?

1. The switch event notification sent by nsAppShell didn't be recorded by GonkSwitch class so HAL::GetCurrentSwitchState() can't get correct status.

2. This patch introduced a new HAL::NotifySwitchStateFromInputDevice(...) and all modules who detected switch status change should call it to update GonkSwitch for recording the current status.

Thanks.

Comment 18

[vasanth]

Hi Dave Hylands & Marco, Could you please provide an update here? This needs to be closed soon

Comment 19

[Dave Hylands [:dhylands]]

Comment on attachment 8362419 [details] [diff] [review]
SendHeadsetStateFromInputDevToGonkSwitch-v2

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

So the patch looks fine to me.

Which phones use the "input" method to determine headphone jack state?

The only thing I didn't like was adding a new property. It would be better if it could be done without having to do that, but that can be improved in a later bug.

Comment 20

[Michael Vines [:m1] [:evilmachines]]

(In reply to Dave Hylands [:dhylands] from comment #19)
> The only thing I didn't like was adding a new property. It would be better
> if it could be done without having to do that, but that can be improved in a
> later bug.

The property was added in bug 895665 actually.  It should definitely be possible to determine the jack event source at runtime though as a follow-up bug.

Comment 21

[Michael Vines [:m1] [:evilmachines]]

I can confirm that attachment 8362419 [details] [diff] [review] resolves the bug over here.  Thanks, and please land in v1.3 ASAP

Comment 22

[Marco Chen [:mchen]]

https://tbpl.mozilla.org/?tree=Try&rev=b73db5c880e8

Comment 23

[Marco Chen [:mchen]]

It seems that all b2g build on try server are failed always.
Will try to run it tomorrow or I will push to in-bound then monitor the status.

Comment 24

[Marco Chen [:mchen]]

https://tbpl.mozilla.org/?tree=Try&rev=31a00835f5bd

Comment 25

[Marco Chen [:mchen]]

Attachment: Check-In_Patch

Carry reviewer's name.

Comment 26

[Marco Chen [:mchen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/785c3b0d53b7

Comment 27

[Marco Chen [:mchen]]

(In reply to Dave Hylands [:dhylands] from comment #19)
> The only thing I didn't like was adding a new property. It would be better
> if it could be done without having to do that, but that can be improved in a
> later bug.

Fired bug 964154 as follow up to this suggestion.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/785c3b0d53b7

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/efae13114cd6