804671 - B2G STK: support PROVIDE LOCAL INFORMATION

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Edgar Chen [:edgar]]

PROVIDE LOCAL INFORMATION is session 6.4.15 in TS 101.267 / TS 11.14

Comment 2

[Edgar Chen [:edgar]]

Attachment: WIP Part 1: IDL For PROVIDE_LOCAL_INFO, v1

implement below item first
1.) location info
2.) date-time and time-zone

Comment 3

[Edgar Chen [:edgar]]

Attachment: WIP Part2: Support PROVIDE_LOCAL_INFO in RIL, v1

Implement below item first:
1). IMEI
2). Location Info
3). Date-Time and Time-Zone

Comment 4

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL For PROVIDE_LOCAL_INFO, v2

1). Combine all response item for PROVIDE_LOCAL_INFO into MozStkLocalInfo.

Comment 5

[Edgar Chen [:edgar]]

Attachment: Part2: Support PROVIDE_LOCAL_INFO in RIL, v2

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.

Comment 6

[Edgar Chen [:edgar]]

Attachment: Part 3: xpcshell tests for PROVIDE_LOCAL_INFORMATION, v1

Comment 7

[Edgar Chen [:edgar]]

Attachment: Part 3: xpcshell tests for PROVIDE_LOCAL_INFORMATION, v1

correct typo in comment

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 10

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL For PROVIDE_LOCAL_INFO, v3

1). add STK_LOCAL_INFO_IMEI.
1). add imei in MozStkLocalInfo.

Comment 11

[Edgar Chen [:edgar]]

Attachment: Part 2: Support PROVIDE_LOCAL_INFO in RIL, v3

Comment 12

[Edgar Chen [:edgar]]

(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.

Comment 13

[Edgar Chen [:edgar]]

Attachment: Part 3: xpcshell tests for PROVIDE_LOCAL_INFO, v2

Address review comment #9. Thanks for the review, Yoshi.

Comment 14

[Edgar Chen [:edgar]]

Attachment: Part 2: Support PROVIDE_LOCAL_INFO in RIL, v3

rebase

Comment 15

[Edgar Chen [:edgar]]

Attachment: Part 3: xpcshell tests for PROVIDE_LOCAL_INFO, v2

rebase

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 17

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL For PROVIDE_LOCAL_INFO, v4

1). update comments

Comment 18

[Edgar Chen [:edgar]]

Attachment: Part 2: Support PROVIDE_LOCAL_INFO in RIL, v4

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.

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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 ?

Comment 20

[Edgar Chen [:edgar]]

(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 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 23

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL For PROVIDE_LOCAL_INFO, v5

1). add MozStkProvideLocalInfo to make a more high-level design.
2). Address review comment #22

Thanks for the review, Yoshi! :)

Comment 24

[Edgar Chen [:edgar]]

Attachment: Part 2: Support PROVIDE_LOCAL_INFO in RIL, v5

Changes according to comment #23

Comment 25

[Edgar Chen [:edgar]]

Attachment: Part 4: marionette tests for PROVIDE_LOCAL_INDO, v1

marionette tests for PROVIDE_LOCAL_INFO

Comment 26

[Edgar Chen [:edgar]]

Attachment: Part 2: Support PROVIDE_LOCAL_INFO in RIL, v5.1

rebase

Comment 27

[Edgar Chen [:edgar]]

Attachment: Part 4: marionette tests for PROVIDE_LOCAL_INDO, v2

re-write marionette tests based on patch part3 of bug 814618.

Comment 28

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

Comment 29

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 30

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 31

[Edgar Chen [:edgar]]

Attachment: Part 2: Support PROVIDE_LOCAL_INFO in RIL, v5.2, r=allstars.chh

correct indent style. (comment #29)

Comment 32

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL For PROVIDE_LOCAL_INFO, v5.1, r=allstars.chh, sr=jonas

add reviewer and super-reviewer into title after review+ and superreview+

Comment 33

[Edgar Chen [:edgar]]

try server: https://tbpl.mozilla.org/?tree=Try&rev=54899d804adf

Comment 34

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ffb3bd63f71
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bd9ff852c5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7169b3dfe53a
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5e4ea3327df

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ffb3bd63f71
https://hg.mozilla.org/mozilla-central/rev/0bd9ff852c5e
https://hg.mozilla.org/mozilla-central/rev/7169b3dfe53a
https://hg.mozilla.org/mozilla-central/rev/e5e4ea3327df

Comment 36

[Edgar Chen [:edgar]]

Request to shira+ as this feature is required for v1.0.1

Comment 37

[Ryan VanderMeulen [:RyanVM]]

This doesn't apply cleanly to b2g18 (I made it to part 2 before I gave up).

Comment 38

[Edgar Chen [:edgar]]

Hi Rayn, I will rebase these patches for b2g18. Thanks.

Comment 39

[Edgar Chen [:edgar]]

Attachment: (b2g18) Part 1: IDL For PROVIDE_LOCAL_INFO, v5.1, r=allstars.chh, sr=jonas

Patch for mozilla-b2g18

Comment 40

[Edgar Chen [:edgar]]

Attachment: (b2g18) Part 2: Support PROVIDE_LOCAL_INFO in RIL, v5.2, r=allstars.chh

Patch for mozilla-b2g18

Comment 41

[Edgar Chen [:edgar]]

Attachment: (b2g18) Part 3: xpcshell tests for PROVIDE_LOCAL_INFO, v2, r=allstars.chh

Comment 42

[Edgar Chen [:edgar]]

Attachment: (b2g18) Part 4: marionette tests for PROVIDE_LOCAL_INDO, v2, r=allstars.chh

Patch for mozilla-b2g18

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/62c030520097
https://hg.mozilla.org/releases/mozilla-b2g18/rev/83a300618d60
https://hg.mozilla.org/releases/mozilla-b2g18/rev/8ad5fbebbc19
https://hg.mozilla.org/releases/mozilla-b2g18/rev/aacd5474fe8b