813893 - B2G RIL: support UMTS CBS Message

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

Bug 778093 has support for UMTS format but is withdrawn for some unclear implementation questions and the lack of marionette test cases. This bug is then opened for re-landing it.

Comment 1

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

See also bug 778093 comment #100.

Comment 2

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

Attachment: Part 1: support UMTS CBS message

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Hi Vicamo,

Since, this is a blocker to Dolphin,
I'd like to follow up this one to finish the implementation & the corresponding test case if you have no concern. :)

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1 v2: Add Support to Read CBS Message in UMTS format.

re-base the patch to let it workable in master branch.

Comment 5

[sam.hua]

the new patch can work in v1.4 (dolphin)

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1 v3: Add Support to Read CBS Message in UMTS format.

The order of MsgId & Serial Number is reversed UMTS format compared to GSM format.

Move the checking of ETWS from readCbMessageIdentifier() and check ETWS info after both message identifier and serial number are decoded.

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: WIP: Patch Part 2: Add Test Case to Verify CB Message in UMTS Format.

Add corresponding test case to verify CB Message in UMTS format.

Set WIP because in function testReceiving_UMTS_Multipart(), we are not able to receive the CB message if we send a CBS PDU with more than 6 pages at once.

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1 v4: Add Support to Read CBS Message in UMTS format.

1. Add readUmtsCbMessage() in GSMPduHelper to support reading CB message in UMTS format.
2. Extra readCbEtwsInfo() from readCbMessageIdentifier() to ensure the etws information can be extracted correctly for CB Message in both GSM/UMTS format because the order of message_id & serial_number in GSM PDU and in UMTS PDU is reversed.
3. Print the exception without JSSON.stringify() to improve the  readability of the exception. Otherwise, an empty object {} was printed.

Hi Hsinyi,

May I have your review for this change?
BTW, I keep the author of this patch as Vicamo because only some minor change was done in this patch. :)

Thanks!

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2 v2: Add Test Case to Verify CB Message in UMTS Format.

1. Add corresponding Marionette Test Case for CB Message in UMTS format.
2. Move common-used test variables into head.js.
3. Add additional XPC shell test case to cover the mutlpart test case with 1~15 pages which are not able to verify in Marionette because the out_buf of the android_modem in emulator is fixed to 1024 bytes.

Hi Hsinyi,

May I have your review for this change?

Thanks!

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2 v3: Add Test Case to Verify CB Message in UMTS Format.

Expand TIMEOUT to 60 seconds to prevent timeout in try server.

Comment 11

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

Comment on attachment 8451542 [details] [diff] [review]
Patch Part 1 v4: Add Support to Read CBS Message in UMTS format.

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

Hi Bevis,

I have some questions :)

::: dom/system/gonk/ril_worker.js
@@ +8569,5 @@
> +   */
> +  readUmtsCbData: function(msg) {
> +    let Buf = this.context.Buf;
> +    let numOfPages = Buf.readUint8();
> +    if (!numOfPages) {

numOfPages should be equal to or less than 15, right? Please add this condition as well.

@@ +8584,5 @@
> +
> +    // Seek back to beginning of CB Data.
> +    Buf.seekIncoming(-numOfPages * (CB_MAX_CONTENT_8BIT + 1));
> +
> +    if (msg.encoding == PDU_DCS_MSG_CODING_8BITS_ALPHABET) {

Can we use 'switch-case' to make the code clearer so the reader could easily tell which coding scheme is referred? I.e.

switch (msg.encoding) {
    case PDU_DCS_MSG_CODING_8BITS_ALPHABET:
      XXXX
      break;
    case PDU_DCS_MSG_CODING_7BITS_ALPHABET:
      XXXX
      break;
}

@@ +8603,5 @@
> +
> +    // 7 Bits alphabet or UCS-2 here.
> +    let body = "";
> +    for (let i = 0; i < numOfPages; i++) {
> +      this.readGsmCbData(msg, pageLengths[i]);

Can we move the implementation for 7bit coding here? We already know the encoding is 7bit and we don't really need the whole |readGsmCbData| function.

@@ +8661,5 @@
>        msg.format = CB_FORMAT_GSM;
>        return this.readGsmCbMessage(msg, pduLength);
>      }
>  
> +    msg.format = CB_FORMAT_UMTS;

Is the pdu length for CB_FORMAT_UMTS defined in the spec? If yes, please safely check the boundary before reading Umte CB message.

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: WIP Patch Part 1 v5: Add Support to Read CBS Message in UMTS format.

1. extract readCbMessageInfoPage() out from readGsmCbData().
2. apply readCbMessageInfoPage() for both readGsmCbData() and readUmtsCbData().

Note: the disadvantage of this approach is that we have to temporarily create a UInt8Array in readCbMessageInfoPage() for the concatenation of 8bit data.

Hi Hsinyi,

My I have your suggestion for the refactoring mentioned above?

Thanks!

Comment 13

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8455236 [details] [diff] [review]
WIP Patch Part 1 v5: Add Support to Read CBS Message in UMTS format.

After off-lined discussion, the suggestion in comment 4 is better.
Re-use of readCbMessageInfoPage() is not a must.

Comment 14

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

Comment on attachment 8452086 [details] [diff] [review]
Patch Part 2 v3: Add Test Case to Verify CB Message in UMTS Format.

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

Looks good! Can we also have test for 8-bit encoding? Thank you :)

::: dom/cellbroadcast/tests/marionette/test_cellbroadcast_umts.js
@@ +7,5 @@
> +function testReceiving_UMTS_MessageAttributes() {
> +  log("Test receiving UMTS Cell Broadcast - Message Attributes");
> +
> +  let verifyCBMessage = (aMessage) => {
> +    // Attributes other than `language` and `body` should always be assigned.

When is 'language' or 'body' not assigned? If the comment is true, why can we have |ok(aMessage.language != null, "aMessage.language");| and |ok(aMessage.body != null, "aMessage.body");|?

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> Comment on attachment 8452086 [details] [diff] [review]
> Patch Part 2 v3: Add Test Case to Verify CB Message in UMTS Format.
> 
> Review of attachment 8452086 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! Can we also have test for 8-bit encoding? Thank you :)

Sure. I'll add it in next update. :)

> 
> ::: dom/cellbroadcast/tests/marionette/test_cellbroadcast_umts.js
> @@ +7,5 @@
> > +function testReceiving_UMTS_MessageAttributes() {
> > +  log("Test receiving UMTS Cell Broadcast - Message Attributes");
> > +
> > +  let verifyCBMessage = (aMessage) => {
> > +    // Attributes other than `language` and `body` should always be assigned.
> 
> When is 'language' or 'body' not assigned? If the comment is true, why can
> we have |ok(aMessage.language != null, "aMessage.language");| and
> |ok(aMessage.body != null, "aMessage.body");|?

Sorry for misleading, the reason is that the test message here is encoded in text format.
I should remove them from here because it has been covered in testReceiving_UMTS_Language_and_Body().

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

update assignee to myself for better status tracking.

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1 v6: Add Support to Read CBS Message in UMTS format.

1. Add boundary check of UMTS PDU.
2. Add UMTS-owned readUmtsCbData() from readGsmCbData to decode the CB data in UMTS format for more efficiency and better readability.

Hi Hsinyi,

May I have your review for this patch?

Thanks!

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2 v4: Add Test Case to Verify CB Message in UMTS Format. r=htsai

1. Add test case for binary CB message.
2. Expand TIMEOUT to 90 seconds to prevent timeout in try server.
3. Remove 'body', 'language' from mandatory attribute checking.

Comment 19

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

Comment on attachment 8462466 [details] [diff] [review]
Patch Part 1 v6: Add Support to Read CBS Message in UMTS format.

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

r=me with comments answered or addressed, thank you, Bevis!

::: dom/system/gonk/ril_worker.js
@@ +8683,5 @@
> +
> +          msg.body += removePaddingCharactors(body);
> +
> +          // Skip padding octets
> +          Buf.seekIncoming(CB_MAX_CONTENT_8BIT - pageLengths[i]);

Do you think we can use another new constant such as CBS_MSG_INFO_PAGE_SIZE = 0x82 rather than CB_MAX_CONTENT_8BIT though these two have the same value? I am asking because I found it confusing that why we use something for 8BIT when decoding a message encoded in 7bit or 16bit.

@@ +8719,5 @@
> +                                                           0,
> +                                                           PDU_NL_IDENTIFIER_DEFAULT,
> +                                                           PDU_NL_IDENTIFIER_DEFAULT);
> +            } else {
> +              Buf.readUint16();

Can you explain this?

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8462466 [details] [diff] [review]
Patch Part 1 v6: Add Support to Read CBS Message in UMTS format.

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

::: dom/system/gonk/ril_worker.js
@@ +8683,5 @@
> +
> +          msg.body += removePaddingCharactors(body);
> +
> +          // Skip padding octets
> +          Buf.seekIncoming(CB_MAX_CONTENT_8BIT - pageLengths[i]);

I see your point.
Yes, CBS_MSG_INFO_PAGE_SIZE is better without confusing! :)

@@ +8720,5 @@
> +                                                           PDU_NL_IDENTIFIER_DEFAULT,
> +                                                           PDU_NL_IDENTIFIER_DEFAULT);
> +            } else {
> +              Buf.readUint16();
> +            }

Sure!

According to |5 CBS Data Coding Scheme| in TS 23.038, 
if you check DCS with 00010001 (UCS2; message preceded by language indication),
"
  The message starts with a two GSM 7-bit default alphabet character 
  representation of the language encoded according to ISO 639 [12]. This is padded 
  to the octet boundary with two bits set to 0 and then followed by 40 characters of 
  UCS2-encoded message. 
"

Hence, the implementation here is to 
1. Read the preceding language indication in 1st page and
2. Skip 2 octets directly for the rest pages.
   (It is supposed to be the same to the one in 1st page)

Bevis

Comment 21

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

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #20)
> Comment on attachment 8462466 [details] [diff] [review]
> Patch Part 1 v6: Add Support to Read CBS Message in UMTS format.
> 
> Review of attachment 8462466 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +8683,5 @@
> > +
> > +          msg.body += removePaddingCharactors(body);
> > +
> > +          // Skip padding octets
> > +          Buf.seekIncoming(CB_MAX_CONTENT_8BIT - pageLengths[i]);
> 
> I see your point.
> Yes, CBS_MSG_INFO_PAGE_SIZE is better without confusing! :)
> 
> @@ +8720,5 @@
> > +                                                           PDU_NL_IDENTIFIER_DEFAULT,
> > +                                                           PDU_NL_IDENTIFIER_DEFAULT);
> > +            } else {
> > +              Buf.readUint16();
> > +            }
> 
> Sure!
> 
> According to |5 CBS Data Coding Scheme| in TS 23.038, 
> if you check DCS with 00010001 (UCS2; message preceded by language
> indication),
> "
>   The message starts with a two GSM 7-bit default alphabet character 
>   representation of the language encoded according to ISO 639 [12]. This is
> padded 
>   to the octet boundary with two bits set to 0 and then followed by 40
> characters of 
>   UCS2-encoded message. 
> "
> 
> Hence, the implementation here is to 
> 1. Read the preceding language indication in 1st page and
> 2. Skip 2 octets directly for the rest pages.
>    (It is supposed to be the same to the one in 1st page)
> 
> Bevis

Thanks for your patience and the explanation :P

Comment 22

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1/2: Add Support to Read CBS Message in UMTS format. r=htsai

update final patch with positive try server result.

Comment 23

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2/2: Add Test Case to Verify CB Message in UMTS Format. r=htsai

update final patch with positive try server result.

Comment 24

[Bevis Tseng[:bevis][:btseng](Exited)]

try server result is green:
https://tbpl.mozilla.org/?tree=Try&rev=3f70060b0291

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/5aec82d6b509
https://hg.mozilla.org/integration/b2g-inbound/rev/533205d74fc6

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5aec82d6b509
https://hg.mozilla.org/mozilla-central/rev/533205d74fc6