714855 - RIL: Implement ICC states transitions and ICC codes (PIN,PIN2,PUK,PUK2) handling functions

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Build ID: 20111220165912

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Most of the work is already done at https://github.com/ferjm/b2g-js-ril.
We still need to implement SIM2 and PUK2 getter and setter and expose this functionality to the DOM.

Comment 2

[Philipp von Weitershausen [:philikon]]

(In reply to Fernando Jiménez [:ferjm] from comment #1)
> Most of the work is already done at https://github.com/ferjm/b2g-js-ril.

Awesome. Can you create a patch against mozilla-central and attach it to this bug. I'll be happy to review it and, once it's ready, commit it.

> We still need to implement SIM2 and PUK2 getter and setter and expose this
> functionality to the DOM.

Yes, we need to figure out how to expose this stuff to content. The settings API has been mentioned many times. I'll try to track down people this week to figure this out. There are other settings/pieces of information exposed by the RIL that (at least some) content would need access to.

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Change PIN and enter PUK functions added. ICC states transitions added; r=philikon

ICC states transitions are based in android java RIL states

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 585697 [details] [diff] [review]
Change PIN and enter PUK functions added. ICC states transitions added; r=philikon

># HG changeset patch
># Parent 200a8d1fb4520c923c2b979b201a0a3f31c5f344
># User Fernando Jiménez <[e-mail]>
>Bug 714855 - Change PIN and enter PUK functions added. ICC states transitions added.
>
>diff --git a/dom/telephony/ril_worker.js b/dom/telephony/ril_worker.js
>--- a/dom/telephony/ril_worker.js
>+++ b/dom/telephony/ril_worker.js
>@@ -522,26 +522,62 @@ let RIL = {
>   },
> 
>   /**
>    * Enter a PIN to unlock the ICC.
>    *
>    * @param pin
>    *        String containing the PIN.
>    *
>-   * Response will call Phone.onEnterSIMPIN().
>+   * Response will call Phone.onEnterICCPIN().
>    */
>   enterICCPIN: function enterICCPIN(pin) {
>     Buf.newParcel(REQUEST_ENTER_SIM_PIN);
>     Buf.writeUint32(1);
>     Buf.writeString(pin);
>     Buf.sendParcel();
>   },
> 
>   /**
>+   * Change the current ICC PIN number
>+   *
>+   * @param oldPin
>+   *        String containing the old PIN value
>+   * @param newPin
>+   *        String containing the new PIN value
>+   *
>+   * Response will call Phone.onChangeICCPIN().
>+   */
>+  changeICCPIN: function changeICCPIN(oldPin, newPin) {
>+    Buf.newParcel(REQUEST_CHANGE_SIM_PIN);
>+    Buf.writeUint32(2);
>+    Buf.writeString(oldPin);
>+    Buf.writeString(newPin);
>+    Buf.sendParcel();
>+  },
>+
>+  /**
>+   * Supplies SIM PUK and a new PIN to unlock the ICC
>+   *
>+   * @param puk
>+   *        String containing the PUK value.
>+   * @param newPin
>+   *        String containing the new PIN value.
>+   *
>+   * Response will call Phone.onEnterICCPUK().
>+   */
>+   enterICCPUK: function enterICCPUK(puk, newPin) {
>+     Buf.newParcel(REQUEST_ENTER_SIM_PUK);
>+     Buf.writeUint32(2);
>+     Buf.writeString(puk);
>+     Buf.writeString(newPin);
>+     Buf.sendParcel();
>+   },
>+
>+  /**
>    * Request the phone's radio power to be switched on or off.
>    *
>    * @param on
>    *        Boolean indicating the desired power state.
>    */
>   setRadioPower: function setRadioPower(on) {
>     Buf.newParcel(REQUEST_RADIO_POWER);
>     Buf.writeUint32(1);
>@@ -789,23 +825,30 @@ RIL[REQUEST_GET_SIM_STATUS] = function R
>       pin1_replaced:  Buf.readUint32(),
>       pin1:           Buf.readUint32(),
>       pin2:           Buf.readUint32()
>     });
>   }
>   Phone.onICCStatus(iccStatus);
> };
> RIL[REQUEST_ENTER_SIM_PIN] = function REQUEST_ENTER_SIM_PIN() {
>+  // Response is the number of retries remaining or -1 if unknown.
>   let response = Buf.readUint32List();
>   Phone.onEnterICCPIN(response);
> };
>-RIL[REQUEST_ENTER_SIM_PUK] = null;
>+RIL[REQUEST_ENTER_SIM_PUK] = function REQUEST_ENTER_SIM_PUK() {
>+  // Response is the number of retries remaining or -1 if unknown.
>+  let response = Buf.readUint32List();
>+  Phone.onEnterICCPUK(response);
>+};
> RIL[REQUEST_ENTER_SIM_PIN2] = null;
> RIL[REQUEST_ENTER_SIM_PUK2] = null;
>-RIL[REQUEST_CHANGE_SIM_PIN] = null;
>+RIL[REQUEST_CHANGE_SIM_PIN] = function REQUEST_CHANGE_SIM_PIN() {
>+  Phone.onChangeICCPIN();
>+};
> RIL[REQUEST_CHANGE_SIM_PIN2] = null;
> RIL[REQUEST_ENTER_NETWORK_DEPERSONALIZATION] = null;
> RIL[REQUEST_GET_CURRENT_CALLS] = function REQUEST_GET_CURRENT_CALLS(length) {
>   let calls_length = 0;
>   // The RIL won't even send us the length integer if there are no active calls.
>   // So only read this integer if the parcel actually has it.
>   if (length) {
>     calls_length = Buf.readUint32();
>@@ -1277,28 +1320,86 @@ let Phone = {
>   },
> 
>   onNetworkStateChanged: function onNetworkStateChanged() {
>     debug("Network state changed, re-requesting phone state.");
>     this.requestNetworkInfo();
>   },
> 
>   onICCStatus: function onICCStatus(iccStatus) {
>-    debug("SIM card state is " + iccStatus.cardState);
>-    debug("Universal PIN state is " + iccStatus.universalPINState);
>+    if ((!iccStatus) || (iccStatus.cardState == CARDSTATE_ABSENT)) {
>+      debug("ICC card abstent");
>+      this.iccStatus = DOM_CARDSTATE_ABSENT;
>+    }
>+
>+    if ((this.radioState == RADIO_STATE_OFF) ||
>+       (this.radioState == RADIO_STATE_UNAVAILABLE) ||
>+       (this.radioState == RADIO_STATE_SIM_NOT_READY) ||
>+       (this.radioState == RADIO_STATE_RUIM_NOT_READY) ||
>+       (this.radioState == RADIO_STATE_NV_NOT_READY) ||
>+       (this.radioState == RADIO_STATE_NV_READY)) {
>+      debug("ICC card not ready");
>+      this.iccStatus = DOM_CARDSTATE_NOT_READY;
>+    }
>+
>+    if ((this.radioState == RADIO_STATE_SIM_LOCKED_OR_ABSENT) ||
>+       (this.radioState == RADIO_STATE_SIM_READY) ||
>+       (this.radioState == RADIO_STATE_RUIM_LOCKED_OR_ABSENT) ||
>+       (this.radioState == RADIO_STATE_RUIM_READY)) {
>+      let app;
>+      if (!(app = iccStatus.apps[iccStatus.gsmUmtsSubscriptionAppIndex])) {
>+        debug("Subscription application is not present in iccStatus. ICC card state: absent");
>+        this.iccStatus = DOM_CARDSTATE_ABSENT;
>+        return;
>+      }
>+
>+      switch (app.app_state) {
>+        case APPSTATE_PIN:
>+          this.iccStatus = DOM_CARDSTATE_PIN_REQUIRED;
>+          break;
>+        case APPSTATE_PUK:
>+          this.iccStatus = DOM_CARDSTATE_PUK_REQUIRED;
>+          break;
>+        case APPSTATE_SUBSCRIPTION_PERSO:
>+          this.iccStatus = DOM_CARDSTATE_NETWORK_LOCKED;
>+          break;
>+        case APPSTATE_READY:
>+          this.iccStatus = DOM_CARDSTATE_READY;
>+          break;
>+        case APPSTATE_UNKNOWN:
>+          this.iccStatus = DOM_CARDSTATE_NOT_READY;
>+          break;
>+        case APPSTATE_DETECTED:
>+          this.iccStatus = DOM_CARDSTATE_NOT_READY;
>+          break;
>+        default:
>+          this.iccStatus = DOM_CARDSTATE_NOT_READY;
>+      }
>+    }
>+    debug("iccStatus: " + this.iccStatus);
>     debug(iccStatus);
>-    //TODO set to simStatus and figure out state transitions.
>-    this.iccStatus = iccStatus; //XXX TODO
>+    this.sendDOMMessage({type: "cardstatechange",
>+                         cardState: this.iccStatus});
>   },
> 
>   onEnterICCPIN: function onEnterICCPIN(response) {
>     debug("REQUEST_ENTER_SIM_PIN returned " + response);
>     //TODO
>   },
> 
>+  onChangeICCPIN: function onChangeICCPIN() {
>+    debug("REQUEST_CHANGE_SIM_PIN");
>+    //TODO
>+  },
>+
>+  onEnterICCPUK: function onEnterICCPUK(response) {
>+    debug("REQUEST_ENTER_SIM_PUK returned " + response);
>+    //TODO
>+  },
>+
>   onNetworkSelectionMode: function onNetworkSelectionMode(mode) {
>     this.networkSelectionMode = mode[0];
>   },
> 
>   onBasebandVersion: function onBasebandVersion(version) {
>     this.basebandVersion = version;
>   },
> 
>@@ -1676,17 +1777,17 @@ let GsmPDUHelper = {
>       number *= 100;
>       number += this.octetToBCD(octet);
>     }
>     return number;
>   },
> 
>   /**
>    * Write numerical data as swapped nibble BCD.
>-   * 
>+   *
>    * @param data
>    *        Data to write (as a string or a number)
>    */
>   writeSwappedNibbleBCD: function writeSwappedNibbleBCD(data) {
>     data = data.toString();
>     if (data.length % 2) {
>       data += "F";
>     }

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Change PIN and enter PUK functions added. ICC states transitions added; r=philikon

Sorry, I forgot to add the ril_consts.js changes to the patch.

Comment 6

[Philipp von Weitershausen [:philikon]]

Comment on attachment 585766 [details]
Change PIN and enter PUK functions added. ICC states transitions added; r=philikon

Awesome! Mostly just nits.

We also need to figure out how the plumbing on the main thread side works. I suspect we will tie this into the network manager, but the details will be in bug 713849.


>diff --git a/dom/telephony/ril_consts.js b/dom/telephony/ril_consts.js
>--- a/dom/telephony/ril_consts.js
>+++ b/dom/telephony/ril_consts.js
>@@ -186,16 +186,34 @@ const RADIO_STATE_SIM_READY = 4;
> const RADIO_STATE_RUIM_NOT_READY = 5;
> const RADIO_STATE_RUIM_READY = 6;
> const RADIO_STATE_RUIM_LOCKED_OR_ABSENT = 7;
> const RADIO_STATE_NV_NOT_READY = 8;
> const RADIO_STATE_NV_READY = 9;
> 
> const CARD_MAX_APPS = 8;

Nit: Move this next to the APP_STATE_* constants

>+/**
>+ * Icc card states
>+ */

Nit: s/Icc/ICC/. Also, ICC stands for "integrated circuit card" so ICC card is a tautological tautology ;)

>+const CARDSTATE_ABSENT = 0;
>+const CARDSTATE_PRESENT = 1;
>+const CARDSTATE_ERROR = 2;

Nit: other consts seem to be called RADIO_STATE_* and CALL_STATE_* etc, so I htink these should be called CARD_STATE_*

>+/**
>+ * RIL_AppState
>+ */

Can you add a slightly more useful comment here, please? 

>+const APPSTATE_UNKNOWN = 0;
>+const APPSTATE_DETECTED = 1;
>+const APPSTATE_PIN = 2; /* If PIN1 or UPin is required */
>+const APPSTATE_PUK = 3; /* If PUK1 or Puk for UPin is required */
>+const APPSTATE_SUBSCRIPTION_PERSO = 4; /* perso_substate should be look at
>+                                          when app_state is assigned to this value */
>+const APPSTATE_READY = 5;

Nit: Coding style. Please use // comments for inline comments.

> RIL[REQUEST_ENTER_SIM_PIN] = function REQUEST_ENTER_SIM_PIN() {
>+  // Response is the number of retries remaining or -1 if unknown.
>   let response = Buf.readUint32List();
>   Phone.onEnterICCPIN(response);
> };

I don't understand this comment. Are you just referring to the first word of the response that indicates the length of the Uint32 list? If so, then I see no reason to explicitly mention this here. If anything it's pretty confusing. Please remove it and in the REQUEST_ENTER_SIM_PUK function.

>   onICCStatus: function onICCStatus(iccStatus) {
>-    debug("SIM card state is " + iccStatus.cardState);
>-    debug("Universal PIN state is " + iccStatus.universalPINState);
>+    if ((!iccStatus) || (iccStatus.cardState == DOM_CARDSTATE_ABSENT)) {
>+      debug("ICC card absent");
>+      this.iccStatus = DOM_CARDSTATE_ABSENT;
>+    }

Should the function return here? It feels like it should... Also, did `iccStatus` just change because of this? If so, we probably want to notify the main thread.

A more philosophical thought: `this.iccStatus` was supposed to simply be a reference to the `iccStatus` parameter. You're now changing it to be one of the DOM_CARDSTATE_* constants. That changes its meaning. I think it should be called `this.gsmCardState` or something (see below on why that name.)

Also, please always prefix debug statements with if (DEBUG), like so:

  if (DEBUG) debug(...);

Please do this here and in all other places.

>+    if ((this.radioState == RADIO_STATE_OFF) ||
>+       (this.radioState == RADIO_STATE_UNAVAILABLE) ||
>+       (this.radioState == RADIO_STATE_SIM_NOT_READY) ||
>+       (this.radioState == RADIO_STATE_RUIM_NOT_READY) ||
>+       (this.radioState == RADIO_STATE_NV_NOT_READY) ||
>+       (this.radioState == RADIO_STATE_NV_READY)) {
>+      debug("ICC card not ready");
>+      this.iccStatus = DOM_CARDSTATE_NOT_READY;
>+    }

Same critique as above: this should probably return, after potentially notifying the main thread.

Also, nit: align on the correct parenthesis. Please do this here and in all other places.

>+    if ((this.radioState == RADIO_STATE_SIM_LOCKED_OR_ABSENT) ||
>+       (this.radioState == RADIO_STATE_SIM_READY) ||
>+       (this.radioState == RADIO_STATE_RUIM_LOCKED_OR_ABSENT) ||
>+       (this.radioState == RADIO_STATE_RUIM_READY)) {
>+      let app;
>+      if (!(app = iccStatus.apps[iccStatus.gsmUmtsSubscriptionAppIndex])) {

Ugh. Please avoid assignments in if() statements. Would be much cleaner like this:

      let app = iccStatus.apps[iccStatus.gsmUmtsSubscriptionAppIndex];
      if (!app) {

On a more technical note, this code has a few implications:

(a) GSM only (that's fine for now)

(b) No support for multiple SIM cards per device

Point (b) has implications on how we represent card state. If we only have 1 card state flag ever, we can only model devices with 1 ICC. Since the RIL supports GSM and CDMA at the same time, I feel like we should at least make sure we don't paint ourselves into a corner. I suggest explicitly treating GSM card state (and eventually CDMA card state) everywhere and reflecting this in the name (`this.gsmCardState` and `gsmCardStateChange`).

>+        debug("Subscription application is not present in iccStatus. ICC card state: absent");
>+        this.iccStatus = DOM_CARDSTATE_ABSENT;
>+        return;
>+      }

Same critique as above: probably want to notify main thread here if `iccStatus` actually changed.

>+  onChangeICCPIN: function onChangeICCPIN() {
>+    debug("REQUEST_CHANGE_SIM_PIN");
>+    //TODO
>+  },
>+
>+  onEnterICCPUK: function onEnterICCPUK(response) {
>+    debug("REQUEST_ENTER_SIM_PUK returned " + response);
>+    //TODO
>+  },

Please just leave these empty for now.

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

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

Thanks for the review and feedback! :)

> >diff --git a/dom/telephony/ril_consts.js b/dom/telephony/ril_consts.js
> >--- a/dom/telephony/ril_consts.js
> >+++ b/dom/telephony/ril_consts.js
> >@@ -186,16 +186,34 @@ const RADIO_STATE_SIM_READY = 4;
> > const RADIO_STATE_RUIM_NOT_READY = 5;
> > const RADIO_STATE_RUIM_READY = 6;
> > const RADIO_STATE_RUIM_LOCKED_OR_ABSENT = 7;
> > const RADIO_STATE_NV_NOT_READY = 8;
> > const RADIO_STATE_NV_READY = 9;
> > 
> > const CARD_MAX_APPS = 8;
> 
> Nit: Move this next to the APP_STATE_* constants
Done!

> >+/**
> >+ * Icc card states
> >+ */
> 
> Nit: s/Icc/ICC/. Also, ICC stands for "integrated circuit card" so ICC card
> is a tautological tautology ;)
lol you are right. Done and changed in other places within ril_worker.

> >+const CARDSTATE_ABSENT = 0;
> >+const CARDSTATE_PRESENT = 1;
> >+const CARDSTATE_ERROR = 2;
> 
> Nit: other consts seem to be called RADIO_STATE_* and CALL_STATE_* etc, so I
> htink these should be called CARD_STATE_*

Done and also changed APPSTATE_* to CARD_APP_STATE_* as all the other consts related to ICC have the CARD_ prefix.

> >+/**
> >+ * RIL_AppState
> >+ */
> 
> Can you add a slightly more useful comment here, please? 
>
Actually, I´ve deleted the comment as the consts name are self explanatory.
 
> >+const APPSTATE_UNKNOWN = 0;
> >+const APPSTATE_DETECTED = 1;
> >+const APPSTATE_PIN = 2; /* If PIN1 or UPin is required */
> >+const APPSTATE_PUK = 3; /* If PUK1 or Puk for UPin is required */
> >+const APPSTATE_SUBSCRIPTION_PERSO = 4; /* perso_substate should be look at
> >+                                          when app_state is assigned to this value */
> >+const APPSTATE_READY = 5;
> 
> Nit: Coding style. Please use // comments for inline comments.
Done.

> > RIL[REQUEST_ENTER_SIM_PIN] = function REQUEST_ENTER_SIM_PIN() {
> >+  // Response is the number of retries remaining or -1 if unknown.
> >   let response = Buf.readUint32List();
> >   Phone.onEnterICCPIN(response);
> > };
> 
> I don't understand this comment. Are you just referring to the first word of
> the response that indicates the length of the Uint32 list? If so, then I see
> no reason to explicitly mention this here. If anything it's pretty
> confusing. Please remove it and in the REQUEST_ENTER_SIM_PUK function.
Yes, it was related to the first word of the response. Already removed.

> >   onICCStatus: function onICCStatus(iccStatus) {
> >-    debug("SIM card state is " + iccStatus.cardState);
> >-    debug("Universal PIN state is " + iccStatus.universalPINState);
> >+    if ((!iccStatus) || (iccStatus.cardState == DOM_CARDSTATE_ABSENT)) {
> >+      debug("ICC card absent");
> >+      this.iccStatus = DOM_CARDSTATE_ABSENT;
> >+    }
> 
> Should the function return here? It feels like it should... Also, did
> `iccStatus` just change because of this? If so, we probably want to notify
> the main thread.
Agree with the return. 
Maybe I am not understanding your comment, but I am not completely sure that we want it to notify to the main thread (you via sendDOMMessage, right?) only if iccStatus has changed. onICCStatus is called on demand after calling RIL.getICCStatus. It is not executed after an event, just like it happens with onRadioStateChange. What if someone needs to know the ICCStatus and it has not changed?

> A more philosophical thought: `this.iccStatus` was supposed to simply be a
> reference to the `iccStatus` parameter. You're now changing it to be one of
> the DOM_CARDSTATE_* constants. That changes its meaning. I think it should
> be called `this.gsmCardState` or something (see below on why that name.)
Done.

> Also, please always prefix debug statements with if (DEBUG), like so:
> 
>   if (DEBUG) debug(...);
> 
> Please do this here and in all other places.
Done for this patch and all over ril_worker.

> >+    if ((this.radioState == RADIO_STATE_OFF) ||
> >+       (this.radioState == RADIO_STATE_UNAVAILABLE) ||
> >+       (this.radioState == RADIO_STATE_SIM_NOT_READY) ||
> >+       (this.radioState == RADIO_STATE_RUIM_NOT_READY) ||
> >+       (this.radioState == RADIO_STATE_NV_NOT_READY) ||
> >+       (this.radioState == RADIO_STATE_NV_READY)) {
> >+      debug("ICC card not ready");
> >+      this.iccStatus = DOM_CARDSTATE_NOT_READY;
> >+    }
> 
> Same critique as above: this should probably return, after potentially
> notifying the main thread.
Same as above.

> Also, nit: align on the correct parenthesis. Please do this here and in all
> other places.
Done.

> >+    if ((this.radioState == RADIO_STATE_SIM_LOCKED_OR_ABSENT) ||
> >+       (this.radioState == RADIO_STATE_SIM_READY) ||
> >+       (this.radioState == RADIO_STATE_RUIM_LOCKED_OR_ABSENT) ||
> >+       (this.radioState == RADIO_STATE_RUIM_READY)) {
> >+      let app;
> >+      if (!(app = iccStatus.apps[iccStatus.gsmUmtsSubscriptionAppIndex])) {
> 
> Ugh. Please avoid assignments in if() statements. Would be much cleaner like
> this:
> 
>       let app = iccStatus.apps[iccStatus.gsmUmtsSubscriptionAppIndex];
>       if (!app) {
Done. Don´t know what I was smoking... :\

> On a more technical note, this code has a few implications:
> 
> (a) GSM only (that's fine for now)
Yes, I thought it was enough for now.

> (b) No support for multiple SIM cards per device
Good point!

> Point (b) has implications on how we represent card state. If we only have 1
> card state flag ever, we can only model devices with 1 ICC. Since the RIL
> supports GSM and CDMA at the same time, I feel like we should at least make
> sure we don't paint ourselves into a corner. I suggest explicitly treating
> GSM card state (and eventually CDMA card state) everywhere and reflecting
> this in the name (`this.gsmCardState` and `gsmCardStateChange`).
What do you mean with `gsmCardStateChange`?

Also, you mean keeping a this.gsmCardState and a this.cdmaCardState, right? I am not familiar with CDMA and its differents with GSM, but if I am not wrong it is also possible to have two GSM SIMs in the same phone, as Stand-by (one SIM at a time) or Active (both SIMs working at the same time). Shouldn´t we treat the card states as arrays? I mean, this.gsmCardState[] and, if it is also possible for CDMA, this.cdmaCardState[].

> >+  onChangeICCPIN: function onChangeICCPIN() {
> >+    debug("REQUEST_CHANGE_SIM_PIN");
> >+    //TODO
> >+  },
> >+
> >+  onEnterICCPUK: function onEnterICCPUK(response) {
> >+    debug("REQUEST_ENTER_SIM_PUK returned " + response);
> >+    //TODO
> >+  },
> 
> Please just leave these empty for now.
Sure :)

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

I am seeing that there is actually a UNSOLICITED_RESPONSE_SIM_STATUS_CHANGED event. So the handler of this event should call getIccStatus. The main thread notification is gonna be sent for sure if the sim status changes. Are we going to expose the getIccStatus function in the Phone object? If not, then I understand why you want to notify *only* if the ICC state changes.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Change PIN and enter PUK functions added. ICC states transitions added; r=philikon

I think this patch addresses all the previous review feedback except for the multi-SIM support.

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to Fernando Jiménez [:ferjm] from comment #7)
> Maybe I am not understanding your comment, but I am not completely sure that
> we want it to notify to the main thread (you via sendDOMMessage, right?)
> only if iccStatus has changed.

IMHO yes.

> onICCStatus is called on demand after calling
> RIL.getICCStatus. It is not executed after an event, just like it happens
> with onRadioStateChange. What if someone needs to know the ICCStatus and it
> has not changed?

Then it already knows the status, no? But let's say it doesn't and the main thread wants to request the ICC status again from the RIL worker... the Phone object already knows the state, so it can respond without having to talk to the RIL.

> > Point (b) has implications on how we represent card state. If we only have 1
> > card state flag ever, we can only model devices with 1 ICC. Since the RIL
> > supports GSM and CDMA at the same time, I feel like we should at least make
> > sure we don't paint ourselves into a corner. I suggest explicitly treating
> > GSM card state (and eventually CDMA card state) everywhere and reflecting
> > this in the name (`this.gsmCardState` and `gsmCardStateChange`).
> 
> What do you mean with `gsmCardStateChange`?

The name of the DOM message (currently "cardstatechange" in your patch).

> Also, you mean keeping a this.gsmCardState and a this.cdmaCardState, right?

That's what I meant indeed. However, after re-reading the Android source code (specifically IccCard::getIccCardState) it seems that they don't actually support the two radio technologies at the same time. I must have misread that earlier. So maybe my concern is mostly invalid.

> I am not familiar with CDMA and its differents with GSM, but if I am not
> wrong it is also possible to have two GSM SIMs in the same phone, as
> Stand-by (one SIM at a time) or Active (both SIMs working at the same time).
> Shouldn´t we treat the card states as arrays? I mean, this.gsmCardState[]
> and, if it is also possible for CDMA, this.cdmaCardState[].

Not really because the RIL only knows about one ICC at a time. But yeah, I withdraw my concern about supporting multiple ICCs for now. Let's just go with one 'cardState' for now. Can you upload an adjusted patch for that? Then I'll happily r+ and commit it :)

Thanks!

Comment 11

[Philipp von Weitershausen [:philikon]]

(In reply to Fernando Jiménez [:ferjm] from comment #8)
> I am seeing that there is actually a UNSOLICITED_RESPONSE_SIM_STATUS_CHANGED
> event. So the handler of this event should call getIccStatus.

Correct. RIL.UNSOLICITED_RESPONSE_SIM_STATUS_CHANGED() should call Phone.onICCStatusChanged() which should call RIL.getICCStatus(). Can you add that to the patch as well? Thanks!

> The main thread notification is gonna be sent for sure if the sim status changes.
> Are we going to expose the getIccStatus function in the Phone object? If not,
> then I understand why you want to notify *only* if the ICC state changes.

Right now I see no reason to add this. I think we will get a better understanding of this once we build out the main thread side.

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Change PIN and enter PUK functions added. ICC states transitions added; r=philikon

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 586714 [details] [diff] [review]
Change PIN and enter PUK functions added. ICC states transitions added; r=philikon

>+const CARD_APP_STATE_UNKNOWN = 0;
>+const CARD_APP_STATE_DETECTED = 1;
>+const CARD_APP_STATE_PIN = 2; // If PIN1 or UPin is required.
>+const CARD_APP_STATE_PUK = 3; // If PUK1 or Puk for UPin is required.
>+const CARD_APP_STATE_SUBSCRIPTION_PERSO = 4; // perso_substate should be look 
>+                                             // at when app_state is assigned 
>+                                             // to this value.

Typo/grammar: s/look/looked/

You haven't addressed comment 11 yet, so I'm going to clear the review flag for now.


(Btw, for easier comparison between the patches, I recommend simply labeling your patches "v1", "v2", etc. If the bug needs multiple patches, I typically do "Part 2 (v4): Foobar")

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v4

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Philipp von Weitershausen [:philikon] from comment #13)
> Comment on attachment 586714 [details] [diff] [review]
> Change PIN and enter PUK functions added. ICC states transitions added;
> r=philikon
> 
> >+const CARD_APP_STATE_UNKNOWN = 0;
> >+const CARD_APP_STATE_DETECTED = 1;
> >+const CARD_APP_STATE_PIN = 2; // If PIN1 or UPin is required.
> >+const CARD_APP_STATE_PUK = 3; // If PUK1 or Puk for UPin is required.
> >+const CARD_APP_STATE_SUBSCRIPTION_PERSO = 4; // perso_substate should be look 
> >+                                             // at when app_state is assigned 
> >+                                             // to this value.
> 
> Typo/grammar: s/look/looked/
>
Ok, done.

> You haven't addressed comment 11 yet, so I'm going to clear the review flag
> for now.
> 
Oh, I thought I added that to the patch :\

> (Btw, for easier comparison between the patches, I recommend simply labeling
> your patches "v1", "v2", etc. If the bug needs multiple patches, I typically
> do "Part 2 (v4): Foobar")
Ok, thanks for the tip :)

Comment 16

[Philipp von Weitershausen [:philikon]]

Comment on attachment 587308 [details] [diff] [review]
v4

Looks good! I will commit this.

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Great! thanks! :)

Comment 18

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/448aa12907aa

Comment 19

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/448aa12907aa