Closed Bug 793073 Opened 7 years ago Closed 7 years ago

B2G RIL: Quickly tapping Airplane Mode causes phone keeping in "Emergency Calls Only"

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: vliu, Assigned: vliu)

Details

Attachments

(1 file, 3 obsolete files)

Method to reproduce this issue.

1. Powered on the device and checked the device in in network coverage.
2. Quickly tapped Airplane Mode less than 1 second.
3. When Airplane Mode kept in off mode, the phone lose network coverage and showed "Emergency Calls Only".
Sounds like we should do some debouncing here, similarly to what I outlined in bug 785982 comment 1. Shian-Yow, we talked about this in Sao Paulo, perhaps you could take this issue? Thanks!
Assignee: nobody → swu
Component: General → DOM: Device Interfaces
Product: Boot2Gecko → Core
Attached patch WIP V1 (obsolete) — Splinter Review
When the issue happens, Vincent found multiple RIL_REQUEST_RADIO_POWER events queued in rild which caused problems in rild layer.  Vincent is checking with Otoro device partner for possible cause.

Attached debouncing patch can avoid this problem.
Attachment #663348 - Flags: review?(philipp)
Comment on attachment 663348 [details] [diff] [review]
WIP V1

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

I'm not sure I like the additional states. This (indirectly) amends the MobileConnection WebAPI. Furthermore, the debounce doesn't really seem to be doing what I as a user would expect from it: eventual consistency with my setting.

I would suggest that while we change the radio power, we keep a flag around until we get the next radio state changed notification. If the `ril.radio.enabled` setting changes at any time in between that, we ignore it. In the radio state changed notification, we again compare the radio state to the `ril.radio.enabled` setting and make adjustments if they're out of sync.
Attachment #663348 - Flags: review?(philipp) → review-
Hi Philikon, Shian-Yow,

May I have chance to take this issue for trying your suggestion? Thanks.
(In reply to vliu from comment #4)
> May I have chance to take this issue for trying your suggestion? Thanks.

Sure, I don't care. Just please don't spend too much time on this, this isn't a blocker. We need to finish blockers first.
(In reply to Philipp von Weitershausen [:philikon] from comment #3)
> I'm not sure I like the additional states. This (indirectly) amends the
> MobileConnection WebAPI. Furthermore, the debounce doesn't really seem to be
> doing what I as a user would expect from it: eventual consistency with my
> setting.
> 
> I would suggest that while we change the radio power, we keep a flag around
> until we get the next radio state changed notification. If the
> `ril.radio.enabled` setting changes at any time in between that, we ignore
> it. In the radio state changed notification, we again compare the radio
> state to the `ril.radio.enabled` setting and make adjustments if they're out
> of sync.

As my understanding, out of sync handling should be already handled in current code logic.

_ensureRadioState() will called inside handleRadioStateChange() where there's a state change:
https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js?rev=3abe394b6110#735

And the syncing is done inside _ensureRadioState() by checking 'this._radioEnabled' in:
https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js?rev=3abe394b6110#762

I think what we need to enhance is adding new flags, if we don't like to amend the MobileConnection WebAPI.
(In reply to vliu from comment #4)
> Hi Philikon, Shian-Yow,
> 
> May I have chance to take this issue for trying your suggestion? Thanks.

Yes, feel free to take it.
Assignee: swu → vliu
Attached patch WIP V2 (obsolete) — Splinter Review
Hi Philikon,

> I would suggest that while we change the radio power, we keep a flag around
> until we get the next radio state changed notification. If the
> `ril.radio.enabled` setting changes at any time in between that, we ignore
> it. In the radio state changed notification, we again compare the radio
> state to the `ril.radio.enabled` setting and make adjustments if they're out
> of sync.

Based on your suggestion, I implemented the code as WIP V2. The main idea is below.
1. The _radioEnabled was tracked by _currentradioEnabled only when debouncing was deactivated.
2. During debouncing, ignoring radio power adjustment if _currentradioEnabled and _radioEnabled were different.
3. _currentradioEnabled won't tracking until deactivating debouncing.

Can you please have a review and give me any suggestion? Thanks.
Attachment #663964 - Flags: review?(philipp)
(In reply to Shian-Yow Wu from comment #6)
> As my understanding, out of sync handling should be already handled in
> current code logic.
> 
> _ensureRadioState() will called inside handleRadioStateChange() where
> there's a state change:
> https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> RadioInterfaceLayer.js?rev=3abe394b6110#735

Very good point!
Comment on attachment 663964 [details] [diff] [review]
WIP V2

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +715,5 @@
>        return;
>      }
> +    if (this._currentradioEnabled != this._radioEnabled) {
> +      // During debouncing, we ignored radio power adjustments when the
> +      // radio state is out of sync.

Well the point is to ignore *all* radio power adjustments while we're bringing up or down the radio power. Why won't 1 flag be enough? Like this:

  if (this._changingRadioPower) {
    // We're changing the radio power currently, ignore any changes.
    return;
  }

and then just add

  this._changingRadioPower = true;

to the `setRadioEnabled()` method and

  this._changingRadioPower = false;

to the `handleRadioStateChange()` method before we call _ensureRadioState(). Seems way simpler to me.
Attachment #663964 - Flags: review?(philipp)
Attached patch WIP V3 (obsolete) — Splinter Review
(In reply to Philipp von Weitershausen [:philikon] from comment #10)
> Comment on attachment 663964 [details] [diff] [review]
> WIP V2
> 
> Review of attachment 663964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +715,5 @@
> >        return;
> >      }
> > +    if (this._currentradioEnabled != this._radioEnabled) {
> > +      // During debouncing, we ignored radio power adjustments when the
> > +      // radio state is out of sync.
> 
> Well the point is to ignore *all* radio power adjustments while we're
> bringing up or down the radio power. 

Yes, your point is right and I should focus on it. Thanks.
Attachment #665228 - Flags: review?(philipp)
Comment on attachment 665228 [details] [diff] [review]
WIP V3

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +728,5 @@
>      }
>      this.rilContext.radioState = newState;
>      //TODO Should we notify this change as a card state change?
>  
> +    this._changingRadioPower = false;

Please put this at the top of the function so we always execute this, even if we take the early exit in the `if`.

@@ +758,5 @@
>  
>      if (this.rilContext.radioState == RIL.GECKO_RADIOSTATE_OFF &&
>          this._radioEnabled) {
>        this.setRadioEnabled(true);
> +      this._changingRadioPower = true;

Please move this to `setRadioEnabled()`

@@ +1249,5 @@
>    _radioEnabled: null,
>  
> +  // Flag to ignore any radio power change requests during We're changing
> +  // the radio power.
> +  _changingRadioPower: null,

false, please.
Attachment #665228 - Flags: review?(philipp)
Attached patch WIP V4Splinter Review
Hi Philikon,

Thanks for your code review on comment 12.
Attachment #665741 - Flags: review?(philipp)
Attachment #665741 - Flags: review?(philipp) → review+
Keywords: checkin-needed
Probably we should set this to blocking-basecamp+ because when the bug occurs, a reboot is needed to recover.
blocking-basecamp: --- → ?
Blocking based on comment #14.
blocking-basecamp: ? → +
Attachment #663348 - Attachment is obsolete: true
Attachment #663964 - Attachment is obsolete: true
Attachment #665228 - Attachment is obsolete: true
Flags: in-testsuite? → in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/c2d3a2496dc9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.