Closed Bug 736706 Opened 8 years ago Closed 7 years ago

B2G SMS: Support Message Classes

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

(Keywords: feature, Whiteboard: [LOE:S])

Attachments

(5 files, 19 obsolete files)

6.93 KB, patch
Details | Diff | Splinter Review
2.80 KB, patch
Details | Diff | Splinter Review
3.09 KB, patch
philikon
: review+
Details | Diff | Splinter Review
18.25 KB, patch
Details | Diff | Splinter Review
13.03 KB, patch
Details | Diff | Splinter Review
See 3GPP 23.040 section 9.2.3.9, when DCS set to message class 2, protocol id set to 0x7F, "the ME must pass the short message in its entirety including all SMS elements contained in the SMS deliver to the (U)SIM using the mechanism described in GSM TS 51.011 and 3GPP TS 31.102."
Blocks: b2g-sms
Depends on: 736941
Assignee: nobody → vyang
About message classes:

1) default: unknown, not class-0.

2) class-0:
The message shall not be automatically stored in the (U)SIM or ME, and the MS  shall send an ack irrespective of whether there is memory available in the (U)SIM or ME.

3) class-1:
The MS shall store the message in the ME by default, but may also store store it in the (U)SIM. The MS shall send an ack to the SC when the message has successfully reached the MS and can be stored.

4) class-2:
The MS shall ensure that the message has been transferred to (U)SIM before sending an ack. The MS shall also return an error if something wrong. This behavior applies in all cases except for an MS supporting TP-PID set to "(U)SIM Data download"(0x7F).

5) class-3:
The MS shall send a ack to the SC when the message has successfully reached the MS and can be stored, irrespective of whether the MS supports an SMS interface to a TE, and without waiting for the message to be transferred to the TE.
Depends on: 739143
Attached patch Part 2: Handle Message Class 1 (obsolete) — Splinter Review
About TP-PID related settings:

1) (U)SIM Data Download(0x7F):
* The DCS shall be set to Message Class 2, or fallback to normal processing;
* The ME must pass the short message in its entirety including all SMS elements contained in the SMS deliver to the (U)SIM using the mechanism described in GSM TS 51.011 and 3GPP TS 31.102.

2) ME Data Download(0x7D):
* The DCS should normally be set to message class 1;
* May discard the short message if no application in the ME exists.

3) ANSI-136 R-DATA (0x7C):
* The DCS shall be set to message class 2, or fallback to normal processing;
* using GSM TS 11.14 and 3GPP 31.102
Depends on: 736707
Depends on bug 736707 to include definitions of TP-PID values.
Attachment #609205 - Attachment is obsolete: true
Attachment #609278 - Attachment is obsolete: true
Attachment #610463 - Attachment description: Part 3: Handle Message Class 3 : WIP → Part 3: Handle Message Class 2 : WIP
Attachment #610511 - Attachment description: Bug 736706 - Part 3: add Buf.copyIncomingToOutgoing() : WIP → Part 3: add Buf.copyIncomingToOutgoing() : WIP
Attachment #610463 - Attachment is obsolete: true
Remove blocking flag for b2g-sms for milestone 3 temporarily.
No longer blocks: b2g-sms
Rename issue summary to support SMS Message Class 1 & 2 only. For ANSI-136 R-DATA, "the ME must pass the short message in its entirely, including all elements contained in the SMS DELIVER"; for (U)SIM Data Download, "the entire user data field is available".
Summary: B2G SMS: Support USIM download → B2G SMS: Support Message Class 1 & 2
Summary: B2G SMS: Support Message Class 1 & 2 → B2G SMS: Support Message Classes
Attachment #610459 - Attachment is obsolete: true
Attachment #610460 - Attachment is obsolete: true
Attachment #610511 - Attachment is obsolete: true
Attachment #610512 - Attachment is obsolete: true
Comment on attachment 662522 [details] [diff] [review]
Part 4: Handle Message Class 2 : v1

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

There seems to be nothing to do in Message Class 1 & 3.

::: dom/system/gonk/ril_worker.js
@@ +3065,5 @@
> +  },
> +
> +  /**
> +   * @see 3GPP TS 23.040 section 9.3.2.3 for RP-ACK
> +   * @see 3GPP TS 23.040 section 9.3.2.4 for RP-ERROR

Actually SMS-DELIVER-REPORT FOR RP-ACK and RP-ERROR, and is defined in section 9.2.2.1a

@@ +3203,5 @@
>  
> +    if (message.messageClass == PDU_DCS_MSG_CLASS_SIM_SPECIFIC) {
> +      switch (message.epid) {
> +        case PDU_PID_ANSI_136_R_DATA:
> +        case PDU_PID_USIM_DATA_DOWNLOAD:

what to do with others?
Unlink bug 739143 for it has little effect on this issue.
No longer depends on: 739143
Comment on attachment 662519 [details] [diff] [review]
Part 2: Handle Message Class 0 : v1

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

::: dom/system/gonk/ril_worker.js
@@ +3028,5 @@
> +    if ((message.messageClass != PDU_DCS_MSG_CLASS_0) && !true) {
> +      // `When a mobile terminated messae is class 0..., the MS shall ...
> +      // irrespective of whether there is memory available in the (U)SIM or
> +      // ME.` ~ 3GPP 23.038 clause 4.
> +      return PDU_FCS_UNSPECIFIED;

That's not complete yet. Class 0 messages should not be stored in RadioInterfaceLayer.
Duplicate of this bug: 791968
Blocks: b2g-stk
blocking-basecamp: --- → ?
Keywords: feature
Whiteboard: [LOE:S]
Attachment #662519 - Attachment is obsolete: true
Attachment #662522 - Attachment is obsolete: true
philikon said he was going to investigate.
Whiteboard: [LOE:S] → [LOE:S][blocked-on-input philikon]
Vicamo, can you explain why you think this should be a blocker? Also, how does this block the STK (bug 791161)?
(In reply to Philipp von Weitershausen [:philikon] from comment #24)
> Vicamo, can you explain why you think this should be a blocker? Also, how
> does this block the STK (bug 791161)?

By documents provided by one of our partners, Data Download via SMS-PP is one of the mandatory features to trigger their internal SIM app to continue processing SIM activation.
Blocks: 792798
update patch in hg
Attachment #662518 - Attachment is obsolete: true
Attachment #662971 - Flags: review?(philipp)
update patch in hg
Attachment #662535 - Attachment is obsolete: true
Attachment #662972 - Flags: review?(philipp)
Attachment #662971 - Attachment description: Part 1: Save message class information : v2 → Part 1: Save message class information : v3
update patch in hg
Attachment #662521 - Attachment is obsolete: true
Attachment #662973 - Flags: review?(philipp)
update patch in hg
Attachment #662536 - Attachment is obsolete: true
Attachment #662974 - Flags: review?(philipp)
Attachment #662974 - Flags: review?(allstars.chh)
Attached patch Part 5: test cases (obsolete) — Splinter Review
Attachment #662975 - Flags: review?(philipp)
Attached patch Part 5: test cases : v2 (obsolete) — Splinter Review
s/2/3/ in a log call.
Attachment #662975 - Attachment is obsolete: true
Attachment #662975 - Flags: review?(philipp)
Attachment #662976 - Flags: review?(philipp)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #25)
> (In reply to Philipp von Weitershausen [:philikon] from comment #24)
> > Vicamo, can you explain why you think this should be a blocker? Also, how
> > does this block the STK (bug 791161)?
> 
> By documents provided by one of our partners, Data Download via SMS-PP is
> one of the mandatory features to trigger their internal SIM app to continue
> processing SIM activation.

Makes sense. Thank you!
Whiteboard: [LOE:S][blocked-on-input philikon] → [LOE:S]
Comment on attachment 662971 [details] [diff] [review]
Part 1: Save message class information : v3

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

Looks good to me. What about xpcshell unit tests for message class? Also I presume that you've run the xpcshell tests in dom/system/gonk/tests?
Attachment #662971 - Flags: review?(philipp) → review+
Comment on attachment 662972 [details] [diff] [review]
Part 2: Handle Message Class 0 : v3

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

::: dom/system/gonk/ril_worker.js
@@ +3034,5 @@
> +    // TODO: Bug 739143: B2G SMS: Support SMS Storage Full event
> +    if ((message.messageClass != PDU_DCS_MSG_CLASS_0) && !true) {
> +      // `When a mobile terminated messae is class 0..., the MS shall ...
> +      // irrespective of whether there is memory available in the (U)SIM or
> +      // ME.` ~ 3GPP 23.038 clause 4.

shall what? Seems like you're missing the verb in that quotation.

Also typo in "messae"
Attachment #662972 - Flags: review?(philipp) → review+
Comment on attachment 662973 [details] [diff] [review]
Part 3: add Buf.copyIncomingToOutgoing() : v2

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

::: dom/system/gonk/ril_worker.js
@@ +392,5 @@
> +    let i = this.incomingReadIndex;
> +    let j = this.outgoingIndex;
> +    while (length--) {
> +      this.outgoingBytes[j++] = this.incomingBytes[i++];
> +      i %= this.INCOMING_BUFFER_LENGTH;

I would prefer if you did all increments and decrements in separate statements.

What are typical values for `length`? I'm asking because JS typed arrays have a built-in copy functionality:

  this.outgoingBytes.set(this.incomingBytes.subarray(this.incomingReadIndex, length), this.outgoingIndex);
  this.incomingReadIndex += length;
  this.outgoingIndex += length;

It's not quite as simple as that, of course, since `this.incomingBytes` is circular. But we have other places in ril_worker already where we deal with this scenario, e.g.: https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#137

If `length` is small compared to `INCOMING_BUFFER_LENGTH`, it's probably not worth it.
Attachment #662973 - Flags: review?(philipp) → review+
Comment on attachment 662974 [details] [diff] [review]
Part 4: Handle Message Class 2 : v3

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

::: dom/system/gonk/ril_worker.js
@@ +3021,5 @@
> +    };
> +    Buf.newParcel(REQUEST_STK_SEND_ENVELOPE_WITH_STATUS, options);
> +
> +    // Skip response_type & request_type
> +    Buf.seekIncoming(-1 * (Buf.currentParcelSize - Buf.readAvailable - 8));

s/8/2 * UINT32_SIZE/

@@ +3067,5 @@
> +    Buf.copyIncomingToOutgoing(4 * tpduLength);
> +
> +    // Write 2 string delimitors for the total string length must be even.
> +    Buf.writeUint16(0);
> +    Buf.writeUint16(0);

Can you not use `Buf.writeStringDelimiter()`? E.g.

  Buf.writeStringDelimiter(0);

It's a bit of a hack, since the string you're writing out has an actual length, but `Buf.writeStringDelimiter()` only cares about even/odd lengths. In any case, calling `Buf.writeStringDelimiter()` makes the intent very clear.

@@ +3092,5 @@
> +    // Success
> +    Buf.writeString(success ? "1" : "0");
> +
> +    // RP-ACK/RP-ERROR PDU
> +    let responsePduLen = (Buf.readAvailable - 4) / 4;

Are you guaranteed to get an integer here? I guess you are because we PDUs are hex encoded UCS2 strings, so they're always multiple of 4 bytes?

@@ +4235,5 @@
> +
> +  let success = false;
> +  if (((sw1 == ICC_STATUS_NORMAL_ENDING) && (sw2 == 0x00))
> +      || (sw1 == ICC_STATUS_NORMAL_ENDING_WITH_EXTRA)) {
> +    success = true;

let success = ((sw1 == ICC_STATUS_NORMAL_ENDING) && (sw2 == 0x00)) ||
              (sw1 == ICC_STATUS_NORMAL_ENDING_WITH_EXTRA);
Attachment #662974 - Flags: review?(philipp) → review+
Attachment #662976 - Flags: review?(philipp) → review+
Attachment #662974 - Flags: review?(allstars.chh) → review+
blocking-basecamp: ? → +
(In reply to Philipp von Weitershausen [:philikon] from comment #33)
> Comment on attachment 662971 [details] [diff] [review]
> Part 1: Save message class information : v3
> 
> Review of attachment 662971 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. What about xpcshell unit tests for message class? Also I
> presume that you've run the xpcshell tests in dom/system/gonk/tests?

AFAIK, test_ril_worker_sms.js has been broken for a long time. The first breakage, which can be fixed with a simple patch, was introduced in bug 712933 due to `message.body` being renamed to `message.fullBody`. The second major breakage was introduced in bug 744709 by bringing in Settings API into RadioInterfaceLayer's constructor. Bug 793373 opened to track this.

I can still create a patch for message class xpcshell test cases and ensure newly added code does not introduce more troubles, but test_ril_worker_sms.js will remain broken until bug 793373 gets solved.
rebase & fix one possible MWI break.
Attachment #662971 - Attachment is obsolete: true
address comment #34
Attachment #662972 - Attachment is obsolete: true
Attachment #664389 - Attachment description: Part 1: Save message class information : v3 → Part 1: Save message class information : v4
Rewrite to utilize typed array builtin set() as recommended in comment #3. Hi Philipp, I raise another review request for it changes too much.
Attachment #662973 - Attachment is obsolete: true
Attachment #664394 - Flags: review?(philipp)
1. Address comment #36. Add as much comments as needed, especially in units of numbers.
2. Correct outgoing parcel data as possible. We don't have test environment here, but we can still try best to match the specification. There might still be implementation differences, let see verify it in the field.
Attachment #662974 - Attachment is obsolete: true
1. Add xpcshell dcs test cases as suggested in comment #33. Also checked those test cases are valid for both sms & voicemail without previous four patches(of course, with small modifications).
2. Add a draft test function for message class 2 to check a few fallback situations.
3. Indentations, typos.
Attachment #662976 - Attachment is obsolete: true
Comment on attachment 664394 [details] [diff] [review]
Part 3: add Buf.copyIncomingToOutgoing() : v3

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

::: dom/system/gonk/ril_worker.js
@@ +408,5 @@
> +      // Not so lucky.
> +      newIncomingReadIndex %= this.INCOMING_BUFFER_LENGTH;
> +      this.outgoingBytes.set(this.incomingBytes.subarray(this.incomingReadIndex, this.INCOMING_BUFFER_LENGTH),
> +                             this.outgoingIndex);
> +      if (newIncomingReadIndex) {

Nit: Since we're not testing for truthiness but non-zero-ness here, I would have written this as

  if (newIncomingReadIndex != 0) {

just to make it very clear. But that's a very minor nit. Nice work otherwise!
Attachment #664394 - Flags: review?(philipp) → review+
Otoro doesn't support "RIL_REQUEST_STK_SEND_ENVELOPE_WITH_STATUS". We shall detect whether ril supports this or not. If not, we need to use "RIL_REQUEST_STK_SEND_ENVELOPE_COMMAND" instead. But please note, some error situations may not be distinguished as in fine-grained as spec says. 

I filed bug 794404 for this.
Blocks: 796904
Blocks: 797277
You need to log in before you can comment on or make changes to this bug.