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

RESOLVED FIXED in 2.1 S7 (24Oct)

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: hsinyi, Assigned: bevis)

Tracking

unspecified
2.1 S7 (24Oct)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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?
(Assignee)

Updated

4 years ago
Blocks: 1072808
(Assignee)

Updated

4 years ago
Assignee: nobody → btseng
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
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
(Assignee)

Comment 3

4 years ago
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) 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.
(Assignee)

Comment 4

4 years ago
Created attachment 8500259 [details] [diff] [review]
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!
Attachment #8500259 - Flags: review?(echen)
(Assignee)

Comment 5

4 years ago
Created attachment 8500260 [details] [diff] [review]
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!
Attachment #8500260 - Flags: review?(echen)
(Assignee)

Comment 6

4 years ago
Created attachment 8500262 [details] [diff] [review]
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
Attachment #8500262 - Flags: review?(echen)
(Assignee)

Updated

4 years ago
Attachment #8500260 - Attachment description: Part2 v2: Decode EXTENDED_DISPLAY & SIGNAL properly. r=echen → Part2 v1: Decode EXTENDED_DISPLAY & SIGNAL properly. r=echen
(Assignee)

Comment 7

4 years ago
Created attachment 8500263 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8500259 - Attachment description: Part 1 v1: Broadcast 'cdma-info-rec-received' system message per record. → Part1 v1: Broadcast 'cdma-info-rec-received' system message per record.

Updated

4 years ago
Attachment #8500259 - Flags: review?(echen) → review+

Comment 8

4 years ago
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
Attachment #8500260 - Flags: review?(echen) → review+

Comment 9

4 years ago
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
Attachment #8500262 - Flags: review?(echen)
(Assignee)

Updated

4 years ago
Blocks: 1080365
Created attachment 8502399 [details] [diff] [review]
Part1 v2: Broadcast 'cdma-info-rec-received' system message per record. r=echen
Attachment #8500259 - Attachment is obsolete: true
Attachment #8502399 - Flags: review+
Created attachment 8502400 [details] [diff] [review]
Part2 v2: Decode EXTENDED_DISPLAY & SIGNAL properly. r=echen

address nits in comment 8.
Attachment #8500260 - Attachment is obsolete: true
Attachment #8502400 - Flags: review+
Created attachment 8502402 [details] [diff] [review]
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!
Attachment #8500262 - Attachment is obsolete: true
Attachment #8502402 - Flags: review?(echen)

Comment 13

4 years ago
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. :)
Attachment #8502402 - Flags: review?(echen) → review+
update try server result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8836a84f9094
Keywords: checkin-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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (24Oct)
You need to log in before you can comment on or make changes to this bug.