Closed Bug 733990 Opened 8 years ago Closed 8 years ago

B2G RIL: Handle errors for ICC IO

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

Attachments

(1 file, 5 obsolete files)

For ICC I/O commands, sometimes it will return error codes in response.
(See the values of sw1 and sw2 in TS 51.011 clause 9.4)
We need to report the error if necessary.
Assignee: nobody → yhuang
Attached patch Handle ICC IO Error v1 (obsolete) — Splinter Review
Convert the status code to error description.

Currently ICC I/O commands are requested inside ril_worker itself, so I didn't pass the error message to RadioInterfaceLayer.

Also in the future if we need to pass the error message to RadioInterfaceLayer, 
we also need to reconsider RIL.handleRequestError, beause currently it doesn't take care of the ICC IO status.

Notice that I didn't 'const' those possible sw2 values in RIL._processICCError, because their corresponding meanings depends on the value of sw1.

thanks
Attachment #607096 - Flags: review?(philipp)
Comment on attachment 607096 [details] [diff] [review]
Handle ICC IO Error v1

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

::: dom/system/gonk/ril_consts.js
@@ +353,5 @@
> +const ICC_ERROR_FUNCTION_NOT_SUPPORTED = "Function not supported";
> +const ICC_ERROR_FILE_NOT_FOUND = "File not found";
> +const ICC_ERROR_RECORD_NOT_FOUND = "Record not found";
> +
> +const ICC_ERROR_UNKNOWN_ERROR = "Unknown error";

There's no need to const these human readable values only exist for debugging output. Just inline these in the debug() calls.

::: dom/system/gonk/ril_worker.js
@@ +1311,5 @@
> +      default:
> +        msg = ICC_ERROR_UNKNOWN_ERROR;
> +        break;
> +    }
> +    debug("ICC I/O error message :" + msg);

This is a lot of code for a little bit of debugging. Like I said above, just log the error code here. Don't even need the switch statements. We can fill in actual error handling at a later time and then we might or might not need those switch statements.
Attachment #607096 - Flags: review?(philipp)
(In reply to Philipp von Weitershausen [:philikon] from comment #2)
> This is a lot of code for a little bit of debugging. Like I said above, just
> log the error code here. Don't even need the switch statements. We can fill
> in actual error handling at a later time and then we might or might not need
> those switch statements.

Hi, philikon:
 So I don't need to print the error message here, just the error code ?
Originally when I read the spec, there are lots of meanings according to the sw1 and sw2.
So I think I should convert the status bytes(sw1, sw2) to meaningful sentences, and report them if necessary. 

Also about the "if necessary", 
should I notify the error to RadioInterfaceLayer? 
I am not sure it is necessary to do so, currently ICC IO commands are issued from ril_worker, not from RadioInterfaceLayer.

thanks
(In reply to Yoshi Huang[:yoshi] from comment #3)
>  So I don't need to print the error message here, just the error code ?

Yep.

> Originally when I read the spec, there are lots of meanings according to the
> sw1 and sw2.
> So I think I should convert the status bytes(sw1, sw2) to meaningful
> sentences, and report them if necessary. 

Why? How often will this happen? Not very often, right? I don't see the point of adding lots of code for human-readable error messages that practically nobody will see. Because right now you have to enable debugging manually. So chances are good whoever's reading those messages will know how to look up the error from the spec. Just add a comment with a pointer to the spec, where to find the meaning of those error values, and we should be ok.

> Also about the "if necessary", 
> should I notify the error to RadioInterfaceLayer? 

I don't see the point. Until we know what exactly to do with that notification, why add this code?
(In reply to Philipp von Weitershausen [:philikon] from comment #4)
> Just add a comment with a pointer to the
> spec, where to find the meaning of those error values, and we should be ok.
> 
Hi, philikon:
okay, then I just simply add some comments and pointer to the spec.

thanks for your suggestions.
Attached patch Handle ICC IO Error v2 (obsolete) — Splinter Review
Revised according to philikon's comments.
Attachment #607096 - Attachment is obsolete: true
Attachment #607868 - Flags: review?(philipp)
Attached patch Handle ICC IO Error v3 (obsolete) — Splinter Review
Revised according philikon's and vicamo's suggestions.
Attachment #607868 - Attachment is obsolete: true
Attachment #608282 - Flags: review?(philipp)
Attachment #607868 - Flags: review?(philipp)
Attached patch Handle ICC IO Error v4 (obsolete) — Splinter Review
Add log for EF id and command when error. 
Otherwise there are many ICC commands now and we might not know which request is error.
Attachment #608282 - Attachment is obsolete: true
Attachment #608616 - Flags: review?(philipp)
Attachment #608282 - Flags: review?(philipp)
Attached patch Handle ICC IO Error v4 (obsolete) — Splinter Review
Attachment #608616 - Attachment is obsolete: true
Attachment #608617 - Flags: review?(philipp)
Attachment #608616 - Flags: review?(philipp)
rebased to 244991519f53
Attachment #608617 - Attachment is obsolete: true
Attachment #610022 - Flags: review?(philipp)
Attachment #608617 - Flags: review?(philipp)
Attachment #610022 - Flags: review?(philipp) → review+
Status: NEW → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/2aaa5dbfcca5
Status: NEW → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.