[Gaia][DSDS][Gaia::Settings] stop using the settings key 'ril.radio.disabled' to turn off RIL radio

RESOLVED FIXED in 1.3 Sprint 4 - 11/8

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: hsinyi, Assigned: arthurcc)

Tracking

unspecified
1.3 Sprint 4 - 11/8
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+)

Details

(Whiteboard: [FT:RIL])

Attachments

(1 attachment)

We've been through several problems with using settings key to control hardware. In Bug 856553, we are going to have a new API for radio control and deprecate the settings key 'ril.radio.disabled.' 

This bug is for handling backward compatibility and new API detection in gaia so that the landing of bug 856553 won't break gaia.
Assignee: nobody → arthur.chen
Hi Hsin-Yi,

Does Gecko stabilize API and have a quick use case like https://bugzilla.mozilla.org/show_bug.cgi?id=927724 ? 

If yes, can you add some use cases here ? If no, you can just comment back later when finished.

Thanks for your great help :D
Flags: needinfo?(htsai)
The basic picture looks promising though we are still refining the API in bug 856553. I'll keep you updated once the new API is review granted. Thank you.

However, the main concept is:
afterwards, gaia would need to call
  var req = navigator.mozMobileConnection.setRadioPower(true);
to turn on radio.

One more readonly attribute 'radioState' is exposed to MobileConnection. 'radiostatechange' will be fired when radio state changes.
Flags: needinfo?(htsai)
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #2)
> The basic picture looks promising though we are still refining the API in
> bug 856553. I'll keep you updated once the new API is review granted. Thank
> you.
> 
> However, the main concept is:
> afterwards, gaia would need to call
>   var req = navigator.mozMobileConnection.setRadioPower(true);
> to turn on radio.
> 
> One more readonly attribute 'radioState' is exposed to MobileConnection.
> 'radiostatechange' will be fired when radio state changes.

OK !! Thanks Hsin-Yi !
Fix the dependency.
No longer blocks: 856553
Depends on: 856553
Fix the dependency.
No longer blocks: 926356
Summary: [Gaia::Settings] stop using the settings key 'ril.radio.disabled' to turn off RIL radio → [Gaia][DSDS][Gaia::Settings] stop using the settings key 'ril.radio.disabled' to turn off RIL radio
Hi Arthur,

Interface is confirmed.
https://bugzilla.mozilla.org/attachment.cgi?id=823820&action=diff

1. add MozMobileConnection.radioState
   possible values: null (unknown), 'enabling', 'enabled', 'disabling'

2. use MozMobileConnection.setRadio(in boolean enabled) to enable or disable the radio.
   a) If you call the api when radioState not in a steady state, you will receive 'InvalidStateError' error.
      non steady => null, 'enabling', 'disabling'
   b) Fire onsuccess/onerror to indicate the result.
      It is suggest to update the setting value only when the request success, so that it could sync with current radio state.

3. add 'radiostatechange' event whenever radioState changes.
   For 2a, you could use the event to get a notice if radioState becomes steady and available to call setRadio()

When setRadio(true), radioState will change from X => 'enabling' => 'enabled'
     setRadio(false),                            X => 'disabling' => 'disabled
> 1. add MozMobileConnection.radioState
>    possible values: null (unknown), 'enabling', 'enabled', 'disabling'

and 'disabled'
Hi Arthur,

We just modify the function name to:
MozMobileConnection.setRadioEnabled(in boolean enabled)
                            ^^^^^^^
Sorry for the change.
Blocks: 926356
blocking-b2g: --- → 1.3?
Target Milestone: --- → 1.3 Sprint 4 - 11/8
Blocks: 856553
No longer depends on: 856553
Alive, Evelyn, could you help review the patch? Thanks.
Attachment #825783 - Flags: review?(ehung)
Attachment #825783 - Flags: review?(alive)
Comment on attachment 825783 [details]
link to https://github.com/mozilla-b2g/gaia/pull/13284

Please group |enable/disable AirplaneMode| codes all together in AirplaneMode module. Thanks.
Attachment #825783 - Flags: review?(alive)
Comment on attachment 825783 [details]
link to https://github.com/mozilla-b2g/gaia/pull/13284

Comments addressed, please review it again, thanks!
Attachment #825783 - Flags: review?(alive)
blocking-b2g: 1.3? → 1.3+
Comment on attachment 825783 [details]
link to https://github.com/mozilla-b2g/gaia/pull/13284

clear review flag because the 'radiostatechange' listener is missing, and I feel we need more discussion regarding this API change. So here are my questions:

1. Does `mozMobileConnection.setRadioEnabled` turn the rest interfaces(BT, wifi, geolocation, ril.data) off? or it only turns ril.data off and we still rely on the control in Gaia to turn off others?

2. either the answer of (1) is yes or no, I think we need to listen 'radiostatechange' for UI update, e.g., we have to disable the toggle when it's in a transition state(enabling, disabling). Or even prevent users to touch BT toggle in this situation.

3. are we going to get rid off `ril.radio.disabled` key? I don't want to reuse it to represent "user preference" because it's confusing.
Attachment #825783 - Flags: review?(ehung)
ni aknow for answering my question 1 in comment 13.
Flags: needinfo?(szchen)
This patch is for landing the new gecko API without break any tests, so that I just did minimal changes. We need to file followup bugs if we would like to do other refactor things.
(In reply to Evelyn Hung [:evelyn] from comment #14)
> ni aknow for answering my question 1 in comment 13.

Hi Evelyn,

1. It only turns off the radio of modem part. Thus all the services of call, sms, data service are stopped. It won't affect BT, wifi, and geolocation (gps). You still have to turn the others off in airplane mode.

For 3, I agree with Arthur's comment. We should file another bug if we want to get rid of the key "ril.radio.disabled". Currently, FMRadio is using that key for airplane mode checking and then toggle it self on and off. From my opinion, it is confusing and we should use another key that could better represent the airplane mode.
Flags: needinfo?(szchen)
(In reply to Arthur Chen [:arthurcc] from comment #15)
> This patch is for landing the new gecko API without break any tests, so that
> I just did minimal changes. We need to file followup bugs if we would like
> to do other refactor things.

okay, then I'm fine to merge this patch first just for continuing Gecko work. But I still don't like this patch, and I can point out a few misses there. Please do file a follow-up bug and make sure it will be fixed in v1.3. Thank you so much. :)
Comment on attachment 825783 [details]
link to https://github.com/mozilla-b2g/gaia/pull/13284

r+ with follow-up bug filed. Thank you.
Attachment #825783 - Flags: review+
(In reply to Arthur Chen [:arthurcc] from comment #15)
> This patch is for landing the new gecko API without break any tests, so that
> I just did minimal changes. We need to file followup bugs if we would like
> to do other refactor things.

Hi Arthur,
Based on your comment, I suggest to modify the bug title to a proper one. The purpose of this patch is to support the new API and also maintain the backward compatibility. We are not really "stop using the settings key 'ril.radio.disabled'" in this bug.
Hi Aknow,

I think the title is fine as it says that stop using 'ril.radio.disabled' to "turn off" RIL radio instead of removing 'ril.radio.disabled'. I'll file followup bugs for changing the key name. Thanks for the suggestion.

(In reply to Szu-Yu Chen [:aknow] from comment #19)
> (In reply to Arthur Chen [:arthurcc] from comment #15)
> > This patch is for landing the new gecko API without break any tests, so that
> > I just did minimal changes. We need to file followup bugs if we would like
> > to do other refactor things.
> 
> Hi Arthur,
> Based on your comment, I suggest to modify the bug title to a proper one.
> The purpose of this patch is to support the new API and also maintain the
> backward compatibility. We are not really "stop using the settings key
> 'ril.radio.disabled'" in this bug.
Whiteboard: [FT:RIL]
Thanks for reviewing!

master: 9005f113136219c644d00fc3d165eb49228caf61
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Backed out the code as it fails to enable airplane mode. In system/js/airplane_mode.js we should use the user specified value instead of the negation of the current state to set radio state.
master: fbfcc8ab43e701956d8a73cb8a1ad472bb573830
Evelyn, could you point out the misses so that I won't miss anything when creating the follow up bugs? Thanks!

(In reply to Evelyn Hung [:evelyn] from comment #17)
> (In reply to Arthur Chen [:arthurcc] from comment #15)
> > This patch is for landing the new gecko API without break any tests, so that
> > I just did minimal changes. We need to file followup bugs if we would like
> > to do other refactor things.
> 
> okay, then I'm fine to merge this patch first just for continuing Gecko
> work. But I still don't like this patch, and I can point out a few misses
> there. Please do file a follow-up bug and make sure it will be fixed in
> v1.3. Thank you so much. :)
Flags: needinfo?(ehung)
The most important thing are (as my comment 13 said): 1). considering all transitioning status; 2) making preference setting clear. We can open one follow up bug for both.
Flags: needinfo?(ehung)
We added airplane mode monitoring (bug 876597) in FM radio app[1], should it be removed too?

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/fm/js/fm.js#L773
Flags: needinfo?(ehung)
We still write to ril.radio.enabled so the patch does not break FM radio app. But suggest to use the radio state change event after bug 856553 lands.
Flags: needinfo?(ehung)
You need to log in before you can comment on or make changes to this bug.