B2G RIL: consider reporting unavailable state or retriggering (non-default) data call

RESOLVED FIXED

Status

Firefox OS
RIL
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jessica, Assigned: jessica)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.2r+, b2g-v2.2r fixed, b2g-master wontfix)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
This is a follow-up to Bug 1179097.

There are cases when non-default data call is not able to recover from "disconnected" state, e.g.:
- after retry times reaches NETWORK_APNRETRY_MAXRETRIES (10)
- when data call is dropped, and data registration is unregistered [1]

We should consider reporting "unavailable" state to upper layer or discuss whether re-triggering data call is needed.

Note that we will face the same problem when doing NetworkManager Enhancement: when should we reconnect non-default data automatically? (e.g. when radio turns off and on, when data registration is lost and back). If there are cases that we do not reconnect them automatically, then we must report an "unavailable" state.

[1] http://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/file/03dd969850a0/dom/system/gonk/RadioInterfaceLayer.js#l2944
(Assignee)

Updated

2 years ago
Assignee: nobody → jjong
(Assignee)

Comment 1

2 years ago
Created attachment 8658625 [details] [diff] [review]
patch, v1.

In this patch, we'll report 'unavailable' state after reaching the maximum retry times (10).
Currently, after data registration is recovered, only default data call will be retriggered, in this patch, we've also added a way to retrigger non-default data calls.
(Assignee)

Comment 2

2 years ago
Comment on attachment 8658625 [details] [diff] [review]
patch, v1.

Edgar, may I have your review on this? Thanks.
Attachment #8658625 - Flags: review?(echen)

Updated

2 years ago
status-b2g-v2.2r: --- → affected
status-b2g-master: --- → wontfix
Depends on: 1179097

Comment 3

2 years ago
Comment on attachment 8658625 [details] [diff] [review]
patch, v1.

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

Looks good. Just one thing, is it possible to add some tests for data registration changes (if emulator can support such behaviour)?
Thank you.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +3213,5 @@
>    get connected() {
>      return this.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED;
>    },
>  
> +

Nit: Remove this empty line.

::: dom/system/gonk/nsINetworkManager.idl
@@ +31,5 @@
>    const long REASON_NONE              = 0;
>    const long REASON_RADIO_SHUTDOWN    = 1;
>    const long REASON_APN_CHANGED       = 2;
>    const long REASON_SERVICEID_CHANGED = 3;
> +  const long REASON_RETRY_FAILED      = 4;

Need an uuid bump?
Attachment #8658625 - Flags: review?(echen) → review+
(Assignee)

Comment 4

2 years ago
Thanks Edgar!

(In reply to Edgar Chen [:edgar][:echen] from comment #3)
> Comment on attachment 8658625 [details] [diff] [review]
> patch, v1.
> 
> Review of attachment 8658625 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Just one thing, is it possible to add some tests for data
> registration changes (if emulator can support such behaviour)?
> Thank you.

Actually, we do test this in test_data_call_state_events.js [1], however this wasn't caught due to some timing issues. If we want to test this properly, we'll need to add some delay (using setTimeout() maybe) between tests, but I am not sure if it's a good thing to do.

[1] http://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/file/86954a963254/dom/datacall/tests/marionette/test_data_call_state_events.js#l102

> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +3213,5 @@
> >    get connected() {
> >      return this.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED;
> >    },
> >  
> > +
> 
> Nit: Remove this empty line.

Will do.

> 
> ::: dom/system/gonk/nsINetworkManager.idl
> @@ +31,5 @@
> >    const long REASON_NONE              = 0;
> >    const long REASON_RADIO_SHUTDOWN    = 1;
> >    const long REASON_APN_CHANGED       = 2;
> >    const long REASON_SERVICEID_CHANGED = 3;
> > +  const long REASON_RETRY_FAILED      = 4;
> 
> Need an uuid bump?

yes, thanks for the reminder.
(Assignee)

Comment 5

2 years ago
Created attachment 8661663 [details] [diff] [review]
patch. r=echen

- Address review comment 3
- Added new reason (REASON_PERMANENT_FAILURE) which is set when data call setup fails with permanent fail cause
Attachment #8658625 - Attachment is obsolete: true
Attachment #8661663 - Flags: review+
(Assignee)

Comment 6

2 years ago
This is a follow-up to Bug 1179097, so should be merged into v2.2r.
blocking-b2g: --- → 2.2r?
2.2r+ due to comment 6
blocking-b2g: 2.2r? → 2.2r+
(Assignee)

Comment 8

2 years ago
try looks good except for some known failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a083acb6d2a&group_state=expanded

checkin-needed for mozilla-b2g37_v2_2r.
Keywords: checkin-needed
(In reply to Jessica Jong [:jjong] [:jessica] from comment #8)
> try looks good except for some known failures:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9a083acb6d2a&group_state=expanded
> 
> checkin-needed for mozilla-b2g37_v2_2r.

done :) https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/68a9516194fe
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-b2g-v2.2r: affected → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.