777123 - B2G RIL: move quirk detection to config/build time

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

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

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 2

[Michael Vines [:m1] [:evilmachines]]

(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

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 6

[Michael Vines [:m1] [:evilmachines]]

Cool, sounds good.  RIL v5 vs v6 is also cetainly known at build time for a particular device.

Comment 7

[Shian-Yow Wu [:swu]]

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.

Comment 8

[Michael Vines [:m1] [:evilmachines]]

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

[Shian-Yow Wu [:swu]]

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?

Comment 10

[Philipp von Weitershausen [:philikon]]

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

[Shian-Yow Wu [:swu]]

vliu, would you like take this bug?  Thanks!

Comment 12

[Vincent Liu[:vliu]]

Yes, I can take this bug.

Comment 13

[Vincent Liu[:vliu]]

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

[Philipp von Weitershausen [:philikon]]

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.

Comment 15

[Michael Wu [:mwu]]

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

Comment 16

[Vincent Liu[:vliu]]

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

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

(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

[Michael Wu [:mwu]]

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

Comment 19

[Vincent Liu[:vliu]]

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

Comment 20

[Vincent Liu[:vliu]]

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?

Comment 21

[Philipp von Weitershausen [:philikon]]

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

Comment 22

[Vincent Liu[:vliu]]

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

Comment 23

[Philipp von Weitershausen [:philikon]]

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

Comment 24

[Philipp von Weitershausen [:philikon]]

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

Comment 25

[Vincent Liu[:vliu]]

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

Comment 26

[Michael Wu [:mwu]]

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.

Comment 27

[Philipp von Weitershausen [:philikon]]

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!

Comment 28

[Vincent Liu[:vliu]]

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

Comment 29

[Philipp von Weitershausen [:philikon]]

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

Comment 30

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee73e7a89076

Comment 31

[Ed Morley [:emorley]]

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