The default bug view has changed. See this FAQ.

B2G RIL: move quirk detection to config/build time

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: philikon, Assigned: vliu)

Tracking

unspecified
mozilla17
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Right now we detect RIL/modem quirks in Gecko at runtime based on various values in propdb.

Since the Gonk part has to be device/modem-specific already anyway, it was suggested [1] that we should bake the RIL quirks into Gonk at config/build time, e.g. by putting them into propd under the "ro.moz.rilquirks" or a similar key.

[1] see bug 766862 comment 25
Were it really possible? Some RIL quirks depends on values of properties exported by rild, so we can only determine their values after rild had been started. Doesn't it follow that it's impossible to generate /system/{build,default}.prop in build time? And "ro.*" is only loaded in init process from the two files at boot time.

About config time, it's possible to retrieve rild properties with a physical device, but there is also an alternative way to config your build environment without.
(In reply to Vicamo Yang [:vicamo] from comment #1)
> Were it really possible? Some RIL quirks depends on values of properties
> exported by rild, so we can only determine their values after rild had been
> started. Doesn't it follow that it's impossible to generate
> /system/{build,default}.prop in build time? 

Not sure I follow.  Is the implication that the version of rild and modem driver not known at build time?  This is certainly not the case.

Perhaps some of the problematic quirks could be cited?  Note that I don't think it's necessary to move all quirk detection to buid time.  Just those quirks that are not detectable at runtime without inspecting build characteristics that have little to do with the ril
(In reply to Michael Vines [:m1] from comment #2)
> Perhaps some of the problematic quirks could be cited?  Note that I don't
> think it's necessary to move all quirk detection to buid time.  Just those
> quirks that are not detectable at runtime without inspecting build
> characteristics that have little to do with the ril

We have following quirks and their dependencies listed below:

1. RILQUIRKS_CALLSTATE_EXTRA_UINT32: "gsm.version.ril-impl" and "ril.model_id"
2. RILQUIRKS_DATACALLSTATE_DOWN_IS_UP: "gsm.version.ril-impl" and "ril.model_id"
3. RILQUIRKS_V5_LEGACY: "gsm.version.ril-impl" and "ro.product.model"
4. RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL: "gsm.version.ril-impl" and "ril.model_id"
5. RILQUIRKS_MODEM_DEFAULTS_TO_EMERGENCY_MODE: "gsm.version.ril-impl" and "ro.product.model"

And those prerequisite properties:

I. "gsm.version.ril-impl": available only after rild is running
II. "ril.model_id": available only after rild is running
III. "ro.product.model": available at build time through $(PRODUCT_MODEL)

Yes, all quirks depends on runtime state of rild.
(In reply to Vicamo Yang [:vicamo] from comment #3)
Just discuss with Vicamo about this.
And he confirms what I have in mind.

When we try to build gecko, 
since we already know which device we're building by config.sh,
we could write which quirks into build.prop (could be an array or bitwise, or whatsoever)

for example, when config galaxy-s2
we should enable RILQUIRKS_CALLSTATE_EXTRA_UINT32, RILQUIRKS_DATACALLSTATE_DOWN_IS_UP, RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL into ro.moz.rilquirks

The only one exception should be RILQUIRKS_V5_LEGACY,
it's used for detecting underlying libril is using RIL v5 or v6,
and it is only detectable at run-time. 

How do you think about this?

Thanks
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #4)
> (In reply to Vicamo Yang [:vicamo] from comment #3)
> for example, when config galaxy-s2
> we should enable RILQUIRKS_CALLSTATE_EXTRA_UINT32,
> RILQUIRKS_DATACALLSTATE_DOWN_IS_UP,
> RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL into ro.moz.rilquirks

Yes, we can add them directly into $(PRODUCT_PROPERTY_OVERRIDES). Thank you Yoshi!
Cool, sounds good.  RIL v5 vs v6 is also cetainly known at build time for a particular device.

Comment 7

5 years ago
We use the following ro.moz.rilquirks.* naming for each of the quirks, how does everyone think?
- RILQUIRKS_CALLSTATE_EXTRA_UINT32: ro.moz.rilquirks.callstate_extra_uint32
- RILQUIRKS_DATACALLSTATE_DOWN_IS_UP: ro.moz.rilquirks.datacallstate_down_is_up
- RILQUIRKS_V5_LEGACY: ro.moz.rilquirks.v5_legacy
- RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL: ro.moz.rilquirks.request_use_dial_emergency_call
- RILQUIRKS_MODEM_DEFAULTS_TO_EMERGENCY_MODE: ro.moz.rilquirks.modem_defaults_to_emergency_mode

Currently, at least we should add rilquirks properties to these platforms: SGS2, Nexus-S, Otoro.
Sadly we'll need shorter quirk property names.  Property values are not exactly boundless either :(
  #define PROP_NAME_MAX   32
  #define PROP_VALUE_MAX  92

Comment 9

5 years ago
Thanks for the information about length information.

If so, we may use:
ro.moz.rilquirks.ceu
ro.moz.rilquirks.ddiu
ro.moz.rilquirks.v5l
ro.moz.rilquirks.rudec
ro.moz.rilquirks.mdtem

Or this way:
ro.moz.rilquirks.opt1
ro.moz.rilquirks.opt2
ro.moz.rilquirks.opt3
ro.moz.rilquirks.opt4
ro.moz.rilquirks.opt5

Any other ideas?
I think we can do better:

ro.moz.ril.callstate_extra_int
ro.moz.ril.callstate_down_is_up
ro.moz.ril.v5_legacy
ro.moz.ril.dial_emergency_call
ro.moz.ril.emergency_by_default

Comment 11

5 years ago
vliu, would you like take this bug?  Thanks!
(Assignee)

Comment 12

5 years ago
Yes, I can take this bug.

Updated

5 years ago
Assignee: nobody → vliu
(Assignee)

Comment 13

5 years ago
Created attachment 647847 [details] [diff] [review]
Patch file to config ril quirks before build stage

A patch is attached for ril quirks configuration(take otoro as example). My idea is trying to do ril quirk setting in config.sh according to platform name. It is easy to manage quirk items because these changes affects all platforms. May I ask for anyone's suggestion? Thanks.
Comment on attachment 647847 [details] [diff] [review]
Patch file to config ril quirks before build stage

This looks ok to me, although I'm not sure this should be part of the config.sh script, for instance. But mwu should chime in here. Also note that ultimately this needs to be a pull request on GitHub.

Two more points:

* If a flag is `false`, I think we can just omit it.
* Please don't forget quirks for the SGS2 and Nexus S.
Attachment #647847 - Flags: feedback?(mwu)

Comment 15

5 years ago
Comment on attachment 647847 [details] [diff] [review]
Patch file to config ril quirks before build stage

Put that stuff here: https://github.com/mozilla-b2g/android-device-otoro/blob/master/full_otoro.mk#L11
Attachment #647847 - Flags: feedback?(mwu) → review-
(Assignee)

Comment 16

5 years ago
Created attachment 648637 [details] [diff] [review]
Patch file to config ril quirks before build stage

This patch checked PRODUCT_PROPERTY_OVERRIDES in full_xx.mk. If it contained in full_xx.mk, the ril quirks added after it. If there is no PRODUCT_PROPERTY_OVERRIDES in full_xx.mk, the patch then new a PRODUCT_PROPERTY_OVERRIDES and added ril quirk after it. How about this patch?
Attachment #647847 - Attachment is obsolete: true
Attachment #648637 - Flags: feedback?(mwu)
(In reply to vliu from comment #16)
> This patch checked PRODUCT_PROPERTY_OVERRIDES in full_xx.mk. If it contained
> in full_xx.mk, the ril quirks added after it. If there is no
> PRODUCT_PROPERTY_OVERRIDES in full_xx.mk, the patch then new a
> PRODUCT_PROPERTY_OVERRIDES and added ril quirk after it. How about this
> patch?

I think mwu meant you should create a pull request in each device repository on github instead of submitting a patch on bugzilla. And, don't write shell scripts to patch product.mk! IMHO, don't manually patch anything anymore. Just edit those mk files and request for pulls.

Comment 18

5 years ago
(In reply to Vicamo Yang [:vicamo] from comment #17)
> (In reply to vliu from comment #16)
> > This patch checked PRODUCT_PROPERTY_OVERRIDES in full_xx.mk. If it contained
> > in full_xx.mk, the ril quirks added after it. If there is no
> > PRODUCT_PROPERTY_OVERRIDES in full_xx.mk, the patch then new a
> > PRODUCT_PROPERTY_OVERRIDES and added ril quirk after it. How about this
> > patch?
> 
> I think mwu meant you should create a pull request in each device repository
> on github instead of submitting a patch on bugzilla. And, don't write shell
> scripts to patch product.mk! IMHO, don't manually patch anything anymore.
> Just edit those mk files and request for pulls.

Yup, exactly.
(Assignee)

Comment 19

5 years ago
(In reply to Michael Wu [:mwu] from comment #18)
> (In reply to Vicamo Yang [:vicamo] from comment #17)
> > (In reply to vliu from comment #16)
> > > This patch checked PRODUCT_PROPERTY_OVERRIDES in full_xx.mk. If it contained
> > > in full_xx.mk, the ril quirks added after it. If there is no
> > > PRODUCT_PROPERTY_OVERRIDES in full_xx.mk, the patch then new a
> > > PRODUCT_PROPERTY_OVERRIDES and added ril quirk after it. How about this
> > > patch?
> > 
> > I think mwu meant you should create a pull request in each device repository
> > on github instead of submitting a patch on bugzilla. And, don't write shell
> > scripts to patch product.mk! IMHO, don't manually patch anything anymore.
> > Just edit those mk files and request for pulls.
> 
> Yup, exactly.

Thanks for everyone's suggestion. I will modify mk file for every device and then pull request.
(Assignee)

Comment 20

5 years ago
I found the RILQUIRKS_V5_LEGACY in function UNSOLICITED_RIL_CONNECTED() is determined at run time. Do we really set this value in build time?
(In reply to vliu from comment #20)
> I found the RILQUIRKS_V5_LEGACY in function UNSOLICITED_RIL_CONNECTED() is
> determined at run time. Do we really set this value in build time?

Set a default at build time, override value at run time.
(Assignee)

Comment 22

5 years ago
Created attachment 649594 [details] [diff] [review]
Patch file to set ril quirks in build time

Hi mwu,
I have done with pull request.
Otoro: https://github.com/mozilla-b2g/android-device-otoro/pull/9
Galaxy-s2: https://github.com/mozilla-b2g/android-device-galaxys2/pull/14
Nexus-s: https://github.com/mozilla-b2g/android-device-crespo/pull/3

Hi Philikon,
Please review the patch if it is work for all platforms.
Attachment #648637 - Attachment is obsolete: true
Attachment #648637 - Flags: feedback?(mwu)
Attachment #649594 - Flags: review?(philipp)
Attachment #649594 - Flags: review?(mwu)
Comment on attachment 649594 [details] [diff] [review]
Patch file to set ril quirks in build time

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

::: dom/system/gonk/ril_worker.js
@@ +692,5 @@
>     * requires the RIL being "warmed up" first, which happens when on
>     * an incoming or outgoing voice call or data call.
>     */
>    rilQuirksInitialized: false,
>    initRILQuirks: function initRILQuirks() {

Now that we know these values at build time, I think we can get just rid of initRILQuirks and read the properties where the RILQUIRKS variables are defined (at top level scope, when ril_worker.js is first executed).

@@ +696,5 @@
>    initRILQuirks: function initRILQuirks() {
>      if (this.rilQuirksInitialized) {
>        return;
>      }
> + 

Nit: trailing space
Attachment #649594 - Flags: review?(philipp)
Attachment #649594 - Flags: review?(mwu)
Attachment #649594 - Flags: feedback+
(In reply to vliu from comment #22)
> Otoro: https://github.com/mozilla-b2g/android-device-otoro/pull/9
> Galaxy-s2: https://github.com/mozilla-b2g/android-device-galaxys2/pull/14
> Nexus-s: https://github.com/mozilla-b2g/android-device-crespo/pull/3

Awesome. Thanks! mwu had already merged Otoro, I merged the remaining two.
(Assignee)

Comment 25

5 years ago
Created attachment 649947 [details] [diff] [review]
Patch file to set ril quirks in build time

Hi mwu,
I found one of ril quirk name in otoro is too long to read by getprop. Please help me to pull request again. Thank you.
https://github.com/mozilla-b2g/android-device-otoro/pull/10

Hi Philikon,
Please review again the patch file. Thanks!
Attachment #649594 - Attachment is obsolete: true
Attachment #649947 - Flags: review?(philipp)
Attachment #649947 - Flags: review?(mwu)

Comment 26

5 years ago
Comment on attachment 649947 [details] [diff] [review]
Patch file to set ril quirks in build time

I'll let philikon merge the pull request if he's happy with this rename.
Attachment #649947 - Flags: review?(mwu)
Comment on attachment 649947 [details] [diff] [review]
Patch file to set ril quirks in build time

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

::: dom/system/gonk/ril_worker.js
@@ +58,3 @@
>  // This flag defaults to true since on RIL v6 and later, we get the
>  // version number via the UNSOLICITED_RIL_CONNECTED parcel.
> +let RILQUIRKS_V5_LEGACY = libcutils.property_get("ro.moz.ril.v5_legacy");

The comment no longer makes sense. Please rewrite it. E.g.

  // This may change at runtime since in RIL v6 and later, we get the version
  // number via the UNSOLICITED_RIL_CONNECTED parcel.

r=me with that.

Merged your pull request. Thanks!
Attachment #649947 - Flags: review?(philipp) → review+
(Assignee)

Comment 28

5 years ago
Created attachment 650439 [details] [diff] [review]
Patch file to set ril quirks in build time

(In reply to Philipp von Weitershausen [:philikon] from comment #27)
> Comment on attachment 649947 [details] [diff] [review]
> Patch file to set ril quirks in build time
> 
> Review of attachment 649947 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +58,3 @@
> >  // This flag defaults to true since on RIL v6 and later, we get the
> >  // version number via the UNSOLICITED_RIL_CONNECTED parcel.
> > +let RILQUIRKS_V5_LEGACY = libcutils.property_get("ro.moz.ril.v5_legacy");
> 
> The comment no longer makes sense. Please rewrite it. E.g.
> 
>   // This may change at runtime since in RIL v6 and later, we get the version
>   // number via the UNSOLICITED_RIL_CONNECTED parcel.
> 
> r=me with that.
> 
> Merged your pull request. Thanks!

A patch is attached for modifying the comment. Please review it. Thanks.
Attachment #649947 - Attachment is obsolete: true
Attachment #650439 - Flags: review?(philipp)
Comment on attachment 650439 [details] [diff] [review]
Patch file to set ril quirks in build time

Thanks!

Note that I already gave you review in the previous comment. There's no need to ask for review again. It slows things down and creates more email for everybody. In the future, please upload a properly formatted patch (yours is missing author info and commit message, see [1] for a checklist) and set the checkin-needed keyword on the bug. Thanks!

[1] http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
Attachment #650439 - Flags: review?(philipp) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee73e7a89076
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/ee73e7a89076
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.