Last Comment Bug 764378 - RIL: sometimes RIL model was not detected
: RIL: sometimes RIL model was not detected
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla16
Assigned To: Vincent Liu[:vliu]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-13 07:15 PDT by Shian-Yow Wu [:swu]
Modified: 2012-06-22 03:45 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch file to solve this issue. (1.30 KB, patch)
2012-06-19 08:53 PDT, Vincent Liu[:vliu]
philipp: feedback-
Details | Diff | Review
Patch file to solve this issue. (2.00 KB, patch)
2012-06-19 19:57 PDT, Vincent Liu[:vliu]
no flags Details | Diff | Review
Patch file to solve this issue. (1.23 KB, patch)
2012-06-19 22:55 PDT, Vincent Liu[:vliu]
philipp: review+
Details | Diff | Review
Patch file to solve this issue. (1.28 KB, patch)
2012-06-20 23:18 PDT, Vincent Liu[:vliu]
philipp: review+
Details | Diff | Review

Description Shian-Yow Wu [:swu] 2012-06-13 07:15:17 PDT
Sometimes RIL fails to get correct RIL model.
On SGS2 or Nexus-S, this may cause problem to receive phone calls.

1. NG case:
I/Gecko   ( 1847): RIL Worker: Handling parcel as REQUEST_VOICE_REGISTRATION_STATE
I/Gecko   ( 1847): RIL Worker: voice registration state: 0,0000,00000000
I/Gecko   ( 1847): RIL Worker: Detected RIL implementation Samsung RIL(IPC) v2.0
I/Gecko   ( 1847): RIL Worker: Detected RIL model 
I/Gecko   ( 1847): RIL Worker: Next parcel size unknown, going to sleep.
I/Gecko   ( 1847): RIL Worker: Received 4 bytes.
I/Gecko   ( 1847): RIL Worker: Already read 0

When this happens, stop/start b2g can recover it.

2. OK case:
I/Gecko   ( 2253): RIL Worker: Handling parcel as REQUEST_VOICE_REGISTRATION_STATE
I/Gecko   ( 2253): RIL Worker: voice registration state: 1,2833,01230737
I/Gecko   ( 2253): RIL Worker: Detected RIL implementation Samsung RIL(IPC) v2.0
I/Gecko   ( 2253): RIL Worker: Detected RIL model I9100
I/Gecko   ( 2253): RIL Worker: Detected I9100, enabling RILQUIRKS_CALLSTATE_EXTRA_UINT32, RILQUIRKS_DATACALLSTATE_DOWN_IS_UP, RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL.
I/Gecko   ( 2253): RIL Worker: New outgoing parcel of type 100
I/Gecko   ( 2253): RIL Worker: Outgoing parcel: 0,0,0,8,100,0,0,0,14,0,0,0
I/Gecko   ( 2253): RIL Worker: Next parcel size unknown, going to sleep.
Comment 1 Shian-Yow Wu [:swu] 2012-06-13 07:56:05 PDT
Not sure why it fails to get RIL model in ril_worker.js.
We should not set "this.rilQuirksInitialized = true" in this condition.
Comment 2 Vincent Liu[:vliu] 2012-06-19 08:53:18 PDT
Created attachment 634453 [details] [diff] [review]
Patch file to solve this issue.

(In reply to Shian-Yow Wu from comment #1)
> Not sure why it fails to get RIL model in ril_worker.js.
> We should not set "this.rilQuirksInitialized = true" in this condition.

I modify code to solve this issue. Please see the attached file. Actually it fails to get model_id at the first time and then it can get the correct model_id in the following calling. This patch can skip the failed acquisition. This is the reason why I add this patch.
Comment 3 Philipp von Weitershausen [:philikon] 2012-06-19 10:26:37 PDT
(In reply to Shian-Yow Wu from comment #1)
> Not sure why it fails to get RIL model in ril_worker.js.

In some cases the RIL needs to be "warmed up", e.g. having made a call or similar.

> We should not set "this.rilQuirksInitialized = true" in this condition.

Good point.
Comment 4 Philipp von Weitershausen [:philikon] 2012-06-19 10:43:06 PDT
Comment on attachment 634453 [details] [diff] [review]
Patch file to solve this issue.

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

::: dom/system/gonk/ril_worker.js
@@ +700,5 @@
>            }
>            RILQUIRKS_DATACALLSTATE_DOWN_IS_UP = true;
> +        } else {
> +          if (DEBUG) debug("Fail to get model_id.");
> +          return;

It would be more correct to test whether actually don't get a value for `model_id` (null or "" both evaluate to false):

  let model_id = libcutils.property_get("ril.model_id");
  if (!model_id) {
    // On some RIL models, the RIL has to be "warmed up" for us to read this property.
    // It apparently isn't warmed up yet, going to try again later.
    if (DEBUG) debug("Could not detect model_id. Going to try later.");
    return;
  }
  if (DEBUG) debug("Detected RIL model " + model_id);
Comment 5 Vincent Liu[:vliu] 2012-06-19 19:57:31 PDT
Created attachment 634723 [details] [diff] [review]
Patch file to solve this issue.

(In reply to Philipp von Weitershausen [:philikon] from comment #4)

> It would be more correct to test whether actually don't get a value for
> `model_id` (null or "" both evaluate to false):
> 
>   let model_id = libcutils.property_get("ril.model_id");
>   if (!model_id) {
>     // On some RIL models, the RIL has to be "warmed up" for us to read this
> property.
>     // It apparently isn't warmed up yet, going to try again later.
>     if (DEBUG) debug("Could not detect model_id. Going to try later.");
>     return;
>   }
>   if (DEBUG) debug("Detected RIL model " + model_id);

Thanks for the suggestion and add the patch file.
Comment 6 Vincent Liu[:vliu] 2012-06-19 22:55:45 PDT
Created attachment 634768 [details] [diff] [review]
Patch file to solve this issue.

(In reply to Philipp von Weitershausen [:philikon] from comment #4)

> It would be more correct to test whether actually don't get a value for
> `model_id` (null or "" both evaluate to false):
> 
>   let model_id = libcutils.property_get("ril.model_id");
>   if (!model_id) {
>     // On some RIL models, the RIL has to be "warmed up" for us to read this
> property.
>     // It apparently isn't warmed up yet, going to try again later.
>     if (DEBUG) debug("Could not detect model_id. Going to try later.");
>     return;
>   }
>   if (DEBUG) debug("Detected RIL model " + model_id);

Thanks for the suggestion and add the patch file.
Comment 7 Philipp von Weitershausen [:philikon] 2012-06-20 16:39:42 PDT
Comment on attachment 634768 [details] [diff] [review]
Patch file to solve this issue.

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

::: dom/system/gonk/ril_worker.js
@@ +691,1 @@
>          if (DEBUG) debug("Detected RIL model " + model_id);

Nit: if we put the debug statement before the `if` block, we will know which RIL model caused us to bail out.

r=me with that.
Comment 8 Vincent Liu[:vliu] 2012-06-20 23:18:54 PDT
Created attachment 635196 [details] [diff] [review]
Patch file to solve this issue.

(In reply to Philipp von Weitershausen [:philikon] from comment #7)
> Comment on attachment 634768 [details] [diff] [review]
> Patch file to solve this issue.
> 
> Review of attachment 634768 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +691,1 @@
> >          if (DEBUG) debug("Detected RIL model " + model_id);
> 
> Nit: if we put the debug statement before the `if` block, we will know which
> RIL model caused us to bail out.
> 
> r=me with that.

Added the patch.
Comment 9 Philipp von Weitershausen [:philikon] 2012-06-21 12:03:20 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b792285b0815

Thanks for the patch, vliu!. In the future, please make sure your patch contains your committer info and a valid commit message. See http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
Comment 10 Ed Morley [:emorley] 2012-06-22 03:45:43 PDT
https://hg.mozilla.org/mozilla-central/rev/b792285b0815

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