Last Comment Bug 733990 - B2G RIL: Handle errors for ICC IO
: B2G RIL: Handle errors for ICC IO
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla14
Assigned To: Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
:
:
Mentors:
Depends on:
Blocks: 720638
  Show dependency treegraph
 
Reported: 2012-03-07 18:38 PST by Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
Modified: 2012-03-29 08:48 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Handle ICC IO Error v1 (5.91 KB, patch)
2012-03-19 02:59 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
Handle ICC IO Error v2 (3.43 KB, patch)
2012-03-20 23:44 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
Handle ICC IO Error v3 (3.43 KB, patch)
2012-03-22 03:28 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
Handle ICC IO Error v4 (3.46 KB, patch)
2012-03-22 23:27 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
Handle ICC IO Error v4 (3.46 KB, patch)
2012-03-22 23:32 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
Handle ICC IO Error v5 (3.38 KB, patch)
2012-03-27 22:48 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review

Description Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-07 18:38:19 PST
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 Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-19 02:59:16 PDT
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
Comment 2 Philipp von Weitershausen [:philikon] 2012-03-20 18:43:17 PDT
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 Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-20 19:28:25 PDT
(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] 2012-03-20 19:45:20 PDT
(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 Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-20 20:04:19 PDT
(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 Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-20 23:44:03 PDT
Created attachment 607868 [details] [diff] [review]
Handle ICC IO Error v2

Revised according to philikon's comments.
Comment 7 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-22 03:28:06 PDT
Created attachment 608282 [details] [diff] [review]
Handle ICC IO Error v3

Revised according philikon's and vicamo's suggestions.
Comment 8 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-22 23:27:27 PDT
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.
Comment 9 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-22 23:32:59 PDT
Created attachment 608617 [details] [diff] [review]
Handle ICC IO Error v4
Comment 10 Vicamo Yang [:vicamo][:vyang] 2012-03-27 22:48:48 PDT
Created attachment 610022 [details] [diff] [review]
Handle ICC IO Error v5

rebased to 244991519f53
Comment 11 Philipp von Weitershausen [:philikon] 2012-03-28 20:47:46 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aaa5dbfcca5
Comment 12 Matt Brubeck (:mbrubeck) 2012-03-29 08:48:20 PDT
https://hg.mozilla.org/mozilla-central/rev/2aaa5dbfcca5

Note You need to log in before you can comment on or make changes to this bug.