Last Comment Bug 787967 - B2G RIL: report roaming if SPN is different from operator name
: B2G RIL: report roaming if SPN is different from operator name
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla18
Assigned To: Alexandre LISSY :gerard-majax
:
:
Mentors:
Depends on: 833116
Blocks: 788191
  Show dependency treegraph
 
Reported: 2012-09-03 09:51 PDT by Alexandre LISSY :gerard-majax
Modified: 2013-01-21 14:53 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Requesting SPN from SIM card (1.47 KB, patch)
2012-09-03 13:58 PDT, Alexandre LISSY :gerard-majax
no flags Details | Diff | Splinter Review
Part 1: Code for requesting SPN from SIM card (1.78 KB, patch)
2012-09-03 15:00 PDT, Alexandre LISSY :gerard-majax
allstars.chh: review-
Details | Diff | Splinter Review
Part 2: Probbing the SPN with other ICC fields (764 bytes, patch)
2012-09-03 15:01 PDT, Alexandre LISSY :gerard-majax
philipp: review+
Details | Diff | Splinter Review
Part 3: Heuristic for detecting when not really roaming (2.96 KB, patch)
2012-09-03 15:03 PDT, Alexandre LISSY :gerard-majax
no flags Details | Diff | Splinter Review
Part 4: Forcing roaming status when needed (1.16 KB, patch)
2012-09-03 15:04 PDT, Alexandre LISSY :gerard-majax
no flags Details | Diff | Splinter Review
Part 3 v2: Heuristic for detecting when not really roaming (2.02 KB, patch)
2012-09-04 14:48 PDT, Alexandre LISSY :gerard-majax
no flags Details | Diff | Splinter Review
Part 4v2: Forcing roaming status when needed (1.00 KB, patch)
2012-09-04 14:49 PDT, Alexandre LISSY :gerard-majax
no flags Details | Diff | Splinter Review
Part 3 v3: Heuristic for detecting when not really roaming (2.00 KB, patch)
2012-09-04 15:15 PDT, Alexandre LISSY :gerard-majax
philipp: review+
Details | Diff | Splinter Review
Part 4 v3: Forcing roaming status when needed (992 bytes, patch)
2012-09-04 15:16 PDT, Alexandre LISSY :gerard-majax
no flags Details | Diff | Splinter Review
Part 3 v4: Heuristic for detecting when not really roaming (2.02 KB, patch)
2012-09-04 15:41 PDT, Alexandre LISSY :gerard-majax
no flags Details | Diff | Splinter Review
Part 1 v2: Code for requesting SPN from SIM card (2.38 KB, patch)
2012-09-05 00:36 PDT, Alexandre LISSY :gerard-majax
allstars.chh: review+
Details | Diff | Splinter Review
Merging all patches in one (4.55 KB, patch)
2012-09-15 14:42 PDT, Alexandre LISSY :gerard-majax
philipp: review+
Details | Diff | Splinter Review
Fixing roaming status. (4.53 KB, patch)
2012-09-20 11:12 PDT, Alexandre LISSY :gerard-majax
no flags Details | Diff | Splinter Review

Description Alexandre LISSY :gerard-majax 2012-09-03 09:51:29 PDT
French network operator "Free" is using roaming with "Orange" while it sets up his network. Having two Free SIM card, one in Android device and one in B2G device, I noticed that only B2G reports roaming.

I suspect we should do as in Android:
https://github.com/android/platform_frameworks_base/blob/jb-release/telephony/java/com/android/internal/telephony/gsm/GsmServiceStateTracker.java#L1264
As justified in:
https://github.com/android/platform_frameworks_base/blob/jb-release/telephony/java/com/android/internal/telephony/gsm/GsmServiceStateTracker.java#L665
Comment 1 Alexandre LISSY :gerard-majax 2012-09-03 13:58:04 PDT
Created attachment 657908 [details] [diff] [review]
Requesting SPN from SIM card

A first need is to request the service provider from the sim card.
Comment 2 Alexandre LISSY :gerard-majax 2012-09-03 15:00:42 PDT
Created attachment 657920 [details] [diff] [review]
Part 1: Code for requesting SPN from SIM card

Introducing the getSPN() function.
Comment 3 Alexandre LISSY :gerard-majax 2012-09-03 15:01:48 PDT
Created attachment 657921 [details] [diff] [review]
Part 2: Probbing the SPN with other ICC fields

Now, we probe the SPN when probbing the other ICC fields
Comment 4 Alexandre LISSY :gerard-majax 2012-09-03 15:03:39 PDT
Created attachment 657922 [details] [diff] [review]
Part 3: Heuristic for detecting when not really roaming

This introduces a heuristic that verifies if we are really roaming or of RIL is just joking. Idea is simple: check sim/network name and country code. If they match, then we are not really roaming.
Comment 5 Alexandre LISSY :gerard-majax 2012-09-03 15:04:37 PDT
Created attachment 657923 [details] [diff] [review]
Part 4: Forcing roaming status when needed

Now we make use of the roaming heuristic and we force it false if someone is fooling is.
Comment 6 Alexandre LISSY :gerard-majax 2012-09-03 15:06:59 PDT
With debug enabled:

I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: icc.rilMessageType=iccinfochange
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: icc.imsi=20815XXXXXXXX
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: icc.spn=Free
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: icc.msisdn=+336XXXXXXX
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: icc.mnc=15
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: icc.mcc=208
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: spn=Free
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: onsl=Free
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: onss=Free
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: equalsOnsl=true
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: equalsOnss=true
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: equalsMcc=true
I/Gecko   (   78): -*- RadioInterfaceLayer: isRoamingBetweenOperators: retval=false
Comment 7 Mounir Lamouri (:mounir) 2012-09-04 05:57:44 PDT
Alexande, if those patch are ready, you should ask a review to a RIL peer. See: https://wiki.mozilla.org/Modules/All#Radio_Interface_Layer
Comment 8 Philipp von Weitershausen [:philikon] 2012-09-04 07:43:42 PDT
Comment on attachment 657920 [details] [diff] [review]
Part 1: Code for requesting SPN from SIM card

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

Yoshi, you know the ICC I/O code very well, can you review this please?
Comment 9 Philipp von Weitershausen [:philikon] 2012-09-04 12:21:29 PDT
Comment on attachment 657922 [details] [diff] [review]
Part 3: Heuristic for detecting when not really roaming

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +531,5 @@
> +   * Fix the roaming. RIL can report roaming in some case it is not
> +   * really the case. See bug 787967
> +   *
> +   * @param message  The voiceMessage or dataMessage from which the
> +   *                 roaming state will be changed (maybe).

I don't think this is true, is it? Perhaps you initially wanted `isRoamingBetweenOperators` to modify the message in place but then changed it to return a value?

Also, we could call this parameter `regInfo` or even `registration` for a bit more clarity.

@@ +538,5 @@
> +  isRoamingBetweenOperators: function isRoamingBetweenOperators(message, operator) {
> +    let icc = this.rilContext.icc;
> +    let retval = message.roaming;
> +
> +    if (icc) {

Bail out early could make this a little easier to read:

  let icc = this.rilContext.icc;
  if (!icc) {
    return registration.roaming;
  }

  ...
  return message.roaming && !(equalsMcc && (equalsOnsl || equalsOnss));

@@ +544,5 @@
> +
> +        let onsl = operator.longName;
> +        let onss = operator.shortName;
> +
> +        let equalsOnsl = (onsl != undefined) && (spn == onsl);

Better:

  onsl && (spn == onsl);

@@ +549,5 @@
> +        let equalsOnss = (onss != undefined) && (spn == onss);
> +
> +        let equalsMcc = icc.mcc == operator.mcc;
> +
> +        retval = message.roaming && !(equalsMcc && (equalsOnsl || equalsOnss));

It feels like we're checking things twice here, given how you invoke this helper in part 4:

  if (data.connected && data.roaming && !this.isRoamingBetweenOperators(data, operatorMessage)) {

I would prefer to have this helper modify `message.roaming` in place and do these checks only once in one place.

@@ +554,5 @@
> +
> +        if (DEBUG) {
> +          debug("isRoamingBetweenOperators: icc=" + JSON.stringify(icc));
> +          debug("isRoamingBetweenOperators: icc.rilMessageType=" + icc.rilMessageType);
> +          debug("isRoamingBetweenOperators: icc.imsi=" + icc.imsi);

Please don't include this block in the final patch. All this information should already be logged as part of the postMessage() calls that we make between ril_worker and RadioInterfaceLayer.
Comment 10 Alexandre LISSY :gerard-majax 2012-09-04 14:48:11 PDT
Created attachment 658246 [details] [diff] [review]
Part 3 v2: Heuristic for detecting when not really roaming

Applying the changes you suggested:
 - in place modification
 - message -> registration
 - remove debug
Comment 11 Alexandre LISSY :gerard-majax 2012-09-04 14:49:06 PDT
Created attachment 658247 [details] [diff] [review]
Part 4v2: Forcing roaming status when needed

Now only calling the isRoamingBetweenOperators() that will do the change itself.
Comment 12 Alexandre LISSY :gerard-majax 2012-09-04 15:15:48 PDT
Created attachment 658257 [details] [diff] [review]
Part 3 v3: Heuristic for detecting when not really roaming

Removing the operator argument, we already have it as part of the 'network' field in registration message.
Comment 13 Alexandre LISSY :gerard-majax 2012-09-04 15:16:38 PDT
Created attachment 658258 [details] [diff] [review]
Part 4 v3: Forcing roaming status when needed

Only passing the voice or data message.
Comment 14 Philipp von Weitershausen [:philikon] 2012-09-04 15:27:08 PDT
Comment on attachment 658257 [details] [diff] [review]
Part 3 v3: Heuristic for detecting when not really roaming

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +533,5 @@
> +    *
> +    * @param registration  The voiceMessage or dataMessage from which the
> +    *                      roaming state will be changed (maybe, if needed).
> +    */
> +   isRoamingBetweenOperators: function isRoamingBetweenOperators(registration) {

Now that we've made the function do in-place changes, maybe `checkRoamingBetweenOperators` is a better name.

As for the function body, it looks good except for a few nits:

* there's some trailing whitespace, a lot of empty lines, an overlong line (80 chars max please)
* it's maybe not as concise as it could be while using somewhat cryptic variable names

I'd probably write it as

  let icc = this.rilContext.icc;
  if (!icc || !registration.connected) {
    return;
  }
		 
  let spn = icc.spn;
  let operator = registration.network;
  let longName = operator.longName;
  let shortName = operator.shortName;
		
  let equalsOnsl = longName && (spn == longName);
  let equalsOnss = shortName && (spn == snortName);
  let equalsMcc = icc.mcc == operator.mcc;
		
  registration.roaming = registration.roaming &&
                         !(equalsMcc && (equalsOnsl || equalsOnss));


r=me with those addressed. Once you get Yoshi's review for Part 1, I suggest you squish all patches into one and upload a final patch for checkin. Thanks!
Comment 15 Alexandre LISSY :gerard-majax 2012-09-04 15:41:59 PDT
Created attachment 658277 [details] [diff] [review]
Part 3 v4: Heuristic for detecting when not really roaming

Addressing latest comments:
 - changing function name (makes a 86-long line though)
 - smaller-easier to read code as suggested
 - removing too cryptics variables
 - removed whitespaces as far as I've found some.
Comment 16 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-09-04 19:53:04 PDT
Comment on attachment 657920 [details] [diff] [review]
Part 1: Code for requesting SPN from SIM card

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

Hi, Lissy
Thanks for your work, however I think this patch is incomplete,
I cannot find ICC_EF_SPN in _getPathIdForICCRecord through all your patches ,nor m-c.
so I gave you r- here.

Can you check that again?

Thanks

::: dom/system/gonk/ril_worker.js
@@ +1172,5 @@
> +      let length = Buf.readUint32();
> +      // Each octet is encoded into two chars.
> +      // Minus 1 because first is used to store display condition
> +      let len = (length / 2) - 1;
> +      let spnDisplayCondition = Buf.readUint32();

let spnDisplayCondition = GsmPDUHelper.readHexOctet();

@@ +1174,5 @@
> +      // Minus 1 because first is used to store display condition
> +      let len = (length / 2) - 1;
> +      let spnDisplayCondition = Buf.readUint32();
> +      this.iccInfo.spn = GsmPDUHelper.readAlphaIdentifier(len);
> +

add Buf.readStringDelimiter(length); here

@@ +1184,5 @@
> +
> +    this.iccIO({
> +      command:   ICC_COMMAND_GET_RESPONSE,
> +      fileId:    ICC_EF_SPN,
> +      pathId:    this._getPathIdForICCRecord(ICC_EF_SPN),

I cannot find ICC_EF_SPN in _getPathIdForICCRecord, 
where did you put it ?
Comment 17 Alexandre LISSY :gerard-majax 2012-09-05 00:36:05 PDT
Created attachment 658399 [details] [diff] [review]
Part 1 v2: Code for requesting SPN from SIM card

Addressing comments:
 - added ICC_EF_SPN in _getPathIdForICCRecord as a sibling of EF_AD
 - fixed display condition reading
 - added Buf.readStringDelimiter() call
Comment 18 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-09-05 00:58:36 PDT
Comment on attachment 658399 [details] [diff] [review]
Part 1 v2: Code for requesting SPN from SIM card

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

Great.
Thanks for your hard work.
Comment 19 Alexandre LISSY :gerard-majax 2012-09-12 01:11:08 PDT
ping?
Comment 20 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-09-12 01:33:58 PDT
(In reply to Alexandre LISSY :gerard-majax from comment #19)
> ping?

See philikon's comment in comment 14.

Also it seems your patch is not from hg, can you also check [1], [2], and [3]?

Thank you

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[2]: https://developer.mozilla.org/en-US/docs/Creating_a_patch
[3]: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
Comment 21 Alexandre LISSY :gerard-majax 2012-09-12 01:50:59 PDT
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #20)
> (In reply to Alexandre LISSY :gerard-majax from comment #19)
> > ping?
> 
> See philikon's comment in comment 14.

Yes, I did the change suggested, but I expected it should be reviewed once again, just for the sake of correctness, we both could forgot something.

> 
> Also it seems your patch is not from hg, can you also check [1], [2], and
> [3]?

It's from git format-patch, so I think it matches the requirements in [1]:

    It has a correctly formatted author name.
    It has a correctly formatted commit message.
    It is generated in git style.

> 
> Thank you
> 
> [1]:
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F
> [2]: https://developer.mozilla.org/en-US/docs/Creating_a_patch
> [3]: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
Comment 22 Alexandre LISSY :gerard-majax 2012-09-15 14:42:31 PDT
Created attachment 661524 [details] [diff] [review]
Merging all patches in one

Merging all patches in one, after rebasing on current master.
Comment 23 Philipp von Weitershausen [:philikon] 2012-09-19 14:43:45 PDT
Comment on attachment 661524 [details] [diff] [review]
Merging all patches in one

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

Nice work!
Comment 24 Philipp von Weitershausen [:philikon] 2012-09-19 14:44:42 PDT
Comment on attachment 661524 [details] [diff] [review]
Merging all patches in one

Unfortunately this patch no longer applies on top of m-c tip. Could you please rebase? Thanks!
Comment 25 Alexandre LISSY :gerard-majax 2012-09-19 16:29:43 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #24)
> Comment on attachment 661524 [details] [diff] [review]
> Merging all patches in one
> 
> Unfortunately this patch no longer applies on top of m-c tip. Could you
> please rebase? Thanks!

My gecko master is at 0b6677a9d5ed3cd8f0dd95f39cd3a0a204129d76, and the branch rebase successfully :(. Are there pending changed between mozilla-central dans the git repo that affects this code ?
Comment 26 Philipp von Weitershausen [:philikon] 2012-09-19 16:45:05 PDT
(In reply to Alexandre LISSY :gerard-majax from comment #25)
> (In reply to Philipp von Weitershausen [:philikon] from comment #24)
> > Comment on attachment 661524 [details] [diff] [review]
> > Merging all patches in one
> > 
> > Unfortunately this patch no longer applies on top of m-c tip. Could you
> > please rebase? Thanks!
> 
> My gecko master is at 0b6677a9d5ed3cd8f0dd95f39cd3a0a204129d76,

I don't know which repository this is from. There are several git ones. The Mercurial repository at http://hg.mozilla.org/mozilla-central/ is the canonical source code repository for Gecko so at the end of the day, no matter how you work on Gecko, this is where patches have to apply.

> and the
> branch rebase successfully :(. Are there pending changed between
> mozilla-central dans the git repo that affects this code ?

Maybe. I was actually trying to apply it to mozilla-inbound [1] which is where patches get landed first before they bubble over to mozilla-central if they pass CI.
Comment 27 Alexandre LISSY :gerard-majax 2012-09-20 01:53:51 PDT
Re-updated my gecko from git://github.com/mozilla/releases-mozilla-central, still no issue :(
Comment 28 Alexandre LISSY :gerard-majax 2012-09-20 11:12:02 PDT
Created attachment 663076 [details] [diff] [review]
Fixing roaming status.

Okay, finally got this conflict, so here we go with the updated patch. Should be harmless, but I'm currently rebuilding gecko and testing on my Nexus S in a couple of minutes.
Comment 29 Alexandre LISSY :gerard-majax 2012-09-20 11:46:27 PDT
Gecko flashed, phone rebooted, PIN code entered, roaming went away :)
Comment 30 Philipp von Weitershausen [:philikon] 2012-09-20 11:56:07 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/509340e36e44
Comment 31 Ryan VanderMeulen [:RyanVM] 2012-09-20 18:32:40 PDT
https://hg.mozilla.org/mozilla-central/rev/509340e36e44

Note You need to log in before you can comment on or make changes to this bug.