736706 - B2G SMS: Support Message Classes

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: Save message class information

Comment 2

[Vicamo Yang [:vicamo][: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.

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: Handle Message Class 1

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Depends on bug 736707 to include definitions of TP-PID values.

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: Save message class information : WIP

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: Handle Message Class 1 : WIP

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: Handle Message Class 2 : WIP

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: add Buf.copyIncomingToOutgoing() : WIP

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 4: Handle Message Class 2 : WIP

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Remove blocking flag for b2g-sms for milestone 3 temporarily.

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: Save message class information : v1

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: Handle Message Class 0 : v1

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: add Buf.copyIncomingToOutgoing() : v1

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 4: Handle Message Class 2 : v1

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

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?

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Unlink bug 739143 for it has little effect on this issue.

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: Handle Message Class 0 : v2

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 4: Handle Message Class 2 : v2

Comment 23

[Andrew Overholt [:overholt]]

philikon said he was going to investigate.

Comment 24

[Philipp von Weitershausen [:philikon]]

Vicamo, can you explain why you think this should be a blocker? Also, how does this block the STK (bug 791161)?

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 26

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: Save message class information : v3

update patch in hg

Comment 27

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: Handle Message Class 0 : v3

update patch in hg

Comment 28

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: add Buf.copyIncomingToOutgoing() : v2

update patch in hg

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 4: Handle Message Class 2 : v3

update patch in hg

Comment 30

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 5: test cases

Comment 31

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 5: test cases : v2

s/2/3/ in a log call.

Comment 32

[Philipp von Weitershausen [:philikon]]

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

Comment 33

[Philipp von Weitershausen [:philikon]]

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?

Comment 34

[Philipp von Weitershausen [:philikon]]

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"

Comment 35

[Philipp von Weitershausen [:philikon]]

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.

Comment 36

[Philipp von Weitershausen [:philikon]]

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);

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 38

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: Save message class information : v4

rebase & fix one possible MWI break.

Comment 39

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: Handle Message Class 0 : v4

address comment #34

Comment 40

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: add Buf.copyIncomingToOutgoing() : v3

Rewrite to utilize typed array builtin set() as recommended in comment #3. Hi Philipp, I raise another review request for it changes too much.

Comment 41

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 4: Handle Message Class 2 : v4

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.

Comment 42

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 5: test cases : v3

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.

Comment 43

[Philipp von Weitershausen [:philikon]]

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!

Comment 44

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37ef7ce25b9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/73d41a3aa7d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/e814f7254496
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3db7fb3a771
https://hg.mozilla.org/integration/mozilla-inbound/rev/bea00714789a

Comment 45

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37ef7ce25b9c
https://hg.mozilla.org/mozilla-central/rev/73d41a3aa7d9
https://hg.mozilla.org/mozilla-central/rev/e814f7254496
https://hg.mozilla.org/mozilla-central/rev/c3db7fb3a771
https://hg.mozilla.org/mozilla-central/rev/bea00714789a