Closed Bug 782513 Opened 7 years ago Closed 7 years ago

B2G RIL: Support multiple instances of RILNetworkInterface

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-b2g shira+
blocking-basecamp +
Tracking Status
b2g18 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: swu, Assigned: swu)

References

Details

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

Attachments

(4 files, 5 obsolete files)

For multiple APNs, we need to support multiple instances of RILNetworkInterface for each data connection.
Assignee: nobody → swu
blocking-basecamp: --- → +
Whiteboard: [LOE:M]
Attached patch WIP V1 (obsolete) — Splinter Review
Attachment #655765 - Flags: feedback?(philipp)
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)
Attached patch WIP V2 (obsolete) — Splinter Review
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]
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.
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.
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
Attached patch WIP V3 (obsolete) — Splinter Review
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
Attachment #659658 - Flags: review?(philipp)
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
Attached patch WIP V4 (obsolete) — Splinter Review
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)
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)
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.
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-
(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?
Attached patch WIP V5 (obsolete) — Splinter Review
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+
Attached patch V5 (r=philikon)Splinter Review
Rebased.
Attachment #662800 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a58c7a717057
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [LOE:M] [WebAPI:P0] → [LOE:M] [WebAPI:P0] [mno11]
blocking-b2g: --- → shira+
You need to log in before you can comment on or make changes to this bug.