804667 - B2G STK: support TIMER MANAGEMENT and timer expiration

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Dietrich Ayala (:dietrich)]

Required for certification?

Comment 2

[Edgar Chen [:edgar]]

Hi, Michael

ETSI TS 102 223 V11.0.0,
6.4.21  TIMER MANAGEMENT,
"This command requests the terminal to manage timers running physically in the terminal."
"When a timer expires (i.e. reaches zero), the terminal shall use the Timer Expiration mechanism to transfer the identifier 
of the timer that has expired and the difference between the time when this transfer occurs and the time when the timer 
was initially started. The terminal shall then deactivate the timer."

In Android STK document, this proactive command is marked as "Implement by Baseband".
http://www.kandroid.org/online-pdk/guide/stk.html

Dose it mean this command will be handled by modem?
Do you have any information about this? :)

Thanks

Comment 3

[Edgar Chen [:edgar]]

Attachment: WIP Part 1: IDL For TIMER_MANAGEMENT, v1

Hi Yoshi:

Could you please return some feedback when you're available? Thanks! :)

Below is the summarizes about this patch:

1. Add new const related to TIMER_MANAGEMENT.

2. Add new structure, MozStkTimer, used in MozStkCommand / MozStkResponse / sendStkTimerExpiration().

3. Add new API for sending Envelope command "Timer Expiration".
   void sendStkTimerExpiration(in jsval timer)

Comment 4

[Edgar Chen [:edgar]]

Attachment: WIP Part 2: Support TIMER_MANAGEMENT in RIL, v1

Hi Yoshi:

Could you please return some feedback when you are available? Thank! :)

Below is the summarizes for this patch:

1. Add new const related to TIMER MANAGEMENT.

2. Add new IPC Command, RIL:SendStkTimerExpiration, for DOM API, sendStkTimerExpiration().

3. Process new field related to TIMER MANAGEMENT in Terminal Response and Envelope command.

4. This method, writeSwappedNibbleBCDNum, is the same as the one in patch of bug 804671. I will remove it, if the patch of bug 804671 is landed first.

Comment 5

[Edgar Chen [:edgar]]

Attachment: WIP Part 2: Support TIMER_MANAGEMENT in RIL, v2

Miss Terminal Profile part. (Marked as supported)

STK_TERMINAL_PROFILE_PROACTIVE_3 have also been defined in patch of bug 804671.
I will adjust code if the patch of bug 804671 is landed first.

Please also see comment #3 for the other summarizes.

Thanks.

Comment 6

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

Comment on attachment 687699 [details] [diff] [review]
WIP Part 2: Support TIMER_MANAGEMENT in RIL, v2

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

::: dom/system/gonk/ril_worker.js
@@ +7425,5 @@
> +   * |  3    | Timer Identifier     |   1    |
> +   */
> +  retrieveTimerId: function retrieveTimerId(length) {
> +    let id = {
> +      id: GsmPDUHelper.readHexOctet()

timerId

@@ +7442,5 @@
> +   * |  5    | Second               |   1    |
> +   */
> +  retrieveTimerValue: function retrieveTimerValue(length) {
> +    let value = {
> +      value: (GsmPDUHelper.readSwappedNibbleBcdNum(1) * 60 * 60) +

timerValue

@@ +7444,5 @@
> +  retrieveTimerValue: function retrieveTimerValue(length) {
> +    let value = {
> +      value: (GsmPDUHelper.readSwappedNibbleBcdNum(1) * 60 * 60) +
> +             (GsmPDUHelper.readSwappedNibbleBcdNum(1) * 60) +
> +             GsmPDUHelper.readSwappedNibbleBcdNum(1)

nit, align this line

Comment 7

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

Comment on attachment 687664 [details] [diff] [review]
WIP Part 1: IDL For TIMER_MANAGEMENT, v1

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

Please split IccManager.cpp to other part of patch, it should be reviewed by DOM-peer.

::: dom/icc/interfaces/SimToolKit.idl
@@ +347,5 @@
> +{
> +  /**
> +   * Identifier of a timer.
> +   */
> +  unsigned short id;

For id, it's only 1 byte, maybe you could use 'octet' here.
Also I suggest timerId, because this value is accessed through 'options' in MozStkCommand, and developers might not know what does id mean.

@@ +352,5 @@
> +
> +  /**
> +   * Length of time during in seconds
> +   */
> +  unsigned long value;

Since it's a Web-API, can we make this member from a more high level pointer of view ? i.e. hour, minutes, seconds.

Also using 'timerValue' for naming.

@@ +477,5 @@
>    boolean hasConfirmed;
> +
> +  /**
> +   * The response for TIMER MANAGEMENT
> +   *

Can you add more documentation here, for example, when the command STK_CMD_TIMER_MANAGEMENT .....

Comment 8

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL For TIMER_MANAGEMENT, v2

address review comment #7

Comment 9

[Edgar Chen [:edgar]]

Attachment: Part 2: modify IccManager for TIME_MANAGEMENT, v1

add IccManager::SendStkTimerExpiration() in IccManager.cpp for new DOM API, sendStkTimerExpiration()

Comment 10

[Edgar Chen [:edgar]]

Attachment: Part 3: Support TIMER_MANAGEMENT in RIL, v3

1). rebase based on patch part2 of bug 804671.
2). address review comment #6

Comment 11

[Edgar Chen [:edgar]]

Attachment: Part 4: xpcshell tests for TIMER_MANAGEMENT, v1

xpcshell tests

Comment 12

[Edgar Chen [:edgar]]

Attachment: Part 5: marionette tests for TIMER_MANAGEMENT, v1

marionette tests

Comment 13

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

Comment on attachment 689614 [details] [diff] [review]
Part 1: IDL For TIMER_MANAGEMENT, v2

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

::: dom/icc/interfaces/SimToolKit.idl
@@ +363,5 @@
> +   */
> +  octet timerId;
> +
> +  /**
> +   * Length of time during in seconds

grammar seems wrong here.

@@ +365,5 @@
> +
> +  /**
> +   * Length of time during in seconds
> +   */
> +  unsigned long timeDuringSeconds;

The naming seems not so good here,
do you think we could also use the term 'timerValue' from spec?

Also maybe in comments we could also reuse from spec
"length of time during which the timer has to run. "

and specify the timer resolution is 1 sec.

@@ +368,5 @@
> +   */
> +  unsigned long timeDuringSeconds;
> +
> +  /**
> +   * The action request from UICC.

requested

@@ +525,5 @@
> +   *    the ME shall response the current value of the timer to the UICC.
> +   *
> +   * @see MozStkTimer
> +   */
> +  jsval timer;

Here is inside MozStkRespose, right?

But seems the comments is talking about proactive command.
Or I don't quite understand what those comments mean here.

::: dom/icc/interfaces/nsIDOMIccManager.idl
@@ +240,5 @@
> +   * Send the "Timer Expiration" Envelope command to ICC for TIMER MANAGEMENT.
> +  `*
> +   * @param timer
> +   *        The identifier and value for a timer
> +   *        - MozStkTimer

From spec it also mentions 
"Timer value: difference between the time when this command is issued and the time when the timer was initially started."

We should also mention that in comments.

Comment 14

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL For TIMER_MANAGEMENT, v3

Address review comment #13.
1). Add more explanation in comment.
2). Change naming. (timeDuringSeconds -> timerValue)

Comment 15

[Edgar Chen [:edgar]]

Attachment: Part 3: Support TIMER_MANAGEMENT in RIL, v4

Comment 16

[Edgar Chen [:edgar]]

Attachment: Part 3: Support TIMER_MANAGEMENT in RIL, v5

fix some bugs.

Comment 17

[Edgar Chen [:edgar]]

Attachment: Part 5: marionette tests for TIMER_MANAGEMENT, v2

Comment 18

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

Comment on attachment 690299 [details] [diff] [review]
Part 1: IDL For TIMER_MANAGEMENT, v3

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

::: dom/icc/interfaces/SimToolKit.idl
@@ +511,5 @@
>    jsval localInfo;
> +
> +  /**
> +   * The response for STK_CMD_TIMER_MANAGEMENT.
> +   * the 'timerValue' is needed if the action of STK_CMD_TIMER_MANAGEMENT is

The

::: dom/icc/interfaces/nsIDOMIccManager.idl
@@ +240,5 @@
> +   * Send the "Timer Expiration" Envelope command to ICC for TIMER MANAGEMENT.
> +  `*
> +   * @param timer
> +   *        The identifier and value for a timer.
> +   *        timerId: identifier of the timer that has expired.

Identifier

@@ +241,5 @@
> +  `*
> +   * @param timer
> +   *        The identifier and value for a timer.
> +   *        timerId: identifier of the timer that has expired.
> +   *        timerValue: different between the time when this command is issued

Difference

@@ +242,5 @@
> +   * @param timer
> +   *        The identifier and value for a timer.
> +   *        timerId: identifier of the timer that has expired.
> +   *        timerValue: different between the time when this command is issued
> +                         and the time when the timer was initially started.

I think we could remove 'the time'

@@ +243,5 @@
> +   *        The identifier and value for a timer.
> +   *        timerId: identifier of the timer that has expired.
> +   *        timerValue: different between the time when this command is issued
> +                         and the time when the timer was initially started.
> +   *        - MozStkTimer

@see MozStkTimer
As in SimToolKit.idl you already did it this way.

Comment 19

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

Comment on attachment 689617 [details] [diff] [review]
Part 2: modify IccManager for TIME_MANAGEMENT, v1

This should be reviewed by DOM peer.

smaug, can you help to review this patch for Edgar?

Thank you.

Comment 20

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

Comment on attachment 690315 [details] [diff] [review]
Part 3: Support TIMER_MANAGEMENT in RIL, v5

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

::: dom/system/gonk/ril_worker.js
@@ +3178,5 @@
> +      let timer = response.timer;
> +      size = size +
> +             ((timer.timerId ? TLV_TIMER_IDENTIFIER : 0) +
> +              (timer.timerValue ? TLV_TIMER_VALUE : 0)) * 2;
> +    }

Maybe we could do as sendICCEnvelopeCommand,
so we don't have to do '* 2' each time when we add a new TLV.

@@ +8938,5 @@
>        PDU_NL_LOCKING_SHIFT_TABLES[PDU_NL_IDENTIFIER_DEFAULT].indexOf(language[1]));
>    },
>  
> +  /**
> +   * Write Location Info Comprehension TLV.

comments should be wrong.

@@ +8949,5 @@
> +      GsmPDUHelper.writeHexOctet(COMPREHENSIONTLV_TAG_TIMER_VALUE |
> +                                 COMPREHENSIONTLV_FLAG_CR);
> +    } else {
> +      GsmPDUHelper.writeHexOctet(COMPREHENSIONTLV_TAG_TIMER_VALUE);
> +    }

GsmPDUHelper.writeHexOctet(COMPREHENSIONTLV_TAG_TIMER_VALUE |
                           (cr ? COMPREHENSIONTLV_FLAG_CR : 0))

Comment 21

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

Comment on attachment 690318 [details] [diff] [review]
Part 5: marionette tests for TIMER_MANAGEMENT, v2

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

Remember to rebase your patch
As Bug 816835 also updates the test script.

Comment 22

[Edgar Chen [:edgar]]

Attachment: Part 1: IDL For TIMER_MANAGEMENT, v3.1

Address review comment #18.

Comment 23

[Edgar Chen [:edgar]]

Attachment: Part 3: Support TIMER_MANAGEMENT in RIL, v5.1, r=allstars.chh

Address review comment #20

Comment 24

[Edgar Chen [:edgar]]

Attachment: Part 5: marionette tests for TIMER_MANAGEMENT, v3, r=allstars.chh

rebase only

Comment 25

[Edgar Chen [:edgar]]

Attachment: Part 2: modify IccManager for TIME_MANAGEMENT, v1, r=bugs

Correct reviewer in title after review+.

Comment 26

[Edgar Chen [:edgar]]

Attachment: Part 4: xpcshell tests for TIMER_MANAGEMENT, v2, r=allstars.chh

rebase

Comment 27

[Edgar Chen [:edgar]]

Attachment: Part 5: marionette tests for TIMER_MANAGEMENT, v4, r=allstars.chh

rebase

Comment 28

[Edgar Chen [:edgar]]

try server: https://tbpl.mozilla.org/?tree=Try&rev=010843010a34

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85508731e988
https://hg.mozilla.org/integration/mozilla-inbound/rev/16ecb1d51f68
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf02a9f81b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c32d392d4f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/2992fbea1370

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/85508731e988
https://hg.mozilla.org/mozilla-central/rev/16ecb1d51f68
https://hg.mozilla.org/mozilla-central/rev/bcf02a9f81b1
https://hg.mozilla.org/mozilla-central/rev/5c32d392d4f9
https://hg.mozilla.org/mozilla-central/rev/2992fbea1370

Comment 31

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

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #20)
> Comment on attachment 690315 [details] [diff] [review]
> Part 3: Support TIMER_MANAGEMENT in RIL, v5
> 
> Review of attachment 690315 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +3178,5 @@
> > +      let timer = response.timer;
> > +      size = size +
> > +             ((timer.timerId ? TLV_TIMER_IDENTIFIER : 0) +
> > +              (timer.timerValue ? TLV_TIMER_VALUE : 0)) * 2;
> > +    }
> 
> Maybe we could do as sendICCEnvelopeCommand,
> so we don't have to do '* 2' each time when we add a new TLV.
> 
Hi Edgar
I just realized you didn't address this comment, 
Did you just forget or you have different opinion on this?

Comment 32

[Edgar Chen [:edgar]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #31)
> (In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #20)
> > Comment on attachment 690315 [details] [diff] [review]
> > Part 3: Support TIMER_MANAGEMENT in RIL, v5
> > 
> > Review of attachment 690315 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/ril_worker.js
> > @@ +3178,5 @@
> > > +      let timer = response.timer;
> > > +      size = size +
> > > +             ((timer.timerId ? TLV_TIMER_IDENTIFIER : 0) +
> > > +              (timer.timerValue ? TLV_TIMER_VALUE : 0)) * 2;
> > > +    }
> > 
> > Maybe we could do as sendICCEnvelopeCommand,
> > so we don't have to do '* 2' each time when we add a new TLV.
> > 
> Hi Edgar
> I just realized you didn't address this comment, 
> Did you just forget or you have different opinion on this?

Hi Yoshi,
Oops! I missed this comment.
I will open a bug to address this comment.
Thanks for your reminding.

Comment 33

[Edgar Chen [:edgar]]

> Hi Yoshi,
> Oops! I missed this comment.
> I will open a bug to address this comment.
> Thanks for your reminding.

Create bug 824194 for this. Thanks

Comment 34

[Edgar Chen [:edgar]]

Attachment: (b2g18) Part 3: Support TIMER_MANAGEMENT in RIL, v5.1, r=allstars.chh

Patch for mozilla-b2g18

Comment 35

[Edgar Chen [:edgar]]

Attachment: (b2g18) Part 5: marionette tests for TIMER_MANAGEMENT, v4, r=allstars.chh

Patch for mozilla-b2g18

Comment 36

[Edgar Chen [:edgar]]

Please merge bug 804671 into b2g18 first then this, or may have conflict.

And for this bug, please use following patches for b2g18.
- Part 1: IDL For TIMER_MANAGEMENT, v3.1
- Part 2: modify IccManager for TIME_MANAGEMENT, v1, r=bugs
- (b2g18) Part 3: Support TIMER_MANAGEMENT in RIL, v5.1, r=allstars.chh
- Part 4: xpcshell tests for TIMER_MANAGEMENT, v2, r=allstars.chh
- (b2g18) Part 5: marionette tests for TIMER_MANAGEMENT, v4, r=allstars.chh

Thanks

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/26f1698c9ef7
https://hg.mozilla.org/releases/mozilla-b2g18/rev/76036656de4e
https://hg.mozilla.org/releases/mozilla-b2g18/rev/09f281d392a8
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e7182d750b7e
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5c50b5891886