716709 - RIL: call state on SGS2 has extra uint32

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

The call state on SGS2 phones has an extra uint32 in it:

  somethingOrOther:   Buf.readUint32(), //XXX TODO whatziz? not in ril.h, but it's in the output...

"Extra" as in: it's not in the RIL header files, doesn't appear on the emulator, and I could imagine it won't on Toru Maguro, either (though I will need to dbl check that). Anyway, we might be able figure out whether this thing is present by doing some length calculation (we know the overall length of the parcel), or by #ifdef'ing it based on some configure parameter (ugggghhhhh).

Comment 1

[Philipp von Weitershausen [:philikon]]

Hrm, while we know the overall length of the parcel, there's a lot of variables here: multiple calls, variable length strings within each call, optional UUS info, etc. The easiest would probably be something like

  #ifdef B2G_HARDWARE_SGS2
        // The Samsung Galaxy S2 sends extra uint32 as part of the call state...
        // Why, we don't know.
        somethingOrOther:   Buf.readUint32(),
  #endif

mwu, master of the build system, how do you feel about adding said preprocessor constant? Can we define as part of 'make config-galaxy-s2' it so that we don't have add a --with flag to Gecko's configure.in?

Comment 2

[Philipp von Weitershausen [:philikon]]

CCing mwu for question in comment 1.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We can determine at runtime if we're on an sgs2 by checking /proc/cpuinfo.

But, what does android do for this problem?

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> We can determine at runtime if we're on an sgs2 by checking /proc/cpuinfo.

If I have nsIFile nonsense available in the Chrome Worker (which I should), then that might work. I'd be looking for this line, right:

Hardware	: SMDKC210

> But, what does android do for this problem?

Good question. Could it be that Samsung forked the Android source code and changed that bit? Does call state actually work with a stock Android build on the SGS2? Somehow I have my doubts.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Philipp von Weitershausen [:philikon] from comment #4)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> > We can determine at runtime if we're on an sgs2 by checking /proc/cpuinfo.
> 
> If I have nsIFile nonsense available in the Chrome Worker (which I should),
> then that might work. I'd be looking for this line, right:
> 
> Hardware	: SMDKC210
> 

Right.  Ideally a version too (/proc/sys/kernel/osrelease).

> > But, what does android do for this problem?
> 
> Good question. Could it be that Samsung forked the Android source code and
> changed that bit? Does call state actually work with a stock Android build
> on the SGS2? Somehow I have my doubts.

Certainly possible.  I haven't tried.  There might be a generic workaround in there though, worth checking.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

It turns out that there's a "ril.model" property or something like that, which should lead to a cleaner patch than parsing /proc/cpuinfo output.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Also, BTW, I would recommend implementing this with a set of quirks flags, set based on the modem/rild we're using.  For example,

 enum { QUIRK_EXTRA_INT32 = 1 << 0 };
 let gQuirks = 0;
 //...
 function init() {
   if (/* galaxy s ii modem*/) gQuirks |= QUIRK_EXTRA_INT32;
 }
 //...
 if (QUIRK_EXTRA_INT32 & gQuirk) {
   Buf.readUint32(), //XXX TODO whatziz? not in ril.h, but it's in the output...
 }

Comment 8

[Philipp von Weitershausen [:philikon]]

From bug 713301 comment #15:
> Thanks for digging through this, Chris. Fabrice had a good idea for finding
> out what device we're on, by using getprop. I can just do the equivalent of 
> 
>   char propValue[32];
>   property_get("ril.model_id", propValue, NULL);
> 
> in js-ctypes in the ril_worker.js to find out whether we need the
> 'somethingOrOther' uint32 in the call state.

Comment 9

[Philipp von Weitershausen [:philikon]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> Also, BTW, I would recommend implementing this with a set of quirks flags,
> set based on the modem/rild we're using.

Yup, though since we're in JS there's not really any penalty to using separate variables rather than bit twiddling a single gQuirks integer.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I don't fully understand what you mean by that, but I trust you! :)

Comment 11

[Philipp von Weitershausen [:philikon]]

Attachment: v1

Fix as proposed in previous comments.

This adds systemlibs.js, a file to which we can add ctypes-based interfaces for other libraries in the future (this will be relevant to the network manager, too, for instance.)

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 593298 [details] [diff] [review]
v1

>diff --git a/dom/system/b2g/ril_worker.js b/dom/system/b2g/ril_worker.js

>+  callQuirksInitialized: false,
>+

There's an English ambiguity here between "please call quirks
initialized", i.e. "call" as a verb in the imperative voice, and "the
phone call quirks have been initialized", i.e. "call" as a noun.  I
parsed the former twice and was a bit confused.  Please rename to
RILD_QUIRKS/rildQuirks or something like that.

>+  initCallQuirks: function initCallQuirks() {

Please note here that this function only works after rild is "warmed
up", as you noted on IRC by placing a call.  (Does it also work after
receiving an incoming call?  Plz check.)

>   let calls = {};
>   for (let i = 0; i < calls_length; i++) {
>-    let call = {
>-      state:              Buf.readUint32(), // CALL_STATE_*
>+    let state          = Buf.readUint32(); // CALL_STATE_*

I'm no JS stylist, but what about doing

  let call = { };
  call.state = ...;
  //...

to avoid having to repeat yourself below?  If there's a reason that's
not done, please let me know.

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 593298 [details] [diff] [review]
v1

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

I'm not super sure I'm into the idea of ctypesing things, but I guess it's bound to happen at some point, and this does make getting to props easier than wiring it thru yet another jsapi thing. r=me with cjones' nits picked, assuming those were even nits.

Comment 14

[Philipp von Weitershausen [:philikon]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #12)
> >   let calls = {};
> >   for (let i = 0; i < calls_length; i++) {
> >-    let call = {
> >-      state:              Buf.readUint32(), // CALL_STATE_*
> >+    let state          = Buf.readUint32(); // CALL_STATE_*
> 
> I'm no JS stylist, but what about doing
> 
>   let call = { };
>   call.state = ...;
>   //...
> 
> to avoid having to repeat yourself below?  If there's a reason that's
> not done, please let me know.

Uh, yeah. Not sure why I didn't do it this way. Will fix and also address the other nits.

Comment 15

[Philipp von Weitershausen [:philikon]]

Attachment: v2

Nits addressed. Can't check in though since HG is down. Sigh. Will do that as soon as it's back up.

Comment 16

[Philipp von Weitershausen [:philikon]]

Attachment: v3

I realized that systemlibs.js wouldn't work when building desktop Firefox with MOZ_B2G_RIL enabled. Having telephony work in a desktop Firefox is actually quite a useful feature sometimes.

Comment 17

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/6f9456fc1e83