Closed Bug 798000 Opened 8 years ago Closed 8 years ago

B2G RIL: support EF_PNN and EF_OPL

Categories

(Core :: DOM: Device Interfaces, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla20
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: brg, Assigned: kk1fff)

References

Details

(Whiteboard: [LOE:M])

Attachments

(3 files, 8 obsolete files)

65.85 KB, image/png
Details
15.43 KB, patch
kk1fff
: review+
Details | Diff | Splinter Review
5.13 KB, patch
kk1fff
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20100101 Firefox/15.0.1
Build ID: 20120905151427

Steps to reproduce:

Insert VIVO SIM card


Actual results:

Carrier name is misspelt. 
This name is wrong in several places, I have found: 
- lock screen
- Notification bar
- Settings --> Cellular&Data
- Settings --> Cellular&Data --> Carrier


Expected results:

The suitable name is VIVO (capital letters), device shows Vivo.

According to the requirements, the name of the carrier is displayed following this rule:
If the first byte of EF_SPN is set to "01" (Display of reg. PLMN required), the device shall display the content of EF_PNN if one is defined in EF_OPL for the network code of the registered PLMN. (If no valid entry is defined in EF_PNN/OPL, the operator name as stored in the device firmware may be displayed.)

I do not know if device firmware is storing this name, but in case so, the name is wrong.
Product: Core → Boot2Gecko
Version: Trunk → unspecified
Nominating for Blocking Basecamp as this is a VIVO certification requirement
blocking-basecamp: --- → ?
Is this something we need in the RIL or is it something we can do in Gaia?
Flags: needinfo?(brg)
Duplicate of this bug: 804688
Andrew, solution to this issue depends on several unhandled SIM records reading/processing and Gaia works as well. Bug 793111 is part of them, but not completed. More missing parts can also be found in the description of bug 804688. I think we should solve all the mess in bug 793111 at once for Gecko RIL part.
Component: General → Gaia
Depends on: 793111
Flags: needinfo?(brg)
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #4)
> Andrew, solution to this issue depends on several unhandled SIM records
> reading/processing and Gaia works as well. Bug 793111 is part of them, but
> not completed. More missing parts can also be found in the description of
> bug 804688. I think we should solve all the mess in bug 793111 at once for
> Gecko RIL part.

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/mobile_info.js

I don't think this is a Gaia issue. All the strings in Gaia spells "VIVO". 

Gaia is getting "Vivo" from MobileComm API.
Component: Gaia → General
Status: UNCONFIRMED → NEW
Ever confirmed: true
blocking-basecamp: ? → +
Assignee: nobody → pwang
Whiteboard: [LOE:M]
Summary: carrier name is not fine → B2G RIL: support EF_PNN and EF_OPL
Attached patch WIP Patch (obsolete) — Splinter Review
Able to read OPL and PNN file from SIM. Function for updating network name haven't been done yet.
Attached patch WIP Patch (obsolete) — Splinter Review
Attachment #685546 - Attachment is obsolete: true
Hi Vicamo, would you help to review this patch? Thanks.
Attachment #685606 - Attachment is obsolete: true
Attachment #688710 - Flags: review?(vyang)
Attached patch Patch: test case (obsolete) — Splinter Review
Attachment #688711 - Flags: review?(vyang)
Comment on attachment 688710 [details] [diff] [review]
Patch: Read EF_PNN, EF_OPL and use them to override network name

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

Could you help add more details like document sources. I'd really want to know the reason for each line to prevent things happened in SPN/SPDI. I'll review/search for document again tomorrow.

::: dom/system/gonk/ril_consts.js
@@ +571,5 @@
>  this.SPDI_TAG_SPDI = 0xa3;
>  this.SPDI_TAG_PLMN_LIST = 0x80;
>  
> +// Full Name IEI from TS 24.008
> +this.TAG_FULL_NETWORK_NAME = 0x43;

How about having PNN_IEI_ as prefix? And please be more specific like:

  // MM INFORMATION message content IEIs
  // See 3GPP TS 24.008 table 9.2.18
  this.PNN_IEI_FULL_NETWORK_NAME = 0x43;

@@ +941,5 @@
>      CBMIR: 16,
>      SPN: 19,
>      DATA_DOWNLOAD_SMS_PP: 28,
> +    PNN: 45,
> +    OPL: 46,

SIM also has both OPL (TS 51.011 section 10.3.42) and PNN (TS 51.011 section 10.3.41)

::: dom/system/gonk/ril_worker.js
@@ +1729,5 @@
> +   */
> +  getOPL: function getOPL() {
> +    let opl = [];
> +    function callback(options) {
> +      if (DEBUG) debug("OPL: Process OPL file");

Looks like a personal debug print.
Attachment #688710 - Flags: review?(vyang)
Fix according to previous review.
Attachment #688710 - Attachment is obsolete: true
Attachment #689155 - Flags: review?(vyang)
Add related spec to the parsing LAI part in getOPL() function.
Attachment #689155 - Attachment is obsolete: true
Attachment #689155 - Flags: review?(vyang)
Attachment #690693 - Flags: review?(vyang)
Comment on attachment 690693 [details] [diff] [review]
Patch: Read EF_PNN, EF_OPL and use them to override network name

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

\(^^)/

::: dom/system/gonk/ril_worker.js
@@ +1713,5 @@
> +              // A value of '00' indicates that the name is to be taken from other
> +              // sources.
> +              return null;
> +            }
> +            pnnIndex = opl.pnnRecordId - 1;

How about:

  pnnEntry = iccPriv.PNN[opl.pnnRecordId - 1];

@@ +1725,5 @@
> +    // the first record in this EF is used for the default network name when
> +    // registered to the HPLMN.
> +    // If we haven't get pnnIndex assigned, we should try to assign default
> +    // value to it.
> +    if (pnnIndex == -1 && mcc == iccInfo.mcc && mnc == iccInfo.mnc) {

if (!pnnEntry && ...)

@@ +1730,5 @@
> +      pnnIndex = 0;
> +    }
> +
> +    if (DEBUG) {
> +      if (pnnIndex != -1) {

if (pnnEntry)

@@ +1738,5 @@
> +        debug("updateNetworkName: Network names will not be overriden");
> +      }
> +    }
> +
> +    if (pnnIndex != -1) {

if (pnnEntry)

@@ +1742,5 @@
> +    if (pnnIndex != -1) {
> +      return [pnn[pnnIndex].fullName, pnn[pnnIndex].shortName];
> +    } else {
> +      return null;
> +    }

if (...) {
  return xxx;
}

return yyy;

@@ +1755,5 @@
> +  getOPL: function getOPL() {
> +    let opl = [];
> +    function callback(options) {
> +      let len = Buf.readUint32();
> +      let readByte = 0;

Orphaned variable.

@@ +1848,5 @@
> +        let name;
> +        switch (tlvTag) {
> +        case PNN_IEI_FULL_NETWORK_NAME:
> +          name = GsmPDUHelper.readNetworkName(tlvLen);
> +          // TODO: Add Country's initials into network name.

let's move this TODO into readNetworkName() and make that function returns a string name directly.

@@ +1858,5 @@
> +          pnnElement.shortName = name.networkName;
> +          break;
> +        case PNN_TAG_PLMN_ADDITIONAL_INFO:
> +          pnnElement.plmnInfo = GsmPDUHelper.readAlphaIdentifier(tlvLen);
> +          break;

We're not pretty sure what to do for this tag, and we may also have problems, although it should never happen, when tlvTag doesn't fall into one of the three. Might have to call to Buf.seekIncoming(PDU_HEX_OCTET_SIZE * tlvLen) instead.
Attachment #690693 - Flags: review?(vyang)
Fix according to previous review.
Attachment #690693 - Attachment is obsolete: true
Attachment #690757 - Flags: review?(vyang)
Attached patch Patch: test case (obsolete) — Splinter Review
Rebase and modified with primary patch.
Attachment #688711 - Attachment is obsolete: true
Attachment #688711 - Flags: review?(vyang)
Attachment #690758 - Flags: review?(vyang)
Comment on attachment 690757 [details] [diff] [review]
Patch: Read EF_PNN, EF_OPL and use them to override network name

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

::: dom/system/gonk/ril_worker.js
@@ +8133,5 @@
> +      return null;
> +    }
> +
> +    // TODO: According to shouldIncludeCountryInitials, add country initials
> +    // to the resulting string.

Please have a bug id for each TODO label :)
Attachment #690757 - Flags: review?(vyang) → review+
Comment on attachment 690758 [details] [diff] [review]
Patch: test case

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +780,5 @@
> +    }
> +  ];
> +
> +  testNetworkName(123, 456, "PNN4Long", "PNN4Short");
> +  testNetworkName(123, 457, "PNN2Long", "PNN2Short");

Have some comments on the two test case, please~~~
Attachment #690758 - Flags: review?(vyang) → review+
Blocks: 820286
r+ in comment 16, adding comment in patch.
Attachment #690757 - Attachment is obsolete: true
Attachment #690780 - Flags: review+
Attached patch Patch: Test caseSplinter Review
r+ in comment 17, adding comments to patch.
Attachment #690758 - Attachment is obsolete: true
Attachment #690783 - Flags: review+
Blocks: 820307
https://hg.mozilla.org/mozilla-central/rev/e2172b8ef3cc
https://hg.mozilla.org/mozilla-central/rev/6fe9b3023000
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.