RIL: call state on SGS2 has extra uint32

RESOLVED FIXED in mozilla13

Status

()

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

People

(Reporter: philikon, Assigned: philikon)

Tracking

Trunk
mozilla13
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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).
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?
CCing mwu for question in comment 1.
We can determine at runtime if we're on an sgs2 by checking /proc/cpuinfo.

But, what does android do for this problem?
(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.
(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.
No longer blocks: 713301
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.
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...
 }
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.
Assignee: nobody → philipp
(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.
I don't fully understand what you mean by that, but I trust you! :)
Created attachment 593298 [details] [diff] [review]
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.)
Attachment #593298 - Flags: review?(kyle)
Attachment #593298 - Flags: feedback?(jones.chris.g)
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.
Attachment #593298 - Flags: feedback?(jones.chris.g) → feedback+
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.
Attachment #593298 - Flags: review?(kyle) → review+
(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.
Created attachment 593616 [details] [diff] [review]
v2

Nits addressed. Can't check in though since HG is down. Sigh. Will do that as soon as it's back up.
Attachment #593298 - Attachment is obsolete: true
Created attachment 593633 [details] [diff] [review]
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.
Attachment #593616 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/6f9456fc1e83
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.