1072367 - [B2G][RIL] broadcast system message "cdma-info-rec-received" in MobileConnectionService

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

Should we broadcast system message "cdma-info-rec-received" in MobileConnectionService instead of in RadioInterfaceLayer, so that we could reduce usage of non-telephony interfaces in RIL?

Comment 1

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

While reviewing current implementation of CdmaPDUHelper.decodeInformationRecord() in ril_worker.js, I found that there might be some issue when receiving multiple records of the same type.
In current implementation, only the last record of the same type will be notified. [1]
After consulting with the assignee of bug 882985, we double confirmed that this wasn't taken into consideration when supporting bug 882985.

I'll address this problem as part of the patches in this bug.

Comment 2

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

2 more issues found in current implementation:
1. For PDU_CDMA_INFO_REC_TYPE_SIGNAL, the 'present' defined in ril.h was explained as "non-zero if signal information record is present". We might have to ignore this record if 'present' is set to zero.
2. For PDU_CDMA_INFO_REC_TYPE_EXTENDED_DISPLAY, if we refer to the implementation in AOSP since Donut, actually, the corresponding parcel shall be handled in the same way to PDU_CDMA_INFO_REC_TYPE_DISPLAY.[1][2] However, we parse it according to the layout defined in 3.7.5.16 in 3GPP2 C.S0005-F but there is no guarantee for this according to the definition in ril.h.

[1] http://androidxref.com/1.6/xref/frameworks/base/telephony/java/com/android/internal/telephony/cdma/CdmaInformationRecords.java#43
[2] http://androidxref.com/1.6/xref/hardware/ril/libril/ril.cpp#1604

Comment 3

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

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #1)
> While reviewing current implementation of
> CdmaPDUHelper.decodeInformationRecord() in ril_worker.js, I found that there
> might be some issue when receiving multiple records of the same type.
> In current implementation, only the last record of the same type will be
> notified. [1]
Sorry, update reference [1].
[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#9975
> After consulting with the assignee of bug 882985, we double confirmed that
> this wasn't taken into consideration when supporting bug 882985.
> 
> I'll address this problem as part of the patches in this bug.

Comment 4

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

Attachment: Part1 v1: Broadcast 'cdma-info-rec-received' system message per record.

This is to fix the problem found in comment 1 to allow multiple records of the same type be received correctly.

Hi Edgar,

May I have your review for this change?

Thanks!

Comment 5

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

Attachment: Part2 v1: Decode EXTENDED_DISPLAY & SIGNAL properly. r=echen

This patch is to handle EXTENDED_DISPLAY & SIGNAL properly.
More detailed explanation is available in comment 2.

Hi Edgar,

May I have your review of this change?

Thanks!

Comment 6

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

Attachment: Part3 v1: broadcast system message "cdma-info-rec-received" in MobileConnectionService.

Changes of this patch are:
1. Move the broadcasting implementation from RadioInterfaceLayer to MobileConnection.
2. give property proper naming.
Note: Only 2 types of records are used in Gaia. It shall be fine to rename these property without impact. [1]

[1] https://github.com/mozilla-b2g/gaia/blob/3802009e1ab6c3ddfc3eb15522e3140a96b33336/apps/system/js/carrier_info_notifier.js#L21

Comment 7

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

Attachment: TestOnly: Fake CdmaInfoRecord for verifying related SystemMessages.

There is no proper way to verify system messages yet.
This modification is helpful to fake CdmaInformationRecords and double check if the system message object was created properly from the log.

Comment 8

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Comment on attachment 8500260 [details] [diff] [review]
Part2 v1: Decode EXTENDED_DISPLAY & SIGNAL properly. r=echen

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

::: dom/system/gonk/ril_worker.js
@@ +10020,4 @@
>            record.signal.type = Buf.readInt32();
>            record.signal.alertPitch = Buf.readInt32();
>            record.signal.signal = Buf.readInt32();
> +          if (!record.signal.present) {

nit: We add |record.signal| into records only if present is 'true'. we don't need to set present into signal through it doesn't really being exposed to Gaia.

if (Buf.readInt32() != 0) {
  Buf.seekIncoming(3 * Buf.UINT32_SIZE);
  continue;
}

record.signal = {
  type: Buf.readInt32(),
  alertPitch: Buf.readInt32(),
  signal: Buf.readInt32()
};

@@ -10046,5 @@
> -
> -          let headerByte = Buf.readInt32();
> -          length--;
> -          // Based on spec, headerByte must be 0x80 now
> -          record.extendedDisplay.indicator = (headerByte >> 7);

"cdma-info-rec-received" won't include |extendedDisplay| any more, please file a bug to Gaia [1] for this change.
Thank you.

[1] https://github.com/mozilla-b2g/gaia/blob/f694a6fdde2063335af9dfa78a0e82aae9df546d/apps/system/js/carrier_info_notifier.js#L25-L30

Comment 9

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Comment on attachment 8500262 [details] [diff] [review]
Part3 v1: broadcast system message "cdma-info-rec-received" in MobileConnectionService.

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

Looks good to me but I would like to review it again after you add xpcshell tests mentioned below. Thank you. :)

One more thing. Please help to file a follow-up bug about writing a test case for system message "cdma-info-rec-received", though emulator doesn't support generating UNSOLICITED_CDMA_INFO_REC and I met some problem when writing a test case for another system message in bug 945576. :(

::: dom/mobileconnection/gonk/MobileConnectionService.js
@@ +1010,5 @@
>                Services.prefs.getBoolPref(kPrefRilDebuggingEnabled);
>      } catch (e) {}
>    },
>  
> +  broadcastCdmaInfoRecordSystemMessage: function(aMessage) {

Add a "_" prefix given that it should be a private method.

@@ +1251,5 @@
> +      plan: aPlan,
> +      number: aNumber,
> +      pi: aPi,
> +      si: aSi
> +    };

nit: here and elsewhere

this._broadcastCdmaInfoRecordSystemMessage({
  clientId: aClientId,
  calledNumber: {
    type: aType,
    plan: aPlan,
    number: aNumber,
    pi: aPi,
    si: aSi
  }
});

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2978,5 @@
>                               hasEtwsInfo ? etwsInfo.emergencyUserAlert : false,
>                               hasEtwsInfo ? etwsInfo.popup : false);
>    },
>  
> +  handleCdmaInformationRecords: function (aRecords) {

nit: don't need a space between 'function' and '('

::: dom/system/gonk/ril_worker.js
@@ +10036,5 @@
>          case PDU_CDMA_INFO_REC_TYPE_LINE_CONTROL:
>            record.lineControl = {};
>            record.lineControl.polarityIncluded = Buf.readInt32();
>            record.lineControl.toggle = Buf.readInt32();
> +          record.lineControl.reverse = Buf.readInt32();

Could you help to add xpcshell tests for this?

@@ +10041,4 @@
>            record.lineControl.powerDenial = Buf.readInt32();
>            break;
>          case PDU_CDMA_INFO_REC_TYPE_T53_CLIR:
> +          record.clirCause = Buf.readInt32();

Ditto

Comment 10

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

Attachment: Part1 v2: Broadcast 'cdma-info-rec-received' system message per record. r=echen

Comment 11

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

Attachment: Part2 v2: Decode EXTENDED_DISPLAY & SIGNAL properly. r=echen

address nits in comment 8.

Comment 12

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

Attachment: Part3 v2: broadcast system message "cdma-info-rec-received" in MobileConnectionService.

Address suggestion in comment 9.

Hi Edgar,

May I have your review again for this change?

Thanks!

Comment 13

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Comment on attachment 8502402 [details] [diff] [review]
Part3 v2: broadcast system message "cdma-info-rec-received" in MobileConnectionService.

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

Thank you. :)

Comment 14

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

update try server result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8836a84f9094

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/0dbedf3aec62
https://hg.mozilla.org/integration/b2g-inbound/rev/80c3e34f21ff
https://hg.mozilla.org/integration/b2g-inbound/rev/dcf3a8f45e6c

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0dbedf3aec62
https://hg.mozilla.org/mozilla-central/rev/80c3e34f21ff
https://hg.mozilla.org/mozilla-central/rev/dcf3a8f45e6c