Closed Bug 804667 Opened 7 years ago Closed 7 years ago

B2G STK: support TIMER MANAGEMENT and timer expiration

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
blocking-b2g shira+
Tracking Status
firefox19 --- wontfix
firefox20 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: philikon, Assigned: edgar)

References

Details

Attachments

(7 files, 13 obsolete files)

5.38 KB, patch
sicking
: superreview+
Details | Diff | Splinter Review
21.45 KB, patch
Details | Diff | Splinter Review
1018 bytes, patch
Details | Diff | Splinter Review
2.98 KB, patch
Details | Diff | Splinter Review
3.00 KB, patch
Details | Diff | Splinter Review
19.51 KB, patch
Details | Diff | Splinter Review
3.02 KB, patch
Details | Diff | Splinter Review
No description provided.
Blocks: 804679
Summary: B2G STK: support TIMER MANAGEMENT → B2G STK: support TIMER MANAGEMENT and timer expiration
Required for certification?
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
Assignee: nobody → echen
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)
Attachment #687664 - Flags: feedback?(allstars.chh)
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.
Attachment #687674 - Flags: feedback?(allstars.chh)
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.
Attachment #687674 - Attachment is obsolete: true
Attachment #687674 - Flags: feedback?(allstars.chh)
Attachment #687699 - Flags: feedback?(allstars.chh)
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
Attachment #687699 - Flags: feedback?(allstars.chh)
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 .....
Attachment #687664 - Flags: feedback?(allstars.chh)
Depends on: 804671
address review comment #7
Attachment #687664 - Attachment is obsolete: true
Attachment #689614 - Flags: feedback?(allstars.chh)
add IccManager::SendStkTimerExpiration() in IccManager.cpp for new DOM API, sendStkTimerExpiration()
Attachment #687699 - Attachment is obsolete: true
1). rebase based on patch part2 of bug 804671.
2). address review comment #6
xpcshell tests
marionette tests
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.
Attachment #689614 - Flags: feedback?(allstars.chh)
Address review comment #13.
1). Add more explanation in comment.
2). Change naming. (timeDuringSeconds -> timerValue)
Attachment #689614 - Attachment is obsolete: true
Attachment #689619 - Attachment is obsolete: true
fix some bugs.
Attachment #690300 - Attachment is obsolete: true
Attachment #689627 - Attachment is obsolete: true
Attachment #690299 - Flags: review?(allstars.chh)
Attachment #689617 - Flags: review?(allstars.chh)
Attachment #690315 - Flags: review?(allstars.chh)
Attachment #689621 - Flags: review?(allstars.chh)
Attachment #690318 - Flags: review?(allstars.chh)
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.
Attachment #690299 - Flags: review?(allstars.chh) → review+
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.
Attachment #689617 - Flags: review?(allstars.chh) → review?(bugs)
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))
Attachment #690315 - Flags: review?(allstars.chh) → review+
Attachment #689621 - Flags: review?(allstars.chh) → review+
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.
Attachment #690318 - Flags: review?(allstars.chh) → review+
Address review comment #18.
Attachment #690299 - Attachment is obsolete: true
Attachment #690765 - Flags: superreview?(jonas)
Address review comment #20
Attachment #690315 - Attachment is obsolete: true
Attachment #689617 - Flags: review?(bugs) → review+
Attachment #690765 - Flags: superreview?(jonas) → superreview+
rebase only
Attachment #690318 - Attachment is obsolete: true
Correct reviewer in title after review+.
Attachment #689617 - Attachment is obsolete: true
rebase
Attachment #689621 - Attachment is obsolete: true
Keywords: checkin-needed
(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?
(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.
> 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
blocking-b2g: --- → shira+
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
You need to log in before you can comment on or make changes to this bug.