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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

Attachment: 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}]}

Comment 1

[:gerard-majax]

Attachment: networks.patch

That's a workaround I have on my gecko

Comment 3

[:gerard-majax]

So ?

Comment 4

[Edgar Chen [:edgar]]

And we need a new quirk for this extra field.

Comment 5

[Edgar Chen [:edgar]]

Attachment: Part 1: [Aries][RIL] Reply from QUERY_AVAILABLE_NETWORKS has extra strings, v1

Comment 6

[Edgar Chen [:edgar]]

Attachment: Part 2: Add mozril quirk ro.moz.ril.avlbl_nw_extra_str to shinano-common

Comment 7

[Edgar Chen [:edgar]]

Attachment: Part 1: [Aries][RIL] Reply from QUERY_AVAILABLE_NETWORKS has extra strings, v2

Comment 8

[Edgar Chen [:edgar]]

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

Comment 9

[Edgar Chen [:edgar]]

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.

Comment 10

[:gerard-majax]

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" ?

Comment 11

[Edgar Chen [:edgar]]

(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

Comment 12

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

(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.

Comment 13

[Edgar Chen [:edgar]]

(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"

Comment 14

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

(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 :)

Comment 15

[Edgar Chen [:edgar]]

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.

Comment 16

[:gerard-majax]

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.

Comment 17

[Edgar Chen [:edgar]]

(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

Comment 18

[:gerard-majax]

(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 :)

Comment 19

[Edgar Chen [:edgar]]

(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.

Comment 20

[Edgar Chen [:edgar]]

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.

Comment 21

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

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!

Comment 22

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

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!

Comment 23

[Edgar Chen [:edgar]]

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

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/ad35e6706f9b

Comment 25

[Edgar Chen [:edgar]]

checkin-needed for github pull request.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/device-shinano-common/commit/6698c2a6aa4115f00b1f25fec32f10b2c5c2f454

Comment 27

[Edgar Chen [:edgar]]

https://hg.mozilla.org/mozilla-central/rev/ad35e6706f9b