754018 - B2G RIL: Read SIM contacts

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P2)

Description

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

We need to retrieve SIM contacts, including FDNs and ADNs.
For USIM, we also need to retrieve EF_PBR.

Comment 1

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

Hi, philikon
Can you kindly help to give me some suggestions about how to expose these SIM contacts to RadioInterfaceLayer? For example do I need to add an interface like 'getSimContacts(type)' for that?

thanks

Comment 2

[Philipp von Weitershausen [:philikon]]

The method needs to be asynchronous and take a callback. What does the `type` parameter specify? Also, we should call it "ICC" or just "card" instead of "SIM".

Comment 3

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

(In reply to Philipp von Weitershausen [:philikon] from comment #2)
> The method needs to be asynchronous and take a callback. What does the
> `type` parameter specify? Also, we should call it "ICC" or just "card"
> instead of "SIM".

Hi Philikon
Great thanks to your suggestions.
The type parameter refers to FDN, ADN and SDN from TS 51.011.
So I may add an interface in nsIRadioInterfaceLayer whose prototype is  
void getICCContacts(type, callback)

Does that make sense to you??

thanks

Comment 4

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

Attachment: [WIP] Get ADN and FDN patch v2

Hi, gwanger
This is the WIP I did so far.
Sorry for the stupid function naming, I'll work on that.

you could do like
http://mxr.mozilla.org/mozilla-central/source/dom/sms/src/ril/SmsDatabaseService.js#611
to get RadioInterfaceLayer,

then call mRIL.getICCContacts("FDN", contactsCallback); 
in which contactsCallback has a function called receiveContactsList

In the receiveContactsList, 
the 1st arg is "FDN" or "ADN",
the 2nd is jsvals, it's an Array consists of contacts, 
currently the format is 
{ alphaId: ..., 
  number: ... }

Currently We haven't done writing SIM contacts yet, 
so you might need to use another phone to enable FDN and add sim contact manually.

thanks

Comment 5

[Philipp von Weitershausen [:philikon]]

Comment on attachment 623000 [details] [diff] [review]
[WIP] Get ADN and FDN patch v2

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1215,5 @@
>    },
>  
> +  _contactsCallback: null,
> +  getICCContacts: function getICCContacts(type, callback) {
> +    this._contactsCallback = callback;

This needs to be a bit smarter, because you could call this method multiple times in a row. This would overwrite _contactsCallback each time. You want to assign a callback id (generated from a simple counter), keep the id -> cb mapping in an object, and pass the id around in the `options` object to and from ril_worker.js. Also, don't forget to delete the callback reference when you're done to avoid leaks!

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +120,5 @@
>    void receiveDataCallList([array,size_is(length)] in nsIRILDataCallInfo dataCalls,
>                             in unsigned long length);
>  };
>  
> +[scriptable, uuid(fdb98f9b-2679-4a6c-86fc-ccc1817899a8)]

Make this a `[scriptable, function , ...]` interface. Then JS consumers can just pass a JS function.

Comment 6

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

Attachment: [WIP] Get ADN and FDN ,v4

Address to philikon's comments. Thanks , philikon :)

Since this bug depends on Bug 753034, I will make a review request when Bug 753034 is fixed.

thanks

Comment 7

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

Attachment: Get ADN and FDN, v5

Send a review request to philikon since Bug 753034 got a review+ (although not checked-in yet).

Hi, philikon:

Should I also file a bug for testing as follow-up ??

thanks

Comment 8

[Gregor Wagner [:gwagner]]

(In reply to Yoshi Huang[:yoshi] from comment #7)
> Created attachment 625047 [details] [diff] [review]
> Get ADN and FDN, v5
> 
> Send a review request to philikon since Bug 753034 got a review+ (although
> not checked-in yet).

Great! I started using it yesterday. 
How did you test it anyway? Is there a way to get contacts in our supported format on the SIM card?


> 
> Should I also file a bug for testing as follow-up ??
> 

Yes please :)

Comment 9

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

(In reply to Gregor Wagner [:gwagner] from comment #8)
> How did you test it anyway? Is there a way to get contacts in our supported
> format on the SIM card?

Hi gwagner, 

Can you say more about "our supported format" ? Do you mean vCard?

I test the my code in ContactService.jsm by myself, 
I call 
XPCOMUtils.defineLazyGetter(this, "mRIL", 
function () {
  return Cc["@mozilla.org/telephony/system-worker-manager;1"].                    getService(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIRadioInterfaceLayer);
});
in ContactService.jsm to get RIL

and then 
mRIL.getICCContacts("ADN", function callback(type, contacts) { ... }); 

to get contacts
The format of the contacts is { alphaId:... , number: ... }, which is from TS 31.102

thanks

Comment 10

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

(In reply to Gregor Wagner [:gwagner] from comment #8)
> > 
> > Should I also file a bug for testing as follow-up ??
> > 
> 
> Yes please :)

File Bug 756931 for this.

Comment 11

[Gregor Wagner [:gwagner]]

(In reply to Yoshi Huang[:yoshi] from comment #9)
> (In reply to Gregor Wagner [:gwagner] from comment #8)
> > How did you test it anyway? Is there a way to get contacts in our supported
> > format on the SIM card?
> 
> Hi gwagner, 
> 
> Can you say more about "our supported format" ? Do you mean vCard?
> 
> I test the my code in ContactService.jsm by myself, 
> I call 
> XPCOMUtils.defineLazyGetter(this, "mRIL", 
> function () {
>   return Cc["@mozilla.org/telephony/system-worker-manager;1"].              
> getService(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIRadioInterfaceLayer);
> });
> in ContactService.jsm to get RIL
> 
> and then 
> mRIL.getICCContacts("ADN", function callback(type, contacts) { ... }); 
> 
> to get contacts
> The format of the contacts is { alphaId:... , number: ... }, which is from
> TS 31.102

Yeah that looks good. Do you have a 2G SIM card for testing and how do you get the contacts on the sim card?

Comment 12

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

(In reply to Gregor Wagner [:gwagner] from comment #11)
> Yeah that looks good. Do you have a 2G SIM card for testing and 

Sorry I didn't have a 2G SIM.
Do I need to get one?


> how do you
> get the contacts on the sim card?

I use other phone to enable FDN and add sim contact(ADN and FDN), is this what you mean?

thanks

Comment 13

[Gregor Wagner [:gwagner]]

(In reply to Yoshi Huang[:yoshi] from comment #12)
> (In reply to Gregor Wagner [:gwagner] from comment #11)
> > Yeah that looks good. Do you have a 2G SIM card for testing and 
> 
> Sorry I didn't have a 2G SIM.
> Do I need to get one?
> 
> 
> > how do you
> > get the contacts on the sim card?
> 
> I use other phone to enable FDN and add sim contact(ADN and FDN), is this
> what you mean?
> 
> thanks

I also tried this but it didn't work for me. I got some error messages from the ril_worker thread. Maybe I didn't store them in the right format. I will try again tomorrow.

Comment 14

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

(In reply to Gregor Wagner [:gwagner] from comment #13)
> I also tried this but it didn't work for me. I got some error messages from
> the ril_worker thread. Maybe I didn't store them in the right format. I will
> try again tomorrow.
Okay, you could post the log and I'll check that ASAP.
thanks

Comment 15

[Gregor Wagner [:gwagner]]

(In reply to Yoshi Huang[:yoshi] from comment #14)
> (In reply to Gregor Wagner [:gwagner] from comment #13)
> > I also tried this but it didn't work for me. I got some error messages from
> > the ril_worker thread. Maybe I didn't store them in the right format. I will
> > try again tomorrow.
> Okay, you could post the log and I'll check that ASAP.
> thanks

The bug was in the patch for bug 753034.
FDN works fine! \o/
I still see some wired behavior with ADN.
I should only have 3 contacts on the card but is somehow tries to read 255 contacts:

I/Gecko   ( 1835): RIL Worker: in parseDiallingNumber {"command":178,"fileId":20282,"pathId":"3f007f10","p1":127,"p2":4,"p3":44,"data":null,"pin2":null,"type":1,"loadAll":true,"requestId":0,"rilRequestType":28,"rilRequestError":0,"recordSize":44,"totalRecords":254}

At the end I get my 3 contacts out of the card but I see a lot of parcel communication.

Comment 16

[Philipp von Weitershausen [:philikon]]

Comment on attachment 625047 [details] [diff] [review]
Get ADN and FDN, v5

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

Looks good, but I do have some questions below. Thanks!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +350,5 @@
> +        }
> +        let callback = this._contactsCallbacks[message.contactType][message.requestId];
> +        if (callback) {
> +          callback.receiveContactsList(message.contactType, message.contacts);
> +          delete this._contactsCallbacks[message.contactType][message.requestId];

Delete first, then call callback. Otherwise the callback might fail and then we leak memory.

@@ +1249,5 @@
> +    }
> +    if (this._contactsCallbacks[type].indexOf(callback) == -1) {
> +      this._contactsCallbacks[type].push(callback);
> +    }
> +    let requestId = this._contactsCallbacks[type].indexOf(callback);

This logic doesn't make too much sense. If I called getICCContacts() twice with the same callback function passed, I would expect that function to be called twice. So I think we should award separate requestIds here.

Kind of an edge case, but we should make it predictable nonetheless.

::: dom/system/gonk/ril_worker.js
@@ +910,5 @@
>        if (len > MSISDN_MAX_NUMBER_SIZE_BYTES) {
>          debug("ICC_EF_MSISDN: invalid length of BCD number/SSC contents - " + len);
>          return;
>        }
> +      this.iccInfo.MSISDN = GsmPDUHelper.readDiallingNumber(len);

Below you write

  "Here the definition of the length is different from SMS spec,"

in the comment for `readDiallingNumber()`. So how come it's just a drop-in replacement for the SMS variant `readAddress()` then? Or, looking at it the other way, why are we still keeping `readAddress()` around if `readDiallingNumber()` can take over? I'm a bit confused. :)

@@ +3728,5 @@
> +   * Here the definition of the length is different from SMS spec, 
> +   * TS 23.040 9.1.2.5, which the length means 
> +   * "number of useful semi-octets within the Address-Value field".
> +   */
> +  readDiallingNumber: function readDiallingNumber(len) {

I think we used the spelling "dialing" everywhere else. Also, I'm not quite sure what the word "dialing" means in this context since this isn't necessarily about telephony. Is "dialing number" a term that's used in the spec?

Comment 17

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

(In reply to Philipp von Weitershausen [:philikon] from comment #16)
> Below you write
> 
>   "Here the definition of the length is different from SMS spec,"
> 
> in the comment for `readDiallingNumber()`. So how come it's just a drop-in
> replacement for the SMS variant `readAddress()` then? Or, looking at it the
> other way, why are we still keeping `readAddress()` around if
> `readDiallingNumber()` can take over? I'm a bit confused. :)
> 
Yeah, here it's a little bit complicated, I have spent some time discussing with Vicamo with this. And realize the difference in it, so I add another function.
The main complication is there are different interpretations about the 'length' field between SMS spec (TS 23.040) and USIM spec (TS 131.102)

From the SMS spec, the length doesn't include the TON/NPI byte, also its value is the length of available semi octets. i.e. if the number is 1234, BCD format will be 2143, length will be 4. (said 4 digits), here the length doesn't include TON/NPI(said 81), so full address in BCD format will be 4812143.

In USIM, the length *includes* TON/NPI byte, also its value is the bytes needed to store that number. The spec doesn't explicitly say so, but I found this when I parse these ICC records. i.e. the number is 1234, BCD will be 2143, then prepends the TON/NPI, said 81, the dialling number in BCD will be 812143, and its length will be 3. (3 bytes), full dialling number will be 3812143

So readAddress is used for SMS, and readDiallingNumber is used for USIM.


> @@ +3728,5 @@
> > +   * Here the definition of the length is different from SMS spec, 
> > +   * TS 23.040 9.1.2.5, which the length means 
> > +   * "number of useful semi-octets within the Address-Value field".
> > +   */
> > +  readDiallingNumber: function readDiallingNumber(len) {
> 
> I think we used the spelling "dialing" everywhere else. Also, I'm not quite
> sure what the word "dialing" means in this context since this isn't
> necessarily about telephony. Is "dialing number" a term that's used in the
> spec?

Yes, from spec TS 31.102 it used the term 'Dialling number' in MSISDN, ADN, and FDN. For others like MBI(MailBox Identifier), it uses Dialling number identifier. Also notice that it used 2 l's in 'Dialling'. I found the spec uses 'Dialling' with 2 l's. So I have used the term following the spec.

I'll address other comments in my next patch, thanks for your suggestions.

Comment 18

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

(In reply to Gregor Wagner [:gwagner] from comment #15)
> I still see some wired behavior with ADN.
> I should only have 3 contacts on the card but is somehow tries to read 255
> contacts:
> 
> I/Gecko   ( 1835): RIL Worker: in parseDiallingNumber
> {"command":178,"fileId":20282,"pathId":"3f007f10","p1":127,"p2":4,"p3":44,
> "data":null,"pin2":null,"type":1,"loadAll":true,"requestId":0,
> "rilRequestType":28,"rilRequestError":0,"recordSize":44,"totalRecords":254}
> 
> At the end I get my 3 contacts out of the card but I see a lot of parcel
> communication.

Hi , gwagner
Yes I know this, but I have checked Android's implementation and seems it also tried to load all APN records like I did.

Anyway if I found there's better way to handle this I'll file a new Bug as follow-up and let you know.

thanks

Comment 19

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

Attachment: Get ADN and FDN, v6

Address to philikon's comments
1. delete callback before calling it
2. Use Math.random to get a unique requestId to handle multiple callbacks.

I've added r=philikon in this patch.

thanks

Comment 20

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

Comment on attachment 625939 [details] [diff] [review]
Get ADN and FDN, v6

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

::: dom/system/gonk/ril_worker.js
@@ +1055,5 @@
> +        debug("ICC_EF_FDN: invalid length of BCD number/SSC contents - " + numLen);
> +        return;
> +      }
> +
> +      addCallback.call(this, {alphaId: alphaId,

I should add a check if addCallback is undefined.

@@ +1074,5 @@
> +        options.p1 < options.totalRecords) {
> +      options.p1++;
> +      this.iccIO(options);
> +    } else {
> +      finishCallback.call(this);

ditto

Comment 21

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

Attachment: Get ADN and FDN, v6

Comment 22

[Gregor Wagner [:gwagner]]

(In reply to Yoshi Huang[:yoshi] from comment #21)
> Created attachment 625954 [details] [diff] [review]
> Get ADN and FDN, v6

Hey Yoshi, I think you added the wrong patch :)

Comment 23

[Gregor Wagner [:gwagner]]

(In reply to Yoshi Huang[:yoshi] from comment #18)
> (In reply to Gregor Wagner [:gwagner] from comment #15)
> > I still see some wired behavior with ADN.
> > I should only have 3 contacts on the card but is somehow tries to read 255
> > contacts:
> > 
> > I/Gecko   ( 1835): RIL Worker: in parseDiallingNumber
> > {"command":178,"fileId":20282,"pathId":"3f007f10","p1":127,"p2":4,"p3":44,
> > "data":null,"pin2":null,"type":1,"loadAll":true,"requestId":0,
> > "rilRequestType":28,"rilRequestError":0,"recordSize":44,"totalRecords":254}
> > 
> > At the end I get my 3 contacts out of the card but I see a lot of parcel
> > communication.
> 
> Hi , gwagner
> Yes I know this, but I have checked Android's implementation and seems it
> also tried to load all APN records like I did.
> 
> Anyway if I found there's better way to handle this I'll file a new Bug as
> follow-up and let you know.
> 
> thanks

Oh ok. Don't worry then. This isn't performance critical at all.

Comment 24

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

Attachment: Get ADN and FDN, v6

Re-Upload the patch, thanks gwagner for reminding this.

Comment 25

[Philipp von Weitershausen [:philikon]]

(In reply to Yoshi Huang[:yoshi] from comment #17)
> So readAddress is used for SMS, and readDiallingNumber is used for USIM.

I understand that. I was confused about replacing readAddress with readDiallingNumber, but I realize now that's in the getMSISDN() function, so that makes sense.

> > @@ +3728,5 @@
> > > +   * Here the definition of the length is different from SMS spec, 
> > > +   * TS 23.040 9.1.2.5, which the length means 
> > > +   * "number of useful semi-octets within the Address-Value field".
> > > +   */
> > > +  readDiallingNumber: function readDiallingNumber(len) {
> > 
> > I think we used the spelling "dialing" everywhere else. Also, I'm not quite
> > sure what the word "dialing" means in this context since this isn't
> > necessarily about telephony. Is "dialing number" a term that's used in the
> > spec?
> 
> Yes, from spec TS 31.102 it used the term 'Dialling number' in MSISDN, ADN,
> and FDN. For others like MBI(MailBox Identifier), it uses Dialling number
> identifier. Also notice that it used 2 l's in 'Dialling'. I found the spec
> uses 'Dialling' with 2 l's. So I have used the term following the spec.

Ok fair enough.

Comment 26

[Philipp von Weitershausen [:philikon]]

Comment on attachment 626280 [details] [diff] [review]
Get ADN and FDN, v6

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

Nice! r=me with nits.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1245,5 @@
> +  getICCContacts: function getICCContacts(type, callback) {
> +    if (!this._contactsCallbacks) {
> +      this._contactsCallbacks = {};
> +    } 
> +    let requestId = parseInt(Math.random() * 1000);

s/parseInt/Math.floor/

::: dom/system/gonk/ril_worker.js
@@ +1034,5 @@
> +   *         The 'options' object passed from RIL.iccIO
> +   *  @param addCallback
> +   *         The function should be invoked when the ICC record is processed 
> +   *         succesfully.
> +   *  @param finsihCallback

typo: finish

@@ +1136,5 @@
> +   *         Request id from RadioInterfaceLayer.
> +   */
> +  getADN: function getADN(options) {
> +    function callback(options) {
> +      let add = function add(contact) {

nit: there's no need for `let foo = function foo()`, just `function foo()` is enough.

@@ +1139,5 @@
> +    function callback(options) {
> +      let add = function add(contact) {
> +          this.iccInfo.ADN.push(contact);
> +      };
> +      let finish = function finish() {

ditto

Comment 27

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

Attachment: Get ADN and FDN, v7

Address to philikon's comments.

- Using Math.floor
- 'finish' typo in comments.
- remove "let add =" and "let finish =" in getADN, getFDN

thanks

Comment 28

[Gregor Wagner [:gwagner]]

(In reply to Yoshi Huang[:yoshi] from comment #27)
> Created attachment 626680 [details] [diff] [review]
> Get ADN and FDN, v7
> 
> Address to philikon's comments.
> 
> - Using Math.floor
> - 'finish' typo in comments.
> - remove "let add =" and "let finish =" in getADN, getFDN
> 
> thanks

Philipp already gave you a r+. You don't have to ask for review again if you only changed the review comments.

Comment 29

[Philipp von Weitershausen [:philikon]]

Comment on attachment 626680 [details] [diff] [review]
Get ADN and FDN, v7

What Gregor said :)

Comment 30

[Philipp von Weitershausen [:philikon]]

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

Comment 31

[Ed Morley [:emorley]]

Please set the target milestone when landing on inbound :-)

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