B2G RIL: report roaming if SPN is different from operator name

RESOLVED FIXED in mozilla18

Status

()

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

People

(Reporter: gerard, Assigned: gerard)

Tracking

unspecified
mozilla18
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 12 obsolete attachments)

4.53 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
Created attachment 657908 [details] [diff] [review]
Requesting SPN from SIM card

A first need is to request the service provider from the sim card.
(Assignee)

Comment 2

5 years ago
Created attachment 657920 [details] [diff] [review]
Part 1: Code for requesting SPN from SIM card

Introducing the getSPN() function.
Attachment #657908 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
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
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
Component: General → DOM: Device Interfaces
Product: Boot2Gecko → Core
Summary: Faulty roaming detection in specific case → B2G RIL: report roaming if SPN is different from operator name
Assignee: nobody → lissyx+mozillians
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?
Attachment #657920 - Flags: review?(allstars.chh)
Attachment #657921 - Flags: review+
(Assignee)

Updated

5 years ago
Blocks: 788191
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.
(Assignee)

Comment 10

5 years ago
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
Attachment #657922 - Attachment is obsolete: true
Attachment #657923 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created attachment 658247 [details] [diff] [review]
Part 4v2: Forcing roaming status when needed

Now only calling the isRoamingBetweenOperators() that will do the change itself.
(Assignee)

Comment 12

5 years ago
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.
Attachment #658246 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
Created attachment 658258 [details] [diff] [review]
Part 4 v3: Forcing roaming status when needed

Only passing the voice or data message.
Attachment #658247 - Attachment is obsolete: true
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!
Attachment #658257 - Flags: review+
(Assignee)

Comment 15

5 years ago
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.
Attachment #658257 - Attachment is obsolete: true
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 ?
Attachment #657920 - Flags: review?(allstars.chh) → review-
(Assignee)

Comment 17

5 years ago
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
Attachment #657920 - Attachment is obsolete: true
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.
Attachment #658399 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #658277 - Flags: review?(philipp)
(Assignee)

Updated

5 years ago
Attachment #658277 - Flags: review?(philipp)
(Assignee)

Comment 19

5 years ago
ping?
(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
(Assignee)

Comment 21

5 years ago
(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
(Assignee)

Comment 22

5 years ago
Created attachment 661524 [details] [diff] [review]
Merging all patches in one

Merging all patches in one, after rebasing on current master.
Attachment #658399 - Attachment is obsolete: true
Attachment #657921 - Attachment is obsolete: true
Attachment #658258 - Attachment is obsolete: true
Attachment #658277 - Attachment is obsolete: true
Attachment #661524 - Flags: review?(philipp)
Comment on attachment 661524 [details] [diff] [review]
Merging all patches in one

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

Nice work!
Attachment #661524 - Flags: review?(philipp) → review+
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!
(Assignee)

Comment 25

5 years ago
(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 ?
(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.
(Assignee)

Comment 27

5 years ago
Re-updated my gecko from git://github.com/mozilla/releases-mozilla-central, still no issue :(
(Assignee)

Comment 28

5 years ago
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.
Attachment #661524 - Attachment is obsolete: true
(Assignee)

Comment 29

5 years ago
Gecko flashed, phone rebooted, PIN code entered, roaming went away :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/509340e36e44
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/509340e36e44
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Depends on: 833116
You need to log in before you can comment on or make changes to this bug.