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

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

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".

(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

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 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.

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

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

(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 :)

(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

(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.

(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?

(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

(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.

(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

(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 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?

(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.

(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

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 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

(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 :)

(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.

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

(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 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

Address to philikon's comments. - Using Math.floor - 'finish' typo in comments. - remove "let add =" and "let finish =" in getADN, getFDN thanks

(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 on attachment 626680 [details] [diff] [review] Get ADN and FDN, v7 What Gregor said :)

Please set the target milestone when landing on inbound :-) https://hg.mozilla.org/mozilla-central/rev/d9064a680e21