Closed Bug 813893 Opened 8 years ago Closed 6 years ago

B2G RIL: support UMTS CBS Message

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-v1.4 affected)

RESOLVED FIXED
2.1 S1 (1aug)
Tracking Status
b2g-v1.4 --- affected

People

(Reporter: vicamo, Assigned: bevis)

References

Details

(Keywords: feature, Whiteboard: RN6/14)

Attachments

(2 files, 10 obsolete files)

11.47 KB, patch
bevis
: review+
Details | Diff | Splinter Review
29.96 KB, patch
bevis
: review+
Details | Diff | Splinter Review
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.
Attached patch Part 1: support UMTS CBS message (obsolete) — Splinter Review
Assignee: nobody → vyang
Whiteboard: RN5/29
Whiteboard: RN5/29 → RN6/14
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Blocks: 1024843
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. :)
Flags: needinfo?(vyang)
re-base the patch to let it workable in master branch.
Attachment #684419 - Attachment is obsolete: true
the new patch can work in v1.4 (dolphin)
Depends on: 1028791
Blocks: 1028791
No longer depends on: 1028791
Flags: needinfo?(vyang)
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.
Attachment #8441239 - Attachment is obsolete: true
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.
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!
Attachment #8450139 - Attachment is obsolete: true
Attachment #8451542 - Flags: review?(htsai)
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!
Attachment #8450141 - Attachment is obsolete: true
Attachment #8451549 - Flags: review?(htsai)
Attachment #8451542 - Attachment description: Part 1 v4: Add Support to Read CBS Message in UMTS format. → Patch Part 1 v4: Add Support to Read CBS Message in UMTS format.
Attachment #8451549 - Attachment description: Patch Part 2: Add Test Case to Verify CB Message in UMTS Format. → Patch Part 2 v2: Add Test Case to Verify CB Message in UMTS Format.
Expand TIMEOUT to 60 seconds to prevent timeout in try server.
Attachment #8451549 - Attachment is obsolete: true
Attachment #8451549 - Flags: review?(htsai)
Attachment #8452086 - Flags: review?(htsai)
Attachment #8452086 - Attachment description: Patch Part 2 v2: Add Test Case to Verify CB Message in UMTS Format. → Patch Part 2 v3: Add Test Case to Verify CB Message in UMTS Format.
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.
Attachment #8451542 - Flags: review?(htsai)
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!
Attachment #8451542 - Attachment is obsolete: true
Attachment #8455236 - Flags: feedback?(htsai)
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.
Attachment #8455236 - Flags: feedback?(htsai)
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");|?
Attachment #8452086 - Flags: review?(htsai) → review+
(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().
update assignee to myself for better status tracking.
Assignee: vyang → btseng
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!
Attachment #8455236 - Attachment is obsolete: true
Attachment #8462466 - Flags: review?(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.
Attachment #8452086 - Attachment is obsolete: true
Attachment #8462472 - Flags: review+
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?
Attachment #8462466 - Flags: review?(htsai) → review+
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
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) 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
update final patch with positive try server result.
Attachment #8462466 - Attachment is obsolete: true
Attachment #8464466 - Flags: review+
update final patch with positive try server result.
Attachment #8462472 - Attachment is obsolete: true
Attachment #8464467 - Flags: review+
Target Milestone: --- → 2.1 S1 (1aug)
Duplicate of this bug: 1024843
You need to log in before you can comment on or make changes to this bug.