782513 - B2G RIL: Support multiple instances of RILNetworkInterface

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Shian-Yow Wu [:swu]]

For multiple APNs, we need to support multiple instances of RILNetworkInterface for each data connection.

Comment 1

[Shian-Yow Wu [:swu]]

Attachment: WIP V1

Comment 2

[Shian-Yow Wu [:swu]]

Attachment: Tesing code (apply for testing only)

This patch is used for testing two APNs.

Comment 3

[Philipp von Weitershausen [:philikon]]

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 ;))

Comment 4

[Shian-Yow Wu [:swu]]

Attachment: WIP V2

Comment 5

[Philipp von Weitershausen [:philikon]]

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.)

Comment 6

[Shian-Yow Wu [:swu]]

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.

Comment 7

[Shian-Yow Wu [:swu]]

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.

Comment 8

[Shian-Yow Wu [:swu]]

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

Comment 9

[Shian-Yow Wu [:swu]]

Attachment: 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.

Comment 10

[Shian-Yow Wu [:swu]]

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. :)

Comment 11

[Shian-Yow Wu [:swu]]

Attachment: 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.

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Attachment: 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.

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Attachment: 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.

Comment 14

[Shian-Yow Wu [:swu]]

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.

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 16

[Philipp von Weitershausen [:philikon]]

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!

Comment 17

[Shian-Yow Wu [:swu]]

(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?

Comment 18

[Shian-Yow Wu [:swu]]

Attachment: 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!

Comment 19

[Shian-Yow Wu [:swu]]

Attachment: V5 (r=philikon)

Rebased.

Comment 20

[Shian-Yow Wu [:swu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a58c7a717057

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a58c7a717057