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

Status: RESOLVED FIXED

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

Description

[leo.bugzilla.gecko]

[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.

Comment 1

[ntiaj12345]

Attachment: Logs for FM Speaker out

Comment 2

[Justin Lebar (not reading bugmail)]

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.

Comment 3

[Alex Keybl [:akeybl]]

Pin - how risky/difficult do you think a change here would be?

Comment 4

[StevenLee[:slee]]

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.

Comment 5

[Justin Lebar (not reading bugmail)]

> 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.

Comment 6

[Pin Zhang [:pzhang] (inactive)]

(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.

Comment 7

[StevenLee[:slee]]

(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.

Comment 8

[StevenLee[:slee]]

(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".

Comment 9

[Pin Zhang [:pzhang] (inactive)]

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?

Comment 10

[Pin Zhang [:pzhang] (inactive)]

Attachment: WIP Patch

Only test on Unagi, I have no Leo.

Comment 11

[Justin Lebar (not reading bugmail)]

Comment on attachment 738946 [details] [diff] [review]
WIP Patch

Leo folks, can you please test this patch out?

Comment 12

[Pin Zhang [:pzhang] (inactive)]

Attachment: Path V1

Comment 13

[Pin Zhang [:pzhang] (inactive)]

Attachment: Patch V1

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Sorry, too quick to leo+ - Pin can you answer Alex's question in comment 3 re: risk?

Comment 15

[Pin Zhang [:pzhang] (inactive)]

(In reply to [e-mail] 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

Comment 16

[Justin Lebar (not reading bugmail)]

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!

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Going ahead with blocking then, assuming Pin will do the testing in comment 15 before uplifting.

Comment 18

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

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.

Comment 19

[Pin Zhang [:pzhang] (inactive)]

(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.

Comment 20

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

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.

Comment 21

[Pin Zhang [:pzhang] (inactive)]

Attachment: 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.

Comment 22

[Pin Zhang [:pzhang] (inactive)]

(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.

Comment 23

[Pin Zhang [:pzhang] (inactive)]

Attachment: Part II: marionette tests

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

Comment 24

[Justin Lebar (not reading bugmail)]

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!

Comment 25

[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.

Comment 26

[Justin Lebar (not reading bugmail)]

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 27

[Justin Lebar (not reading bugmail)]

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.

Comment 28

[Justin Lebar (not reading bugmail)]

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".

Comment 29

[Pin Zhang [:pzhang] (inactive)]

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.

Comment 30

[Pin Zhang [:pzhang] (inactive)]

(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.

Comment 31

[Justin Lebar (not reading bugmail)]

> So I think we need loop Marionette team in for further discussion.

Sounds good.

Comment 32

[Pin Zhang [:pzhang] (inactive)]

Attachment: Patch V2

Fixed the nits mentioned in Comment 28.

Comment 33

[Justin Lebar (not reading bugmail)]

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.

Comment 34

[Pin Zhang [:pzhang] (inactive)]

(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?

Comment 35

[Justin Lebar (not reading bugmail)]

> 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.

Comment 36

[Malini Das [:mdas] - Away, not checking bugmail]

(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.

Comment 37

[Justin Lebar (not reading bugmail)]

Another question: Are these tests run in a multiprocess fashion?

Comment 38

[Jonathan Griffin (:jgriffin)]

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.

Comment 39

[Pin Zhang [:pzhang] (inactive)]

Can we simulate plugging/unplugging the antenna?

Comment 40

[Malini Das [:mdas] - Away, not checking bugmail]

(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.

Comment 41

[Pin Zhang [:pzhang] (inactive)]

(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?

Comment 42

[Pin Zhang [:pzhang] (inactive)]

(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.

Comment 43

[Justin Lebar (not reading bugmail)]

> 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.

Comment 44

[Pin Zhang [:pzhang] (inactive)]

(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.

Comment 45

[Pin Zhang [:pzhang] (inactive)]

(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

Comment 46

[Justin Lebar (not reading bugmail)]

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.

Comment 47

[Justin Lebar (not reading bugmail)]

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?

Comment 48

[Malini Das [:mdas] - Away, not checking bugmail]

(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.

Comment 49

[Malini Das [:mdas] - Away, not checking bugmail]

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 :)

Comment 50

[Malini Das [:mdas] - Away, not checking bugmail]

Aha, apparently it's not so quiet.

Slee, do you have any insight for Comment #48?

Comment 51

[Justin Lebar (not reading bugmail)]

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.

Comment 52

[StevenLee[:slee]]

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.

Comment 53

[Vicamo Yang [:vicamo][:vyang]]

I've filed bug 872417 for FM radio emulation.  Let's discuss implementation details there.

Comment 54

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 56

[Wayne Chang [:wchang]]

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.

Comment 57

[Wayne Chang [:wchang]]

Forgot to set NI? to Pin :) just a reminder

Comment 58

[Pin Zhang [:pzhang] (inactive)]

Attachment: Patch V3

Rebased on the branch b2g18_v1_1_0_hd, and passed the tests.

Comment 59

[Pin Zhang [:pzhang] (inactive)]

Attachment: Revised marionette tests from bug 911063.

The original path for bug 911063 covers bug 876597 which is not fixed on v1.1, so I removed the bug 876597 test here.

Comment 60

[Pin Zhang [:pzhang] (inactive)]

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

Comment 61

[Wayne Chang [:wchang]]

(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.

Comment 62

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 63

[Pin Zhang [:pzhang] (inactive)]

(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.

Comment 64

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 65

[Ryan VanderMeulen [:RyanVM]]

These tests aren't actually running. Presumably you need to edit dom/fm/Makefile.in with a TEST_DIRS += test?

Comment 66

[Ryan VanderMeulen [:RyanVM]]

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

Comment 67

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 68

[Ryan VanderMeulen [:RyanVM]]

Trunk has the same issue with the tests being landed but not enabled. I've reopened bug 911063 for that.

Comment 69

[Pin Zhang [:pzhang] (inactive)]

(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

Comment 70

[Pin Zhang [:pzhang] (inactive)]

(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

Comment 71

[Ryan VanderMeulen [:RyanVM]]

Does anything need to be done for v1.2? As mentioned in comment 67, at a minimum, the tests never landed there.

Comment 72

[Pin Zhang [:pzhang] (inactive)]

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.

Comment 73

[Ryan VanderMeulen [:RyanVM]]

Yes, bug 870676 is on v1.2. Do these revised tests need to land on m-c (v1.3) as well?

Comment 74

[Pin Zhang [:pzhang] (inactive)]

(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/

Comment 75

[Ryan VanderMeulen [:RyanVM]]

Great, thanks for clarifying :)