Closed Bug 862672 Opened 10 years ago Closed 9 years ago

[FMRadio] The FM radio is not immediately disabled when the headset is removed, if the headset is removed while the radio is enabling

Categories

(Firefox OS Graveyard :: Gaia::FMRadio, defect, P1)

ARM
Gonk (Firefox OS)

Tracking

(blocking-b2g:hd+, b2g18 wontfix, b2g-v1.1hd fixed, b2g-v1.2 unaffected)

RESOLVED FIXED
blocking-b2g hd+
Tracking Status
b2g18 --- wontfix
b2g-v1.1hd --- fixed
b2g-v1.2 --- unaffected

People

(Reporter: leo.bugzilla.gecko, Assigned: pzhang)

References

Details

(Whiteboard: c=)

Attachments

(4 files, 5 obsolete files)

[Precondition]
 Eject the headset - FM Radio is disable nomally - insert the headset - FM Radio is enabling
[symbtoms]
 Before condition icon change stop to start, eject the headset, FM Radio keep enabled state.

FM Radio can`t be disabled during enabling state.
I checked mozFMRadio.disable() is called in fm.js (function onAntennaChange()). 
But It didn`t call FMRadio::Disable() in FMRadio.cpp. 

If you can`t reproduce this case, I will inform leo member gone to San Francisco on a business trip.
blocking-b2g: --- → leo?
I sat with the Leo folks today, and they showed me that a symptom of this bug is that if you load the FM app and slowly insert and remove the headphones so they make partial contact with the headphone jack, sound will sometimes play out of the phone's speaker.

To elaborate on the penultimate paragraph of comment 0, it sounds like they looked at the code and noticed that we're getting an antennachange event and calling mozFMRadio.disable(), but because that event occurs /while/ the radio is enabling, it doesn't do anything (and in fact FMRadio::Disable() is never called).

It sounds like Gecko is not doing the right thing here at all.  If the antenna is pulled out while the radio is enabling, we should cancel the enabling, or at least mute the radio.

The Leo folks told me they would like to get this bug resolved by the end of the week.  I can review changes here, but I don't have cycles to write code.  It would be helpful to get a leo? triage determination here; I don't know how that works.

Pin, if you don't have time to work on this, perhaps slee can help.
Flags: needinfo?(akeybl)
Flags: needinfo?(slee)
Summary: [FMRadio] Can`t disabled FMRadio when eject the headset, during enabling FMRadio. → [FMRadio] The FM radio is not immediately disabled when the headset is removed, if the headset is removed while the radio is enabling
Pin - how risky/difficult do you think a change here would be?
Flags: needinfo?(akeybl)
If we need to solve the problem in Gecko, gaia should always call FMRadio::Disable(). IIRC, Gecko does not register listener to headset event. I am not sure if it would cause inconsistent UI status.
Bug 853742 is trying to add "enabling" state. Maybe we can use that state. When mozFMRadio.disable() finds that it's "enabling" state, just set the state to "disabling". Then onenabled callback goes back, it does the disable job. And we should also prevent users to press the start/stop button during these status.
Flags: needinfo?(slee) → needinfo?(pzhang)
> If we need to solve the problem in Gecko, gaia should always call FMRadio::Disable().

I don't understand what you're suggesting.

We need to solve this problem in Gecko, regardless of what Gaia calls.  What Gaia does should not matter for this bug.  If the headphones are pulled out while the FM radio is enabling, that should cause the enable() to fail, even if Gaia does not call fmRadio.disable().

> IIRC, Gecko does not register listener to headset event. I am not sure if it would cause 
> inconsistent UI status.

The enable() call returns a DOM request, right?  We can return failure from the DOM request if the headset is pulled out while we're enabling.  I don't see how this will mess up the UI, unless the UI does not listen for failure on the enable() call, in which case the UI will get messed up under other circumstances as well.

> Bug 853742 is trying to add "enabling" state.

This state lives in Gaia, right?  Gecko already has an "enabling" state.  I don't understand how bug 853742 matters here.
(In reply to Justin Lebar [:jlebar] from comment #5)
> > If we need to solve the problem in Gecko, gaia should always call FMRadio::Disable().
> 
> I don't understand what you're suggesting.
> 
> We need to solve this problem in Gecko, regardless of what Gaia calls.  What
> Gaia does should not matter for this bug.  If the headphones are pulled out
> while the FM radio is enabling, that should cause the enable() to fail, even
> if Gaia does not call fmRadio.disable().
> 
Yes, we need to solve this problem in Gecko.

I checked the code in hal, it does nothing when calling the `EnableFMRadio` while the `runTavaruaRadio` thread is running, so I will figure out is it possible to cancel the enabling thread, if not, then we should have a workaround in DOMFMRadioParent.jsm. 

> > IIRC, Gecko does not register listener to headset event. I am not sure if it would cause 
> > inconsistent UI status.
> 
> The enable() call returns a DOM request, right?  We can return failure from
> the DOM request if the headset is pulled out while we're enabling.  I don't
> see how this will mess up the UI, unless the UI does not listen for failure
> on the enable() call, in which case the UI will get messed up under other
> circumstances as well.
> 
We also need to change the code in Gaia for this bug.
Assignee: nobody → pzhang
Flags: needinfo?(pzhang)
(In reply to Justin Lebar [:jlebar] from comment #5)
> We need to solve this problem in Gecko, regardless of what Gaia calls.  What
> Gaia does should not matter for this bug.  If the headphones are pulled out
> while the FM radio is enabling, that should cause the enable() to fail, even
> if Gaia does not call fmRadio.disable().
Sorry, I didn't notice the code in FMRadio.cpp. I just read the code and found that we should fix the problem in Gecko.
(In reply to Pin Zhang [:pzhang] from comment #6)
> I checked the code in hal, it does nothing when calling the `EnableFMRadio`
> while the `runTavaruaRadio` thread is running, so I will figure out is it
> possible to cancel the enabling thread, if not, then we should have a
> workaround in DOMFMRadioParent.jsm. 
There is no way to cancel the the enabling thread now. If you want to do that, you need an additional status indicating the state is "enabling".
I and Steven talked, and we think that it may need to introduce a new state, i.e. enabling, to the FMRadio API, but that would be a long cycle, it's impossible to be finished in this week, so we need some workaround in Gecko without changing the API.

And here is a solution we have:https://bugzilla.mozilla.org/show_bug.cgi?id=862672
 Introduce a internal `disabling` state in Gecko, and any calls will fail when it's disabling.

So let's consider such a scene:
  1) Plug in the headphone, and open FM radio
     FM app does: call mozFMRadio.enable
  2) Unplug the headphone quickly, say .5s after step 1
     FM app does: call mozFMRadio.disable
  3) Plug in the headphone quickly, say .5s after step 2
     FM app does: call mozFMRadio.enable
  4) Unplug the headphone quickly again, say .5s after step 3
     FM app does: call mozFMRadio.disable

Then what happens in Gecko respectively:
  Step 1: Call EnableFMRadio in gonk, and `enabling` is set to TRUE
  Step 2: Because the FM radio is enabing, mute the audio, and set `disabling` to TRUE, fail the DOMRequest of step 1, and wait for gonk callback
  Step 3: Because the FM radio is disabling/enabling, the enable call fails.
  Step 4: Because the FM radio is disabling, the disable call fails.
  Finally: After the FM radio is enabled because of step 1, we call DisableFMRadio in gonk, and then call success of the DOMRequest for step 2.

Justin, any comments?
Attached patch WIP Patch (obsolete) — Splinter Review
Only test on Unagi, I have no Leo.
Attachment #738946 - Flags: feedback?(justin.lebar+bug)
Comment on attachment 738946 [details] [diff] [review]
WIP Patch

Leo folks, can you please test this patch out?
Attachment #738946 - Flags: feedback?(leo.bugzilla.gecko)
Attached patch Path V1 (obsolete) — Splinter Review
Attachment #739420 - Flags: review?(justin.lebar+bug)
Attached patch Patch V1 (obsolete) — Splinter Review
Attachment #739420 - Attachment is obsolete: true
Attachment #739420 - Flags: review?(justin.lebar+bug)
Attachment #739421 - Flags: review?(justin.lebar+bug)
blocking-b2g: leo? → leo+
Sorry, too quick to leo+ - Pin can you answer Alex's question in comment 3 re: risk?
blocking-b2g: leo+ → leo?
Flags: needinfo?(pzhang)
(In reply to lsblakk@mozilla.com from comment #14)
> Sorry, too quick to leo+ - Pin can you answer Alex's question in comment 3
> re: risk?

The only change of the behaviors of the WebFM API is `mozFMRadio.enable` may fail, so the FM App might be affected, however I have provided a patch for bug 853742 which has been landed (not v1.0.1 yet, see the James' comment 61).

I tried to run the API tests on Gaia for this patch, but I found that the test-agent is broken on device[1], I have asked Gaia team colleagues (Tim & Yuren) for help.

I will write some new tests for this bug, and if the patch passed all of the tests, then I would say the risk is really low.

[1] https://github.com/mozilla-b2g/gaia/issues/9296
Flags: needinfo?(pzhang)
Depends on: 864246
I'm under the gun for landing a blocker this week, so unfortunately I won't be able to do this review in a timely fashion.  I'm sorry!
Going ahead with blocking then, assuming Pin will do the testing in comment 15 before uplifting.
blocking-b2g: leo? → leo+
Apply  Patch V1 and can solve this bug, but plug-in out headset twice and can found FM app doesn't auto resume side effect, user can still re-enable it. BTW, this patch LGTM.
(In reply to Randy Lin [:rlin] from comment #18)
> Apply  Patch V1 and can solve this bug, but plug-in out headset twice and
> can found FM app doesn't auto resume side effect, user can still re-enable
> it. BTW, this patch LGTM.

Did you update your Gaia? I wrote a patch for bug 853742 to solve the problem.
gaia ver up to 4/24 /master, also can found your patch.
This problem isn't easy to reproduce, My STR is plug-in out headset around 1sec twice.
Attached file Logs for comment 20
(In reply to Randy Lin [:rlin] from comment #20)
> gaia ver up to 4/24 /master, also can found your patch.
> This problem isn't easy to reproduce, My STR is plug-in out headset around
> 1sec twice.

I plugged and unplugged the headphone back and forth, finally reproduce it ...

I added some logs and tried it again, here is the logs when I reproduced it again.

I found that unplugged events are fired more than once between which there is no plugged event, what I expected should be firing plug/unplug events alternately, so I think that's the reason.
(In reply to Pin Zhang [:pzhang] from comment #21)
> Created attachment 741715 [details]
> Logs for comment 20
> 
> (In reply to Randy Lin [:rlin] from comment #20)
> > gaia ver up to 4/24 /master, also can found your patch.
> > This problem isn't easy to reproduce, My STR is plug-in out headset around
> > 1sec twice.
> 
> I plugged and unplugged the headphone back and forth, finally reproduce it
> ...
> 
> I added some logs and tried it again, here is the logs when I reproduced it
> again.
> 
> I found that unplugged events are fired more than once between which there
> is no plugged event, what I expected should be firing plug/unplug events
> alternately, so I think that's the reason.

I filed bug 865601 to track this issue.
No longer depends on: 864246
Attached patch Part II: marionette tests (obsolete) — Splinter Review
I migrated the unit test[1] from Gaia, and wrote one for this bug.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/fm/test/unit/api_test.js
Attachment #741883 - Flags: review?(justin.lebar+bug)
Attachment #738946 - Attachment is obsolete: true
Attachment #738946 - Flags: feedback?(leo.bugzilla.gecko)
Attachment #738946 - Flags: feedback?(justin.lebar+bug)
Comment on attachment 739421 [details] [diff] [review]
Patch V1

Hi, Leo.  We twisted some arms here to get you guys a patch quickly, since you identified this as an issue that was important to you.  It would be great if you could now test this patch in a similarly timely fashion so we can verify that it fixes the problem for you.

Thanks!
Attachment #739421 - Flags: feedback?(leo.bugzilla.gecko)
Flags: needinfo?(leo.bugzilla.gecko)
> Hi, Leo.  We twisted some arms here to get you guys a patch quickly, since
> you identified this as an issue that was important to you.  It would be
> great if you could now test this patch in a similarly timely fashion so we
> can verify that it fixes the problem for you.

Hi, jlebar
I already tested Attachment 739421 [details] [diff] a week ago and that`s good to resolve the issue. 
We also need to test Comment 18 with bug 853742. Like Comment 20`s opinion, it is not easy to reproduce and fix the problem. We will follow your opinion. 
Thanks.
Flags: needinfo?(leo.bugzilla.gecko)
Attachment #739421 - Flags: feedback?(leo.bugzilla.gecko)
Comment on attachment 741883 [details] [diff] [review]
Part II: marionette tests

Sorry it took me so long to get to this, Patrick.

Thanks a /lot/ for writing tests.  I have some general questions about them,
which may just be a result of my ignorance about how marionette works.

1. Should we be checking that the antenna is available?  These tests seem to
assume that it is.  The a*team people tell me on IRC that Marionette works on
devices as well as the emulator, so I don't think we can assume the antenna is
always available.

2. The tests seem to have non-trivial race conditions when run in a
multi-process situation.

For example, test_bug862672.js does

  fm.enable();
  fm.disable();

and then checks that the enable failed.  I don't think that this is a valid
check in a multi-process situation.

Suppose we're running the test code in a child process.  Suppose the child runs
fm.enable() and sends the enable request up to the parent.  Then the child gets
swapped off the CPU and the parent runs.  At this point, the parent might run
the whole fm-enabling cycle and then send the fm-successfully-enabled message
down to the child.

Now, fm.enable().onsuccess will not /run/ until after the disable() call goes
through.  But I don't think it's correct to test that fm.enable().onsuccess
is never called, because that depends on scheduling.

I think most of the other tests here have this same problem.

Do you know if these tests are multi-process?  I assume we at least want them
to be compatible with multi-process, instead of testing behavior that we can
count on only in the single-process case.

If we can, we definitely should test the behavior of the API under the
circumstances described in comment 0.  That probably requires a test that lets
us simulate plugging/unplugging the antenna.  Ideally, we'd be able to test
whether the device plays any sound, but I don't know if we have that
capability.
Comment on attachment 741883 [details] [diff] [review]
Part II: marionette tests

I'm going to clear this r? for now, but we can set it again if we decide we want to take this patch after all.
Attachment #741883 - Flags: review?(justin.lebar+bug)
Comment on attachment 739421 [details] [diff] [review]
Patch V1

This patch looks good to me, although it's hard to reason about because of the
way we have our logic spread out between JS and C++.

Are you interested in working to rewrite this code in C++ sometime soon, like
we've been musing about for a while?  There's a bit of a learning curve
associated with our IPC layer, but baku and I can help.

>   _enableFMRadio: function(msg) {
>     let frequencyInKHz = this._roundFrequency(msg.data * 1000);
> 
>-    // If the FM radio is already enabled or it is currently being enabled
>-    // or the given frequency is out of range, return false.
>-    if (this._isEnabled || this._enabling || !frequencyInKHz) {
>+    // If the FM radio is already enabled or it is currently being
>+    // enabled/disabled or the given frequency is out of range, return false.
>+    if (this._isEnabled || this._disabling ||
>+        this._enabling || !frequencyInKHz) {
>       this._sendMessage("DOMFMRadio:enable:Return", false, null, msg);
>       return;
>     }
> 
>     this._enabling = true;
>+    // Cache the enable request, just in case, disable is requested
>+    // when the FM Radio HW is being enabled.

         Cache the enable request just in case disable() is called while the FM
         radio HW is being enabled.

         [No commas around "just in case", s/when/while/.]

>+    this._enablingMsg = msg;
>     let self = this;
> 
>     FMRadio.addEventListener("enabled", function on_enabled() {
>-      debug("FM Radio is enabled!");
>       self._enabling = false;
>+      self._enablingMsg = null;
> 
>       FMRadio.removeEventListener("enabled", on_enabled);
> 
>+      // If it's disabling, invoke the success callback of disable request.

           s/it's/we're/

But also, I don't think the comment matches what we do.  "If we're disabling,
go disable the radio now." would be a better description, as I understand this
code.

>+      if (self._disabling) {
>+        self._doDisableFMRadio(self._disablingMsg);
>+        return;
>+      }

>+  _disableFMRadio: function(msg) {
>+    // If the FM radio is disabling or is disabled without enabling, return false.

         If the FM radio is disabling or is disabled and not enabling, return false.

>+    if (this._disabling || (!this._enabling && !this._isEnabled)) {
>+      this._sendMessage("DOMFMRadio:disable:Return", false, null, msg);
>+      return;
>+    }
>+
>+    // If it's currently enabling, hold the message until FMRadio is enabled.

         If the radio is currently enabling, we send an enable-failed message
         immediately.  When the radio finishes enabling, we'll send the
         disable-succeeded message.

>+    if (this._enabling) {
>+      debug("FM Radio is enabling, mute the audio!");
>+      debug('Set _disabling to true 222');

        You probably don't need both of these messages, or the "222".
Attachment #739421 - Flags: review?(justin.lebar+bug) → review+
I was in PTO last week, sorry for late reply.

(In reply to Justin Lebar [:jlebar] from comment #26)
> Comment on attachment 741883 [details] [diff] [review]
> Part II: marionette tests
> 
> Sorry it took me so long to get to this, Patrick.
> 
> Thanks a /lot/ for writing tests.  I have some general questions about them,
> which may just be a result of my ignorance about how marionette works.
> 
> 1. Should we be checking that the antenna is available?  These tests seem to
> assume that it is.  The a*team people tell me on IRC that Marionette works on
> devices as well as the emulator, so I don't think we can assume the antenna
> is
> always available.
> 
So I think we need loop Marionette team in for further discussion.

> 2. The tests seem to have non-trivial race conditions when run in a
> multi-process situation.
> 
> For example, test_bug862672.js does
> 
>   fm.enable();
>   fm.disable();
> 
> and then checks that the enable failed.  I don't think that this is a valid
> check in a multi-process situation.
> 
> Suppose we're running the test code in a child process.  Suppose the child
> runs
> fm.enable() and sends the enable request up to the parent.  Then the child
> gets
> swapped off the CPU and the parent runs.  At this point, the parent might run
> the whole fm-enabling cycle and then send the fm-successfully-enabled message
> down to the child.
> 
> Now, fm.enable().onsuccess will not /run/ until after the disable() call goes
> through.  But I don't think it's correct to test that fm.enable().onsuccess
> is never called, because that depends on scheduling.
> 
Agree

> I think most of the other tests here have this same problem.
> 
> Do you know if these tests are multi-process?  I assume we at least want them
> to be compatible with multi-process, instead of testing behavior that we can
> count on only in the single-process case.
> 
No, I don't know.

> If we can, we definitely should test the behavior of the API under the
> circumstances described in comment 0.  That probably requires a test that
> lets
> us simulate plugging/unplugging the antenna.  Ideally, we'd be able to test
> whether the device plays any sound, but I don't know if we have that
> capability.
(In reply to Justin Lebar [:jlebar] from comment #28)
> Comment on attachment 739421 [details] [diff] [review]
> Patch V1
> 
> This patch looks good to me, although it's hard to reason about because of
> the
> way we have our logic spread out between JS and C++.
> 
> Are you interested in working to rewrite this code in C++ sometime soon, like
> we've been musing about for a while?  There's a bit of a learning curve
> associated with our IPC layer, but baku and I can help.
> 
I want to have a try, but I need to check the schedule with my manager, so I need to know what the plan is.
> So I think we need loop Marionette team in for further discussion.

Sounds good.
Flags: needinfo?(mdas)
Attached patch Patch V2 (obsolete) — Splinter Review
Fixed the nits mentioned in Comment 28.
Attachment #739421 - Attachment is obsolete: true
Attachment #745754 - Flags: review?(justin.lebar+bug)
I gave you r+ on the last patch, so you don't have to ask for another review.  I'm happy to look at the patch again if you'd like, particularly if there is something in particular you'd like me to check.  Just let me know.
(In reply to Justin Lebar [:jlebar] from comment #33)
> I gave you r+ on the last patch, so you don't have to ask for another
> review.  I'm happy to look at the patch again if you'd like, particularly if
> there is something in particular you'd like me to check.  Just let me know.

OK, it's just nits fixing, then is it OK to mark it with r+ by myself?
> then is it OK to mark it with r+ by myself?

Yes.  I'm not sure you even need to do this when requesting checkin-needed, but it seems that most people do.
Attachment #745754 - Flags: review?(justin.lebar+bug)
(In reply to Pin Zhang [:pzhang] from comment #29)
> I was in PTO last week, sorry for late reply.
> 
> (In reply to Justin Lebar [:jlebar] from comment #26)
> > Comment on attachment 741883 [details] [diff] [review]
> > Part II: marionette tests
> > 
> > Sorry it took me so long to get to this, Patrick.
> > 
> > Thanks a /lot/ for writing tests.  I have some general questions about them,
> > which may just be a result of my ignorance about how marionette works.
> > 
> > 1. Should we be checking that the antenna is available?  These tests seem to
> > assume that it is.  The a*team people tell me on IRC that Marionette works on
> > devices as well as the emulator, so I don't think we can assume the antenna
> > is
> > always available.
> > 
> So I think we need loop Marionette team in for further discussion.


Marionette runs on devices (phones/pandaboards), emulators and against desktop b2g. Since it runs in all these environments, you can't assume you have access to the antenna/have a working radio system. 

The way the webQA team gets tests like these running is by adding a manifest flag that indicates if a test requires the antenna (antenna = false for the default section, then set antenna = true for the test that needs it), and when running the tests, pass --type=<other flags>-antenna will deactivate any antenna tests. To enable this flag, you have to set the default value in testing/marionette/client/marionette/tests/unit-tests.ini, and set antenna = true in the default section of /dom/fm/test/marionette/manifest.ini. We'll have to add the appropriate 'antenna' flag to the mozharness scripts that run the marionette tests in tbpl.

Alternatively, you can check if you have a working radio layer in your test and just end the test if you do not. The problem with this approach is that if there's a bug in how you're finding out if there's a radio layer or not, it may return that there isn't one when there is one, and will skip the test when it shouldn't.

Now, I'm not sure of any other ways, I'd like to get jgriffin's input before we go ahead with adding manifest flags.
Flags: needinfo?(mdas) → needinfo?(jgriffin)
Another question: Are these tests run in a multiprocess fashion?
These tests are not currently run in a multiprocess fashion.  Fixing that is on our to-do list, but not at the top of it.

I think using the manifest flag approach is a better solution than adding logic to the tests, since that would require a lot of duplicated code.  As Malini mentioned, it will require a change to the buildbot script that is used to run the tests, but that's an easy thing to do.
Flags: needinfo?(jgriffin)
Can we simulate plugging/unplugging the antenna?
(In reply to Pin Zhang [:pzhang] from comment #39)
> Can we simulate plugging/unplugging the antenna?

I'm not aware of a way of doing this.
(In reply to Pin Zhang [:pzhang] from comment #30)
> (In reply to Justin Lebar [:jlebar] from comment #28)
> > Comment on attachment 739421 [details] [diff] [review]
> > Patch V1
> > 
> > This patch looks good to me, although it's hard to reason about because of
> > the
> > way we have our logic spread out between JS and C++.
> > 
> > Are you interested in working to rewrite this code in C++ sometime soon, like
> > we've been musing about for a while?  There's a bit of a learning curve
> > associated with our IPC layer, but baku and I can help.
> > 
> I want to have a try, but I need to check the schedule with my manager, so I
> need to know what the plan is.
 
@Justin, I think I can take it, either partime or fulltime, it depends on the target date, so what is the deadline?
(In reply to Malini Das [:mdas] from comment #40)
> (In reply to Pin Zhang [:pzhang] from comment #39)
> > Can we simulate plugging/unplugging the antenna?
> 
> I'm not aware of a way of doing this.

Then I can't think of a better way to test this bug rather than what I did in test_bug862672.js.
> it depends on the target date, so what is the deadline?

There's no deadline!

I think it's important work because it should simplify our code, it should make the API more correct (particularly, but not exclusively, in the case when the radio is accessed by multiple processes), and it will teach us about how feasible bug 864943 is on a larger scale.  But we of course have to weigh this work against the other work you can do.
(In reply to Justin Lebar [:jlebar] from comment #43)
> > it depends on the target date, so what is the deadline?
> 
> There's no deadline!
> 
> I think it's important work because it should simplify our code, it should
> make the API more correct (particularly, but not exclusively, in the case
> when the radio is accessed by multiple processes), and it will teach us
> about how feasible bug 864943 is on a larger scale.  But we of course have
> to weigh this work against the other work you can do.

OK, I will start the work next week, it may take me several weeks to learn
IPC/WebIDL and rewrite all the codes in c++, anyway, I will submit a WIP patch
for review as soon as I can.
(In reply to Pin Zhang [:pzhang] from comment #44)
> (In reply to Justin Lebar [:jlebar] from comment #43)
> > > it depends on the target date, so what is the deadline?
> > 
> > There's no deadline!
> > 
> > I think it's important work because it should simplify our code, it should
> > make the API more correct (particularly, but not exclusively, in the case
> > when the radio is accessed by multiple processes), and it will teach us
> > about how feasible bug 864943 is on a larger scale.  But we of course have
> > to weigh this work against the other work you can do.
> 
> OK, I will start the work next week, it may take me several weeks to learn
> IPC/WebIDL and rewrite all the codes in c++, anyway, I will submit a WIP
> patch
> for review as soon as I can.

Filed Bug 870676 to track the task
I volunteer baku to help with the webidl and ipc; he's on Copenhagen time (I think?), so that may be easier than getting hold of me or bent during your working hours.  Although of course we're happy to help as well.
Before we go rewriting this whole thing (or in parallel with that work), it would be nice to have tests, so we'll feel somewhat confident that the new implementation doesn't break things.  It seems like we've almost figured out how to write tests here, but we're not quite there.

>> Can we simulate plugging/unplugging the antenna?
>
> I'm not aware of a way of doing this.

What would we have to do in order to make this work, Malini?  Presumably there's some driver for the FM radio in the emulator; do we just need to figure out how to talk to this?
(In reply to Justin Lebar [:jlebar] from comment #47)
> Before we go rewriting this whole thing (or in parallel with that work), it
> would be nice to have tests, so we'll feel somewhat confident that the new
> implementation doesn't break things.  It seems like we've almost figured out
> how to write tests here, but we're not quite there.
> 
> >> Can we simulate plugging/unplugging the antenna?
> >
> > I'm not aware of a way of doing this.
> 
> What would we have to do in order to make this work, Malini?  Presumably
> there's some driver for the FM radio in the emulator; do we just need to
> figure out how to talk to this?

The way marionette changes device state, like battery state, is by using already provided methods for this in the emulator console(http://developer.android.com/tools/devices/emulator.html#console). Since the fm radio seems like it's outside of the built-in support we get for free with the android emulator, I'm not entirely sure how we can do this, so we'll need a RIL contact for either of these suggestions:

1) There is this: http://developer.android.com/tools/devices/emulator.html#events, which allows you to trigger hardware events, so if there is some hardware event for shutting down and turning on the radio, then we can send these events using 'runEmulatorCmd' calls in your marionette test.

2) Failing this, the emulator would need to have some way to emulate a radio for which there seems to be support: http://developer.android.com/tools/help/emulator.html under System there's a -radio option for the radio modem device. I'm not sure what 'device' you'll need for that, I think you'll need to loop in one of the RIL developers for more information. Once you have the device, they'll also have to figure out how to emulate disabling/enabling it. If they can loop in that service with the emulator console, then you can use 'runEmulatorCmd', if not, we'll have to figure out a way to talk to the service that controls the state of the radio device.
I'd add a feedback contact, but I'm not sure who's on the RIL team anymore, and #b2g is quiet during my current work hours :)
Aha, apparently it's not so quiet.

Slee, do you have any insight for Comment #48?
Flags: needinfo?(slee)
Another option would be to write a fake back-end that gets substituted for GonkFMRadio.cpp.

That's of course not optimal because we'd like to test the Gonk code, too.  But the majority of code and bugs have lived outside this file.
I thin if we need to test FM radio on emulator, we should implement by ourselves. 

But I think it's not easy to test FM radio on emulator. As I know the FM radio behaviors are different between uangi and galaxy SII.

If we need to implement a fake FM radio on emulator, I think Vicamo can help. He has done much work of BT on emulator.
Flags: needinfo?(slee) → needinfo?(vyang)
I've filed bug 872417 for FM radio emulation.  Let's discuss implementation details there.
Flags: needinfo?(vyang)
This has taken a turn that looks like a lot more work (and risk) than we could take in v1.1 and is better suited to v1.2 or beyond unless a partner brings this back to nomination/blocking.
blocking-b2g: leo+ → -
Whiteboard: c=
We have an HD blocker duping to this bug here and after speaking with Pin, we'd uplift this patch here to v1.1HD to resolve bug 929940.
blocking-b2g: - → hd+
Forgot to set NI? to Pin :) just a reminder
Flags: needinfo?(pzhang)
Attached patch Patch V3Splinter Review
Rebased on the branch b2g18_v1_1_0_hd, and passed the tests.
Attachment #745754 - Attachment is obsolete: true
Flags: needinfo?(pzhang)
The original path for bug 911063 covers bug 876597 which is not fixed on v1.1, so I removed the bug 876597 test here.
Attachment #741883 - Attachment is obsolete: true
Hi Wayne, I talked to Marco, it seems that we can't run tests on TryServer for b2g18 like what we usually do on m-c, so I'm afraid we need to land the patches first and then check if it's OK on the TBPL:
   https://tbpl.mozilla.org/?tree=Mozilla-B2g18
(In reply to Pin Zhang [:pzhang] from comment #60)
> Hi Wayne, I talked to Marco, it seems that we can't run tests on TryServer
> for b2g18 like what we usually do on m-c, so I'm afraid we need to land the
> patches first and then check if it's OK on the TBPL:
>    https://tbpl.mozilla.org/?tree=Mozilla-B2g18

Can you land this to v1.1HD to do this? This bug has a HD+ flag.
Keywords: checkin-needed
Wait, so this only needs landing on v1.1hd? If it needs fixing elsewhere, I need patches on top of mozilla-central as well. Please clarify what needs landing where and then request checkin again.
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #62)
> Wait, so this only needs landing on v1.1hd? If it needs fixing elsewhere, I
> need patches on top of mozilla-central as well. Please clarify what needs
> landing where and then request checkin again.

Yes, only land on V1.1hd, thanks.
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/bfde060000be
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/2ccf62b09cad

I assume that v1.2 and v1.3 are unaffected?
Status: NEW → RESOLVED
Closed: 9 years ago
status-b2g-v1.2: --- → ?
Flags: needinfo?(pzhang)
Keywords: checkin-needed
Resolution: --- → FIXED
These tests aren't actually running. Presumably you need to edit dom/fm/Makefile.in with a TEST_DIRS += test?
I pushed this patch to actually enable the tests:
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/fa03d0230e50

And they're all timing out, so I reverted the change. Presumably you want these tests to actually run and pass?
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/2fb8b5dc1459

https://tbpl.mozilla.org/php/getParsedLog.php?id=30604804&tree=Mozilla-B2g18-v1.1.0hd
AFAICT, at a minimum, the tests from bug 911063 never landed for v1.2, so we'll need them there as well along with whatever other fixes are necessary for them to actually pass.
Trunk has the same issue with the tests being landed but not enabled. I've reopened bug 911063 for that.
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #68)
> Trunk has the same issue with the tests being landed but not enabled. I've
> reopened bug 911063 for that.

The tests only works on b2g device which actually has FM HW, we don't support FM radio simulation yet, see bug 872417
Flags: needinfo?(pzhang)
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #65)
> These tests aren't actually running. Presumably you need to edit
> dom/fm/Makefile.in with a TEST_DIRS += test?

It might because I marked them with b2g=true in manifest.ini:
  http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/test/marionette/manifest.ini#2
Does anything need to be done for v1.2? As mentioned in comment 67, at a minimum, the tests never landed there.
Flags: needinfo?(pzhang)
Has bug 870676 beed landed in v1.2? If yes, then I think the patch for bug 911063 should be landed, or please land the revised tests (attachment 830671 [details] [diff] [review]) for this bug, thanks.
Flags: needinfo?(pzhang)
Yes, bug 870676 is on v1.2. Do these revised tests need to land on m-c (v1.3) as well?
Flags: needinfo?(pzhang)
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #73)
> Yes, bug 870676 is on v1.2. Do these revised tests need to land on m-c
> (v1.3) as well?

No, because bug 911063 is already on m-c[1], and the revised tests is only for the branch without bug 870676.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/test/marionette/
Flags: needinfo?(pzhang)
Great, thanks for clarifying :)
You need to log in before you can comment on or make changes to this bug.