B2G RIL: Handle request errors

RESOLVED FIXED in mozilla13

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sinker, Assigned: Price Tseng)

Tracking

Trunk
mozilla13
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
We need to report errors for B2G RIL requests rather than silent faults.
(Reporter)

Updated

6 years ago
Blocks: 710489
(Reporter)

Comment 1

6 years ago
Created attachment 584270 [details] [diff] [review]
Handle request B2G RIL request error
(Reporter)

Comment 2

6 years ago
Created attachment 584272 [details] [diff] [review]
Handle request B2G RIL request error, v2

--
Attachment #584270 [details] [diff] - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #584270 - Attachment is obsolete: true
Comment on attachment 584272 [details] [diff] [review]
Handle request B2G RIL request error, v2

I don't really understand why you're sending the RIL request constant to the main thread. As you mention in your comment, this will be meaningless to Gecko. Instead, I think we want to associate the request type with the corresponding request (if there was one), and send a more meaningful message to main thread, containing contextual information about the request that failed.
(Also, in the future, if you would like a patch reviewed, please request review for it. Otherwise it's likely nobody will look at it.)
(Reporter)

Comment 5

6 years ago
Comment on attachment 584272 [details] [diff] [review]
Handle request B2G RIL request error, v2

This patch is out-of-date since we have made a lot of change.
Attachment #584272 - Attachment is obsolete: true
Duplicate of this bug: 725099
For meaningful error handling, the mainthread should be notified about what operation actually failed. To that end, I think we should hold on to the `options` object that we receive via postMessage from the mainthread. `Buf.newParcel()` is already set up to do this. Then when we process parcels and find errors, we can simply attach the error to `options` and send the entire object back to the mainthread which can then deduce the error and notify callbacks or whatever (the mainthread now has complete flexibility over how to manage the round trip)
(Reporter)

Comment 8

6 years ago
There is no unique identification for every request from main thread.  Since ril_worker.js works in async semantically, we need a unique idenitfication that main thread or other threads can identify messages from ril_worker.js for requests uniquely.  We can do it by attaching a serial number maintained by RadioInterfaceLayer.js on every message; the options object at ril_worker.js side.

For unsolicited responses, ril_worker.js need to attach an SN without conflicting with ones generated by RadioInterfaceLayer.js, or to assign a special number (-1 for example).
(In reply to Thinker Li [:sinker] from comment #8)
> There is no unique identification for every request from main thread.

Not yet. My point was that we can easily add it if we pass the whole `options` argument around, for instance in the way you suggest.
(Assignee)

Updated

6 years ago
Assignee: nobody → ptseng
(Assignee)

Comment 10

6 years ago
Hi Philipp:

According to the former comments, I did some impelementations as below:

In Buf::newParcel(), I added the rilRequestErr member variable for the options object, and set to 0 as default (No error).

  newParcel: function newParcel(type, options) {
    ...
    if (!options) {
      options = {};
    }
    options.rilRequestType = type;
    options.rilRequestErr = 0;
    this.tokenRequestMap[token] = options;
    ...
  },


When HAL responses, Buf::processParcel() is invoked and trying to parse the parcel. When error occurs, the options::rilRequestError variable 
keeps track of the error. 

  processParcel: function processParcel() {
    ...
    options = this.tokenRequestMap[token];
    request_type = options.rilRequestType;
    if (error) {
      options.rilRequestError = error;
      RIL.handleReqError(options);
      return;
    }
    ... 
  },


Finally, RIL::handleReqErr is called, which sends the "options" object to RadioInterfaceLayer::onmessage() through Phone::sendDOMMessage()

  handleReqErr: function handleReqErr(options) {
    Phone.sendDOMMessage({type: "error", options: options});
  },

How do you think? Is the implementation similar to your thoughts?

I got two more questions here:
1. What is the defition of RIL request error? Are the conditions such as "No SIM card", "Wrong phone number" defined as errors, too?
2. How much information should the "options" object take when error occurs. Now the object just contains rilRequestType and rilRequestErr variables. Is it enough for the application in upper layer?

Thanks
(In reply to Price Tseng from comment #10)
> According to the former comments, I did some impelementations as below:

Cool! Can you please upload it in patch form next time? Thanks!

> In Buf::newParcel(), I added the rilRequestErr member variable for the
> options object, and set to 0 as default (No error).

Let's call it `rilRequestError`. We can afford the extra 2 characters to make it a complete English word :)

>   newParcel: function newParcel(type, options) {
>     ...
>     if (!options) {
>       options = {};
>     }
>     options.rilRequestType = type;
>     options.rilRequestErr = 0;

Let's initialize this to `null` because we don't know yet whether there was an error or not.

> When HAL responses,

s/HAL/RIL/

> Buf::processParcel() is invoked and trying to parse the
> parcel. When error occurs, the options::rilRequestError variable 
> keeps track of the error. 
> 
>   processParcel: function processParcel() {
>     ...
>     options = this.tokenRequestMap[token];
>     request_type = options.rilRequestType;
>     if (error) {
>       options.rilRequestError = error;

You can unconditionally assign options.rilRequestError = error. The RIL will set it to 0 (no error).

Speaking of numbers, it's probably a good idea to define the different RIL request error values in ril_consts.js as ERROR_*. They're in ril.h as RIL_E_*.

> Finally, RIL::handleReqErr is called, which sends the "options" object to
> RadioInterfaceLayer::onmessage() through Phone::sendDOMMessage()
> 
>   handleReqErr: function handleReqErr(options) {

Again, we can spare the extra characters and make it proper words. Let's call this handleRequestError.

>     Phone.sendDOMMessage({type: "error", options: options});

No need to make the `options` object a subobject. It's leaner if we just do

  options.type = "error";
  Phone.sendDOMMessage(options);

> I got two more questions here:
> 1. What is the defition of RIL request error? Are the conditions such as "No
> SIM card", "Wrong phone number" defined as errors, too?

Yes, when you try to place a phone call and it's an invalid number / there's no SIM card / etc., you will indeed get a RIL request error.

> 2. How much information should the "options" object take when error occurs.
> Now the object just contains rilRequestType and rilRequestErr variables. Is
> it enough for the application in upper layer?

That's the beauty of it. It contains *all* the information that the "upper layer" (RadioInterfaceLayer.js on the main thread) gave it. It's essentially a black box of information that we loop around.
(Assignee)

Comment 12

6 years ago
Created attachment 603647 [details] [diff] [review]
Keep track of the error type

Associate the error type with the original message when errors occur
Attachment #603647 - Flags: review?(philipp)
Comment on attachment 603647 [details] [diff] [review]
Keep track of the error type

You introduced a bunch of trailing whitespace. Also, please always indent by 2 spaces. r=me with those addressed.

A note about patch generation: please generate them with 8 lines of context and the username and log message already included. See https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F for instructions.
Attachment #603647 - Flags: review?(philipp) → review+
Summary: Handle request error for B2G RIL → B2G RIL: Handle request errors
(In reply to Philipp von Weitershausen [:philikon] from comment #13)
> You introduced a bunch of trailing whitespace. Also, please always indent by
> 2 spaces. r=me with those addressed.

Since these were so small, I addressed these and checked the patch in:

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