733990 - B2G RIL: Handle errors for ICC IO

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: 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

Comment 2

[Philipp von Weitershausen [:philikon]]

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.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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

Comment 4

[Philipp von Weitershausen [:philikon]]

(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?

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Handle ICC IO Error v2

Revised according to philikon's comments.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Handle ICC IO Error v3

Revised according philikon's and vicamo's suggestions.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: 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.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Handle ICC IO Error v4

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Handle ICC IO Error v5

rebased to 244991519f53

Comment 11

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2aaa5dbfcca5

Comment 12

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/2aaa5dbfcca5