1113054 - [B2G][RIL] Support RIL version 10

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

From AOSP 5.0 (Lollipop), the version of RIL is using v10 [1].
We should add corresponding const or handler in MOZ_RIL for the new parcels introduced in RIL v10.
And also some partner's specific parcel may get conflict with those new parcels, we should fix them as well.

[1] https://android.googlesource.com/platform/hardware/ril/+/lollipop-release/include/telephony/ril.h

Comment 1

[Edgar Chen [:edgar]]

Attachment: diff_with_v9.patch

Comment 2

[Edgar Chen [:edgar]]

Attachment: Patch, v1

Comment 3

[Steven Yang [:styang]]

Hi Edgar, do you think this issue can be done by sprint 3(~Jan/9)?thanks.

Comment 4

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

Per discussion with EPM, |Bug 1107303 - (gonk-L-RIL) [meta] RIL Android L Porting (feature-b2g:2.2)| focuses on nexus5-L porting for now. As this bug is for the future DSDS device, flame-L, and more RIL feature support, this isn't the blocker of bug 1107303. I'll remove the dependency for now. Please correct me if I get something wrong, thank you.

Comment 5

[Edgar Chen [:edgar]]

Per comment #4, it is not in 2.2+ list, remove n?.

Comment 6

[Edgar Chen [:edgar]]

Attachment: Patch, v2

Comment 7

[Edgar Chen [:edgar]]

Attachment: Patch, v3

Comment 8

[Edgar Chen [:edgar]]

Comment on attachment 8561861 [details] [diff] [review]
Patch, v3

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

I did some basic tests in nexus-5-l and flame-kk (with also applying the patch of bug 1130938), both of two devices works good.
Although I don't know if the secure element related parcel [1] works as expected, but we could track them in bug 1120177 and fix the issue there.

Hi Hsinyi, may I have your review? Thank you.

[1] TRANSMIT_APDU_BASIC, OPEN_CHANNEL and CLOSE_CHANNEL

Comment 9

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

This could be a solution for NFC 2.2 feature testing.

Comment 10

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

Comment on attachment 8561861 [details] [diff] [review]
Patch, v3

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

Looks really great!

::: dom/system/gonk/ril_worker.js
@@ +6389,5 @@
> +  this.sendChromeMessage(options);
> +};
> +RilObject.prototype[REQUEST_SIM_CLOSE_CHANNEL] = function REQUEST_SIM_CLOSE_CHANNEL(length, options) {
> +  if (options.rilRequestError) {
> +    options.error = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];

oops... how did we end up with this :P

b.t.w, let's just use |this.sendDefaultResponse(options)| for the whole function REQUEST_SIM_CLOSE_CHANNEL.

@@ +6399,5 @@
> +  this.sendChromeMessage(options);
> +};
> +RilObject.prototype[REQUEST_SIM_TRANSMIT_APDU_CHANNEL] = function REQUEST_SIM_TRANSMIT_APDU_CHANNEL(length, options) {
> +  if (options.rilRequestError) {
> +    options.error = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];

A flaw has existing for a while... 
s/options.error/options.errorMsg/

Comment 11

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai (Chinese new year OOO Feb. 18 ~ Feb. 27) [:hsinyi] from comment #10)
> Comment on attachment 8561861 [details] [diff] [review]
> Patch, v3
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +6389,5 @@
> > +  this.sendChromeMessage(options);
> > +};
> > +RilObject.prototype[REQUEST_SIM_CLOSE_CHANNEL] = function REQUEST_SIM_CLOSE_CHANNEL(length, options) {
> > +  if (options.rilRequestError) {
> > +    options.error = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
> 
> oops... how did we end up with this :P
> 
> b.t.w, let's just use |this.sendDefaultResponse(options)| for the whole
> function REQUEST_SIM_CLOSE_CHANNEL.
> 

This reminds me bug 991582, I should continue the work there. :p

> @@ +6399,5 @@
> > +  this.sendChromeMessage(options);
> > +};
> > +RilObject.prototype[REQUEST_SIM_TRANSMIT_APDU_CHANNEL] = function REQUEST_SIM_TRANSMIT_APDU_CHANNEL(length, options) {
> > +  if (options.rilRequestError) {
> > +    options.error = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
> 
> A flaw has existing for a while... 
> s/options.error/options.errorMsg/

Thanks for catching this.

Comment 12

[Edgar Chen [:edgar]]

Attachment: Patch, v4, r=hsinyi

Address review comment #10.
- Use |this.sendDefaultResponse(options)| for REQUEST_SIM_CLOSE_CHANNEL.
- s/options.error/options.errorMsg/

Comment 13

[Edgar Chen [:edgar]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b0f8bd0c6e8

Comment 14

[Edgar Chen [:edgar]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Krzysztof Mioduszewski[:tauzen]]

If we want to use this for 2.2 NFC/SecureElement testing as Wesley wrote in comment 9, this needs to be uplifted to 2.2. Is this possible?

Comment 18

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

I think this should be uplifted to 2.2 to unblock SecureElement feature testing.
Edgar, could you ask for approval?

Comment 19

[Vincent Chang[:vchang][changyihsin]]

Clear NI.

Comment 20

[Edgar Chen [:edgar]]

(In reply to Krzysztof Mioduszewski[:tauzen] from comment #17)
> If we want to use this for 2.2 NFC/SecureElement testing as Wesley wrote in
> comment 9, this needs to be uplifted to 2.2. Is this possible?

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #18)
> I think this should be uplifted to 2.2 to unblock SecureElement feature
> testing.
> Edgar, could you ask for approval?

Sure, I will ask for approval and provide patch for 2.2 if needed.

Comment 21

[Edgar Chen [:edgar]]

Attachment: [b2g37_v2_2] Patch, v1, r=hsinyi

Patch for b2g37_v2_2.

Comment 22

[Edgar Chen [:edgar]]

Comment on attachment 8572511 [details] [diff] [review]
[b2g37_v2_2]  Patch, v1, r=hsinyi

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Feature
User impact if declined: User cannot use secureElement feature
Testing completed: Try server and manually test on real device
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: No

Try Server result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d217e734497e

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/83e91fc19166

Comment 24

[Krzysztof Mioduszewski[:tauzen]]

Master version is working fine for me. Was trying to verify the 2.2 version, and I'm getting a TypeError, on closing the channel.

>I/Gecko   ( 2921): RIL Worker: [0] Handling parcel as REQUEST_SIM_CLOSE_CHANNEL
>I/Gecko   ( 2921): RIL Worker: Parcel handling threw TypeError: this.sendDefaultResponse is not a function
>I/Gecko   ( 2921): REQUEST_SIM_CLOSE_CHANNEL@resource://gre/modules/ril_worker.js:6689:3

Do we need a separate bug for that, or can we fix this here?

Comment 25

[Edgar Chen [:edgar]]

(In reply to Krzysztof Mioduszewski[:tauzen] from comment #24)
> Master version is working fine for me. Was trying to verify the 2.2 version,
> and I'm getting a TypeError, on closing the channel.
> 
> >I/Gecko   ( 2921): RIL Worker: [0] Handling parcel as REQUEST_SIM_CLOSE_CHANNEL
> >I/Gecko   ( 2921): RIL Worker: Parcel handling threw TypeError: this.sendDefaultResponse is not a function
> >I/Gecko   ( 2921): REQUEST_SIM_CLOSE_CHANNEL@resource://gre/modules/ril_worker.js:6689:3
> 
> Do we need a separate bug for that, or can we fix this here?

Let me provide a follow-up patch to fix it here (Keeps ni? to me for tracking). Thank you.

Comment 26

[Edgar Chen [:edgar]]

Attachment: [b2g37_v2_2] (follow-up) Fix TypeError, v1

Comment 27

[Edgar Chen [:edgar]]

Comment on attachment 8574480 [details] [diff] [review]
[b2g37_v2_2] (follow-up) Fix TypeError, v1

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

Hi :tauzen, would you mind helping to verify this patch? Thank you.

Comment 28

[Edgar Chen [:edgar]]

Comment on attachment 8574480 [details] [diff] [review]
[b2g37_v2_2] (follow-up) Fix TypeError, v1

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

Hi Hsinyi, |sendDefaultResponse| haven't yet be introduced in 2.2, I should use the old way to handle RIL solicited response when preparing 2.2 patch. Would you mind reviewing this follow-up patch? Thank you.

Comment 29

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

Comment on attachment 8574480 [details] [diff] [review]
[b2g37_v2_2] (follow-up) Fix TypeError, v1

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

Thank you!

Comment 30

[Krzysztof Mioduszewski[:tauzen]]

Comment on attachment 8574480 [details] [diff] [review]
[b2g37_v2_2] (follow-up) Fix TypeError, v1

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

Verified using demo wallet app. I'm now able to successfully close channel, everything work fine. Thanks!

Comment 31

[Edgar Chen [:edgar]]

Comment on attachment 8574480 [details] [diff] [review]
[b2g37_v2_2] (follow-up) Fix TypeError, v1

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Patch for b2g37_v2_2
User impact if declined: NFC/SecureElement gets a TypeError on closing the channel.
Testing completed: Try server and manually test.
Risk to taking this patch (and alternatives if risky): Low 
String or UUID changes made by this patch: None

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2fb0cbebed9

Comment 32

[bhavana bajaj [:bajaj]]

Comment on attachment 8574480 [details] [diff] [review]
[b2g37_v2_2] (follow-up) Fix TypeError, v1

Thanks for adding the try result :)

Comment 33

[Edgar Chen [:edgar]]

(In reply to bhavana bajaj [:bajaj] from comment #32)
> Comment on attachment 8574480 [details] [diff] [review]
> [b2g37_v2_2] (follow-up) Fix TypeError, v1
> 
> Thanks for adding the try result :)

Hi Ryan, could you help to merge this follow-up patch? I missed tracking this, sorry about that.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/b06978fc0442

Comment 36

[Pi Wei Cheng [:piwei] (inactive)]

backend fix. unable to verify.