B2G STK: support PROVIDE LOCAL INFORMATION

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: philikon, Assigned: edgar)

Tracking

unspecified
mozilla20
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:shira+, firefox19 wontfix, firefox20 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

Attachments

(8 attachments, 16 obsolete attachments)

4.92 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
2.64 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
18.89 KB, patch
Details | Diff | Splinter Review
4.81 KB, patch
Details | Diff | Splinter Review
4.87 KB, patch
Details | Diff | Splinter Review
14.25 KB, patch
Details | Diff | Splinter Review
4.84 KB, patch
Details | Diff | Splinter Review
3.04 KB, patch
Details | Diff | Splinter Review
No description provided.
Blocks: 804679
PROVIDE LOCAL INFORMATION is session 6.4.15 in TS 101.267 / TS 11.14
Assignee: nobody → echen
implement below item first
1.) location info
2.) date-time and time-zone
Implement below item first:
1). IMEI
2). Location Info
3). Date-Time and Time-Zone
1). Combine all response item for PROVIDE_LOCAL_INFO into MozStkLocalInfo.
Attachment #684637 - Attachment is obsolete: true
1). Combine all response item for PROVIDE_LOCAL_INFO into MozStkLocalInfo.
2). add function writeTimestamp() in GsmPDUHelper.
3). add function writeLanguageTlv() in StkProactiveCmdHelper.
4). remove unnecessary code and comment.
Attachment #684639 - Attachment is obsolete: true
correct typo in comment
Attachment #685509 - Attachment is obsolete: true
Comment on attachment 685506 [details] [diff] [review]
Part2: Support PROVIDE_LOCAL_INFO in RIL, v2

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

::: dom/system/gonk/ril_worker.js
@@ +2789,5 @@
> +
> +        GsmPDUHelper.writeHexOctet(COMPREHENSIONTLV_TAG_IMEI);
> +        GsmPDUHelper.writeHexOctet(8);
> +        for (let i = 0; i < imei.length/2; i++) {
> +          GsmPDUHelper.writeHexOctet(parseInt(imei.substr(i*2, 2), 16));

space between operators.

@@ +5360,5 @@
>  
>    /**
> +   * Convert a BCD number to an octet (number)
> +   *
> +   * Only take two digit with absolute value.

*digits*

@@ +6313,5 @@
> +    let min = date.getMinutes();
> +    let sec = date.getSeconds();
> +    // The Time Zone indicates the different between the local time and GMT.
> +    // Expressed in quarters of an hours.
> +    let zone = date.getTimezoneOffset()/15;

nit, space between operators.

@@ +6317,5 @@
> +    let zone = date.getTimezoneOffset()/15;
> +
> +    let octet = this.BCDToOctet(zone);
> +
> +    // If the time zone is -0800 GMT, 480 will be returned by date.getTimezoneOffset().

Could you add more comments to make the code easier to read?
For example, what's the value returned from date.getTimezoneOffset() ?
what's the definition of the timezone in spec,
why you OR 0x08 when zone > 0?

@@ +6328,5 @@
> +    this.writeSwappedNibbleBCDString(mon);
> +    this.writeSwappedNibbleBCDString(day);
> +    this.writeSwappedNibbleBCDString(hour);
> +    this.writeSwappedNibbleBCDString(min);
> +    this.writeSwappedNibbleBCDString(sec);

Do you think it's better to write those values directly without introducing another variables?
for example,
this.writeSwappedNibbleBCDString(date.getFullYear() - PDU_TIMESTAMP_YEAR_OFFSET);

@@ +7151,5 @@
> +    switch (cmdDetails.commandQualifier) {
> +      case STK_LOCAL_INFO_IMEI:
> +        response = {command: cmdDetails};
> +        if(RIL.IMEI == null) {
> +          response.resultCode = STK_RESULT_REQUIRED_VALUES_MISSING;

should not be STK_RESULT_REQUIRED_VALUES_MISSING here.

@@ +7697,5 @@
> +      GsmPDUHelper.writeHexOctet(2);
> +
> +      // ISO 639-1, Alpha-2 code
> +      // TS 123.038, clause 6.2.1, GSM 7 bit Default Alphabet
> +      let code1 = PDU_NL_LOCKING_SHIFT_TABLES[0].indexOf(language[0]);

PDU_NL_LOCKING_SHIFT_TABLES[PDU_NL_IDENTIFIER_DEFAULT]

@@ +7701,5 @@
> +      let code1 = PDU_NL_LOCKING_SHIFT_TABLES[0].indexOf(language[0]);
> +      let code2 = PDU_NL_LOCKING_SHIFT_TABLES[0].indexOf(language[1]);
> +
> +      GsmPDUHelper.writeHexOctet(code1);
> +      GsmPDUHelper.writeHexOctet(code2);

Again, do you think it's easier writing these octets directly, without adding 'code1' and 'code2' ?
Comment on attachment 685512 [details] [diff] [review]
Part 3: xpcshell tests for PROVIDE_LOCAL_INFORMATION, v1

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +129,5 @@
> +  let helper = worker.GsmPDUHelper;
> +
> +  // current date
> +  let date_input = new Date();
> +  let date_output = new Date();

use camelCase.

@@ +143,5 @@
> +  do_check_eq(date_input.getTimezoneOffset(), date_output.getTimezoneOffset());
> +
> +  // 2034-01-23 12:34:56 -0800 GMT
> +  let time = Date.UTC(2034, 1, 23, 12, 34, 56);
> +  time = time - 8*60*60*1000;

nit, space.

@@ +578,5 @@
> +
> +  berTlv = berHelper.decode(local_info_2.length);
> +  ctlvs = berTlv.value;
> +  tlv = stkHelper.searchForTag(COMPREHENSIONTLV_TAG_COMMAND_DETAILS, ctlvs);
> +  do_check_eq(tlv.value.commandNumber, 0x02);

Command Number is a feature reserved for future use. Currently all STK implementation use 1 for commandNumber. We should do the same.
1). add STK_LOCAL_INFO_IMEI.
1). add imei in MozStkLocalInfo.
Attachment #685505 - Attachment is obsolete: true
(In reply to Edgar Chen [:edgar] from comment #11)
> Created attachment 685940 [details] [diff] [review]
> Part 2: Support PROVIDE_LOCAL_INFO in RIL, v3

Address review comment #8. Thanks for the review, Yoshi.
Address review comment #9. Thanks for the review, Yoshi.
Attachment #685506 - Attachment is obsolete: true
Attachment #685512 - Attachment is obsolete: true
rebase
Attachment #685940 - Attachment is obsolete: true
Attachment #686400 - Flags: review?(allstars.chh)
rebase
Attachment #685948 - Attachment is obsolete: true
Attachment #686403 - Flags: review?(allstars.chh)
Attachment #685939 - Flags: superreview?(jonas)
Attachment #685939 - Flags: review?(allstars.chh)
Comment on attachment 685939 [details] [diff] [review]
Part 1: IDL For PROVIDE_LOCAL_INFO, v3

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

comments should be updated.
Attachment #685939 - Flags: superreview?(jonas)
Attachment #685939 - Flags: review?(allstars.chh)
1). update comments
Attachment #685939 - Attachment is obsolete: true
1). Add define for 8th byte and 9th byte of Terminal Profile. Some bit is used for PROVIDE_LOCAL_INFO.
2). TLV size need to be multiplied by 2.
3). Adjust function name.
Attachment #686400 - Attachment is obsolete: true
Comment on attachment 686486 [details] [diff] [review]
Part 1: IDL For PROVIDE_LOCAL_INFO, v4

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

::: dom/icc/interfaces/nsIDOMIccManager.idl
@@ +202,5 @@
> +   */
> +  const unsigned short STK_LOCAL_INFO_LOCATION_INFO  = 0x00;
> +  const unsigned short STK_LOCAL_INFO_IMEI           = 0x01;
> +  const unsigned short STK_LOCAL_INFO_DATE_TIME_ZONE = 0x03;
> +  const unsigned short STK_LOCAL_INFO_LANGUAGE       = 0x04;

When will these values be used ?
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #19)
> Comment on attachment 686486 [details] [diff] [review]
> Part 1: IDL For PROVIDE_LOCAL_INFO, v4
> 
> Review of attachment 686486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/icc/interfaces/nsIDOMIccManager.idl
> @@ +202,5 @@
> > +   */
> > +  const unsigned short STK_LOCAL_INFO_LOCATION_INFO  = 0x00;
> > +  const unsigned short STK_LOCAL_INFO_IMEI           = 0x01;
> > +  const unsigned short STK_LOCAL_INFO_DATE_TIME_ZONE = 0x03;
> > +  const unsigned short STK_LOCAL_INFO_LANGUAGE       = 0x04;
> 
> When will these values be used ?

These values are used for indicating which information is required.
Defined in "Qualifier" field of Command Detailed. (TS 102.223 session 8.6)
Thanks! :)
Comment on attachment 686403 [details] [diff] [review]
Part 3: xpcshell tests for PROVIDE_LOCAL_INFO, v2

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

This patch looks good to me, but I'd like to review this with implementation.
Attachment #686403 - Flags: review?(allstars.chh)
Comment on attachment 686486 [details] [diff] [review]
Part 1: IDL For PROVIDE_LOCAL_INFO, v4

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

::: dom/icc/interfaces/SimToolKit.idl
@@ +458,5 @@
>    boolean hasConfirmed;
> +
> +  /**
> +   * The response for proactive command PROVIDE_LOCAL_INFORMATION
> +   *

you can refer to 'STK_CMD_PROVIDE_LOCAL_INFO' directly
1). add MozStkProvideLocalInfo to make a more high-level design.
2). Address review comment #22

Thanks for the review, Yoshi! :)
Attachment #686486 - Attachment is obsolete: true
Changes according to comment #23
Attachment #686985 - Attachment is obsolete: true
marionette tests for PROVIDE_LOCAL_INFO
rebase
Attachment #688172 - Attachment is obsolete: true
re-write marionette tests based on patch part3 of bug 814618.
Attachment #689552 - Attachment is obsolete: true
Attachment #688167 - Flags: review?(allstars.chh)
Attachment #689606 - Flags: review?(allstars.chh)
Attachment #686403 - Flags: review?(allstars.chh)
Comment on attachment 688167 [details] [diff] [review]
Part 1: IDL For PROVIDE_LOCAL_INFO, v5

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

Good! thanks

You also need to request for sr? for IDL change
Attachment #688167 - Flags: review?(allstars.chh) → review+
Attachment #688167 - Flags: superreview?(jonas)
Comment on attachment 689606 [details] [diff] [review]
Part 2: Support PROVIDE_LOCAL_INFO in RIL, v5.1

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

::: dom/system/gonk/ril_worker.js
@@ +8808,5 @@
> +    GsmPDUHelper.writeTimestamp(date);
> +  },
> +
> +  writeLanguageTlv: function writeLanguageTlv(language) {
> +      GsmPDUHelper.writeHexOctet(COMPREHENSIONTLV_TAG_LANGUAGE);

indent seems wrong here. should be only 2 ws.
Attachment #689606 - Flags: review?(allstars.chh) → review+
Attachment #686403 - Flags: review?(allstars.chh) → review+
Attachment #689607 - Flags: review?(allstars.chh)
Comment on attachment 689607 [details] [diff] [review]
Part 4: marionette tests for PROVIDE_LOCAL_INDO, v2

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

Thanks for your hard work.
Attachment #689607 - Flags: review?(allstars.chh) → review+
Attachment #688167 - Flags: superreview?(jonas) → superreview+
add reviewer and super-reviewer into title after review+ and superreview+
Attachment #688167 - Attachment is obsolete: true
Request to shira+ as this feature is required for v1.0.1
blocking-b2g: --- → shira?
blocking-b2g: shira? → shira+
This doesn't apply cleanly to b2g18 (I made it to part 2 before I gave up).
Hi Rayn, I will rebase these patches for b2g18. Thanks.
You need to log in before you can comment on or make changes to this bug.