Bug 699235 (b2g-telephony)

[meta] B2G Telephony

RESOLVED FIXED

Status

()

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

People

(Reporter: qdot, Assigned: philikon)

Tracking

unspecified
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

Attachments

(3 obsolete attachments)

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.
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.
Depends on: 698621
Work will probably happen at

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

Will land patches through this bug.
Assignee: nobody → kmachulis
Depends on: 699242
Depends on: 699256
We now have a wiki page.

http://wiki.mozilla.org/B2G/RIL
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
Created attachment 575626 [details] [diff] [review]
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)?
Attachment #575626 - Flags: review+

Updated

6 years ago
Attachment #575626 - Flags: review+

Updated

6 years ago
Attachment #575626 - Attachment is patch: true

Comment 6

6 years ago
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.
(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

6 years ago
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

6 years ago
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.
(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.
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.
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.
Created attachment 576230 [details]
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.
Attachment #575626 - Attachment is obsolete: true
Attachment #576230 - Flags: feedback?(gal)

Comment 14

6 years ago
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.
Attachment #576230 - Flags: feedback?(gal) → feedback+

Comment 15

6 years ago
Any updates here?
(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.
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.
(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.
Created attachment 578482 [details] [diff] [review]
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.)
Attachment #576230 - Attachment is obsolete: true
Prior attachment is against current head of http://github.com/cgjones/mozilla-central, hash da62104d4d28196374c3978066dfc98feaba86ca
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.
Attachment #578482 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Summary: Create JS RIL Stack for B2G Telephony → [meta] Create JS RIL Stack for B2G Telephony
(Assignee)

Updated

6 years ago
Depends on: 699222
(Assignee)

Updated

6 years ago
Depends on: 707629
(Assignee)

Updated

6 years ago
Depends on: 707883
Morphing this bug further to include all aspects that are necessary for telephony.
Summary: [meta] Create JS RIL Stack for B2G Telephony → [meta] B2G Telephony
(Assignee)

Updated

6 years ago
Depends on: 708446
(Assignee)

Updated

6 years ago
Depends on: 708528
Depends on: 708926
(Assignee)

Updated

6 years ago
Blocks: 709564
(Assignee)

Updated

6 years ago
Alias: b2g-telephony
(Assignee)

Updated

6 years ago
Depends on: 709565
(Assignee)

Updated

6 years ago
Depends on: 709862
(Assignee)

Updated

6 years ago
Depends on: 709915
(Assignee)

Updated

6 years ago
Depends on: 710092
(Assignee)

Updated

6 years ago
Blocks: 710489
(Assignee)

Updated

6 years ago
No longer blocks: 709564
(Assignee)

Updated

6 years ago
No longer depends on: 708926
(Assignee)

Updated

6 years ago
No longer depends on: 710092
(Assignee)

Updated

6 years ago
No longer depends on: 709915
(Assignee)

Updated

6 years ago
Component: DOM → DOM: Device Interfaces
QA Contact: general → device-interfaces
(Assignee)

Updated

6 years ago
Depends on: 674726
(Assignee)

Updated

6 years ago
Depends on: 712942
(Assignee)

Updated

6 years ago
Depends on: 712944
Depends on: 713301
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.)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #23)
> Are we delivering "incoming" events through the DOM telephony api yet?

Yes we are.
(Assignee)

Updated

6 years ago
Depends on: 714968
Depends on: 714973
Blocks: 715782
(Assignee)

Updated

5 years ago
Depends on: 716709

Updated

5 years ago
Depends on: 719331
(Assignee)

Updated

5 years ago
Depends on: 720811
(Assignee)

Updated

5 years ago
Depends on: 720892
(Assignee)

Updated

5 years ago
No longer depends on: 720892
(Assignee)

Updated

5 years ago
Depends on: 721266

Updated

5 years ago
Depends on: 722841
(Assignee)

Updated

5 years ago
No longer depends on: 722841
(Assignee)

Updated

5 years ago
Depends on: 725599
Depends on: 727795
No longer depends on: 727795
Depends on: 727803
Depends on: 727795
(Assignee)

Updated

5 years ago
No longer depends on: 727803
(Assignee)

Updated

5 years ago
No longer depends on: 727795
Depends on: 731786
(Assignee)

Updated

5 years ago
No longer depends on: 731786
Assignee: kyle → philipp
Depends on: 735165
Depends on: 735167
Depends on: 735170
(Assignee)

Updated

5 years ago
No longer depends on: 735170
(Assignee)

Updated

5 years ago
Depends on: 740230

Updated

5 years ago
Depends on: 746496

Updated

5 years ago
Depends on: 746886
(Assignee)

Updated

5 years ago
Depends on: 749794
Depends on: 748187
(Assignee)

Updated

5 years ago
No longer depends on: 748187
Depends on: 751550
Depends on: 757852

Updated

5 years ago
Depends on: 757773

Updated

5 years ago
Blocks: 715784

Updated

5 years ago
No longer blocks: 715782
Whiteboard: [b2g:blocking+]
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
Whiteboard: [b2g:blocking+]
No longer depends on: 746886
No longer depends on: 751550
(Assignee)

Updated

5 years ago
Depends on: 766273
Initial implementation is basically complete. Breaking dependency on the one non-implementation related bug, and closing this.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
No longer depends on: 719331
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 719331
Depends on: 1008471
You need to log in before you can comment on or make changes to this bug.