Closed Bug 1094572 Opened 5 years ago Closed 5 years ago

B2G RIL: handle data call fail cause accordingly

Categories

(Firefox OS Graveyard :: RIL, defect)

defect
Not set

Tracking

(b2g-v2.0 affected, b2g-v2.0M affected, b2g-v2.1 affected, b2g-v2.2 fixed)

RESOLVED FIXED
2.2 S1 (5dec)
Tracking Status
b2g-v2.0 --- affected
b2g-v2.0M --- affected
b2g-v2.1 --- affected
b2g-v2.2 --- fixed

People

(Reporter: jessica, Assigned: jessica)

References

Details

Attachments

(2 files, 2 obsolete files)

We should check the status returned by RIL_REQUEST_SETUP_DATA_CALL and act accordingly, such as to retry or not, see [1].

[1] https://github.com/mozilla-b2g/android-hardware-ril/blob/master/include/telephony/ril.h#l366-406
Attached patch patch, v1. (obsolete) — Splinter Review
Comment on attachment 8521268 [details] [diff] [review]
patch, v1.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +3825,4 @@
>      this.state = RIL.GECKO_NETWORK_STATE_DISCONNECTED;
> +
> +    if (this.requestedNetworkIfaces.length === 0) {
> +      if (DEBUG) this.debug("This DataCall is not requested anymore.")

missed semicolon, will fix it in the upcoming patch.
Comment on attachment 8521268 [details] [diff] [review]
patch, v1.

I wonder if we need to detect and block the case where retry is not needed, but data call setup is still triggered by dataregistration/networkinfo change. Actually, updateRILNetworkInterface() should only be called when there is a change, but it is always called now due to design limitation.

Edgar, may I have your review? Thanks.
Attachment #8521268 - Flags: review?(echen)
Comment on attachment 8521268 [details] [diff] [review]
patch, v1.

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

Thank you.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ -775,5 @@
>          oldSettings.oldEnabled = oldSettings.enabled;
>          oldSettings.enabled = false;
>        }
>  
> -      if (oldConnHandler.anyDataConnected()) {

It seems we could remove |anyDataConnected()| as well.

::: dom/system/gonk/ril_worker.js
@@ +4142,5 @@
>        if (newDataCall.status != DATACALL_FAIL_NONE) {
> +        if (newDataCallOptions) {
> +          newDataCallOptions.status = newDataCall.status;
> +          newDataCallOptions.suggestedRetryTime = newDataCall.suggestedRetryTime;
> +        }

Hmm, the code here isn't straightforward to understand to me. But I haven't better idea to handle this, because the |_processDataCallList()| is used for both REQUEST_SETUP_DATA_CALL and REQUEST_DATA_CALL_LIST now. Let's see if we need to do some refactoring.
Attachment #8521268 - Flags: review?(echen) → review+
Thanks for the review, Edgar.

(In reply to Edgar Chen [:edgar][:echen] from comment #4)
> Comment on attachment 8521268 [details] [diff] [review]
> patch, v1.
> 
> Review of attachment 8521268 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you.
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ -775,5 @@
> >          oldSettings.oldEnabled = oldSettings.enabled;
> >          oldSettings.enabled = false;
> >        }
> >  
> > -      if (oldConnHandler.anyDataConnected()) {
> 
> It seems we could remove |anyDataConnected()| as well.

Yes, will do.

> 
> ::: dom/system/gonk/ril_worker.js
> @@ +4142,5 @@
> >        if (newDataCall.status != DATACALL_FAIL_NONE) {
> > +        if (newDataCallOptions) {
> > +          newDataCallOptions.status = newDataCall.status;
> > +          newDataCallOptions.suggestedRetryTime = newDataCall.suggestedRetryTime;
> > +        }
> 
> Hmm, the code here isn't straightforward to understand to me. But I haven't
> better idea to handle this, because the |_processDataCallList()| is used for
> both REQUEST_SETUP_DATA_CALL and REQUEST_DATA_CALL_LIST now. Let's see if we
> need to do some refactoring.

I agree that this is kind of confusing, especially the part that we pass completely different things to |_sendDataCallError()| on data call setup failure and on data call drop. In data call drop case, we are not copying the corresponding apn, username, password, etc, so it relies mostly on 'datacallstatechange' that is sent later. This needs some refactoring indeed.
Attached patch patch (final). (obsolete) — Splinter Review
Address review comment 4:
- remove |anyDataConnected()|
Attachment #8521268 - Attachment is obsolete: true
Attachment #8526588 - Flags: review+
Attached patch patch (final).Splinter Review
Sorry, add missing semicolon in line 3816.
Attachment #8526588 - Attachment is obsolete: true
Attachment #8526627 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/8af17af9fc2e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
See Also: → 1104627
Target Milestone: --- → 2.2 S1 (5dec)
Attached patch patch (v2.0m)Splinter Review
Since I am going to be OOO tomorrow, I am attaching the patch for v2.0m, just in case.
Duplicate of this bug: 1104627
You need to log in before you can comment on or make changes to this bug.