B2G RIL: Handle errors for ICC IO

RESOLVED FIXED in mozilla14

Status

()

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

People

(Reporter: allstars, Assigned: allstars)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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.
Blocks: 720638
(Assignee)

Updated

5 years ago
Assignee: nobody → yhuang
(Assignee)

Comment 1

5 years ago
Created attachment 607096 [details] [diff] [review]
Handle ICC IO Error v1

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

Comment 3

5 years ago
(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?
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
Created attachment 607868 [details] [diff] [review]
Handle ICC IO Error v2

Revised according to philikon's comments.
Attachment #607096 - Attachment is obsolete: true
Attachment #607868 - Flags: review?(philipp)
(Assignee)

Comment 7

5 years ago
Created attachment 608282 [details] [diff] [review]
Handle ICC IO Error v3

Revised according philikon's and vicamo's suggestions.
Attachment #607868 - Attachment is obsolete: true
Attachment #608282 - Flags: review?(philipp)
Attachment #607868 - Flags: review?(philipp)
(Assignee)

Comment 8

5 years ago
Created attachment 608616 [details] [diff] [review]
Handle ICC IO Error v4

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

Comment 9

5 years ago
Created attachment 608617 [details] [diff] [review]
Handle ICC IO Error v4
Attachment #608616 - Attachment is obsolete: true
Attachment #608617 - Flags: review?(philipp)
Attachment #608616 - Flags: review?(philipp)
Created attachment 610022 [details] [diff] [review]
Handle ICC IO Error v5

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+
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Keywords: checkin-needed
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aaa5dbfcca5
Status: REOPENED → NEW
https://hg.mozilla.org/mozilla-central/rev/2aaa5dbfcca5
Status: NEW → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.