B2G RIL: Support multiple instances of RILNetworkInterface

RESOLVED FIXED in mozilla18

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: swu, Assigned: swu)

Tracking

unspecified
mozilla18
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-b2g:shira+, blocking-basecamp:+, b2g18 fixed, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [LOE:M] [WebAPI:P0] [mno11])

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

6 years ago
For multiple APNs, we need to support multiple instances of RILNetworkInterface for each data connection.
(Assignee)

Updated

6 years ago
Blocks: 710489, 762426, 772747

Updated

6 years ago
Assignee: nobody → swu
blocking-basecamp: --- → +
(Assignee)

Updated

6 years ago
Whiteboard: [LOE:M]
(Assignee)

Comment 1

6 years ago
Created attachment 655765 [details] [diff] [review]
WIP V1
Attachment #655765 - Flags: feedback?(philipp)
(Assignee)

Comment 2

6 years ago
Created attachment 655771 [details] [diff] [review]
Tesing code (apply for testing only)

This patch is used for testing two APNs.
Comment on attachment 655765 [details] [diff] [review]
WIP V1

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +143,5 @@
>  });
>  
>  
>  function RadioInterfaceLayer() {
> +  this.RILNetworkInterfaces = {};

AFAICT you only need this map for shutting down active network interfaces. In fact, we need to shut down *all* network interface objects, not just the active ones. (E.g. ones that aren't connecting successfully have a timer that auto-reconnects, and we need to make sure we shut down that timer.)

@@ +145,5 @@
>  
>  function RadioInterfaceLayer() {
> +  this.RILNetworkInterfaces = {};
> +  this.defaultRILNetworkInterface = new RILNetworkInterface();
> +  this.defaultRILNetworkInterface.type = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE;

I would make the `type` a constructor argument. Also, we can save separately acquiring the RIL object below by passing it in. So something like:

  this.dataNetworkInterface = newRILNetworkInterface(this, Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE);

(Note that I've changed the name slightly. "default" doesn't really explain what the network interface is used for, and "RIL" is superfluous ;))
Attachment #655765 - Flags: feedback?(philipp)
(Assignee)

Comment 4

6 years ago
Created attachment 656436 [details] [diff] [review]
WIP V2
Attachment #655765 - Attachment is obsolete: true
Attachment #656436 - Flags: review?(philipp)
Comment on attachment 656436 [details] [diff] [review]
WIP V2

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

Please note that bug 744700 is almost ready to land, and it was in my queue before yours ;). So I think you should rebase on top of that. Given that, I'm just going to clear the review because I think I want to see the final patch before it lands. Otherwise it'd be r+. :)

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1728,5 @@
>      this.worker.postMessage({rilMessageType: msgType, requestId: requestId});
>    }
>  };
>  
> +function RILNetworkInterface(mRIL, type)

Nit: s/mRIL/ril/

(The 'm' prefix stands for member... We're using it here slightly inconsistently. If you want to prefix arguments, you should use 'a', e.g. `aRIL, aType`. But the existing style in this file doesn't do that.)
Attachment #656436 - Flags: review?(philipp)
Whiteboard: [LOE:M] → [LOE:M] [WebAPI:P0]
(Assignee)

Comment 6

6 years ago
When testing two APNs, I found there are a couple of things to be fixed _processDataCallList() of ril_worker.js.

One thing is handling of ifname is null or "" in datacalls[] object.  On Otoro, if we establish one data call, there will be totally 20 entries received from RILD.  One entry with valid ifname for new created data call and 19 entries with ifname=null.
Our current implementation copies all 20 entries from datacall[] to currentDataCalls[].

For example, if data call cid=0 established, we got the following datacalls[] for cid=0 & cid=1.

"0":{"status":0,"suggestedRetryTime":-1,"cid":"0","active":2,"type":"IP","ifname":"rmnet0","ipaddr":"111.80.51.225/30","dns":["168.95.1.1","168.95.192.1"],"gw":"111.80.51.226","ip":"111.80.51.225","netmask":"255.255.255.252","broadcast":"111.80.51.227"},

"1":{"status":0,"suggestedRetryTime":-1,"cid":"1","active":0,"type":"","ifname":"","ipaddr":"","dns":"","gw":"","ip":null,"netmask":null,"broadcast":null}

Later on, if data call with cid=1 also established, not all info in cid=1 entry were updated to currentDataCalls[].

For entry with ifname=null, I think we should:
1) if it's not in currentDataCalls[], don't add it from datacalls[] to currentDataCalls[].  
2) if it's in currentDataCalls[], remove it from currentDataCalls[].

I will provide a WIP patch for this.
(Assignee)

Comment 7

6 years ago
The other problem I encountered in _processDataCallList() is, sometimes the first entry in datacalls[] from RILD doesn't start from cid=0, and below is an example.

datacalls: {"1":{"status":0,"suggestedRetryTime":-1,"cid":"1","active":2,"type":"IP","ifname":"rmnet1","ipaddr":"10.49.166.77/30","dns":["10.1.7.1","10.1.7.2"],"gw":"10.49.166.78","ip":"10.49.166.77","netmask":"255.255.255.252","broadcast":"10.49.166.79"}}

In this condition, the line below in _processDataCallList() might retrieve wrong data call entry.

updatedDataCall = datacalls[currentDataCall.cid];

see https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?rev=fcc61274ec43#2788

Will provide a WIP patch for this, too.
(Assignee)

Comment 8

6 years ago
The 3rd problem is, there is no 'callIndex' in currentDataCall, so the following line in _processDataCallList() doesn't work.

https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?rev=fcc61274ec43#2793
(Assignee)

Comment 9

6 years ago
Created attachment 659658 [details] [diff] [review]
WIP V3

Changes in this patch:

1. Addressed previous review comments

2. Rebased on bug 744700

3. Unregister RILNetworkInterface to Network Manager when stated becomes RIL.GECKO_NETWORK_STATE_DISCONNECTED.
If data call 1 disconnected, we have to unregister it in order not to confuse with the state after a different data call 2 connected later.

4. Fix issues as stated in comment 6,7,8.
Attachment #656436 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #659658 - Flags: review?(philipp)
(Assignee)

Comment 10

6 years ago
Hi philikon,

The WIP V3 patch was rebased on Patch V7.2 of bug 744700, and the rebase result on latest Patch V7.3 was the same.  So this patch is the up to date one.  Please help to review, thank you. :)
Status: NEW → ASSIGNED
(Assignee)

Comment 11

6 years ago
Created attachment 661178 [details] [diff] [review]
WIP V4

Hi, philikon,

Sorry, after further testing, I've added some fix and need to submit a new patch.  Please let me know if it's fine for you to review base on new patch, and I'll obsolete previous patch, thank you!

The main difference between V3 and V4 is:
1. Call unregisterNetworkInterface() when data call state is GECKO_NETWORK_STATE_UNKNOWN, instead of GECKO_NETWORK_STATE_DISCONNECTED.
2. Fixed a bug of incorrect 'break' position in V3.
Depends on: 744700
Attachment #661178 - Flags: review?(philipp)
Attachment #659658 - Attachment is obsolete: true
Attachment #659658 - Flags: review?(philipp)
Created attachment 662022 [details] [diff] [review]
Array-ize APN Settings/RILNetworkInterface: V1

Here is another implementation if this issue. Tying to neutralize every APN settings set, the only occurrence of "internet" type (AKA "default" in Android's terminology) is in handleDataCallError() for now, but I think I should not automatically connect data connections of other than "internet" type. During initializing process, the handler function will look up next possible APN settings set automatically. I think these changes will make bug 772747 more easier and have a more generic way in manipulating APN settings sets and RILNetworkInterfaces.
Attachment #662022 - Flags: feedback?(swu)
Attachment #662022 - Flags: feedback?(philipp)
Created attachment 662023 [details] [diff] [review]
Array-ize APN Settings/RILNetworkInterface: Gaia changes

settings key "ril.data.apn" => "ril.data.0.apn", etc. Also add an additional "ril.data.0.types" setting, but I'd better name it "ril.data.0.usages" to keep the same naming used in apps/settings/serviceprovider.xml.
(Assignee)

Comment 14

6 years ago
Comment on attachment 662022 [details] [diff] [review]
Array-ize APN Settings/RILNetworkInterface: V1

It's good to have Array-ized APN settings.
I suggest to have this as a follow up bug, so we can do more testing.
Attachment #662022 - Flags: feedback?(swu) → feedback+
Comment on attachment 662022 [details] [diff] [review]
Array-ize APN Settings/RILNetworkInterface: V1

Just have a working version first and refine it in following task.
Attachment #662022 - Flags: feedback?(philipp)
Comment on attachment 661178 [details] [diff] [review]
WIP V4

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1960,5 @@
> +
> +    if (this.state == RIL.GECKO_NETWORK_STATE_UNKNOWN &&
> +       this.registeredAsNetworkInterface) {
> +      let networkManager = Cc["@mozilla.org/network/manager;1"]
> +                             .getService(Ci.nsINetworkManager);

Please create a lazy service getter at the top of the file for this.

::: dom/system/gonk/ril_worker.js
@@ +2788,4 @@
>        if (datacalls) {
> +        for (let datacallIndex in datacalls) {
> +          let datacall = datacalls[datacallIndex];
> +          if (datacall.cid == currentDataCall.cid) {

I don't understand why you're doing this. `datacalls` and `this.currentDataCalls` should be mapping CID -> datacall object. Manually iterating seems like a waste of cycles.

r- because either you or I are not understanding something correctly, and I'd like to resolve this misunderstanding before we commit the code. :)

The rest looks really good!
Attachment #661178 - Flags: review?(philipp) → review-
(Assignee)

Comment 17

6 years ago
(In reply to Philipp von Weitershausen [:philikon] from comment #16)
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1960,5 @@
> > +
> > +    if (this.state == RIL.GECKO_NETWORK_STATE_UNKNOWN &&
> > +       this.registeredAsNetworkInterface) {
> > +      let networkManager = Cc["@mozilla.org/network/manager;1"]
> > +                             .getService(Ci.nsINetworkManager);
> 
> Please create a lazy service getter at the top of the file for this.
> 

Ok.

> ::: dom/system/gonk/ril_worker.js
> @@ +2788,4 @@
> >        if (datacalls) {
> > +        for (let datacallIndex in datacalls) {
> > +          let datacall = datacalls[datacallIndex];
> > +          if (datacall.cid == currentDataCall.cid) {
> 
> I don't understand why you're doing this. `datacalls` and
> `this.currentDataCalls` should be mapping CID -> datacall object. Manually
> iterating seems like a waste of cycles.
> 

Right, manually iterating doesn't look good.  But I found some issues when testing two APNs by original code, and it was explained in comment 7 and comment 8.  Do you have any suggestions to make it simpler?
(Assignee)

Comment 18

6 years ago
Created attachment 662800 [details] [diff] [review]
WIP V5

Hi philikon,

After testing and double checking again, you are right, it's not needed to use manual iteration in comment 7.  I must be too tired when working on the change for comment 7.  :)

New patch addressed comment 16, please review again, thanks!
Attachment #661178 - Attachment is obsolete: true
Attachment #662800 - Flags: review?(philipp)
Attachment #662800 - Flags: review?(philipp) → review+
(Assignee)

Comment 19

6 years ago
Created attachment 664913 [details] [diff] [review]
V5 (r=philikon)

Rebased.
Attachment #662800 - Attachment is obsolete: true
(Assignee)

Comment 20

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a58c7a717057
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/a58c7a717057
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED

Updated

6 years ago
Whiteboard: [LOE:M] [WebAPI:P0] → [LOE:M] [WebAPI:P0] [mno11]
blocking-b2g: --- → shira+
status-b2g18: --- → fixed

Updated

6 years ago
status-b2g18-v1.0.1: --- → fixed
You need to log in before you can comment on or make changes to this bug.