Closed
Bug 862672
Opened 12 years ago
Closed 11 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)
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)
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
[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.
Reporter | ||
Updated•12 years ago
|
blocking-b2g: --- → leo?
Comment 1•12 years ago
|
||
Comment 2•12 years ago
|
||
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)
Updated•12 years ago
|
Flags: needinfo?(slee)
Updated•12 years ago
|
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
Comment 3•12 years ago
|
||
Pin - how risky/difficult do you think a change here would be?
Flags: needinfo?(akeybl)
Comment 4•12 years ago
|
||
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)
Comment 5•12 years ago
|
||
> 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.
Assignee | ||
Comment 6•12 years ago
|
||
(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)
Comment 7•12 years ago
|
||
(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•12 years ago
|
||
(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".
Assignee | ||
Comment 9•12 years ago
|
||
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?
Assignee | ||
Comment 10•12 years ago
|
||
Only test on Unagi, I have no Leo.
Attachment #738946 -
Flags: feedback?(justin.lebar+bug)
Comment 11•12 years ago
|
||
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)
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #739420 -
Attachment is obsolete: true
Attachment #739420 -
Flags: review?(justin.lebar+bug)
Attachment #739421 -
Flags: review?(justin.lebar+bug)
Updated•12 years ago
|
blocking-b2g: leo? → leo+
status-b2g18:
--- → affected
Comment 14•12 years ago
|
||
Sorry, too quick to leo+ - Pin can you answer Alex's question in comment 3 re: risk?
blocking-b2g: leo+ → leo?
Flags: needinfo?(pzhang)
Assignee | ||
Comment 15•12 years ago
|
||
(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)
Comment 16•12 years ago
|
||
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•12 years ago
|
||
Going ahead with blocking then, assuming Pin will do the testing in comment 15 before uplifting.
blocking-b2g: leo? → leo+
Comment 18•12 years ago
|
||
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.
Assignee | ||
Comment 19•12 years ago
|
||
(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•12 years ago
|
||
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.
Assignee | ||
Comment 21•12 years ago
|
||
(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.
Assignee | ||
Comment 22•12 years ago
|
||
(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.
Assignee | ||
Comment 23•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #738946 -
Attachment is obsolete: true
Attachment #738946 -
Flags: feedback?(leo.bugzilla.gecko)
Attachment #738946 -
Flags: feedback?(justin.lebar+bug)
Comment 24•12 years ago
|
||
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)
Updated•12 years ago
|
Flags: needinfo?(leo.bugzilla.gecko)
Reporter | ||
Comment 25•12 years ago
|
||
> 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)
Updated•12 years ago
|
Attachment #739421 -
Flags: feedback?(leo.bugzilla.gecko)
Comment 26•12 years ago
|
||
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•12 years ago
|
||
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 28•12 years ago
|
||
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+
Assignee | ||
Comment 29•12 years ago
|
||
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.
Assignee | ||
Comment 30•12 years ago
|
||
(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•12 years ago
|
||
> So I think we need loop Marionette team in for further discussion.
Sounds good.
Updated•12 years ago
|
Flags: needinfo?(mdas)
Assignee | ||
Comment 32•12 years ago
|
||
Fixed the nits mentioned in Comment 28.
Attachment #739421 -
Attachment is obsolete: true
Attachment #745754 -
Flags: review?(justin.lebar+bug)
Comment 33•12 years ago
|
||
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.
Assignee | ||
Comment 34•12 years ago
|
||
(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•12 years ago
|
||
> 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.
Updated•12 years ago
|
Attachment #745754 -
Flags: review?(justin.lebar+bug)
Comment 36•12 years ago
|
||
(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)
Comment 38•12 years ago
|
||
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)
Comment 40•12 years ago
|
||
(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.
Assignee | ||
Comment 41•12 years ago
|
||
(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?
Assignee | ||
Comment 42•12 years ago
|
||
(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•12 years ago
|
||
> 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.
Assignee | ||
Comment 44•12 years ago
|
||
(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.
Assignee | ||
Comment 45•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
Aha, apparently it's not so quiet. Slee, do you have any insight for Comment #48?
Flags: needinfo?(slee)
Comment 51•12 years ago
|
||
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•12 years ago
|
||
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)
Comment 53•12 years ago
|
||
I've filed bug 872417 for FM radio emulation. Let's discuss implementation details there.
Flags: needinfo?(vyang)
Comment 54•12 years ago
|
||
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=
Comment 56•11 years ago
|
||
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+
Assignee | ||
Comment 58•11 years ago
|
||
Rebased on the branch b2g18_v1_1_0_hd, and passed the tests.
Attachment #745754 -
Attachment is obsolete: true
Flags: needinfo?(pzhang)
Assignee | ||
Comment 59•11 years ago
|
||
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
Assignee | ||
Comment 60•11 years ago
|
||
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•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 62•11 years ago
|
||
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
Assignee | ||
Comment 63•11 years ago
|
||
(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
Comment 64•11 years ago
|
||
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: 11 years ago
status-b2g-v1.1hd:
--- → fixed
status-b2g-v1.2:
--- → ?
Flags: needinfo?(pzhang)
Keywords: checkin-needed
Resolution: --- → FIXED
Comment 65•11 years ago
|
||
These tests aren't actually running. Presumably you need to edit dom/fm/Makefile.in with a TEST_DIRS += test?
Comment 66•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
Trunk has the same issue with the tests being landed but not enabled. I've reopened bug 911063 for that.
Assignee | ||
Comment 69•11 years ago
|
||
(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)
Assignee | ||
Comment 70•11 years ago
|
||
(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•11 years ago
|
||
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)
Assignee | ||
Comment 72•11 years ago
|
||
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)
Comment 73•11 years ago
|
||
Yes, bug 870676 is on v1.2. Do these revised tests need to land on m-c (v1.3) as well?
Flags: needinfo?(pzhang)
Assignee | ||
Comment 74•11 years ago
|
||
(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)
You need to log in
before you can comment on or make changes to this bug.
Description
•