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)
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.
Assignee | ||
Comment 1•10 years ago
|
||
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
Assignee | ||
Comment 2•10 years ago
|
||
(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.
Assignee | ||
Updated•10 years ago
|
User Story: (updated)
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
Corresponding test cases. Hi Edgar, May I have your review for this change? Thanks!
Attachment #8504690 -
Flags: review?(echen)
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
User Story: (updated)
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
(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 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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. :)
Comment 10•10 years ago
|
||
(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 11•10 years ago
|
||
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)
Assignee | ||
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
Modify test according to the change in patch part1.
Attachment #8504690 -
Attachment is obsolete: true
Attachment #8506654 -
Flags: review?(echen)
Updated•10 years ago
|
Attachment #8506652 -
Flags: review?(echen) → review+
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
Oops! Sorry, my bad. :( Update the right one for review.
Attachment #8506654 -
Attachment is obsolete: true
Attachment #8507647 -
Flags: review?(echen)
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
update final patch with positive try server result.
Attachment #8506652 -
Attachment is obsolete: true
Attachment #8508419 -
Flags: review+
Assignee | ||
Comment 19•10 years ago
|
||
address nits and update final patch with positive try server result.
Attachment #8507647 -
Attachment is obsolete: true
Attachment #8508420 -
Flags: review+
Assignee | ||
Comment 20•10 years ago
|
||
(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)
Comment 21•10 years ago
|
||
Okey. I'll merge the patch locally and build the temp version for verify. Update later.
Flags: needinfo?(arvin.zhang)
Assignee | ||
Comment 22•10 years ago
|
||
(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)
Comment 23•10 years ago
|
||
(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.
Comment 24•10 years ago
|
||
(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)
Assignee | ||
Comment 25•10 years ago
|
||
(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. :(
Assignee | ||
Comment 26•10 years ago
|
||
update try server result: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0245d11a5243
Keywords: checkin-needed
Comment 27•10 years ago
|
||
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?
Comment 28•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/a0fe9d394e01 https://hg.mozilla.org/integration/b2g-inbound/rev/1992aef31eab
Flags: in-testsuite+
Keywords: checkin-needed
Comment 29•10 years ago
|
||
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)
Comment 30•9 years ago
|
||
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)
Reporter | ||
Comment 31•9 years ago
|
||
[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?
Reporter | ||
Comment 32•9 years ago
|
||
Hi Bevis: Please help provide 2.1 patch once got approval. Thanks!! Shawn
Flags: needinfo?(sku) → needinfo?(btseng)
Comment 33•9 years ago
|
||
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.
Reporter | ||
Comment 34•9 years ago
|
||
(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
Assignee | ||
Comment 35•9 years ago
|
||
create patch for v2.1 branch.
Attachment #8533604 -
Flags: review+
Assignee | ||
Comment 36•9 years ago
|
||
create patch for v2.1 branch. try server result: https://tbpl.mozilla.org/?tree=Try&rev=78a9f7e545f2
Flags: needinfo?(btseng)
Attachment #8533605 -
Flags: review+
Comment 37•9 years ago
|
||
(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
Updated•9 years ago
|
blocking-b2g: 2.1? → ---
Comment 38•9 years ago
|
||
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)
Reporter | ||
Comment 39•9 years ago
|
||
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)
Comment 40•9 years ago
|
||
(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.
Description
•