Closed Bug 1111990 Opened 5 years ago Closed 5 years ago

[B2G][RIL] Should only access EF_FDN when FDN service is available

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S3 (9jan)

People

(Reporter: edgar, Assigned: jdai)

References

Details

Attachments

(1 file, 3 obsolete files)

According to TS 131.102 Clause 4.2.24. EF_FDN is presented only if service n° 4 and/or service n° 89 is available. We should follow this behaviour in ril_worker.
(In reply to Edgar Chen [:edgar][:echen] from comment #0)
> According to TS 131.102 Clause 4.2.24. EF_FDN is presented only if service
> n° 4 and/or service n° 89 is available. We should follow this behaviour in
  ^^^^
  typo, it should be `n° 2`

> ril_worker.
Assignee: nobody → jdai
1. Add RUIM FDN ICC Services Table support base on GPP2 C.S0023-D 3.4.18.
2. Check FDN service is available before use.
3. Update the related testcase.
Attachment #8542480 - Flags: review?(echen)
Comment on attachment 8542480 [details] [diff] [review]
Support access EF_FDN only if FDN service is available

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

Please see my comments below. Thank you.

::: dom/system/gonk/ril_consts.js
@@ +1315,5 @@
>      SPDI: 51
>    },
>    // @see 3GPP2 C.S0023-D 3.4.18 (RUIM).
>    ruim: {
> +  	FDN: 3,

nit: indention.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
@@ +129,5 @@
>    function do_test(aSimType, aContactType, aExpectedContact, aEnhancedPhoneBook) {
>      ril.appType = aSimType;
>      ril._isCdma = (aSimType === CARD_APPTYPE_RUIM);
>      ril.iccInfoPrivate.cst = (aEnhancedPhoneBook) ?
> +                                    [0x22, 0x0C, 0x0, 0x0, 0x0]:

I am wondering why you set 0x22 for the 1st byte of CDMA Service Table.

And I found one interesting thing in |isICCServiceAvailable| of ril_worker, for SIM/RUIM we didn't take the `second bit` into account, I think we need to correct that, maybe in a follow-up bug.

@@ +131,5 @@
>      ril._isCdma = (aSimType === CARD_APPTYPE_RUIM);
>      ril.iccInfoPrivate.cst = (aEnhancedPhoneBook) ?
> +                                    [0x22, 0x0C, 0x0, 0x0, 0x0]:
> +                                    [0x22, 0x00, 0x0, 0x0, 0x0];
> +    ril.iccInfoPrivate.sst =  [0x22, 0x0, 0x0, 0x0, 0x0];

Ditto, and I think we should have two different sst for SIM and USIM.

@@ +242,5 @@
>  
>    function do_test(aSimType, aContactType, aContact, aPin2, aFileType, aHaveIapIndex, aEnhancedPhoneBook) {
>      ril.appType = aSimType;
>      ril._isCdma = (aSimType === CARD_APPTYPE_RUIM);
> +    ril.iccInfoPrivate.cst = (aEnhancedPhoneBook) ? [0x22, 0x0C, 0x0, 0x0, 0x0]

Ditto

@@ +244,5 @@
>      ril.appType = aSimType;
>      ril._isCdma = (aSimType === CARD_APPTYPE_RUIM);
> +    ril.iccInfoPrivate.cst = (aEnhancedPhoneBook) ? [0x22, 0x0C, 0x0, 0x0, 0x0]
> +                                                  : [0x22, 0x00, 0x0, 0x0, 0x0];
> +    ril.iccInfoPrivate.sst =  [0x22, 0x0, 0x0, 0x0, 0x0];

Ditto
Attachment #8542480 - Flags: review?(echen)
(In reply to Edgar Chen [:edgar][:echen] from comment #3)
> Comment on attachment 8542480 [details] [diff] [review]
> Support access EF_FDN only if FDN service is available
> 
> Review of attachment 8542480 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my comments below. Thank you.
> 
> ::: dom/system/gonk/ril_consts.js
> @@ +1315,5 @@
> >      SPDI: 51
> >    },
> >    // @see 3GPP2 C.S0023-D 3.4.18 (RUIM).
> >    ruim: {
> > +  	FDN: 3,
> 
> nit: indention.
Will do.
> 
> ::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
> @@ +129,5 @@
> >    function do_test(aSimType, aContactType, aExpectedContact, aEnhancedPhoneBook) {
> >      ril.appType = aSimType;
> >      ril._isCdma = (aSimType === CARD_APPTYPE_RUIM);
> >      ril.iccInfoPrivate.cst = (aEnhancedPhoneBook) ?
> > +                                    [0x22, 0x0C, 0x0, 0x0, 0x0]:
> 
> I am wondering why you set 0x22 for the 1st byte of CDMA Service Table.
> 
It should set 0x20 for the 1st byte. I will modify it.
> And I found one interesting thing in |isICCServiceAvailable| of ril_worker,
> for SIM/RUIM we didn't take the `second bit` into account, I think we need
> to correct that, maybe in a follow-up bug.
I file a follow-up Bug 1116702 to trace this issue.
> 
> @@ +131,5 @@
> >      ril._isCdma = (aSimType === CARD_APPTYPE_RUIM);
> >      ril.iccInfoPrivate.cst = (aEnhancedPhoneBook) ?
> > +                                    [0x22, 0x0C, 0x0, 0x0, 0x0]:
> > +                                    [0x22, 0x00, 0x0, 0x0, 0x0];
> > +    ril.iccInfoPrivate.sst =  [0x22, 0x0, 0x0, 0x0, 0x0];
> 
> Ditto, and I think we should have two different sst for SIM and USIM.
Will do.
> 
> @@ +242,5 @@
> >  
> >    function do_test(aSimType, aContactType, aContact, aPin2, aFileType, aHaveIapIndex, aEnhancedPhoneBook) {
> >      ril.appType = aSimType;
> >      ril._isCdma = (aSimType === CARD_APPTYPE_RUIM);
> > +    ril.iccInfoPrivate.cst = (aEnhancedPhoneBook) ? [0x22, 0x0C, 0x0, 0x0, 0x0]
> 
> Ditto
Will do.
> 
> @@ +244,5 @@
> >      ril.appType = aSimType;
> >      ril._isCdma = (aSimType === CARD_APPTYPE_RUIM);
> > +    ril.iccInfoPrivate.cst = (aEnhancedPhoneBook) ? [0x22, 0x0C, 0x0, 0x0, 0x0]
> > +                                                  : [0x22, 0x00, 0x0, 0x0, 0x0];
> > +    ril.iccInfoPrivate.sst =  [0x22, 0x0, 0x0, 0x0, 0x0];
> 
> Ditto
Will do.
1. Revise indention.
2. Update the related testcase base on Comment 4.
Attachment #8542480 - Attachment is obsolete: true
Attachment #8542876 - Flags: review?(echen)
Comment on attachment 8542876 [details] [diff] [review]
Support access EF_FDN only if FDN service is available.(V2)

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

I guess you uploaded a wrong file. ;)
Attachment #8542876 - Flags: review?(echen)
It's my bad. Upload a correct patch.
Attachment #8542876 - Attachment is obsolete: true
Attachment #8542904 - Flags: review?(echen)
Comment on attachment 8542904 [details] [diff] [review]
Support access EF_FDN only if FDN service is available.(V3)

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

Looks good, please remember to add r=me in commit message, thank you.
Attachment #8542904 - Flags: review?(echen) → review+
https://hg.mozilla.org/mozilla-central/rev/355429828a63
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S3 (9jan)
Blocks: 1157082
You need to log in before you can comment on or make changes to this bug.