Closed Bug 1072367 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S7 (24Oct)

People

(Reporter: hsinyi, Assigned: bevis)

References

Details

Attachments

(4 files, 3 obsolete files)

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?
Blocks: 1072808
Assignee: nobody → btseng
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.
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
(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.
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)
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)
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)
Attachment #8500260 - Attachment description: Part2 v2: Decode EXTENDED_DISPLAY & SIGNAL properly. r=echen → Part2 v1: Decode EXTENDED_DISPLAY & SIGNAL properly. r=echen
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.
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.
Attachment #8500259 - Flags: review?(echen) → review+
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 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)
Blocks: 1080365
address nits in comment 8.
Attachment #8500260 - Attachment is obsolete: true
Attachment #8502400 - Flags: review+
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 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+
You need to log in before you can comment on or make changes to this bug.