720638 - B2G RIL: Read MSISDN

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

We need to retrieve and expose the phone number string for the active line, for example, the MSISDN for a GSM phone.

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

It seems the MSISDN cannot be retrieved. Through RIL we can retrieve the IMSI (International Mobile Subscriber Identity) but I wonder if this is valid enough for the goal of this bug. The SIM card has an IMSI that is sent to the HLR (Home Location Register) which is in charge of doing the mapping IMSI to MSISDN. Carriers could store the MSISDN on the SIM  but they do not usually do it since it is not required in the GSM protocol.

Comment 2

[jacobtopper]

Maybe I'm missing something, but in my experience getting access to the phone number if fairly simple on Android. Other platforms I can't speak for, but I found this by searching through bugs for Android Firefox so I'm assuming that's what we're talking about. Exposing it to javascript land is outside of my area of expertise, but here's the android documentation:

http://developer.android.com/reference/android/telephony/TelephonyManager.html#getLine1Number()

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to jacobtopper from comment #2)
> Maybe I'm missing something, but in my experience getting access to the
> phone number if fairly simple on Android. Other platforms I can't speak for,
> but I found this by searching through bugs for Android Firefox so I'm
> assuming that's what we're talking about. Exposing it to javascript land is
> outside of my area of expertise, but here's the android documentation:
> 
> http://developer.android.com/reference/android/telephony/TelephonyManager.
> html#getLine1Number()

Thank you, but B2G does not use any of the Android Java APIs, so this not useful to us at all. Also, if you read the documentation for the method you linked to:

  Returns the phone number string for line 1, for example, the MSISDN for a GSM phone. Return null if it is unavailable.

you'll find that's exactly what Fernando is talking about, and that it's not necessarily available.

Comment 4

[Philipp von Weitershausen [:philikon]]

Fernando, we should still try to expose this information if we can. Also, CDMA phones definitely have this capability.

Comment 5

[jacobtopper]

(In reply to Philipp von Weitershausen [:philikon] from comment #3)

Sorry about that. I'm unfamiliar with the B2G project and apparently made the wrong assumption based on this bug's appearance in bugs for Firefox for Android. As for that documentation, I said in my experience because having developed multiple Android apps that needed phone numbers, I've never found that function to fail me unless it's used on a non-phone device.

Comment 6

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

Hi 
I would like to take this bug if no one is working on it
thanks

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

(In reply to yoshi huang [:yhuang] from comment #6)
> Hi 
> I would like to take this bug if no one is working on it
> than

Go for it! :)

Comment 8

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

Hi, there
I got two questions to ask,

1. is it okay to call RIL.xxx() in Phone.onXXX(), i.e. call RIL method in callback method?
   should I post the msg back to Main Thread then call RIL.xxx() from Main Thread? if so, how
   also I find that in Phone.onCallStateChanged(), it calls RIL.getCurrentCalls(), 
   does this mean I can do roughly the same in those callback methods?

2. I try to add a new method in Phone, i.e. Phone.getMSISDN, but seems it cannot work anyway, 
   When I launch the dialer app, there's *nothing* I can do, I just hang in the dialer app. 
   so if I'd like to add a method in Phone obj, 
   should I also need to modify some header files or IDLs accordingly ?
   
thanks

Comment 9

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

thanks for Thinker for clarification 
please ignore my previous stupid questions 

now my current status
using REQUEST_SIM_IO (AT +CRSM) to get MSISDN

1. command: GET_RESPONSE  (0xc0)
   field  : EF_MSISDN (0x6f40)
   pathid : '3f000700' 
   p1     : 0
   p2     : 0
   p3     : 15 
   data   : null
   pin2   : null

response: 
   sw1 : 0x90 
   sw2 : 0x0
   response: 00001a6f40040000ffff0102011a

this response is okay
it shows I got 1 record, ( 1a/1a =1, and its size = 1a)

2. command: READ_RECORD  (0xb2)
   field  : EF_MSISDN (0x6f40)
   pathid : '3f000700' 
   p1     : 1 (record number)
   p2     : 4 (READ_RECORD ABSOLUTE MODE, see TS102 221)
   p3     : 26 (size of record)
   data   : null
   pin2   : null

response 
   sw1 : 0x90
   sw2 : 0x0 
   response: ffffffffffffffffffffffffffffffffffffffffffffffffffff (total my requested 26 bytes with each byte is ff)

Is there something wrong with my parameters?
or it's just my sim card/operator doesn't have this information 

thanks

Comment 10

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

Attachment: [WIP]getMSISDN by using iccIO

as previous comment
first uses GET_RESPONSE, then uses READ_RECORD
but the response in READ_RECORD in my sim cards are all fffffffffffffffffffffff

Comment 11

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

test on B2G emulator, 
the patch can get MSISDN from qemu

I/Gecko(161): RIL Worker: response=ffffffffffffffffffffffffffffffffffff07815155255155f4ffffffffffff

Comment 12

[Philipp von Weitershausen [:philikon]]

(In reply to yoshi huang [:yhuang] from comment #11)
> test on B2G emulator, 
> the patch can get MSISDN from qemu
> 
> I/Gecko(161): RIL Worker:
> response=ffffffffffffffffffffffffffffffffffff07815155255155f4ffffffffffff

I see something similar with my AT&T prepaid SIM card:

I/Gecko   ( 2614): RIL Worker: response=ffffffffffffffffffffffffffffffffffff0781415147w8yxfzffffffffffff

where 415748wxyz is my phone number. So it seems we're dealing with swapped nibble BCD (like with SMS) and some sort of prefix consisting of 'ff' * 18 and '7018'.

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 601567 [details] [diff] [review]
[WIP]getMSISDN by using iccIO

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

Going to clear the review flag since this is just a WIP. I expect a final patch to clean up `_handleMSISDNResponse` (it should live on the Phone object) and get rid of all debug/dump output, or at least gate the ones that you want to keep with `if (DEBUG)`.

Also, we need to actually convert the MSISDN to the actual phone number. Please harness the readSwappedNibbleBCD code for that.

::: dom/system/b2g/ril_worker.js
@@ +934,5 @@
> +    Buf.writeUint32(options.p2);
> +    Buf.writeUint32(options.p3);
> +    Buf.writeString(options.data);
> +    if (options.pin2 != null)
> +      Buf.writeString(pin2);

Nit: always do braces, please.

@@ +1364,5 @@
> +  //TODO
> +  iccIOCommand: 0,
> +  
> +  EFID: 0,
> +

What are these? Please document.

@@ +2195,5 @@
>  
>    /**
> +   *  get MSISDN
> +   */ 
> +  getMSISDN: function getMSISDN(options) {

I don't think this method should take `options`

Comment 14

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

Attachment: [WIP v2]getMSISDN by using iccIO

revise code with last comment
also retrieve MSISDN from response with BCD format
and MSISDN will be stored in Phone.MSISDN as the IMEI does

but I didn't use the original readSwappedNibbleBCD because we already got the data from Buffer, and the original one will try to read from Buffer again.
maybe need to revise the original one so it can be reused

Comment 15

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

Attachment: patch for getMSISDN by using iccIO

using iccIO to get MSISDN
revise base on philikon's comment
also create a similar function of GsmPDUHelp.readSwappedNibbleBCD to decode BCD

Comment 16

[Philipp von Weitershausen [:philikon]]

Comment on attachment 602796 [details] [diff] [review]
patch for getMSISDN by using iccIO

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

This is looking a bit better, but I would still like it to be cleaned up more. I think I also found a solution for the _readSwappedNibbleBCD duplication.

On a more general note, please note our general coding style: https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide. Also please use proper spelling, capitalization, and punctuation in all comments. Most of the comments you added do not comply to this.

::: dom/system/b2g/ril_worker.js
@@ +1368,5 @@
> +   * issued command and EF field, so add these two members to keep track
> +   * of the last operations. 
> +   */ 
> +  iccIOCommand: 0,
> +  EFID: 0,

Please don't. Please pass this information around in the `options` object. In RIL.iccIO you do

  let token = Buf.newParcel(REQUEST_SIM_IO, options);

and then it will be passed into RIL[REQUEST_SIM_IO]:

  RIL[REQUEST_SIM_IO] = function REQUEST_SIM_IO(length, options) {
    // ... extract options.command and options.EFID here.
  }

@@ +1724,4 @@
>    },
>  
>    onIMEI: function onIMEI(imei) {
> +    debug("onIMEI " + imei);

Gate this with `if (DEBUG)` and perhaps add a more useful debug message, e.g.: debug("Read IMEI: " + imei).

@@ +1736,5 @@
> +    if (DEBUG) {
> +      debug('sw1='+sw1);
> +      debug('sw2='+sw2);
> +      debug('response='+response);
> +    }

Please clean this up and make it more useful, e.g.:

  debug("ICC I/O (" + sw1 + "/" + sw2 + ") response: " + response);

(This is just an example. Perhaps

@@ +1738,5 @@
> +      debug('sw2='+sw2);
> +      debug('response='+response);
> +    }
> +
> +    switch(this.EFID) {

Coding style nit: space before `(`. Please fix this here and everywhere else.

@@ +1747,5 @@
> +  },
> +  
> +  _handleMSISDNResponse: function _handleMSISDNResponse(sw1, sw2, response) {
> +    //  see GSM11.11 section 9.4 for sw1 and sw2
> +    if (sw1 != 0x90 && sw1 != 0x9f) {

Magic numbers! Please const them.

@@ +1748,5 @@
> +  
> +  _handleMSISDNResponse: function _handleMSISDNResponse(sw1, sw2, response) {
> +    //  see GSM11.11 section 9.4 for sw1 and sw2
> +    if (sw1 != 0x90 && sw1 != 0x9f) {
> +      //TODO: error 

Please refer to a bug number here. If there isn't one yet, please file it.

@@ +1761,5 @@
> +      let b = parseInt(response[i] + response[i+1], 16) & 0xff;
> +      data.push(b);
> +    }
> +
> +    if (DEBUG) debug('iccIOCommand='+this.iccIOCommand);

Ditto re: cleanup.

@@ +1770,5 @@
> +            command : ICC_COMMAND_READ_RECORD,
> +            field   : ICC_EF_MSISDN,
> +            pathid  : '3f007f10',
> +            p1      : 1,
> +            p2      : 4,

Magic values (pathid, p1, p2)! Please const them.

Also, coding style nits:
* use double quotes for strings,
* no space before the `:`
* indent by 2 spaces, not 4.
Please fix these here and everywhere else.

@@ +1786,5 @@
> +        let number;
> +
> +        if (numberLength > MAX_NUMBER_SIZE_BYTES) {
> +          if (DEBUG) debug('invalid number length');
> +          number = "";

Init number to "", then you don't need this `if` branch at all.

@@ +1789,5 @@
> +          if (DEBUG) debug('invalid number length');
> +          number = "";
> +        } else {
> +          number = this._readSwappedNibbleBCD(d, footerOffset+2, numberLength -1)
> +              .toString(); 

Nit: spaces around operators.

@@ +1804,5 @@
> +
> +  // use code from PduGSMHelper.readSwappedNibbleBCD
> +  // but here we already got the data from Buffer
> +  // so we can't call Buff.readOctet again
> +  _readSwappedNibbleBCD: function _readSwappedNibbleBCD(data, offset, length) {

Right now RIL[REQUEST_SIM_IO] reads the complete response string right away. Which means we have to create the `data` array and can't reuse PduGSMHelper.readSwappedNibbleBCD. But if RIL[REQUEST_SIM_IO] would not read the whole string and instead left it up to the handler to do that, we could avoid this code duplication and the `data` array.

Please do this and also rename _handleMSISDNResponse to readMSISDNResponse() to clarify that it now actually reads directly from the buffer.

@@ +2280,5 @@
>    /**
> +   *  get MSISDN
> +   */ 
> +  getMSISDN: function getMSISDN() {
> +    debug("getMSISDN");

Ditto re: cleanup and `if (DEBUG)`

@@ +2282,5 @@
> +   */ 
> +  getMSISDN: function getMSISDN() {
> +    debug("getMSISDN");
> +    this.EFID = ICC_EF_MSISDN;
> +    this.iccIOCommand = ICC_COMMAND_GET_RESPONSE;

Add these to `options` (`command` already is there) and use `newParcel()` like I showed above to associate `options` with the returning response parcel.

@@ +2289,5 @@
> +      field   : ICC_EF_MSISDN,
> +      pathid  : '3f007f10',
> +      p1      : 0,
> +      p2      : 0,
> +      p3      : 15,

Ditto re: magic values.

@@ +2328,1 @@
>  };

Unrelated whitespace change. Please remove.

@@ +2902,3 @@
>  /**
>   * Global stuff.
>   */

Ditto

Comment 17

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

> @@ +1804,5 @@
> > +
> > +  // use code from PduGSMHelper.readSwappedNibbleBCD
> > +  // but here we already got the data from Buffer
> > +  // so we can't call Buff.readOctet again
> > +  _readSwappedNibbleBCD: function _readSwappedNibbleBCD(data, offset, length) {
> 
> Right now RIL[REQUEST_SIM_IO] reads the complete response string right away.
> Which means we have to create the `data` array and can't reuse
> PduGSMHelper.readSwappedNibbleBCD. But if RIL[REQUEST_SIM_IO] would not read
> the whole string and instead left it up to the handler to do that, we could
> avoid this code duplication and the `data` array.
> 

Hi, Philikon
I've also considered this situation,
But doing so will move the code to RIL[REQUEST_SIM_IO], not in Phone.onICCIO
this is different from other callbacks
(Although I'm awared that you're going to merge Phone and RIL objects,
 but for the time being, shouldn't I conform to other functions?)

Also what will Phone.onICCIO do now? 
should we leave it empty?

thanks

Comment 18

[Philipp von Weitershausen [:philikon]]

(In reply to yoshi huang [:yhuang] from comment #17)
> But doing so will move the code to RIL[REQUEST_SIM_IO], not in Phone.onICCIO
> this is different from other callbacks
> (Although I'm awared that you're going to merge Phone and RIL objects,
>  but for the time being, shouldn't I conform to other functions?)

I would say no. Let's take a step towards that merging.

> Also what will Phone.onICCIO do now? 

Well, it would dispatch to the readMSISDNResponse() method (and in the future potentially other methods.)

Comment 19

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

philikon, got it, thanks :)

Comment 20

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

(In reply to Philipp von Weitershausen [:philikon] from comment #16)
> Right now RIL[REQUEST_SIM_IO] reads the complete response string right away.
> Which means we have to create the `data` array and can't reuse
> PduGSMHelper.readSwappedNibbleBCD. But if RIL[REQUEST_SIM_IO] would not read
> the whole string and instead left it up to the handler to do that, we could
> avoid this code duplication and the `data` array.
> 
> Please do this and also rename _handleMSISDNResponse to readMSISDNResponse()
> to clarify that it now actually reads directly from the buffer.
Hi, philikon

But the type of the response is of String (UTF-16),
so readSwappedNibbleBCD still cannot be reused, right?

Also for the my original _handleMSISDNResponse, 
for ICC IO, It takes 2-pass to get the real data:
1st is GET_RESPONSE, which will respond type and size information.
2nd is READ_RECORD, which will respond BCD-format number.
But both passes need to parse the raw data(i.e. the 'response' argument), 
therefore I put them in _handleMSISDNResponse to make their code structure
more consistent.
If we still need to use the original readSwappedNibbleBCD, wouldn't it be 
harder to construct/read the code?
for example, if now it's GET_RESPONSE, we could call Buf.readString() 
otherwise it's READ_RECORD, we cannot call Buf.readXXX anymore because we're
trying to make use of readSwappedNibbleBCD.

thanks

Comment 21

[Philipp von Weitershausen [:philikon]]

(In reply to yoshi huang [:yhuang] from comment #20)
> But the type of the response is of String (UTF-16),
> so readSwappedNibbleBCD still cannot be reused, right?

Oh, right. So then we should factor the reusable pieces out to a method in the `Buf` object that both `Phone.readMSISDNResponse` and `GsmPDUHelper.readSwappedNibbleBCD` would use.

Comment 22

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

the response of the SIM_ICCIO is not of BCD format in GET_RESPONSE
also it's not entirely BCD-format even in READ_RECORD, 
it has lots of 'ff' in prefix and postfix 

what about refactoring the original readSwappedNibbleBCD?

readSwappedNibbleBCD: function readSwappedNibbleBCD(length) {
  let data = [];
  for (let i = 0; i < length; i++)
    data.push(this.readHexOctet());
  return this.readBCD(data, 0, length);
 },

readBCD: function readBCD(data, offset, length) {
 let number = 0;
  for (let i = offset; i < offset + length; i++) {
    let octet = data[i];
    // If the first nibble is an "F" , only the second nibble is to be taken
    // into account.
    if ((octet & 0xf0) == 0xf0) {
      number *= 10;
      number += octet & 0x0f;
      continue;
    }
    number *= 100;
    number += this.octetToBCD(octet);
  }
  return number;
}


thanks

Comment 23

[Philipp von Weitershausen [:philikon]]

(In reply to yoshi huang [:yhuang] from comment #22)
> the response of the SIM_ICCIO is not of BCD format in GET_RESPONSE
> also it's not entirely BCD-format even in READ_RECORD, 
> it has lots of 'ff' in prefix and postfix 

Sure, but I guess the whole point of BCD is that `f` is ignored, right? We could just make readSwappedNibbleBCD() more resilient to `ff` nibble pairs (ignore them). Is anything needed besides that?

> readSwappedNibbleBCD: function readSwappedNibbleBCD(length) {
>   let data = [];

This will allocate a new array -- we've been trying really hard to not allocate new objects in the parsing process. We're not even producing and concatenating strings when we don't have to, letting the JIT and Type Inference system do entirely integer math. IIRC this made a good difference in perf when we implemented this.

Comment 24

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

(In reply to Philipp von Weitershausen [:philikon] from comment #23)
> Sure, but I guess the whole point of BCD is that `f` is ignored, right? We
> could just make readSwappedNibbleBCD() more resilient to `ff` nibble pairs
> (ignore them). Is anything needed besides that?
philikon, you're right, but my point is in 'parsing the response of all ICC IO commands', not just parsing the BCD-format string. 
The 'response' parameter is of type string, 
so the original code will be looked like:
RIL[REQUEST_SIM_IO] = function REQUEST_SIM_IO(...) {
  let sw1 = Buf.readUint32();
  let sw2 = Buf.readUint32();
  let response = Buf.readString();
  ...
}

now it will be looked like

RIL[REQUEST_SIM_IO] = function REQUEST_SIM_IO(...) {
  let sw1 = Buf.readUint32();
  let sw2 = Buf.readUint32();
  // Here we don't read response anymore

  switch (options.command) {
    case ICC_GET_RESPONSE:
      // readString here
      // but actually it's several chunks of bytes
      let response = Buf.readString();
      ...
      break;
    
    case ICC_READ_RECORD:
      // Actually it still does much the same thing as Buf.readString
      // because response is of type string
      // So here is my concern, is it okay that we move the common code Buf.readString to these two switch cases?
      // At first look, I may think ril replies different data types of responses according to those icc commands.  
      let response = GsmPDUHelper.readStringAsBCD(); 
 
  }
  ...
}


Now back to my patch
How about adding a function object in the readBCD code to get the octet?

readSwappedNibbleBCD: function readSwappedNibbleBCD(length) {
  function getbyte() {
    return Buf.readHexOctet();
  }
  return this.readBCD(getbyte, length);
},

readStringAsBCD: function readStringAsBCD() {
    function getbyte() {
      let b = String.fromCharCode(Buf.readUint16()) + 
          String.fromCharCode(Buf.readUint16()); 
      return parseInt(b, 16);
    }
    ... // read length
    let bcd = this.readBCD(getbyte, length);
    ... // read delimiter
    return bcd;
  },

  // Original readSwappedNibbleBCD
  readBCD: function readBCD(getbyte, length) {
    let number = 0;
    for (let i = 0; i < length; i++) {
      let octet = getbyte();  // call the function object to get octet
      if ((octet & 0xff) == 0xff) // to bypass 0xff
        continue;
      // If the first nibble is an "F" , only the second nibble is to be taken
      // into account.
      if ((octet & 0xf0) == 0xf0) {
        number *= 10;
        number += octet & 0x0f;
        continue;
      }
      number *= 100;
      number += this.octetToBCD(octet);
    }
    return number;
  },

thanks

Comment 25

[Philipp von Weitershausen [:philikon]]

(In reply to yoshi huang [:yhuang] from comment #24)
> RIL[REQUEST_SIM_IO] = function REQUEST_SIM_IO(...) {
>   let sw1 = Buf.readUint32();
>   let sw2 = Buf.readUint32();
>   let response = Buf.readString();
>   ...
> }
> 
> now it will be looked like
> <snip>

Correct!

> Now back to my patch
> How about adding a function object in the readBCD code to get the octet?
> 
> readSwappedNibbleBCD: function readSwappedNibbleBCD(length) {
>   function getbyte() {
>     return Buf.readHexOctet();
>   }
>   return this.readBCD(getbyte, length);
> },
> 
> readStringAsBCD: function readStringAsBCD() {
>     function getbyte() {
>       let b = String.fromCharCode(Buf.readUint16()) + 
>           String.fromCharCode(Buf.readUint16()); 
>       return parseInt(b, 16);
>     }

This function doesn't make any sense to me. I think you got String.fromCharCode and parseInt mixed up here.

In any case, summarizing our IRC discussion, readSwappedNibbleBCD as it is right now should just work because the format of the data is the same. The only modification it will need is to ignore 'ff' hex octets.

Comment 26

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

Attachment: patch for getMSISDN by using iccIO v2

Revised according to philikon's comments:
Try to use GsmPDUHelper.readSwappedNibbleBCD to read response

Comment 27

[Philipp von Weitershausen [:philikon]]

Comment on attachment 603643 [details] [diff] [review]
patch for getMSISDN by using iccIO v2

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

Yay, this looks great! Just a few minor things.

::: dom/system/b2g/ril_worker.js
@@ +931,5 @@
> +   *      Integer type, check commands from TS 27.007 +CRSM  
> +   *    @param fileid
> +   *      Integer type, EF id, check TS 51.011 Chapter 10
> +   */
> +  iccIO: function iccIO (request, options) {

Why do we need two separate objects (`request` and `options`)? It seems that `options` always is a subset of `request`, at least that's how it's right now.

In all other functions, we just have one object representing the entire parcel request. That way we can also relay all information back to the main thread, instead of just a subset.

@@ +2374,5 @@
>      let number = 0;
>      for (let i = 0; i < length; i++) {
>        let octet = this.readHexOctet();
> +      if (octet == 0xff)
> +        continue;

Nit: always need braces. Also, couldn't hurt to add a small comment here :)

@@ +2399,5 @@
> +    let bcd = this.readSwappedNibbleBCD(length / 2);
> +    let delimiter = Buf.readUint16();
> +    if (!(length & 1)) {
> +      delimiter |= Buf.readUint16();
> +    }

It would be good to add the same delimiter check as in `Buf.readString()`

Comment 28

[Philipp von Weitershausen [:philikon]]

Comment on attachment 603643 [details] [diff] [review]
patch for getMSISDN by using iccIO v2

A few more smaller nits:

>+   /**
>+   *  Request an ICC I/O operation.
>+   * 
>+   *  @param request 
>+   *    See TS 27.007 "restricted SIM" operation,
>+   *    "AT Command +CRSM"
>+   *    The sequence is in the same order as how libril reads this parcel,
>+   *    see the struct RIL_SIM_IO_v5 or RIL_SIM_IO_v6 defined in ril.h
>+   *    @param command 
>+   *      Integer type, check commands from TS 27.007 +CRSM  

Rather than pointing people to some spec, why not say that it's one of the ICC_COMMAND_*.

Also, please follow the indentation convention for JavaDoc-style @params

>+   *    @param pin2
>+   *      String type , optional

Better:

   *  @param pin2 [optional]
   *         String containing the PIN2.

>+   *  @param options
>+   *    Used for callback.
>+   *    @param command 
>+   *      Integer type, check commands from TS 27.007 +CRSM  
>+   *    @param fileid
>+   *      Integer type, EF id, check TS 51.011 Chapter 10

This can go away per previous comment.

>+    switch (options.command) {
>+      case ICC_COMMAND_GET_RESPONSE:
>+        let response = Buf.readString();
>+        let recordSize = parseInt(response[RESPONSE_DATA_RECORD_LENGTH*2] + 
>+            response[RESPONSE_DATA_RECORD_LENGTH * 2 + 1], 16) & 0xff;   

Nit: trailing space and spaces around all operators.


Also, a note about patch generation: please generate them with 8 lines of context and the username and log message already included. See https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F for instructions.

Comment 29

[Philipp von Weitershausen [:philikon]]

Comment on attachment 603643 [details] [diff] [review]
patch for getMSISDN by using iccIO v2

>+  readMSISDNResponse: function readMSISDNResponse(options) {
>+    let sw1 = Buf.readUint32();
>+    let sw2 = Buf.readUint32();
>+    // See GSM11.11 section 9.4 for sw1 and sw2
>+    if (sw1 != STATUS_NORMAL_ENDING) {
>+      // TODO: error 
>+      // Wait for fix for Bug 713451 to report error.
>+      debug("Error in iccIO");
>+    }

Bug 713451 will not address this particular case. Can you file a new bug please?

Comment 30

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #28)
> >+    switch (options.command) {
> >+      case ICC_COMMAND_GET_RESPONSE:
> >+        let response = Buf.readString();
> >+        let recordSize = parseInt(response[RESPONSE_DATA_RECORD_LENGTH*2] + 
> >+            response[RESPONSE_DATA_RECORD_LENGTH * 2 + 1], 16) & 0xff;   
> 
> Nit: trailing space and spaces around all operators.

Actually, I just realized that response.substr() might be a cleaner option here...

Comment 31

[Philipp von Weitershausen [:philikon]]

I ended up quickly addressing the remaining nits and landed the patch:

https://hg.mozilla.org/mozilla-central/rev/36c4724de5bf

Thanks, Yoshi!

Comment 32

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

Hi Philikon
Thank you , too :) 
I really appreciate it ,still got a lot to learn

For the error handling, since Bug 713451 is fixed,
I'll check if I can provide another patch,
if not I'll just fire a new bug

Thanks

Comment 33

[Philipp von Weitershausen [:philikon]]

(In reply to yoshi huang [:yhuang] from comment #32)
> For the error handling, since Bug 713451 is fixed,
> I'll check if I can provide another patch,
> if not I'll just fire a new bug

Please file a new bug either way since this bug is now RESOLVED/FIXED. Thanks!

Comment 34

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

Just file a new bug for handling ICC IO errors, Bug 733990 

thanks

Comment 36

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

Attachment: get MSISDN v3, fix typo and set MSISDN to null if not present

This patch fix the typo and make MSISDN still null if not present, instead of empty string, and this patch is generated from latest m-c revision 89726, so I didn't make my last patch obsoletes(attachment 603643 [details] [diff] [review]).

Test on galaxy s2 MSISDN still returns empty string as before, I think the rild or operator didn't check EF_Path at all.

Sorry for my mistake.

Comment 37

[Philipp von Weitershausen [:philikon]]

Comment on attachment 607498 [details] [diff] [review]
get MSISDN v3, fix typo and set MSISDN to null if not present

Please don't reopen bugs for follow-up fixes. Bug 737352 is the place to deal with this. Bugs are only reopened when the patches that were landed were backed out. This was not the case here.

Please attach your patch to bug 737352. Thanks!