869776 - [B2G][CDMA] Set cell broadcast channel and disable/enable the CB function in CDMA network

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Ken Chang[:kenkai]]

We may need this interface, REQUEST_CDMA_SET_BROADCAST_SMS_CONFIG, to set cell broadcast channel in CDMA network.

Comment 1

[Ken Chang[:kenkai]]

(In reply to Ken Chang from comment #0)
> We may need this interface, REQUEST_CDMA_SET_BROADCAST_SMS_CONFIG, to set
> cell broadcast channel in CDMA network.

And we also need the interface, REQUEST_CDMA_SMS_BROADCAST_ACTIVATION, to activated the CB function.

Comment 2

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

Not really working on it.

Comment 3

[Sandip Kamat]

Ken, Is UX definition needed here?

Comment 4

[Ken Chang[:kenkai]]

Sandip, yes, it is better to have the UX definition.

Comment 5

[Szu-Yu Chen [:aknow] (inactive)]

We should support these two ril commands.
  REQUEST_CDMA_SMS_BROADCAST_ACTIVATION
  REQUEST_CDMA_SET_BROADCAST_SMS_CONFIG

ACTIVATION is just the same as it in GSM.
However, the data for REQUEST_CDMA_SET_BROADCAST_SMS_CONFIG has different format.

In GSM:
Config data is an array of (serviceId range, codeScheme range)
The cell broadcast search list is config from
  1. setting "ril.cellbroadcast.searchlist" ex: "12,15-34"
  2. sim card (CBMI/CBMID/CBMIR)
Then, we merge all the search list into an array.
So the `Config data` send to ril will be

[(12-12, 0x00-FF), (15-34, 0x00-FF) ... ]

In CDMA:
Config data is an array of (service_category, language).

My proposal is to use a similar setting "ril.cdma.cellbroadcast.searchlist"
Ex:
(i) "12,15-34" <= if we don't want to specify the language indicator, or
(ii) "12/1,15-34/6" <= use language indicator

Gecko could split the the range and send the request to ril
So the correspondent `Config data` for ril will be
(i) [(12,0), (15,0), (16,0), ..., (34,0)] or
(ii) [(12,1), (15,6), (16,6), ..., (34,6)]

Comment 6

[Michael Schwartz [:m4]]

UX folks should know that stock Android shows the same screen for GSM or CDMA (no language selection option given) should they want to take that into account.  Things might be different if we're targeting dual-tech (DSDS/DSDA) markets however.

Comment 7

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1: Support cdma cell broadcast config

Comment 8

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2: Test cell broadcast config

Comment 9

[Szu-Yu Chen [:aknow] (inactive)]

vicamo, 

There are some rules on sim card should be followed. 
See 3gpp2 C.S0023 (spec of R-UIM) and search BCSMS
(also http://goo.gl/RlUPYN )

First, we should check EFCST (CDMA Service Table) and find Service n14 (Service Category Program for BC-SMS)

If n14 exists, ...

EFBCSMScfg (Broadcast Short Message Configuration), operator setting
 -- disallow cb
 -- allow all service categories
 -- only allow the service categories programmed in EFBCSMStable

EFBCSMSpref(Broadcast Short Message Preference), user setting
 similar to EFBCSMScfg

EFBCSMStable (Broadcast Short Message Table) ... 
EFBCSMSP (Broadcast Short Message Parameter) ...

Not sure whether we have to handle these logic. If we are lucky, modem might be powerful enough to handle all of them. I'll check this part later.

Comment 10

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 783516 [details] [diff] [review]
Part 2: Test cell broadcast config

Cancel the review until the question or issue in Comment 9 is fixed.

Comment 11

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1#2: Support cdma cell broadcast config

Comment 12

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#2: Test cell broadcast config

Comment 13

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

Comment on attachment 784909 [details] [diff] [review]
Part 1#2: Support cdma cell broadcast config

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

::: dom/system/gonk/ril_worker.js
@@ +4533,5 @@
> +    // CBMI should be ready in GSM.
> +    if (!("MMI" in this.cellBroadcastConfigs) ||
> +        (!this._isCdma && !this._isCBMIReady())) {
> +      return false;
> +    }

That's too complicated.  Please have:

  if (!("MMI" in this.cellBroadcastConfigs)) {
    return false;
  }

  if (!this._isCdma) {
    if (!("CBMI" in this.cellBroadcastConfigs) ||
        !("CBMID" in this.cellBroadcastConfigs) ||
        !("CBMIR" in this.cellBroadcastConfigs)) {
      return false;
    }
  }

  return true;

Comment 14

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

Comment on attachment 784910 [details] [diff] [review]
Part 2#2: Test cell broadcast config

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

::: dom/system/gonk/tests/test_ril_worker_cellbroadcast_config.js
@@ +17,5 @@
> +    }
> +  });
> +
> +  let parcelTypes = [];
> +  let org_newParcel = worker.Buf.newParcel;

orig_newParcel?  What does 'org_newParcel' mean?

@@ +26,5 @@
> +
> +  function setup(options) {
> +    if (options && options.cdma) {
> +      worker.RIL._isCdma = true;
> +    }

We can just have:

  function setup(isCdma) {
    worker.RIL._isCdma = isCdma;
    ...

@@ +46,5 @@
> +  setup({cdma: true});
> +  worker.RIL.setCellBroadcastDisabled({disabled: true});
> +  // Makesure that request parcel is sent out.
> +  do_check_neq(parcelTypes.indexOf(REQUEST_CDMA_SMS_BROADCAST_ACTIVATION), -1);
> +  do_check_eq(worker.RIL.cellBroadcastDisabled, true);

Can we merge these repeating steps into one common test function? So we have:

  test(false, REQUEST_GSM_SMS_BROADCAST_ACTIVATION);
  test(true, REQUEST_CDMA_SMS_BROADCAST_ACTIVATION);

@@ +76,5 @@
> +    recordedParcel = null;
> +    worker.Buf.sendParcel = function() {
> +      org_sendParcel.apply(this, arguments);
> +      if (parcelTypes[parcelTypes.length - 1] == parcelType) {
> +        recordedParcel = currentParcel;

We all know where does the parcel data begin, and that's already addressed in other test script.  So, can you just skip all the complex steps here and validate the sent parcel directly?

@@ +189,5 @@
> +  function setup(options) {
> +    if (options && options.cdma) {
> +      worker.RIL._isCdma = true;
> +    }
> +    worker.RIL.cellBroadcastDisabled = false;

You don't need this in validating |mergedCellBroadcastConfig|, right?

@@ +208,5 @@
> +
> +  // Test merge cell broadcast config. (CDMA)
> +  setup({cdma: true});
> +  worker.RIL._mergeAllCellBroadcastConfigs();
> +  do_check_eq(worker.RIL.mergedCellBroadcastConfig.toString(), "1,2,4,7");

Can we have a test function like:

  test(isCdma, configs, expected) {
    ...
  }

  test(false, {<current configs>}, <expected merged config string>);
  test(true, ....);

Then we can test all possible executing branches of |mergedCellBroadcastConfig|.

Comment 15

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 784910 [details] [diff] [review]
Part 2#2: Test cell broadcast config

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

::: dom/system/gonk/tests/test_ril_worker_cellbroadcast_config.js
@@ +17,5 @@
> +    }
> +  });
> +
> +  let parcelTypes = [];
> +  let org_newParcel = worker.Buf.newParcel;

Original function of Buf.newParcel.
I will replace the function later but would like to keep the original version and use it.

@@ +26,5 @@
> +
> +  function setup(options) {
> +    if (options && options.cdma) {
> +      worker.RIL._isCdma = true;
> +    }

Don't like this. It lacks the information when called by 'setup(true)'
... However, I will accept the suggestion.

@@ +76,5 @@
> +    recordedParcel = null;
> +    worker.Buf.sendParcel = function() {
> +      org_sendParcel.apply(this, arguments);
> +      if (parcelTypes[parcelTypes.length - 1] == parcelType) {
> +        recordedParcel = currentParcel;

Sorry, I can't got the idea between your comment and the code.
The function here is to record the parcel which we are interested because it is possible to sent out several parcels at the same time.

Comment 16

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#3: Test cell broadcast config

Comment 17

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#4: Test cell broadcast config

Comment 18

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

Comment on attachment 786097 [details] [diff] [review]
Part 2#4: Test cell broadcast config

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

::: dom/system/gonk/tests/test_ril_worker_cellbroadcast_config.js
@@ +44,5 @@
> +
> +  run_next_test();
> +});
> +
> +add_test(function test_ril_worker_cellbroadcast_config() {

I mean:

  let worker = newWorker();

  function test(isCdma, configs, expected) {
    let parcelType = isCdma ? REQUEST_CDMA_SET_BROADCAST_SMS_CONFIG
                            : REQUEST_GSM_SET_BROADCAST_SMS_CONFIG;

    let found = false;
    worker.postRILMessage = function (id, parcel) {
      let type = parcel[4] | (parcel[5] << 8) | \
                 (parcel[6] << 16) | (parcel[7] << 24)
      if (type != parcelType) {
        return;
      }

      found = true;
      do_check_eq(parcel.slice(???).toString(), expected);
    },

    worker.RIL._isCdma = isCdma;
    worker.RIL.cellBroadcastDisabled = false;
    worker.RIL.mergedCellBroadcastConfig = configs;

    worker.RIL.setSmsBroadcastConfig(worker.RIL.mergedCellBroadcastConfig);
    do_check_eq(found, true);
  }

  // GSM
  test(false, <config_1>, <expected_1>);
  ...
  test(false, <config_n>, <expected_n>);

  // CDMA
  test(true, <config_1>, <expected_1>);
  ...
  test(true, <config_n>, <expected_n>);

Comment 19

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#5: Test cell broadcast config

Comment 20

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

Comment on attachment 788027 [details] [diff] [review]
Part 2#5: Test cell broadcast config

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

Thank you.

Comment 21

[Szu-Yu Chen [:aknow] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=e575f108f765

Comment 22

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 1: Support cdma cell broadcast config. r=vicamo

Comment 23

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 2: Test cell broadcast config. r=vicamo

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8c23a78dd46d
https://hg.mozilla.org/integration/b2g-inbound/rev/186e3f794eb0

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8c23a78dd46d
https://hg.mozilla.org/mozilla-central/rev/186e3f794eb0