Closed Bug 1072069 Opened 10 years ago Closed 10 years ago

B2G RIL: read Mailbox numbers (0x6F17) for CPHS support.

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S8 (7Nov)

People

(Reporter: sku, Assigned: bevis)

Details

User Story

Requirement detail from carrier:
[Voicemail Number Recall]
· The device must first attempt to recover the voicemail name & number from (U)SIM EF-MBDN in accordance with the current 3GPP specifications e.g. TS 131.102.
· Where no voicemail name & number entry exists in (U)SIM EF-MBDN, the device must check to see if (U)SIM EF-MBNo exists, in accordance with the Common PCN Handset Specification (CPHS), and attempt to recover the voicemail name & number from there.

Attachments

(5 files, 5 obsolete files)

279.20 KB, application/pdf
Details
15.70 KB, patch
bevis
: review+
Details | Diff | Splinter Review
7.68 KB, patch
bevis
: review+
Details | Diff | Splinter Review
14.93 KB, patch
bevis
: review+
Details | Diff | Splinter Review
7.68 KB, patch
bevis
: review+
Details | Diff | Splinter Review
According to CPHS spec., Mailbox numbers (0x6F17) indicate the voice mail address, gecko should check CSP (0x6F15) first to see if 0x6F17 is valid, then read accordingly to have better voice mail support.

Please check attachment for more detail description.
From vendor's feedback, it's still possible to provide SIM w/ EF_VOICEMAIL but w/o EF_MBDN.

This bug is to enhance the recall of voicemail number from EF_VOICEMAL defined in CPHS if EF_MBDN is not available.
Assignee: nobody → btseng
(In reply to shawn ku [:sku] from comment #0)
> Created attachment 8494199 [details]
> Cphs4_2_generic-69969.pdf
> 
> According to CPHS spec., Mailbox numbers (0x6F17) indicate the voice mail
> address, gecko should check CSP (0x6F15) first to see if 0x6F17 is valid,
> then read accordingly to have better voice mail support.

After reviewing the spec, instead of checking CSP(0x6F15), we should check bit5 and bit6 of the CPHS Service Table in CPHS_INFO(0x6F16) for accessing Mailbox Numbers(0x6F17).
> 
> Please check attachment for more detail description.

The plan support this is to read 1st record of Mailbox Numbers(0x6f17) and notify 'iccmbdn' change when
1. the 1st record of Mailbox number is valid, and
2. the MBDN is not available or empty.
User Story: (updated)
This is to try to read the CPHS_MBN if we are not able to get valid voicemail record from MBDN.

Hi Edgar,

May I have your review for this change?

Thanks!
Attachment #8504687 - Flags: review?(echen)
Corresponding test cases.

Hi Edgar,

May I have your review for this change?

Thanks!
Attachment #8504690 - Flags: review?(echen)
NI reporter & vendor to see if we have the possibility to have the test SIM with CPHS EFs included for further verification before landing the fix.
Flags: needinfo?(sku)
Flags: needinfo?(arvin.zhang)
User Story: (updated)
Hi Bevis,

Sorry for reply late.
I checked the two blank sim cards locally but there's no EF_MBDN(0X6FC7) and EF_MBNo(0X6F17) found. I'll ask PM's help to confirm whether there's the test SIM with CPHS EFs. So keep NI here.

Thanks.
(In reply to helloarvin from comment #6)
> Hi Bevis,
> 
> Sorry for reply late.
> I checked the two blank sim cards locally but there's no EF_MBDN(0X6FC7) and
> EF_MBNo(0X6F17) found. I'll ask PM's help to confirm whether there's the
> test SIM with CPHS EFs. So keep NI here.
> 
> Thanks.

Thanks, Arvin! :)
Comment on attachment 8504687 [details] [diff] [review]
Part 1 v1: Read CPHS Mailbox Numbers (0x6F17) when MBDN is invalid.

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

Thanks for your patch, please see my comments below.

::: dom/system/gonk/ril_worker.js
@@ +12157,5 @@
>        case ICC_EF_PNN:
>        case ICC_EF_SMS:
>        case ICC_EF_GID1:
> +      case ICC_EF_CPHS_INFO:
> +      case ICC_EF_CPHS_MBN:

The CPHS spec doesn't have EF path information for USIM, we just follow the setup in AOSP.
Please add some comments about the path of EF_CPHS_* for USIM is from AOSP. Thank you.

@@ +13502,5 @@
>        let contact =
>          this.context.ICCPDUHelper.readAlphaIdDiallingNumber(options.recordSize);
> +      if ((!contact ||
> +           (!contact.alphaId || contact.alphaId == "" &&
> +            !contact.number || contact.number == "")) &&

I guess you wanna do `(foo1 || foo2) && (bar1 || bar2)`.
If so, please add parentheses around || operation, otherwise && will be processed first. [1]

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Expressions_and_Operators#Operator_precedence

@@ +14092,5 @@
> +   * Read CPHS Phase & Service Table from CPHS Info.
> +   *
> +   * @See  B.3.1.1 CPHS Information in CPHS Phase 2.
> +   */
> +  readCphsInfo: function(onComplete) {

Please have different callbacks for success and error case, just like other utility functions.

readCphsInfo: function(onsuccess, onerror) { ...

@@ +14111,5 @@
> +          }
> +          this.context.debug("CPHS INFO: " + str);
> +        }
> +
> +        /*

nit: /**

@@ +14139,5 @@
> +        let cphsPhase = cphsInfo[0];
> +        if (cphsPhase == 1) {
> +          // Clear 'Phase 2 only' services.
> +          cphsInfo[1] &= 0x3F;
> +          if (cphsInfo.length > 2) {

Why we need this check? From the spec, it seems EF_CPHS_INFORMATION has at least 3 btyes.

@@ +14157,5 @@
> +        onComplete();
> +      }
> +    }
> +
> +    function onerror(errorMsg){

nit: space between `)` and `{`

@@ +14175,5 @@
> +
> +  /**
> +   * Read CPHS MBN. (Mailbox Numbers)
> +   *
> +   * @See B.4.2.2Voice Message Retrieval and Indicator Clearing

nit: space between `B.4.2.2` and `Voice`.

@@ +14184,5 @@
> +      let contact =
> +        this.context.ICCPDUHelper.readAlphaIdDiallingNumber(options.recordSize);
> +      if (!contact ||
> +          (RIL.iccInfoPrivate.mbdn !== undefined &&
> +           RIL.iccInfoPrivate.mbdn === contact.number)) {

We read EF_CPHS_MBN only if we can not get valid mbdn from EF_MBDN. Do we still need this checks for RIL.iccInfoPrivate.mbdn?

::: dom/system/gonk/tests/test_ril_worker_icc_SimRecordHelper.js
@@ +191,5 @@
>        }
>      }
>    }
>  
> +  let expectCalled = ["getIMSI", "readAD", "readCphsInfo"];

We still expect readSST should be called in fetchSimRecords, so please add `readSST` back (You probably need to trigger the callback in fake_readCpnsInfo).
Attachment #8504687 - Flags: review?(echen)
Comment on attachment 8504687 [details] [diff] [review]
Part 1 v1: Read CPHS Mailbox Numbers (0x6F17) when MBDN is invalid.

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

::: dom/system/gonk/ril_worker.js
@@ +12157,5 @@
>        case ICC_EF_PNN:
>        case ICC_EF_SMS:
>        case ICC_EF_GID1:
> +      case ICC_EF_CPHS_INFO:
> +      case ICC_EF_CPHS_MBN:

Sure. I'll add more comment on this in next update. :)

@@ +13502,5 @@
>        let contact =
>          this.context.ICCPDUHelper.readAlphaIdDiallingNumber(options.recordSize);
> +      if ((!contact ||
> +           (!contact.alphaId || contact.alphaId == "" &&
> +            !contact.number || contact.number == "")) &&

Oh no. So shame on making this mistake.. orz.

@@ +14092,5 @@
> +   * Read CPHS Phase & Service Table from CPHS Info.
> +   *
> +   * @See  B.3.1.1 CPHS Information in CPHS Phase 2.
> +   */
> +  readCphsInfo: function(onComplete) {

Will do.

@@ +14111,5 @@
> +          }
> +          this.context.debug("CPHS INFO: " + str);
> +        }
> +
> +        /*

Will do.

@@ +14139,5 @@
> +        let cphsPhase = cphsInfo[0];
> +        if (cphsPhase == 1) {
> +          // Clear 'Phase 2 only' services.
> +          cphsInfo[1] &= 0x3F;
> +          if (cphsInfo.length > 2) {

My concern is that we don't know how this EF looks like for the SIM with CPHS Phase 1.
Just want to have a boundary check before making change on it.

@@ +14157,5 @@
> +        onComplete();
> +      }
> +    }
> +
> +    function onerror(errorMsg){

Will do.

@@ +14175,5 @@
> +
> +  /**
> +   * Read CPHS MBN. (Mailbox Numbers)
> +   *
> +   * @See B.4.2.2Voice Message Retrieval and Indicator Clearing

Will do.

@@ +14184,5 @@
> +      let contact =
> +        this.context.ICCPDUHelper.readAlphaIdDiallingNumber(options.recordSize);
> +      if (!contact ||
> +          (RIL.iccInfoPrivate.mbdn !== undefined &&
> +           RIL.iccInfoPrivate.mbdn === contact.number)) {

This is to have a protection not sending iccmbdn to upper layer if we read these records again when toggling airplan mode On/Off. :)

::: dom/system/gonk/tests/test_ril_worker_icc_SimRecordHelper.js
@@ +191,5 @@
>        }
>      }
>    }
>  
> +  let expectCalled = ["getIMSI", "readAD", "readCphsInfo"];

I see. I'll add fake_readCphsInfo to ensure this. :)
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #9)
> Comment on attachment 8504687 [details] [diff] [review]
> Part 1 v1: Read CPHS Mailbox Numbers (0x6F17) when MBDN is invalid.
> 
> Review of attachment 8504687 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
>
> @@ +14139,5 @@
> > +        let cphsPhase = cphsInfo[0];
> > +        if (cphsPhase == 1) {
> > +          // Clear 'Phase 2 only' services.
> > +          cphsInfo[1] &= 0x3F;
> > +          if (cphsInfo.length > 2) {
> 
> My concern is that we don't know how this EF looks like for the SIM with
> CPHS Phase 1.
> Just want to have a boundary check before making change on it.
> 
> @@ +14184,5 @@
> > +      let contact =
> > +        this.context.ICCPDUHelper.readAlphaIdDiallingNumber(options.recordSize);
> > +      if (!contact ||
> > +          (RIL.iccInfoPrivate.mbdn !== undefined &&
> > +           RIL.iccInfoPrivate.mbdn === contact.number)) {
> 
> This is to have a protection not sending iccmbdn to upper layer if we read
> these records again when toggling airplan mode On/Off. :)

Oh, I see. I am ok with above checks now. Thank you.
Comment on attachment 8504690 [details] [diff] [review]
Part 2 v1: Test cases to verify CPHS_INFO and CPHS_MBN and the recovery of CPHS_MBN when failure from readMBDN.

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

Looks good, but I would like to review it again after you address the comments for part1. Thank you.

::: dom/system/gonk/tests/test_ril_worker_icc_SimRecordHelper.js
@@ +1299,5 @@
> +add_test(function test_read_cphs_info() {
> +  let worker = newUint8Worker();
> +  let context = worker.ContextPool._contexts[0];
> +  let RIL = context.RIL;
> +  let helper = context.GsmPDUHelper;

nit: s/helper/pduHelper/

@@ +1383,5 @@
> +add_test(function test_read_voicemail_number() {
> +  let worker = newUint8Worker();
> +  let context = worker.ContextPool._contexts[0];
> +  let RIL = context.RIL;
> +  let helper = context.GsmPDUHelper;

Ditto.

@@ +1387,5 @@
> +  let helper = context.GsmPDUHelper;
> +  let recordHelper = context.SimRecordHelper;
> +  let buf    = context.Buf;
> +  let io     = context.ICCIOHelper;
> +  let mbnData = [

nit: Move |mbnData| into |io.loadLinearFixedEF| because it is only used there,

@@ +1450,5 @@
> +  let recordHelper = context.SimRecordHelper;
> +  let iccUtilsHelper = context.ICCUtilsHelper;
> +  let buf    = context.Buf;
> +  let io     = context.ICCIOHelper;
> +  let mbnData = [

Ditto
Attachment #8504690 - Flags: review?(echen)
address suggestion in comment 8 to:
1. add onsucess/onerror callback for readCphsInfo.
2. Add more explanation for the questions asked in comment 8.
3. fix the problem found in readCphsMDN.
4. modify test_fetch_sim_records() according to the change of readCphsInfo.
Attachment #8504687 - Attachment is obsolete: true
Attachment #8506652 - Flags: review?(echen)
Modify test according to the change in patch part1.
Attachment #8504690 - Attachment is obsolete: true
Attachment #8506654 - Flags: review?(echen)
Attachment #8506652 - Flags: review?(echen) → review+
Comment on attachment 8506654 [details] [diff] [review]
Part 2 v2: Test cases to verify CPHS_INFO and CPHS_MBN and the recovery of CPHS_MBN when failure from readMBDN.

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

I guess you uploaded a wrong file. ;)
Attachment #8506654 - Flags: review?(echen)
Oops! Sorry, my bad. :(
Update the right one for review.
Attachment #8506654 - Attachment is obsolete: true
Attachment #8507647 - Flags: review?(echen)
Finally found you, the test sim with CPHS EFs!

Bevis, please feel free to notify me so that we can verify the feature with it in time.

Thanks.

PS: Clear the needinfo of sku meantime.
Flags: needinfo?(sku)
Flags: needinfo?(arvin.zhang)
Comment on attachment 8507647 [details] [diff] [review]
Part 2 v2: Test cases to verify CPHS_INFO and CPHS_MBN and the recovery of CPHS_MBN when failure from readMBDN.

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

Thank you.

::: dom/system/gonk/tests/test_ril_worker_icc_SimRecordHelper.js
@@ +1346,5 @@
> +    recordHelper.readCphsInfo(() => { onsuccess = true; },
> +                              () => { onerror = true; });
> +
> +    do_check_true((cphsSt)? onsuccess : onerror);
> +    do_check_false((cphsSt)? onerror : onsuccess);

nit: spaces around operator `?`.

@@ +1348,5 @@
> +
> +    do_check_true((cphsSt)? onsuccess : onerror);
> +    do_check_false((cphsSt)? onerror : onsuccess);
> +    if (cphsSt) {
> +        do_check_eq(RIL.iccInfoPrivate.cphsSt.length, cphsSt.length);

nit: indention.
Attachment #8507647 - Flags: review?(echen) → review+
update final patch with positive try server result.
Attachment #8506652 - Attachment is obsolete: true
Attachment #8508419 - Flags: review+
address nits and update final patch with positive try server result.
Attachment #8507647 - Attachment is obsolete: true
Attachment #8508420 - Flags: review+
(In reply to helloarvin from comment #16)
> Finally found you, the test sim with CPHS EFs!
> 
> Bevis, please feel free to notify me so that we can verify the feature with
> it in time.
> 
> Thanks.
> 
> PS: Clear the needinfo of sku meantime.

Hi Arvin,

I've finalized the patch in mozilla-central branch as attached.
Note: the solution is to retrieve the voicemail number from EF_CHPS_MBN only if the EF_MBDN in USIM 
is not valid or empty.

Would you please give it a try?

In addition, you can also enable the debug flag in ril_consts.js by set this.DEBUG_ALL to true:
http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_consts.js
Then, if you met any problem, please help to have the adb logcat captured for further analysis.

Thanks!
Flags: needinfo?(arvin.zhang)
Okey.

I'll merge the patch locally and build the temp version for verify.
Update later.
Flags: needinfo?(arvin.zhang)
(In reply to helloarvin from comment #21)
> Okey.
> 
> I'll merge the patch locally and build the temp version for verify.
> Update later.

Hi Arvin,

Is there any update about this?
Flags: needinfo?(arvin.zhang)
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #22)
> (In reply to helloarvin from comment #21)
> > Okey.
> > 
> > I'll merge the patch locally and build the temp version for verify.
> > Update later.
> 
> Hi Arvin,
> 
> Is there any update about this?

Hi Bevis,

We still need little more time to finish verify because we should wait for the feedback from system test member of Shanghai Company(I'm in Tianjin).

Besides, it took me much time to fix patch conflict for there're more than ten patches on ril_work.js locally:-)

I'll update in time and just keep NI state.

Thanks.
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #22)
> (In reply to helloarvin from comment #21)
> > Okey.
> > 
> > I'll merge the patch locally and build the temp version for verify.
> > Update later.
> 
> Hi Arvin,
> 
> Is there any update about this?

Dear Bevis,

Sorry to update late. The patch is finally verified successfully!

The device will always try to recover voicemail name & number from (U)SIM EF-MBDN and attempt to read from EF-MBNo if there's no entry exists in (U)SIM EF-MBDN.

Thank you very much!

BTW, could you please help land the patch into the branch v1.4 and v2.1?
Flags: needinfo?(arvin.zhang)
(In reply to helloarvin from comment #24)
> (In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #22)
> > (In reply to helloarvin from comment #21)
> > > Okey.
> > > 
> > > I'll merge the patch locally and build the temp version for verify.
> > > Update later.
> > 
> > Hi Arvin,
> > 
> > Is there any update about this?
> 
> Dear Bevis,
> 
> Sorry to update late. The patch is finally verified successfully!
> 
> The device will always try to recover voicemail name & number from (U)SIM
> EF-MBDN and attempt to read from EF-MBNo if there's no entry exists in
> (U)SIM EF-MBDN.
> 
> Thank you very much!
> 
> BTW, could you please help land the patch into the branch v1.4 and v2.1?

Hi Arvin,

Thanks for your update.
For both v1.4 and v2.1, since this bug is a new feature to support instead of bug fixing.
I'm afraid that you have to cherry pick this patch instead in current stage.

Sorry for inconvenience.  :(
Hi Bevis,

We'll merge the patch into v1.4 branch locally for there's already no 1.4-blocker flag. But it should be landed into the branch v2.1 to ensure better support for protocols.

How do you think,thanks?
https://hg.mozilla.org/mozilla-central/rev/a0fe9d394e01
https://hg.mozilla.org/mozilla-central/rev/1992aef31eab
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
Dear Shawn,

As our previous discussion, could you please help land the patch into the branch v2.1?

Thanks a lot.

Arvin
Flags: needinfo?(sku)
[Blocking Requested - why for this release]:
Partner needs this path on 2.1 branch.
And, this is a generic feature on Voice Mail enhancement based on CPHS spec.

Please kindly consider 2.1+.
blocking-b2g: --- → 2.1?
Hi Bevis:
 Please help provide 2.1 patch once got approval.
Thanks!!
Shawn
Flags: needinfo?(sku) → needinfo?(btseng)
TRiage, lets not block on this yet as I need some more clarification on the files being modified, shawn I'll get in touch with you.
(In reply to bhavana bajaj [:bajaj] from comment #33)
> TRiage, lets not block on this yet as I need some more clarification on the
> files being modified, shawn I'll get in touch with you.

Hi Bhavana:
 Agree with you that we should not block on this.
I would like Bevis to prepare 2.1 patch, and let partner to take/apply by them self.

Does that sound good to you?

Thanks!!
Shawn
(In reply to shawn ku [:sku] from comment #34)
> (In reply to bhavana bajaj [:bajaj] from comment #33)
> > TRiage, lets not block on this yet as I need some more clarification on the
> > files being modified, shawn I'll get in touch with you.
> 
> Hi Bhavana:
>  Agree with you that we should not block on this.
> I would like Bevis to prepare 2.1 patch, and let partner to take/apply by
> them self.
> 
> Does that sound good to you?
yep, I am removing it from the blocking nom.
> 
> Thanks!!
> Shawn
blocking-b2g: 2.1? → ---
Hi Bevis,
  Thank you very much for preparing the patch on v2.1 branch.

Hi Shawn,
  Could you please help explain why we don't land the generic feature into the branch?
  Thanks.
Flags: needinfo?(sku)
Hi(In reply to helloarvin from comment #38)
> Hi Bevis,
>   Thank you very much for preparing the patch on v2.1 branch.
> 
> Hi Shawn,
>   Could you please help explain why we don't land the generic feature into
> the branch?
>   Thanks.

Hi Arvin:
 It's due to 2.1 mainline was frozen for new feature already.

You can apply this in your local branch, or file a new bug when 2.1s is branched out.
We can do the patch cherry-pick then.

Please let me know if you still have any concerns.
Thanks!!
Shawn
Flags: needinfo?(sku)
(In reply to shawn ku [:sku] from comment #39)
> Hi(In reply to helloarvin from comment #38)
> > Hi Bevis,
> >   Thank you very much for preparing the patch on v2.1 branch.
> > 
> > Hi Shawn,
> >   Could you please help explain why we don't land the generic feature into
> > the branch?
> >   Thanks.
> 
> Hi Arvin:
>  It's due to 2.1 mainline was frozen for new feature already.
> 
> You can apply this in your local branch, or file a new bug when 2.1s is
> branched out.
> We can do the patch cherry-pick then.
> 
> Please let me know if you still have any concerns.
> Thanks!!
> Shawn

Get it.
Thanks for your kind explanation.

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

Attachment

General

Created:
Updated:
Size: