709566 - B2G SMS: PDU parser and formatter

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

We're going to need to parse incoming PDU data and format outgoing texts as PDU. This should live in the RIL worker space since it's pretty low-level and purely mechanical, so there's no reason to do it on the main thread.

In the GSM case, the RIL simply returns a regular RIL-type string (that means 16 bits wide) containing hex-encoded octets. Pretty sparse. Fernando has a prototype that would take an incoming string from the RIL, reads hex characters (2-length strings), converts them to octets, etc. I wonder if this could be simpler and less allocate-y by reading stuff straight off the incoming byte stream...

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Philipp von Weitershausen [:philikon] from comment #0)
> In the GSM case, the RIL simply returns a regular RIL-type string (that
> means 16 bits wide) containing hex-encoded octets. Pretty sparse. Fernando
> has a prototype that would take an incoming string from the RIL, reads hex
> characters (2-length strings), converts them to octets, etc. I wonder if
> this could be simpler and less allocate-y by reading stuff straight off the
> incoming byte stream...

Yes, you are right. It would be more efficient to do the parsing directly from the incoming byte stream. I'll try to do that after implementing the functionality to format outgoing SMS as PDU.

Comment 2

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1): PDU parser for GSM

This is the PDU parsing. Much thanks to ferjm for the head start on this.

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 582623 [details] [diff] [review]
Part 1 (v1): PDU parser for GSM

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

How did you/ferjm figure out the parts of the protocol? How much of this was reading the android java versus reading the SMS spec? Just wondering if we should annotate the protocol layout via the spec like happens in the android modules.

Otherwise, code looks solid. r=me.

::: dom/telephony/ril_worker.js
@@ +1514,5 @@
> +   *        Number of nibble *pairs* to read.
> +   *
> +   * @return the decimal as a number.
> +   */
> +  readBCD: function readBCD(length) {

Might we want to change the name of this to "readSwappedNibble"? I suspect some of these PDU helpers will be useful elsewhere, and I don't want someone cutting/pasting this into another namespace thinking it'll "just work" for reading BCD?

@@ +1578,5 @@
> +   *        XXX TODO
> +   *
> +   * @return a string.
> +   */
> +  readUCS2String: function readUCS2String(length) {

Doesn't our current string reader in Buf do this already? Though when writing this in python I had to chop the first 2 bytes (0xfffe), which may be a standard header.

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #3)
> How did you/ferjm figure out the parts of the protocol? How much of this was
> reading the android java versus reading the SMS spec? Just wondering if we
> should annotate the protocol layout via the spec like happens in the android
> modules.

ferjm figured most of it out. The spec is ok, there are a few good descriptions of it on the interwebs. The Java source code is, as usual, scattered over dozens of files. I briefly took a look at it after we had most of it working, but it really wasn't worth it.

> ::: dom/telephony/ril_worker.js
> @@ +1514,5 @@
> > +   *        Number of nibble *pairs* to read.
> > +   *
> > +   * @return the decimal as a number.
> > +   */
> > +  readBCD: function readBCD(length) {
> 
> Might we want to change the name of this to "readSwappedNibble"? I suspect
> some of these PDU helpers will be useful elsewhere, and I don't want someone
> cutting/pasting this into another namespace thinking it'll "just work" for
> reading BCD?

It should be kinda implied that it's part of the *Gsm*PDUHelper object, but ok. I'll call it readSwappedNibbleBCD() to be absolutely clear.

> > +   *        XXX TODO
> > +   *
> > +   * @return a string.
> > +   */
> > +  readUCS2String: function readUCS2String(length) {
> 
> Doesn't our current string reader in Buf do this already?

This is about two levels removed from that. SMS unicode strings are encoded as UCS2 of which each byte then is encoded as hex. So GsmPDUHelper.readUCS2String() is going to have to read two hex octets at a time (using GsmPDUHelper.readHexOctet(), which in itself reads two bytes from the input stream, using Buf.readUint16()) and convert them to one Unicode character.

Is your head spinning yet? :)

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v2): PDU parser for GSM

Addressed Kyle's review comments.

Comment 6

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v1): PDU serializer for GSM

Much like Part 1, this is pretty rough and at best a start into the fun land of SMS encodings and formats. But, this gets us there for the default 7bit alphabet (western European languages). I will file follow-ups for all the stuff that's still left to do, if I haven't already.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 583952 [details] [diff] [review]
Part 2 (v1): PDU serializer for GSM

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

Looks good

r=me with nits picked

::: dom/telephony/ril_worker.js
@@ +1497,5 @@
> +      nibble += 55;
> +    }
> +    Buf.writeUint16(nibble);
> +  },
> +

You may want to comment that you're adding ASCII values (i.e. nibble += 48; // '0').

@@ +1928,5 @@
> +    }
> +    let udhi = ""; // TODO for now this is unsupported
> +    if (udhi) {
> +      firstOctet |= 0x40;
> +    }

Might want to name these flag constants?

Comment 8

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06818336b636
https://hg.mozilla.org/integration/mozilla-inbound/rev/0379638036d7

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/06818336b636
https://hg.mozilla.org/mozilla-central/rev/0379638036d7