Closed Bug 958963 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.3 C3/1.4 S3(31jan)
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: vasanth, Assigned: mchen)

References

()

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

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
blocking-b2g: --- → 1.3?
I've seen this bug as well. Probably a recent regression.
blocking-b2g: 1.3? → 1.3+
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.
Flags: needinfo?(pzhang)
QA Contact: jzimbrick
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?
Flags: needinfo?(vasanth)
Vasanth, does this reproduce on the QRD 7x27a w/ v1.3 Gecko/Gaia?
(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.
(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.
Since JZimbrick can't reproduce, clear the needinfo request here.
Flags: needinfo?(pzhang)
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
Flags: needinfo?(vasanth)
Marco, we're still tracking this one for CS -- can we make progress now based on comment #8?
Flags: needinfo?(mchen)
Investigate it by myself first.
Assignee: nobody → mchen
Flags: needinfo?(mchen)
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.
Flags: needinfo?(vasanth)
(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.
Flags: needinfo?(mchen)
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
Flags: needinfo?(vasanth)
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.
Flags: needinfo?(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.
(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.
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.
Attachment #8361535 - Attachment is obsolete: true
Attachment #8362419 - Flags: review?(dhylands)
Hi Dave Hylands & Marco, Could you please provide an update here? This needs to be closed soon
Flags: needinfo?(mchen)
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.
Attachment #8362419 - Flags: review?(dhylands) → review+
(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.
I can confirm that attachment 8362419 [details] [diff] [review] resolves the bug over here.  Thanks, and please land in v1.3 ASAP
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.
Attached patch Check-In_PatchSplinter Review
Carry reviewer's name.
Attachment #8365148 - Flags: review+
(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.
https://hg.mozilla.org/mozilla-central/rev/785c3b0d53b7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Flags: in-moztrap?
Flags: in-moztrap? → in-moztrap+
You need to log in before you can comment on or make changes to this bug.