830264 - B2G CDMA: Support SMS

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Standard : 3GPP2 C.S0015-B v2.0
http://www.3gpp2.org/public_html/specs/C.S0015-B_v2.0_051006.pdf

Current progress
1. Receive
1.1. Support encoding : Unicode, 7-bit ASCII, IA5, IS-91, GSM 7-bit default alphabet
1.2. Support both segmented/un-segmented message
2. Deliver
2.1 Support encoding : Unicode, 7-bit ASCII
2.2 Support un-segmented message

Comment 1

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

RIL Parcel Format, tailing '*' indicates parameter defined in C.S0015

Type       Parameter
===== Service Info Part =============
Integer    Teleservice Identity*
Byte       Service Present Indicator
           1 : Broadcast
           0 : P2P SMS if Teleservice Identity is non-zero, ACK otherwise
Integer    Service Category*
===== Address Part =============
Integer    Digit Mode of Address*
Integer    Number Mode of Address*
Integer    Number Type of Address*
Integer    Number Plan of Address*
Byte       Length of Address*
Byte*n     Address, one digit per Byte*
===== Subaddress Part =============
Integer    Number Type of Subaddress*
Integer    Odd of Subaddress*
Byte       Length of Subaddress*
Byte*n     Subaddress, one digit per Byte*
===== Subparameter Part =============
Byte*n     Subparameter data*

Comment 2

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

On segmented message, Teleservice ID set to 0x1005(WEMT) instead of 0x1002(WMT/SMS).
User data header is embedded into user data(subparameter 0x01), between NUM_FIELDS and user data payload.

Structure of subparameter 0x01:
+--------+--------+----------+------------+-------------+-----------+
| 8 bits | 8 bits |  5 bits  |   8 bits   |   n bits    |  m bits   |
+--------+--------+----------+------------+-------------+-----------+
|   ID   | Length | Encoding | NUM_FIELDS | user header | user data |
|  0x01  |        |          |            |   payload   |  payload  |
+--------+--------+----------+------------+-------------+-----------+
Length is length of subparameter data, including user header, in byte.

user header payload for segmented message:
+--------+----------+---------+-----------+---------+---------+-----------+
| 8 bits |  8 bits  | 8 bits  |  8 bits   | 8 bits  | 8 bits  |  n bits   |
+--------+----------+---------+-----------+---------+---------+-----------+
| header | concated | length  |  segment  |   max   | current |  padding  |
|  size  |  sms ID  |         | reference | segment | segment |           |
+--------+----------+---------+-----------+---------+---------+-----------+
|   5    |    0     |    3    |           |         |         |           |
+--------+----------+---------+-----------+---------+---------+-----------+
header size is length of header data, in byte.

Padding is required to make whole user header payload aligned with encoding digit length, i.e., 7/8/16-bit aligned for 7/8/16-bit encoding.

So in current case, length of user header payload is 6 bytes.
No padding is required for 8/16-bit encoding, but 1 bit padding is required to make user header payload 7-bit aligned(total 49 bits = 7 septets).

Also, NUM_FIELDS is size of (user header payload + user data payload), in encoding digit length instead of byte.
So in 8-bit encoding, NUM_FIELDS = 6 + length of user data payload,
     16-bit encoding, NUM_FIELDS = 3 + length of user data payload,
      7-bit encoding, NUM_FIELDS = 7 + length of user data payload.

Comment 3

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Add constants for CDMA SMS.

Add constants for sending/receiving CDMA SMS

Comment 4

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002. Add buffer with bitwise read/write capability.

Handling CDMA SMS PDU requires bitwise read/write capability, but Buf handles data in byte.
So bitBuffer is introduced to provide bitwise read/write functionality on a byte array for PDU helpers.

Comment 5

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0003. Add CDMA PDU helper.

Add PDU helper for CDMA, currently only core functions for SMS is provided.
The interface/object of CDMA SMS functions are compatible with currently GSM SMS functions.

Comment 6

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0004. Receive CDMA SMS.

Handle incoming CDMA SMS.
a new argument |smsType| is added to |_processReceivedSms()| to identify incoming SMS type.

Comment 7

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0005. Send CDMA SMS.

Add SMS delivery function in SMS, and send SMS based on current network type.

Comment 8

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

Comment on attachment 709593 [details] [diff] [review]
0001. Add constants for CDMA SMS.

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

Please also point out the origin of these numbers in comments.

::: dom/system/gonk/ril_consts.js
@@ +2426,5 @@
> +this.PDU_CDMA_MSG_TYPE_ACK = 0x02;        // Acknowledge
> +
> +// SMS Teleservice Identity
> +this.PDU_CDMA_MSG_TELESERIVCIE_ID_SMS = 0x1002;   // SMS
> +this.PDU_CDMA_MSG_TELESERIVCIE_ID_WEMT = 0x1005;  // Wireless Enhanced Messaging Teleservice, required for fragmented SMS

line wrap please

Comment 9

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

Comment on attachment 709594 [details] [diff] [review]
0002. Add buffer with bitwise read/write capability.

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

::: dom/system/gonk/ril_worker.js
@@ +7289,5 @@
> +      return null;
> +    }
> +
> +    if (length > this.readCacheSize) {
> +      var bytesToRead = Math.ceil((length - this.readCacheSize) / 8);

Please use 'let' instead.  In this function 'var' works, but is still risky when new changes come in.

@@ +7333,5 @@
> +    }
> +
> +    // Aligned part, just copy
> +    this.writeCache = 0;
> +    this.writeCacheSize = 0;

Please move the three lines into |if (this.writeCacheSize) {...}|

Comment 10

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

Comment on attachment 709595 [details] [diff] [review]
0003. Add CDMA PDU helper.

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

8 line context please.

::: dom/system/gonk/ril_worker.js
@@ +7504,5 @@
> +   *
> +   * @param address
> +   *        String of address to be encoded
> +   */
> +  addrEncoder: function addrEncoder(address) {

"encoder" is a noun, but function name should be a verb.

Comment 11

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

Comment on attachment 709596 [details] [diff] [review]
0004. Receive CDMA SMS.

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

::: dom/system/gonk/ril_worker.js
@@ +1816,3 @@
>      this.acknowledgeSMS(options.result == PDU_FCS_OK, options.result);
>    },
>  

8 line context please.

@@ +3606,4 @@
>     *
>     * @return Message parsed or null for invalid message.
>     */
> +  _processReceivedSms: function _processReceivedSms(length, smsType) {

Thank you for trying hard to merge the processing flow of the two types of SMS message.  But they're so different that the code reused here is nearly nothing but |if (!lenght) {...}|.  I'm wondering whether can we separate the processing flow and have something like:

  let GsmPDUHelper = {
    readMessage: function () {
      // Move messageStringLength reading here.
    },

    processReceivedSms: function (length) {
      // Move message class handling here and return
      // [message, PDU_FCS_OK] or [null, PDU_FCS_FOO]
      // when no more processing is required.
    }
  };

  let CdmaPDUHelper = {
    processReceivedSms: function (length) {
      // Same here.
    }
  };

  let RIL = {
    processSmsMultipart: function (message) {
      // It's originally named _processSmsDeliver but has
      // now only multipart handling left inside.
    }
  };

  RIL[UNSOLICITED_RESPONSE_NEW_SMS] = function (length) {
    let [message, result] =
      GsmPDUHelper.processReceivedSms(length);
    if (message) {
      result = this.processSmsMultipart(message);
    }
    ...
  };

  RIL[UNSOLICITED_RESPONSE_CDMA_NEW_SMS]
      = function (length) {
    let [message, result] =
      GsmPDUHelper.processReceivedSms(length);
    if (message) {
      result = this.processSmsMultipart(message);
    }
    ...
  };

Comment 12

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

Comment on attachment 709597 [details] [diff] [review]
0005. Send CDMA SMS.

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

::: dom/system/gonk/ril_worker.js
@@ +5001,4 @@
>  RIL[REQUEST_CDMA_FLASH] = null;
>  RIL[REQUEST_CDMA_BURST_DTMF] = null;
>  RIL[REQUEST_CDMA_VALIDATE_AND_WRITE_AKEY] = null;
> +RIL[REQUEST_CDMA_SEND_SMS] = function REQUEST_CDMA_SEND_SMS(length, options) {

Please reuse this function since it's exactly the same with RIL[REQUEST_SEND_SMS].

Comment 13

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Add constants for CDMA SMS, v1.1

Address comment 8

Comment 14

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002. Add buffer with bitwise read/write capability, v1.1

Comment 15

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002. Add buffer with bitwise read/write capability, v1.1

Address comment 9

Comment 16

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0003. Add CDMA PDU helper, v1.1

Address comment 10.

Comment 17

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0004. Refactory GSM SMS flow.

Address refactory suggestion in comment 11.

Comment 18

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0004. Receive CDMA SMS, v1.1

Address refactory suggestion in comment 11.

Comment 19

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0005. Receive CDMA SMS, v1.1

Address refactory suggestion in comment 11.
Fix wrong index of last attachment.

Comment 20

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0006. Send CDMA SMS, v1.1

Address refactory suggestion in comment 12.

Comment 21

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

Comment on attachment 717690 [details] [diff] [review]
0001. Add constants for CDMA SMS, v1.1

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

::: dom/system/gonk/ril_consts.js
@@ +2421,5 @@
>  this.MMI_SC_BA_MT = "353";
>  
> +// CDMA constants
> +// SMS Message Type
> +this.PDU_CDMA_MSG_TYPE_P2P = 0x00;        // Point-to-Point

Please also point out the origin of these numbers in comments.

Comment 22

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

Comment on attachment 717692 [details] [diff] [review]
0002. Add buffer with bitwise read/write capability, v1.1

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

::: dom/system/gonk/ril_worker.js
@@ +7209,5 @@
>  
> +/**
> + * Provide buffer with bitwise read/write function so make encoding/decoding easier.
> + */
> +let bitBuffer = {

BitBufferHelper?

Comment 23

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

Comment on attachment 717694 [details] [diff] [review]
0004. Refactory GSM SMS flow.

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

::: dom/system/gonk/ril_worker.js
@@ +6595,5 @@
> +    if (message.epid == PDU_PID_SHORT_MESSAGE_TYPE_0) {
> +      // `A short message type 0 indicates that the ME must acknowledge receipt
> +      // of the short message but shall discard its contents.` ~ 3GPP TS 23.040
> +      // 9.2.3.9
> +      return [message, PDU_FCS_OK];

Shall return [null, PDU_FCS_OK] instead to prevent TYPE_0 messages being further processed.

Comment 24

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

Comment on attachment 717696 [details] [diff] [review]
0005. Receive CDMA SMS, v1.1

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

::: dom/system/gonk/ril_worker.js
@@ +1729,5 @@
>      }
> +    if (this._isCdma) {
> +      this.acknowledgeCdmaSms(options.result == PDU_FCS_OK, options.result);
> +    } else {
> +      this.acknowledgeSMS(options.result == PDU_FCS_OK, options.result);

Then please also rename it to 'acknowledgeGsmSms'.

Comment 25

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

Comment on attachment 717697 [details] [diff] [review]
0006. Send CDMA SMS, v1.1

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

Great job!

Comment 26

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Add constants for CDMA SMS, v1.2

Address comment 21

Comment 27

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002. Add buffer with bitwise read/write capability, v1.2

Address comment 22

Comment 28

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0003. Add CDMA PDU helper, v1.2

Address comment 22

Comment 29

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0004. Refactory GSM SMS flow, v1.1

Fix return value per comment 23

Comment 30

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0005. Receive CDMA SMS, v1.2

Address comment 25

Comment 31

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Chuck Lee [:chucklee] from comment #30)
> Created attachment 717797 [details] [diff] [review]
> 0005. Receive CDMA SMS, v1.2
> 
> Address comment 25

s/comment 25/comment 24

Comment 32

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

try for current patch set : https://tbpl.mozilla.org/?tree=Try&rev=b7b32915cf1f

Comment 33

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bb31f0b063f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0dbbe8a3afc
https://hg.mozilla.org/integration/mozilla-inbound/rev/310f40bb3b6a
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f3ab746de8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c94decbf663f
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa7820b84e2f

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6bb31f0b063f
https://hg.mozilla.org/mozilla-central/rev/e0dbbe8a3afc
https://hg.mozilla.org/mozilla-central/rev/310f40bb3b6a
https://hg.mozilla.org/mozilla-central/rev/a4f3ab746de8
https://hg.mozilla.org/mozilla-central/rev/c94decbf663f
https://hg.mozilla.org/mozilla-central/rev/fa7820b84e2f