[Aries][RIL] Reply from QUERY_AVAILABLE_NETWORKS has more data than expected

RESOLVED FIXED

Status

Firefox OS
RIL
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: edgar, Assigned: edgar)

Tracking

({foxfood})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Created attachment 8625803 [details]
logact.log

06-23 11:50:52.937   317   783 I Gecko   : RIL Worker: [0] Handling parcel as REQUEST_QUERY_AVAILABLE_NETWORKS
06-23 11:50:52.957   317   783 I Gecko   : RIL Worker: [0] Error processing operator tuple: Error: Invalid network tuple (should be 5 or 6 digits): Rogers Wireless
06-23 11:50:52.957   317   783 I Gecko   : RIL Worker: [0] Error processing operator tuple: Error: Invalid network tuple (should be 5 or 6 digits): lte
06-23 11:50:52.957   317   783 I Gecko   : RIL Worker: [0] Error processing operator tuple: Error: Invalid network tuple (should be 5 or 6 digits): available
06-23 11:50:52.957   317   783 I Gecko   : RIL Worker: [0] Error processing operator tuple: Error: Invalid network tuple (should be 5 or 6 digits): Rogers Wireless
06-23 11:50:52.957   317   783 I Gecko   : RIL Worker: [0] Error processing operator tuple: Error: Invalid network tuple (should be 5 or 6 digits): lte
06-23 11:50:52.957   317   783 I Gecko   : RIL Worker: [0] Error processing operator tuple: Error: Invalid network tuple (should be 5 or 6 digits): available
06-23 11:50:52.957   317   783 I Gecko   : RIL Worker: Next parcel size unknown, going to sleep.
06-23 11:50:52.957   317   317 I Gecko   : -*- RadioInterface[0]: Received message from worker: {"rilMessageClientId":0,"rilMessageToken":254,"rilMessageType":"getAvailableNetworks","rilRequestType":48,"networks":[{"longName":"Bell","shortName":"Bell","mcc":"302","mnc":"610","state":"available"},{"longName":"wcdma","shortName":"TELUS","mcc":"TEL","mnc":"US"},{"longName":"available","shortName":"wcdma","mcc":null,"mnc":null},{"longName":"302720","shortName":"available","mcc":null,"mnc":null},{"longName":"RogersWireless","shortName":"302720","mcc":null,"mnc":null},{"longName":"WIND","shortName":"WIND","mcc":"302","mnc":"490","state":"forbidden"},{"longName":"wcdma","shortName":"Rogers Wireless","mcc":null,"mnc":null},{"longName":"available","shortName":"gsm","mcc":"TEL","mnc":"US"},{"longName":"302220","shortName":"available","mcc":null,"mnc":null},{"longName":"Bell","shortName":"302610","mcc":null,"mnc":null}]}
Summary: [Aries][RIL] Unable to QUERY_AVAILABLE_NETWORKS → [Aries][RIL] Reply from QUERY_AVAILABLE_NETWORKS has more data than expected
Created attachment 8625867 [details] [diff] [review]
networks.patch

That's a workaround I have on my gecko
Duplicate of this bug: 1177101
Keywords: foxfood
So ?
Flags: needinfo?(echen)
(Assignee)

Updated

2 years ago
Assignee: nobody → echen
Flags: needinfo?(echen)
(Assignee)

Comment 4

2 years ago
And we need a new quirk for this extra field.
(Assignee)

Comment 5

2 years ago
Created attachment 8630834 [details] [diff] [review]
Part 1: [Aries][RIL] Reply from QUERY_AVAILABLE_NETWORKS has extra strings, v1
(Assignee)

Comment 6

2 years ago
Created attachment 8630835 [details] [review]
Part 2: Add mozril quirk ro.moz.ril.avlbl_nw_extra_str to shinano-common
(Assignee)

Comment 7

2 years ago
Created attachment 8630836 [details] [diff] [review]
Part 1: [Aries][RIL] Reply from QUERY_AVAILABLE_NETWORKS has extra strings, v2
Attachment #8630834 - Attachment is obsolete: true
(Assignee)

Comment 8

2 years ago
Comment on attachment 8630836 [details] [diff] [review]
Part 1: [Aries][RIL] Reply from QUERY_AVAILABLE_NETWORKS has extra strings, v2

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

Hi Hsinyi,

In this patch, I add a new quirk, "ro.moz.ril.avlbl_nw_extra_str", for the extra string in the response of QUERY_AVAILABLE_NETWORKS on z3c.
And also do some clean-up for other quirks,
Could you please help to review this?

Thank you
Attachment #8630836 - Flags: review?(htsai)
(Assignee)

Comment 9

2 years ago
Comment on attachment 8630835 [details] [review]
Part 2: Add mozril quirk ro.moz.ril.avlbl_nw_extra_str to shinano-common

Hi Alexandre,

I add a new quirk, "ro.moz.ril.avlbl_nw_extra_str", for the extra string in the response of QUERY_AVAILABLE_NETWORKS for z3c.
Could you please help to review this?

Thank you.
Attachment #8630835 - Flags: review?(lissyx+mozillians)
Comment on attachment 8630835 [details] [review]
Part 2: Add mozril quirk ro.moz.ril.avlbl_nw_extra_str to shinano-common

I'm not a big fan of the name :). Why making it "obscured" ?

Anyway, and I don't know why there is this behavior in the KK Sony blobs, but it looks obvious there is a pattern of extending some of the RIL replies. So maybe, instead of landing a new property each time, we should have a more generic one?

We already have the use for the extra string present, and for the extra field in the signal level.

What's your opinion Hsin-Yi ? If you consider we should keep two separates properties, let's go with r+ after renaming the property to something less obscure, like "avlbl_nw_extra_str" => "available_networks_extra_str" ?
Attachment #8630835 - Flags: review?(lissyx+mozillians)
Attachment #8630835 - Flags: review+
Attachment #8630835 - Flags: feedback?(htsai)
(Assignee)

Comment 11

2 years ago
(In reply to Alexandre LISSY :gerard-majax from comment #10)
> Comment on attachment 8630835 [details] [review]
> Part 2: Add mozril quirk ro.moz.ril.avlbl_nw_extra_str to shinano-common
> 
> I'm not a big fan of the name :). Why making it "obscured" ?

It is because there is a limitation for the length of property name [1], the max length is 32.
`ro.moz.ril.available_networks_extra_str` is too long, so I use the abbreviated version. :p

[1] https://github.com/android/platform_bionic/blob/lollipop-mr1-release/libc/include/sys/system_properties.h#L38
(In reply to Alexandre LISSY :gerard-majax from comment #10)
> Comment on attachment 8630835 [details] [review]
> Part 2: Add mozril quirk ro.moz.ril.avlbl_nw_extra_str to shinano-common
> 
> I'm not a big fan of the name :). Why making it "obscured" ?
> 
> Anyway, and I don't know why there is this behavior in the KK Sony blobs,
> but it looks obvious there is a pattern of extending some of the RIL
> replies. So maybe, instead of landing a new property each time, we should
> have a more generic one?
> 
> We already have the use for the extra string present, and for the extra
> field in the signal level.
> 
> What's your opinion Hsin-Yi ? If you consider we should keep two separates
> properties, let's go with r+ after renaming the property to something less
> obscure, like "avlbl_nw_extra_str" => "available_networks_extra_str" ?

The quirks we used to have were defined by "feature" not specifically by "device." However, I tend to thinking that quirk by device might make the code looks clearer in this case. So having a shinano-specific quirk sounds a good idea to me. Just one thing to note, if we want to have that shinano quirk, we should not keep using the existing "ro.moz.ril.signal_extra_int", we should have a new name, instead.

Updated

2 years ago
Attachment #8630835 - Flags: feedback?(htsai)
(Assignee)

Comment 13

2 years ago
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> The quirks we used to have were defined by "feature" not specifically by
> "device." However, I tend to thinking that quirk by device might make the
> code looks clearer in this case. So having a shinano-specific quirk sounds a
> good idea to me. Just one thing to note, if we want to have that shinano
> quirk, we should not keep using the existing "ro.moz.ril.signal_extra_int",
> we should have a new name, instead.

What about reusing the existed property `ro.product.device` for device quirk? Then we don't need to introduce a new property. But note that Z3 and Z3C has different device name. If we reuse this as device quirk, we have to check both of them.

The value of `ro.product.device` in different devices:
- Z3 --> "shinano"
- Z3C --> "aries"
- Nexus-5 --> "hammerhead"
Flags: needinfo?(htsai)
(In reply to Edgar Chen [:edgar][:echen] from comment #13)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> > The quirks we used to have were defined by "feature" not specifically by
> > "device." However, I tend to thinking that quirk by device might make the
> > code looks clearer in this case. So having a shinano-specific quirk sounds a
> > good idea to me. Just one thing to note, if we want to have that shinano
> > quirk, we should not keep using the existing "ro.moz.ril.signal_extra_int",
> > we should have a new name, instead.
> 
> What about reusing the existed property `ro.product.device` for device
> quirk? Then we don't need to introduce a new property. But note that Z3 and
> Z3C has different device name. If we reuse this as device quirk, we have to
> check both of them.
> 
> The value of `ro.product.device` in different devices:
> - Z3 --> "shinano"
> - Z3C --> "aries"
> - Nexus-5 --> "hammerhead"

Sounds good to me :)
Flags: needinfo?(htsai)
(Assignee)

Comment 15

2 years ago
Comment on attachment 8630836 [details] [diff] [review]
Part 1: [Aries][RIL] Reply from QUERY_AVAILABLE_NETWORKS has extra strings, v2

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

I am going to address comment #13, so cancel the review first.
Thank you, Hsinyi and Alexandre.
Attachment #8630836 - Flags: review?(htsai)
Edgar, I'm against this approach because it should be specific to KK Sony's blobs. As far as we could cross check, blobs provided by Sony AOSP Lollipop won't have those extra fields. I do have recent builds to share if you need to investigate.
Flags: needinfo?(echen)
(Assignee)

Comment 17

2 years ago
(In reply to Alexandre LISSY :gerard-majax from comment #16)
> Edgar, I'm against this approach because it should be specific to KK Sony's
> blobs. As far as we could cross check, blobs provided by Sony AOSP Lollipop
> won't have those extra fields. I do have recent builds to share if you need
> to investigate.

Please share it to me, I can take a look. Thank you.

I thought `ro.product.device` is a generic property which is generated by android build system [1].
And the value is from the PRODUCT_DEVICE[2] defined in .mk file.

[1] http://androidxref.com/5.1.1_r6/xref/build/tools/buildinfo.sh#23
[2] https://github.com/mozilla-b2g/device-sony-aries/blob/sony-aosp-l/aries.mk#L18
Flags: needinfo?(echen) → needinfo?(lissyx+mozillians)
(In reply to Edgar Chen [:edgar][:echen] from comment #17)
> (In reply to Alexandre LISSY :gerard-majax from comment #16)
> > Edgar, I'm against this approach because it should be specific to KK Sony's
> > blobs. As far as we could cross check, blobs provided by Sony AOSP Lollipop
> > won't have those extra fields. I do have recent builds to share if you need
> > to investigate.
> 
> Please share it to me, I can take a look. Thank you.
> 
> I thought `ro.product.device` is a generic property which is generated by
> android build system [1].
> And the value is from the PRODUCT_DEVICE[2] defined in .mk file.
> 
> [1] http://androidxref.com/5.1.1_r6/xref/build/tools/buildinfo.sh#23
> [2]
> https://github.com/mozilla-b2g/device-sony-aries/blob/sony-aosp-l/aries.
> mk#L18

That's exactly the point: product device is not enough to discriminate between KK and L :)
Flags: needinfo?(lissyx+mozillians)
(Assignee)

Comment 19

2 years ago
(In reply to Alexandre LISSY :gerard-majax from comment #16)
> Edgar, I'm against this approach because it should be specific to KK Sony's
> blobs. As far as we could cross check, blobs provided by Sony AOSP Lollipop
> won't have those extra fields. I do have recent builds to share if you need
> to investigate.

(In reply to Alexandre LISSY :gerard-majax from comment #18)
> That's exactly the point: product device is not enough to discriminate
> between KK and L :)

I have checked the aries-l build, it doesn't have those extra fields (in both signal and available network), so using `ro.product.device` isn't a proper way for sure. Due to this, I would like to go back to the original proposal: define quirk by "feature" which seems a simpler and clearer way to me. Thank you.
(Assignee)

Comment 20

2 years ago
Comment on attachment 8630836 [details] [diff] [review]
Part 1: [Aries][RIL] Reply from QUERY_AVAILABLE_NETWORKS has extra strings, v2

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

According to comment #19, may I have your review again? Thank you.
Attachment #8630836 - Flags: review?(htsai)
Comment on attachment 8630835 [details] [review]
Part 2: Add mozril quirk ro.moz.ril.avlbl_nw_extra_str to shinano-common

comment 19 makes sense to me!
Attachment #8630835 - Flags: feedback+
QA Whiteboard: [foxfood-triage]
Comment on attachment 8630836 [details] [diff] [review]
Part 1: [Aries][RIL] Reply from QUERY_AVAILABLE_NETWORKS has extra strings, v2

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

Ship it!
Attachment #8630836 - Flags: review?(htsai) → review+
(Assignee)

Comment 23

2 years ago
Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=80f68f2dc13c&exclusion_profile=false

Comment 24

2 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/ad35e6706f9b
(Assignee)

Comment 25

2 years ago
checkin-needed for github pull request.
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/device-shinano-common/commit/6698c2a6aa4115f00b1f25fec32f10b2c5c2f454
Keywords: checkin-needed
(Assignee)

Comment 27

2 years ago
https://hg.mozilla.org/mozilla-central/rev/ad35e6706f9b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.