699235 - (b2g-telephony) [meta] B2G Telephony

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

Create a stack to handle sending/receiving/parsing Android Parcels from/to the RIL IO thread, as well as maintaining the state of the phone stack (radio state, call list, etc...), and communicating with the telephony DOM in the main thread.

Comment 1

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

Remember that a good portion of this is similarly implemented in the mock-ril, distributed with B2G.

http://android.git.linaro.org/gitweb?p=platform/hardware/ril.git;a=tree;f=mock-ril/src/js;h=2039ad66530a3a26c2ec928d241b19c72adf67c3;hb=master

Note that it's pretty heavily protobuf'd/v8'd, but the general idea is there.

Comment 2

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

Work will probably happen at

http://www.github.com/kmachulis-mozilla/mozilla-central

Will land patches through this bug.

Comment 3

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

We now have a wiki page.

http://wiki.mozilla.org/B2G/RIL

Comment 4

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

Until we figure out where exactly this should go in m-c, I'm building the JS framework on github at

http://www.github.com/kmachulis-mozilla/b2g-js-ril

Ideally, we want the JS RIL stack to be a copy of the minimalist structure we've already build in python (since that's the language jaoo/ferjm/I are familiar with). This python file is now in the b2g-js-ril repo at

https://github.com/kmachulis-mozilla/b2g-js-ril/blob/master/python/dialer.py

Comment 5

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

Attachment: First pass at JS RIL manager

Checking in first (non-working) patch for JS RIL, to see if basic structure is ok. This was written as part of http://www.github.com/kmachulis-mozilla/b2g-js-ril and isn't actually ready to be merged into m-c yet, but the patch does apply and puts the js files in dom/telephony/worker-js

Some questions that still need to be answered:

- What should PostMessage send to the main thread?

- The assumption is that nothing in the worker thread should block, ever. Does that still hold true?

- RILManager used to call a parcel's callbacks after it was done constructing the parcel. There's now an explicit function to call the callbacks (currently commented out), which we can call at some other time if we want to keep the receive loop running (since that's dictated by whatever is sending in data from the radio). Do we want to keep it this way, or was the old way working (since, once again, callbacks shouldn't ever block)?

Comment 6

[Andreas Gal :gal]

Comment on attachment 575626 [details] [diff] [review]
First pass at JS RIL manager

>+/**
>+ * Base implementation.
>+ */
>+function RILParcel(data) {

Wrapping every parcel into this class creates an allocation.

>+    let arg;
>+    [this.response_type, arg] = new Int32Array(this.buffer, 0, 2);

This allocates another 2 objects for every parcel.

>+  /**
>+   * Turn a string into an array buffer of 16-bit chars
>+   */
>+  strToByteArray: function(s) {
>+    var buf = ArrayBuffer(s.length * 2);
>+    var uint16Buf = Uint16Array(buf);
>+    var i = 0;
>+    for(i = 0; i < s.length; ++i)
>+    {
>+	    uint16Buf[i] = s.charCodeAt(i);
>+    }
>+    return buf;
>+  },
>+  /**
>+   * Turn an array buffer of 16-bit chars into a string
>+   */
>+  byteArrayToStr: function (start, len) {
>+    var st = "";
>+    for each (x in Uint16Array(this.buffer, start, len)) st += String.fromCharCode(x);
>+    return st;

This yields horrible performance due to the way iteration works on arrays.

>+       // TODO: Actually unpack data correctly
>+       let pn_str = this.strToByteArray(this.data[0]);
>+       this.buffer = new ArrayBuffer(4 + pn_str.byteLength + 4 + 4 + 4);
>+       Uint32Array(this.buffer, 0, 1)[0] = pn_str.byteLength / 2;
>+       (Uint8Array(this.buffer, 4, pn_str.byteLength)).set(Uint8Array(pn_str, 0, pn_str.byteLength));
>+       //Uint32Array(this.buffer, ).set([0,0,0]);

Again globs of object constructions.

Kyle, this new code is a really bad idea. The previous version was carefully constructed to avoid object allocation as much as possible. This code is going to be pretty GC happy which is probably a bad idea for code that has real-time requirements.

Comment 7

[Philipp von Weitershausen [:philikon]]

(In reply to Andreas Gal :gal from comment #6)
> >+    let arg;
> >+    [this.response_type, arg] = new Int32Array(this.buffer, 0, 2);
> 
> This allocates another 2 objects for every parcel.

A lot of these types of allocations can be avoided by getting a (native) DataView for the buffer and only looking at that. Then one can read any types of values via the DataView and avoid having to create lots of different typed array objects.

> >+  /**
> >+   * Turn an array buffer of 16-bit chars into a string
> >+   */
> >+  byteArrayToStr: function (start, len) {
> >+    var st = "";
> >+    for each (x in Uint16Array(this.buffer, start, len)) st += String.fromCharCode(x);
> >+    return st;
> 
> This yields horrible performance due to the way iteration works on arrays.

It would be lovely if typed arrays (or the DataView) had a native call for getting their data as a string. Or of course we could convert all data into 16 bit strings before we hand it to JS in the first place, and then in the JS code we just pick the values from those strings and substring when we want actual strings.

No idea whether this would be better or not. I bet the performance, GC, and real-time characteristics will probably depend on the types, frequency, and size of parcels (which I have no idea about.)

Comment 8

[Andreas Gal :gal]

Well we will get a DataView soon, but even with that, this code is _way_ complicated and over-abstracted. I am going to take a stab at squeezing out some if it.

Comment 9

[Andreas Gal :gal]

There seems to be two data types in those packets. Int32 and strings. Write two functions that (bitshift) read those from a bytearray or string using an index you supply, and two functions to write this out. DataView is really not helpful here. Don't over-abstract. Make it simple and brief. This code should be 100 lines or less, plus constants.

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to Andreas Gal :gal from comment #9)
> There seems to be two data types in those packets. Int32 and strings.

Oh, having seen Uint8 and what not in there I figured we needed all sorts of different types. Yeah, that makes things a lot easier. Probably easiest would be working with strings where each char is constructed from 2 bytes. That's what the RIL strings seem to look like and we'd avoid tedious string mechanics in JS.

Comment 11

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

Well, uh, damn. Quite the "Welcome to Javascript".

So, one thing about the "real-time"ness and the GC. I really don't think we're sending/receiving nearly enough packets that the allocations I'm incurring would lock the GC. There's an interesting question of whether that could cause a DDOS attack against the GC somehow if we did that, but, really, we're getting maybe 100-200 packets a minute under normal use, shouldn't have more that 10-20 outstanding at any time. Nothing is being streamed. The RIL protocol isn't even made for that.

Comment 12

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

Ok the more I think about this, the more the disconnect is startin to make sense. I've been in implementations of this in languages that either have easier memory overlays (c), native overlays (pbufs via v8 in mockril) or, uh, whatever excuse java is. Was planning on cleaning up te dataview stuff (I realize how gross the uint8view indexing was), but I guess I can remove the parcel class completely and just parse packets and change the state of the phone object as we go (by adding more functions to the array weve already got going in parcel.js) should be much lighter weight.

Comment 13

[Philipp von Weitershausen [:philikon]]

Attachment: lightweight impl (WIP)

Kyle and I paired on a more lightweight implementation yesterday. Here's the WIP so far. This is somewhat inspired by Andreas's prototype, except that we use a TypedArray ringbuffer to even avoid new string allocations. We're now working on writing tests for it and hooking it up to the socket so we can have the phone dialing again.

Comment 14

[Andreas Gal :gal]

Comment on attachment 576230 [details]
lightweight impl (WIP)

Make the ringbuffer grow dynamically every time the tail catches the head. Should be pretty straight forward. Just memcpy and update the constants.

Very nice otherwise.

Comment 15

[Andreas Gal :gal]

Any updates here?

Comment 16

[Philipp von Weitershausen [:philikon]]

(In reply to Andreas Gal :gal from comment #15)
> Any updates here?

Kyle and I are revving quickly on GitHub: http://github.com/kmachulis-mozilla/b2g-js-ril and my fork of that.

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

I'm not sure if this comment fits in here. Anyway, since we have been quiet for a while, I just wanted to keep you posted about what jaoo and I are currently doing regarding the RIL stuff. We've been working a bit on figuring out how the 3G works and how we could set the connection up (I've made a comment on github about that), we've also been trying to see how to receive calls and SMS. We wasn't able to make any successful test regarding 3G or calls reception yet, even without the rild2 patch, on a fresh B2G build. We are wondering if there is a problem within the libril propietary library itself. Maybe it is similar to the github issue #56 related to the mediaserver. Some sort of differences between versions issue. jaoo is probably starting to work on this.
SMS reception is working fine without the rild2 patch, so while we figure out what is going on with the issues on the proprietary libril over the B2G build, I am probably starting to prototype a PDU JS parser if none of you is already working on that. I've seen that Philip wrote a TODO about that on the b2g-js-ril repo.

Comment 18

[Philipp von Weitershausen [:philikon]]

(In reply to Fernando Jiménez from comment #17)
> SMS reception is working fine without the rild2 patch, so while we figure
> out what is going on with the issues on the proprietary libril over the B2G
> build, I am probably starting to prototype a PDU JS parser if none of you is
> already working on that. I've seen that Philip wrote a TODO about that on
> the b2g-js-ril repo.

That's a great idea! Feel free to work a fork of the b2g-js-ril repo, we'll be happy to integrate your work.

Comment 19

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

Attachment: Patch for implementing full B2G RIL Stack

This is everything. Dials the phone. Hooray.

(Still causes phone to heat up due to RIL Thread not sleeping, but signals didn't fix it on android. Need to talk to cjones.)

Comment 20

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

Prior attachment is against current head of http://github.com/cgjones/mozilla-central, hash da62104d4d28196374c3978066dfc98feaba86ca

Comment 21

[Philipp von Weitershausen [:philikon]]

Comment on attachment 578482 [details] [diff] [review]
Patch for implementing full B2G RIL Stack

Let's make this a tracking bug. I have split this up and applied to m-c, will attach patches to individual bugs.

Comment 22

[Philipp von Weitershausen [:philikon]]

Morphing this bug further to include all aspects that are necessary for telephony.

Comment 23

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

With pretty close to m-c tip, I see the following on my (real) b2g phone when I call it

I/Gecko   ( 2587): -*- TelephonyWorker component: Received message: {"type":"callstatechange","callState":"incoming","callIndex":1,"number":"15106845513","name":null}

but I don't see any incoming-call UI pop up in gaia.  Are we delivering "incoming" events through the DOM telephony api yet?  (If so, this is a gaia bug.)

Comment 24

[Philipp von Weitershausen [:philikon]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #23)
> Are we delivering "incoming" events through the DOM telephony api yet?

Yes we are.

Comment 25

[Dietrich Ayala (:dietrich)]

Initial implementation is basically complete. Breaking dependency on the one non-implementation related bug, and closing this.