Closed Bug 764378 Opened 12 years ago Closed 12 years ago

RIL: sometimes RIL model was not detected

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: swu, Assigned: vliu)

Details

Attachments

(1 file, 3 obsolete files)

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.
Not sure why it fails to get RIL model in ril_worker.js.
We should not set "this.rilQuirksInitialized = true" in this condition.
Attached patch Patch file to solve this issue. (obsolete) — Splinter Review
(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.
Attachment #634453 - Flags: feedback?(philipp)
(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 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);
Attachment #634453 - Flags: feedback?(philipp) → feedback-
Attached patch Patch file to solve this issue. (obsolete) — Splinter Review
(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.
Attachment #634723 - Flags: review?(philipp)
Attached patch Patch file to solve this issue. (obsolete) — Splinter Review
(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.
Attachment #634453 - Attachment is obsolete: true
Attachment #634723 - Attachment is obsolete: true
Attachment #634723 - Flags: review?(philipp)
Attachment #634768 - Flags: review?(philipp)
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.
Attachment #634768 - Flags: review?(philipp) → review+
(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.
Attachment #634768 - Attachment is obsolete: true
Attachment #635196 - Flags: review?(philipp)
Attachment #635196 - Flags: review?(philipp) → review+
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
Assignee: nobody → vliu
Keywords: checkin-needed
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/b792285b0815
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: