FM is turned off when enabling on airplane mode both in Gecko and Gaia.

RESOLVED FIXED in 1.3 C2/1.4 S2(17jan)

Status

Firefox OS
Gaia::System
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: pzhang, Assigned: pzhang)

Tracking

unspecified
1.3 C2/1.4 S2(17jan)
ARM
Gonk (Firefox OS)
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [FT:System-Platform])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
+++ This bug was initially created as a clone of Bug #814918 +++

Here is what we did in Gaia (Bug #814918):
  https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/airplane_mode.js#L174
and this is what we did in Gecko (Bug #870676):
  http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/FMRadioService.cpp#701

we should only keep one of them.

:Tim, what do you think?
(Assignee)

Updated

4 years ago
Flags: needinfo?(timdream)
Priority: P1 → --
I am not sure "ril.radio.disabled" is going away or not. If so it's probably better to do everything in Gaia.

If that is not going away, it would make more sense to remove the Gaia part for now.

BTW, I don't see any code that monitor the change of the settings value in Gecko, will FM radio actually be turned off (by Gecko along) when the user toggle the setting?
Blocks: 908549
Flags: needinfo?(timdream) → needinfo?(htsai)
Whiteboard: [FT:System-Platform]
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #1)
> I am not sure "ril.radio.disabled" is going away or not. If so it's probably
> better to do everything in Gaia.
> 
we don't trigger airplane mode via mozSettings anymore, but use MobileConnection.setRadioEnabled() instead. (see bug 928851)
So it's necessary to remove the logic of listening "ril.radio.disabled" in Gecko.
(Assignee)

Comment 3

4 years ago
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #1)
> I am not sure "ril.radio.disabled" is going away or not. If so it's probably
> better to do everything in Gaia.
> 
> If that is not going away, it would make more sense to remove the Gaia part
> for now.
> 
> BTW, I don't see any code that monitor the change of the settings value in
> Gecko, will FM radio actually be turned off (by Gecko along) when the user
> toggle the setting?

The code to monitor settings' change is:
  http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/FMRadioService.cpp#96
(Assignee)

Comment 4

4 years ago
(In reply to Evelyn Hung [:evelyn] from comment #2)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #1)
> > I am not sure "ril.radio.disabled" is going away or not. If so it's probably
> > better to do everything in Gaia.
> > 
> we don't trigger airplane mode via mozSettings anymore, but use
> MobileConnection.setRadioEnabled() instead. (see bug 928851)
> So it's necessary to remove the logic of listening "ril.radio.disabled" in
> Gecko.

I see, I will remove it from Gecko.

I searched 'ril.radio.disabled' in Gaia, there are several apps that monitored this setting, like costcontrol and fm (bug 876597), how about them?
(Assignee)

Comment 5

4 years ago
(In reply to Pin Zhang [:pzhang] from comment #3)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #1)
> > I am not sure "ril.radio.disabled" is going away or not. If so it's probably
> > better to do everything in Gaia.
> > 
> > If that is not going away, it would make more sense to remove the Gaia part
> > for now.
> > 
> > BTW, I don't see any code that monitor the change of the settings value in
> > Gecko, will FM radio actually be turned off (by Gecko along) when the user
> > toggle the setting?
> 
> The code to monitor settings' change is:
>  
> http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/FMRadioService.
> cpp#96

And this is the test:
 http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/test/marionette/test_bug876597.js
(Assignee)

Comment 6

4 years ago
(In reply to Evelyn Hung [:evelyn] from comment #2)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #1)
> > I am not sure "ril.radio.disabled" is going away or not. If so it's probably
> > better to do everything in Gaia.
> > 
> we don't trigger airplane mode via mozSettings anymore, but use
> MobileConnection.setRadioEnabled() instead. (see bug 928851)
> So it's necessary to remove the logic of listening "ril.radio.disabled" in
> Gecko.

Hi Evelyn, how can I know the airplane node is disabled or not now?
Assignee: nobody → pzhang
Thanks for Evelyn's response.
Flags: needinfo?(htsai)

Comment 8

4 years ago
(In reply to Pin Zhang [:pzhang] from comment #6)
> Hi Evelyn, how can I know the airplane node is disabled or not now?

I think FMRadio in Gecko didn't need to handle airplan mode and that should be implemented from gaia side to control normal FMRadio Web APIs.

Comment 9

4 years ago
(In reply to Evelyn Hung [:evelyn] from comment #2)
> we don't trigger airplane mode via mozSettings anymore, but use
> MobileConnection.setRadioEnabled() instead. (see bug 928851)
> So it's necessary to remove the logic of listening "ril.radio.disabled" in
> Gecko.

Hi Evelyn,

Please help to make sure that the mechanism for airplan mode can be workable for devices without RIL.
Thanks.
(Assignee)

Comment 10

4 years ago
(In reply to Marco Chen [:mchen] from comment #8)
> (In reply to Pin Zhang [:pzhang] from comment #6)
> > Hi Evelyn, how can I know the airplane node is disabled or not now?
> 
> I think FMRadio in Gecko didn't need to handle airplan mode and that should
> be implemented from gaia side to control normal FMRadio Web APIs.

I'm kinda confused, if user enabled the airplane mode in settings app, and then opened an installed-not-built-in FM radio app, how the Gaia handle the scenario if Gecko don't handle it?

Comment 11

4 years ago
(In reply to Pin Zhang [:pzhang] from comment #10)
> I'm kinda confused, if user enabled the airplane mode in settings app, and
> then opened an installed-not-built-in FM radio app, how the Gaia handle the
> scenario if Gecko don't handle it?

From my point of view, Gaia just follow the description of Web API to control the FMRadio instance. It should not take care such kind of things.

The Gecko should handle/protect or allow stuff of multiple clients using single HW source, and currently it seems we allowed multiple clients to control single HW simultaneously.
(Assignee)

Comment 12

4 years ago
(In reply to Marco Chen [:mchen] from comment #11)
> (In reply to Pin Zhang [:pzhang] from comment #10)
> > I'm kinda confused, if user enabled the airplane mode in settings app, and
> > then opened an installed-not-built-in FM radio app, how the Gaia handle the
> > scenario if Gecko don't handle it?
> 
> From my point of view, Gaia just follow the description of Web API to
> control the FMRadio instance. It should not take care such kind of things.
> 
> The Gecko should handle/protect or allow stuff of multiple clients using
> single HW source, and currently it seems we allowed multiple clients to
> control single HW simultaneously.

Even if we only allow one client one time, I think we still have the problem. What i still don't understand is, if we support airplane mode, and don't allow FM radio to be enabled by design, then how can we block the request of enabling the FM radio from a third party FM radio APP in Gaia?
(Assignee)

Comment 13

4 years ago
Created attachment 8335187 [details] [diff] [review]
bug-938015-fix
Attachment #8335187 - Flags: review?(khuey)
(Assignee)

Comment 14

4 years ago
(In reply to Pin Zhang [:pzhang] from comment #13)
> Created attachment 8335187 [details] [diff] [review]
> bug-938015-fix

According to the patch for bug 928851, we still use the setting key 'ril.airplane.disabled' to remember the status of airplane mode, so I kept the airplane mode checking codes in the FM radio enabling logic.
Comment on attachment 8335187 [details] [diff] [review]
bug-938015-fix

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

Sorry for the delay here, I've been swamped since Thanksgiving.

r=me
Attachment #8335187 - Flags: review?(khuey) → review+
(Assignee)

Comment 16

4 years ago
Created attachment 8357548 [details] [diff] [review]
0001-Bug-938015-do-not-turn-off-FM-radio-when-enabling-on.patch

Hi Kyle, the old patch can't be applied on the latest code, I rebased it and removed useless nullptr checking in FMRadioService::Disable, please help review it again, thanks!
Attachment #8335187 - Attachment is obsolete: true
Attachment #8357548 - Flags: review?(khuey)
Attachment #8357548 - Flags: review?(khuey) → review+
(Assignee)

Comment 17

4 years ago
Hi, Kyle, thanks!
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/e60f0de566ec
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e60f0de566ec
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
You need to log in before you can comment on or make changes to this bug.