B2G 3G: Configure APN settings through SettingsService.

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaoo, Assigned: jaoo)

Tracking

Trunk
mozilla17
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:-, blocking-basecamp:-)

Details

Attachments

(1 attachment, 5 obsolete attachments)

We were configuring the APN setting for data calls through Gecko's preferences so this bug is for changing current implementation.
(Assignee)

Updated

5 years ago
Assignee: nobody → josea.olivera
Blocks: 710493
(Assignee)

Comment 1

5 years ago
Created attachment 644699 [details] [diff] [review]
WIP v1

This patch depends on bug 740640.
Attachment #644699 - Flags: feedback?(philipp)

Updated

5 years ago
Blocks: 772747
Comment on attachment 644699 [details] [diff] [review]
WIP v1

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

I'm much in favour of this! Great job! See below about some improvements, though.

::: dom/system/gonk/NetworkManager.js
@@ +22,5 @@
>  const TOPIC_INTERFACE_REGISTERED     = "network-interface-registered";
>  const TOPIC_INTERFACE_UNREGISTERED   = "network-interface-unregistered";
>  const TOPIC_DEFAULT_ROUTE_CHANGED    = "network-default-route-changed";
>  
> +const DEBUG = true;

ahem.

(I highly recommend having a separate patch in your queue for debugging purposes. That way you never contaminate your "real" patches with that stuff.)

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +625,5 @@
>        this.setRadioEnabled(false);
>      }
>    },
>  
> +  _ensureAPNSettings: function _ensureAPNSettings() {

"_ensureAPNSettings" is not an ideal name. It sounds like a value check rather than something that actually changes the state of the data connection. How about "updateRILNetworkInterface"?

@@ +1064,5 @@
> +  _rilDataUser: null,
> +  _rilDataPasswd: null,
> +  _rilDataHttpProxyHost: null,
> +  _rilDataHttpProxyPort: null,
> +  _oldRilDataEnabledState: null,

I would prefer if we did this in a more structured approach, e.g. have

  dataSettings: {},

and then do `this.dataSettings["disabled"]` etc. (Please always use double quotes for strings.). This will also make your gigantic `switch` statements a lot simpler and we can just pass `this.dataSettings` around to ril_worker.js and RILNetworkInterface.
Attachment #644699 - Flags: feedback?(philipp) → feedback+
Comment on attachment 644699 [details] [diff] [review]
WIP v1

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1884,5 @@
>    get connected() {
>      return this.state == RIL.GECKO_NETWORK_STATE_CONNECTED;
>    },
>  
> +  connect: function connect(data) {

Hi, philikon and jaoo
How do you think if we change data to a more high level argument, said 'options', or 'apn' here?

So this function can be used by MMS and AGPS.
(see Bug 772747)

For example:

options: {
  type: "data', // or 'mms', 'supl'
  apn: ...,
  user, ...,
  passwd, ...,
}
(Assignee)

Comment 4

5 years ago
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #3)
> Comment on attachment 644699 [details] [diff] [review]
> WIP v1

> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1884,5 @@
> >    get connected() {
> >      return this.state == RIL.GECKO_NETWORK_STATE_CONNECTED;
> >    },
> >  
> > +  connect: function connect(data) {
> 
> Hi, philikon and jaoo
> How do you think if we change data to a more high level argument, said
> 'options', or 'apn' here?
> 
> So this function can be used by MMS and AGPS.
> (see Bug 772747)
> 
> For example:
> 
> options: {
>   type: "data', // or 'mms', 'supl'
>   apn: ...,
>   user, ...,
>   passwd, ...,
> }

That is already changed in the new version of the patch that I gonna attach today. The connect function will receive an object as Philipp suggested in comment #2. 

+  connect: function connect(data) {
     ...
+    if (data) {
+      // Save the APN data locally for using them in connection retries. 
+      this.dataCallSettings = data;
     }

     this.mRIL.setupDataCall(RIL.DATACALL_RADIOTECHNOLOGY_GSM,
-                            apn, user, passwd,
+                            this.dataCallSettings["ril.data.apn"], 
+                            this.dataCallSettings["ril.data.user"], 
+                            this.dataCallSettings["ril.data.passwd"],
                             RIL.DATACALL_AUTH_PAP_OR_CHAP, "IP");
     ...
   }

Does it look good? 

I guess you could get here `this.dataCallSettings["ril.data.apn.type"]` if the caller provides it.
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #4)
> +  connect: function connect(data) {
>      ...
> +    if (data) {
> +      // Save the APN data locally for using them in connection retries. 
> +      this.dataCallSettings = data;
>      }
> 
>      this.mRIL.setupDataCall(RIL.DATACALL_RADIOTECHNOLOGY_GSM,
> -                            apn, user, passwd,
> +                            this.dataCallSettings["ril.data.apn"], 
> +                            this.dataCallSettings["ril.data.user"], 
> +                            this.dataCallSettings["ril.data.passwd"],
>                              RIL.DATACALL_AUTH_PAP_OR_CHAP, "IP");
>      ...

Oh, maybe need philikon's opinion,
because MMS and AGPS may also use this function,(by using MMS APN and AGPS APN),
so I am thinking maybe we replace the word 'data' as it may suggest that this is a DataConnection.

Or maybe we create another RILNetworkInterfaces for MMS and APN ?
(Assignee)

Comment 6

5 years ago
Created attachment 645753 [details] [diff] [review]
WIP v2

This patch addresses what Philipp suggested in comment #2.
Attachment #644699 - Attachment is obsolete: true
Attachment #645753 - Flags: review?(philipp)
(Assignee)

Updated

5 years ago
Blocks: 740640
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #4)
> I guess you could get here `this.dataCallSettings["ril.data.apn.type"]` if
> the caller provides it.

That's not going to work. I agree with Yoshi here, and if you read my comment 2 carefully, you'll see that that's what I meant.
Comment on attachment 645753 [details] [diff] [review]
WIP v2

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +632,5 @@
> +    if (typeof this.dataCallSettings["ril.data.apn"] === 'undefined' ||
> +        typeof this.dataCallSettings["ril.data.user"] === 'undefined' ||
> +        typeof this.dataCallSettings["ril.data.passwd"] === 'undefined' ||
> +        typeof this.dataCallSettings["ril.data.roaming.enabled"] === 'undefined' ||
> +        typeof this.dataCallSettings["ril.data.enabled"] === 'undefined') {

No `typeof` needed. Just compare to `null`:

  if (this.dataCallSettings["apn"] == null || ...)

although I'm inclined to treat missing/null values for apn, user, passwd as empty strings. It often works with mobile providers.

@@ +639,5 @@
> +      return;
> +    }
> +    if (this._oldRilDataEnabledState == this.dataCallSettings["ril.data.enabled"]) {
> +      debug("No changes for ril.data.enabled flag. Nothing to do.");
> +      return;

That seems wrong. What if I change the APN name or the password? Shouldn't we reconnect?

@@ +959,5 @@
> +      case "ril.data.apn":
> +      case "ril.data.user":
> +      case "ril.data.passwd":
> +        debug(setting.key + " is now " + setting.value);
> +        this.dataCallSettings[setting.key] = setting.value;

Strip "ril.data." from the key.

@@ +1030,1 @@
>    handle: function handle(aName, aResult) {

Sigh. I wish that `handle()` and `handleMozSettingsChanged()` had the same signature so we could just use the same method here.

@@ +1035,5 @@
> +        this._ensureRadioState();
> +        break;
> +      case "ril.data.enabled":
> +      case "ril.data.roaming.enabled":
> +        this.dataCallSettings[aName] = aResult == null ? false : aResult;

Ditto.

@@ +1053,3 @@
>    handleError: function handleError(aErrorMessage) {
>      debug("There was an error reading the 'ril.radio.disabled' setting., " +
>            "default to radio on.");

You're going to have to update this message. We now deal with more than just the `ril.radio.disabled` setting.

@@ +1059,5 @@
> +    this.dataCallSettings["ril.data.enabled"] = false;
> +    this.dataCallSettings["ril.data.roaming.enabled"] = false;
> +    this.dataCallSettings["ril.data.apn"] = "";
> +    this.dataCallSettings["ril.data.user"] = "";
> +    this.dataCallSettings["ril.data.passwd"] = "";

Why not

  this.dataCallSettings = {};
Attachment #645753 - Flags: review?(philipp) → feedback+
(Assignee)

Comment 9

5 years ago
Created attachment 646384 [details] [diff] [review]
WIP v3
Attachment #645753 - Attachment is obsolete: true
Attachment #646384 - Flags: review?(philipp)
(In reply to Philipp von Weitershausen [:philikon] from comment #8)
> Comment on attachment 645753 [details] [diff] [review]
> WIP v2
> 
> Review of attachment 645753 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +632,5 @@
> > +    if (typeof this.dataCallSettings["ril.data.apn"] === 'undefined' ||
> > +        typeof this.dataCallSettings["ril.data.user"] === 'undefined' ||
> > +        typeof this.dataCallSettings["ril.data.passwd"] === 'undefined' ||
> > +        typeof this.dataCallSettings["ril.data.roaming.enabled"] === 'undefined' ||
> > +        typeof this.dataCallSettings["ril.data.enabled"] === 'undefined') {
> 
> No `typeof` needed. Just compare to `null`:
> 
>   if (this.dataCallSettings["apn"] == null || ...)
> 
> although I'm inclined to treat missing/null values for apn, user, passwd as
> empty strings. It often works with mobile providers.

What I want to do here is to ensure that all the APN data have been read from the setting DB before setting the data call up at boot. I have changed a bit the way of checking it. Every time I read an APN setting (name, user, etc.) I save its key and I only set the data call up if I have read every key.
 
Now we treat missing null values for apn, user and passwd. The RIL worker receives the following message and the data call is set up properly as well. That works with Telefonica as you mentioned.

I/Gecko   (  105): RIL Worker: Received DOM message {"type":"setupDataCall","radioTech":1,"apn":null,"user":null,"passwd":null,"chappap":3,"pdptype":"IP"}

> @@ +639,5 @@
> > +      return;
> > +    }
> > +    if (this._oldRilDataEnabledState == this.dataCallSettings["ril.data.enabled"]) {
> > +      debug("No changes for ril.data.enabled flag. Nothing to do.");
> > +      return;
> 
> That seems wrong. What if I change the APN name or the password? Shouldn't
> we reconnect?

The logic here is the same that we did in the past. 

+    // We only watch at "ril.data.enabled" flag changes for connecting or
+    // disconnecting the data call. If the value of "ril.data.enabled" is
+    // true and any of the remaining flags change the setting application
+    // should turn this flag to false and then to true in order to reload
+    // the new values and reconnect the data call.
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #10)
> +    // We only watch at "ril.data.enabled" flag changes for connecting or
> +    // disconnecting the data call. If the value of "ril.data.enabled" is
> +    // true and any of the remaining flags change the setting application
> +    // should turn this flag to false and then to true in order to reload
> +    // the new values and reconnect the data call.

Ah I remember now... You're right, there was a good reason for doing it this way because it leaves the UI in control of restarting the data call. So we want to make sure that Gaia flips the flag after changing any of the other values. Can you file (or even fix) that, please?
Comment on attachment 646384 [details] [diff] [review]
WIP v3

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +635,5 @@
>    },
>  
> +  updateRILNetworkInterface: function updateRILNetworkInterface() {
> +    for each (let key in DATA_CALL_SETTING_KEYS) {
> +      if(this._dataCallSettingsRead.indexOf(key) == -1) {

Nit: space after `if`.

@@ +640,5 @@
> +        debug("We haven't read completely the APN data from the " +
> +              "settings DB yet. Wait for that.");
> +        return;
> +      }
> +    }

I see now what you're trying to do here. It would be more efficient and elegant to start out with what is essentially DATA_CALL_SETTING_KEYS

  this._dataCallSettingsToRead = ["ril.data.enabled", ...];  // in constructor

and then remove items as we read them. That way, we would merely be left with testing for `this._dataCallSettingsToRead.length` here, which is much easier.

@@ +1018,5 @@
> +        this._radioEnabled = !aResult;
> +        this._ensureRadioState();
> +        break;
> +      case "ril.data.enabled":
> +        this._oldRilDataEnabledState = this.dataCallSettings["enabled"]; 

Please add

  // Fall through!

here

@@ +1025,5 @@
> +      case "ril.data.user":
> +      case "ril.data.passwd":
> +        let key = aName.search("roaming") != -1 ?
> +          "roaming" :
> +          aName.substring(aName.lastIndexOf(".") + 1, aName.length);

Meh, just strip the first 9 characters:

  key = key.slice(9);

Also no need to treat "roaming.enabled" as a special case. If we really care, we could rename it to "ril.data.roaming_enabled".

@@ +1028,5 @@
> +          "roaming" :
> +          aName.substring(aName.lastIndexOf(".") + 1, aName.length);
> +        this.dataCallSettings[key] = aResult;
> +        debug("'" + aName + "'" + " is now " + this.dataCallSettings[key]);
> +        if(this._dataCallSettingsRead.indexOf(aName) == -1) {

Nit: space after `if`.
Attachment #646384 - Flags: review?(philipp) → feedback+
Created attachment 646501 [details] [diff] [review]
WIP v4
Attachment #646384 - Attachment is obsolete: true
Attachment #646501 - Flags: review?(philipp)
Gaia pull request https://github.com/mozilla-b2g/gaia/pull/2895 for changing the setting app since we have change an APN setting key.
Comment on attachment 646501 [details] [diff] [review]
WIP v4

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

r=me with final points below addressed.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +186,5 @@
> +  gSettingsService.getLock().get("ril.data.user", this);
> +  gSettingsService.getLock().get("ril.data.passwd", this);
> +  gSettingsService.getLock().get("ril.data.roaming_enabled", this);
> +  gSettingsService.getLock().get("ril.data.enabled", this);
> +  this._dataCallSettingsToRead = DATA_CALL_SETTING_KEYS; 

Well, no. There was a reason why I spelled my example out the way I did. What you're doing here is just saving a reference to the global array. So when we read settings, you're modifying that. Either you want to say

  this._dataCallSettingsToRead = DATA_CALL_SETTING_KEYS.slice();

Or, you know, just get rid of the DATA_CALL_SETTING_KEYS const because this is the only place we'd need it. (Please do the latter as I indicated previously.)

@@ +634,5 @@
>      }
>    },
>  
> +  updateRILNetworkInterface: function updateRILNetworkInterface() {
> +    if (this._dataCallSettingsToRead.length > 0) {

Just

  if (this._dataCallSettingsToRead.length)

is enough.

@@ +983,5 @@
>    observe: function observe(subject, topic, data) {
>      switch (topic) {
>        case kMozSettingsChangedObserverTopic:
>          let setting = JSON.parse(data);
> +        this.handle(setting.key, setting.value);

<3
Attachment #646501 - Flags: review?(philipp) → review+
Created attachment 646881 [details] [diff] [review]
WIP v5

Latest comments addressed.
Attachment #646501 - Attachment is obsolete: true
Attachment #646881 - Flags: review?(philipp)
Comment on attachment 646881 [details] [diff] [review]
WIP v5

(Thanks! No need to ask for review again since I already gave you r+.)
Attachment #646881 - Flags: review?(philipp) → review+
Comment on attachment 646881 [details] [diff] [review]
WIP v5

Oops, I've tested it after latest airplane mode changes in gaia and I've noticed the data call does not reconnect after toggling off airplane mode. Looking at it.
Attachment #646881 - Attachment is obsolete: true
Created attachment 647178 [details] [diff] [review]
WIP v6
Is this ready to be pushed now?
(In reply to Philipp von Weitershausen [:philikon] from comment #20)
> Is this ready to be pushed now?

Yes it is. I did not ask for review at you since you already gave me r+. Just finished a test again after a clean build and it seems everything is working.
Great, thanks for the update. Next time, please set the checkin-needed keyword to indicate that the patch is ready to be pushed. Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c7c41b33485
Target Milestone: --- → mozilla17
blocking-basecamp: --- → ?
https://hg.mozilla.org/mozilla-central/rev/4c7c41b33485
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Philikon says this is very useful but not critical so basecamp-'ing (even though it's fixed :).
blocking-basecamp: ? → -
blocking-kilimanjaro: --- → -
You need to log in before you can comment on or make changes to this bug.