764378 - RIL: sometimes RIL model was not detected

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Shian-Yow Wu [:swu]]

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]]

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]]

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

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

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]]

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

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

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]]

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

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]]

https://hg.mozilla.org/mozilla-central/rev/b792285b0815