Security element APIs and access control enforcer(ACE) for NFC need to use icc communication channel APIs defined in nsIIccProvider.idl [1] to exchange APDUs. The client using these APIs could be in chrome process or content process. However, current APIs definition doesn't seem to meet this requirement when using the APIs in chrome process. Because these APIs use nsIDOMRequest as return value and window object in its second argument. We would like to redefine the interface to fulfill our requirement for NFC development. [1] http://dxr.mozilla.org/mozilla-central/source/dom/icc/interfaces/nsIIccProvider.idl?from=nsIIccProvider.idl&case=true#94

I believe this issue could be fixed in Bug 864489, which is huge work. Considering the schedule, I realize that we need to have a *transient* solution for SE and that would be refining only "iccOpenChannel", "iccExchangeAPDU" and "iccCloseChannel" first IMO. We'd like to avoid jsval as much as possible; however, as this is a transient solution, I'd also like to keep this bug as simple as possible, i.e. I suggest we keep use jsval in "iccExchangeAPDU" here. jsval usage will be discarded eventually in bug 864489. A possible solution would look like: /* New interface */ interface nsIIccCallback : nsISupports { void notifySuccess(); void notifyError(in AString error); void notifyExchangeAPDUSuccess(in long sw1, in long sw2, in AString response); }; partial interface nsIIccProvider { void iccOpenChannel(in unsigned long clientId, in AString aid, in nsIIccCallback callback); void iccExchangeAPDU(in unsigned long clientId, in long channel, in jsval apdu, in nsIIccCallback callback); void iccCloseChannel(in unsigned long clientId, in long channel, in nsIIccCallback callback); }; How does that sound?

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1) > I believe this issue could be fixed in Bug 864489, which is huge work. > Considering the schedule, I realize that we need to have a *transient* > solution for SE and that would be refining only "iccOpenChannel", > "iccExchangeAPDU" and "iccCloseChannel" first IMO. > > We'd like to avoid jsval as much as possible; however, as this is a > transient solution, I'd also like to keep this bug as simple as possible, > i.e. I suggest we keep use jsval in "iccExchangeAPDU" here. jsval usage will > be discarded eventually in bug 864489. > > A possible solution would look like: > > /* New interface */ > interface nsIIccCallback : nsISupports > { > void notifySuccess(); > void notifyError(in AString error); > void notifyExchangeAPDUSuccess(in long sw1, in long sw2, in AString > response); > }; > > partial interface nsIIccProvider > { > void iccOpenChannel(in unsigned long clientId, > in AString aid, > in nsIIccCallback callback); > void iccExchangeAPDU(in unsigned long clientId, > in long channel, > in jsval apdu, > in nsIIccCallback callback); > void iccCloseChannel(in unsigned long clientId, > in long channel, > in nsIIccCallback callback); > }; > > How does that sound? Looks good to me, but we need one more success callback which carries `channelId` for `iccOpenChannel`. Thank you.

(In reply to Edgar Chen [:edgar][:echen] from comment #2) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #1) > > I believe this issue could be fixed in Bug 864489, which is huge work. > > Considering the schedule, I realize that we need to have a *transient* > > solution for SE and that would be refining only "iccOpenChannel", > > "iccExchangeAPDU" and "iccCloseChannel" first IMO. > > > > We'd like to avoid jsval as much as possible; however, as this is a > > transient solution, I'd also like to keep this bug as simple as possible, > > i.e. I suggest we keep use jsval in "iccExchangeAPDU" here. jsval usage will > > be discarded eventually in bug 864489. > > > > A possible solution would look like: > > > > /* New interface */ > > interface nsIIccCallback : nsISupports > > { > > void notifySuccess(); > > void notifyError(in AString error); > > void notifyExchangeAPDUSuccess(in long sw1, in long sw2, in AString > > response); > > }; > > > > partial interface nsIIccProvider > > { > > void iccOpenChannel(in unsigned long clientId, > > in AString aid, > > in nsIIccCallback callback); > > void iccExchangeAPDU(in unsigned long clientId, > > in long channel, > > in jsval apdu, > > in nsIIccCallback callback); > > void iccCloseChannel(in unsigned long clientId, > > in long channel, > > in nsIIccCallback callback); > > }; > > > > How does that sound? > > Looks good to me, but we need one more success callback which carries > `channelId` for `iccOpenChannel`. > Thank you. Ah ha, you are right :)

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1) > I believe this issue could be fixed in Bug 864489, which is huge work. > Considering the schedule, I realize that we need to have a *transient* > solution for SE and that would be refining only "iccOpenChannel", > "iccExchangeAPDU" and "iccCloseChannel" first IMO. > > We'd like to avoid jsval as much as possible; however, as this is a > transient solution, I'd also like to keep this bug as simple as possible, > i.e. I suggest we keep use jsval in "iccExchangeAPDU" here. jsval usage will > be discarded eventually in bug 864489. > Thanks for Hsin-Yi and Edgar's proposal. It looks good to me. To meet our milestone for NFC SE development, I think it makes sense to minimize the effort for interface change in this bug. It requires QC to release a transient version of QC RIL based on the interface discussed here. I think Hsin-Yi's team is working hardly to make freezing RIL related interfaces happened. After they finish that, we could have QC to release formal version of QC RIL. Does it make sense to you guys?

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1) > I believe this issue could be fixed in Bug 864489, which is huge work. > Considering the schedule, I realize that we need to have a *transient* > solution for SE and that would be refining only "iccOpenChannel", > "iccExchangeAPDU" and "iccCloseChannel" first IMO. > > We'd like to avoid jsval as much as possible; however, as this is a > transient solution, I'd also like to keep this bug as simple as possible, > i.e. I suggest we keep use jsval in "iccExchangeAPDU" here. jsval usage will > be discarded eventually in bug 864489. > > A possible solution would look like: > > /* New interface */ > interface nsIIccCallback : nsISupports > { > void notifySuccess(); > void notifyError(in AString error); > void notifyExchangeAPDUSuccess(in long sw1, in long sw2, in AString > response); > }; > > partial interface nsIIccProvider > { > void iccOpenChannel(in unsigned long clientId, > in AString aid, > in nsIIccCallback callback); > void iccExchangeAPDU(in unsigned long clientId, > in long channel, > in jsval apdu, > in nsIIccCallback callback); > void iccCloseChannel(in unsigned long clientId, > in long channel, > in nsIIccCallback callback); > }; > > How does that sound? Hsin-Yi, even though this is a transient solution the use of JSVAL just adds unnecessary complexity in our implementation. Since apdu is sort of a structure of fixed data types, can we please either define a new strongly type interface that we pass as apdu or expand apdu inline in the exchange APDU? Freezing of RIL Interface is still far from completion so we shouldn't really wait for that before making this change. Ideally we make the change as soon as Moz has resources to do so and QC will adapt to it ASAP.

Having icc* methods to accept a callback instead of returning a DOMRequest works for me. Thanks!

Oh, I was to fast in my previous comment. I think we need to have a success callback which will pass a channel ID, right? It could be something like: > interface nsIIccCallback : nsISupports > { > void notifySuccess(in long channelId); > void notifyError(in AString error); > void notifyExchangeAPDUSuccess(in long sw1, in long sw2, in AString > response); > }; For |iccOpenChannel| and |iccCloseChannel| we would put the channel ID there, and for |iccExchangeAPDU| we would use the |notifyExchangeAPDUSuccess| anyway. What do you think?

- Can we rename 'nsIIccCallback' interface to 'nsIIccChannelCallback' or some such (because 'Icc' is much broader in that sense) ? - Can we rename 'notifyExchangeAPDUSuccess' to 'notifyExchangeAPDUResponse' as sw1 and sw2 can contain error codes as well? My two cents on the following: Can we not simply have one callback (overload it with args ?) interface nsIIccChannelCallback : nsISupports { // 'details.channelId ' : Present for all three operations // 'details.sw1 , details.sw2 etc' present only for iccExchangeApdu() onResult(in long status, in jsval details); (or, simply) handleMessage (in jsval details); };

(In reply to Anshul from comment #5) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #1) > > I believe this issue could be fixed in Bug 864489, which is huge work. > > Considering the schedule, I realize that we need to have a *transient* > > solution for SE and that would be refining only "iccOpenChannel", > > "iccExchangeAPDU" and "iccCloseChannel" first IMO. > > > > We'd like to avoid jsval as much as possible; however, as this is a > > transient solution, I'd also like to keep this bug as simple as possible, > > i.e. I suggest we keep use jsval in "iccExchangeAPDU" here. jsval usage will > > be discarded eventually in bug 864489. > > > > A possible solution would look like: > > > > /* New interface */ > > interface nsIIccCallback : nsISupports > > { > > void notifySuccess(); > > void notifyError(in AString error); > > void notifyExchangeAPDUSuccess(in long sw1, in long sw2, in AString > > response); > > }; > > > > partial interface nsIIccProvider > > { > > void iccOpenChannel(in unsigned long clientId, > > in AString aid, > > in nsIIccCallback callback); > > void iccExchangeAPDU(in unsigned long clientId, > > in long channel, > > in jsval apdu, > > in nsIIccCallback callback); > > void iccCloseChannel(in unsigned long clientId, > > in long channel, > > in nsIIccCallback callback); > > }; > > > > How does that sound? > Hsin-Yi, even though this is a transient solution the use of JSVAL just adds > unnecessary complexity in our implementation. I had this proposal because I tried to look for good balance b/w the tight schedule for SE and technical solution. I thought we have been used jsval for exchangeAPDU for a long time, and you already had that implementation. Since we didn't change the basic behaviour of exchangeAPDU() I reason it could create barely overhead for you, and this could be acceptable for all of us, no? Please let us know if I didn't get your situation clear. > Since apdu is sort of a > structure of fixed data types, can we please either define a new strongly > type interface that we pass as apdu or expand apdu inline in the exchange > APDU? Could you provide a complete data structure here? > > Freezing of RIL Interface is still far from completion so we shouldn't > really wait for that before making this change. Ideally we make the change > as soon as Moz has resources to do so and QC will adapt to it ASAP.

(In reply to Siddartha P from comment #8) > - Can we rename 'nsIIccCallback' interface to 'nsIIccChannelCallback' or > some such (because 'Icc' is much broader in that sense) ? Naming nsIIccChannelCallback sounds good to me. However, I'd like to see if Edgar has concerns, especially considering our further plan bug 864489. > - Can we rename 'notifyExchangeAPDUSuccess' to 'notifyExchangeAPDUResponse' > as sw1 and sw2 can contain error codes as well? > Good idea. No objection :) > My two cents on the following: > Can we not simply have one callback (overload it with args ?) > interface nsIIccChannelCallback : nsISupports > { > // 'details.channelId ' : Present for all three operations > // 'details.sw1 , details.sw2 etc' present only for iccExchangeApdu() > onResult(in long status, in jsval details); > > (or, simply) > > handleMessage (in jsval details); > }; Sadly, no. For a new interface, we don't want to use jsval that causes trouble to other partners. We should have a well-defined arguments or interfaces. And actually we are already working on getting rid of jsval usage for ril internal interfaces.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #10) > (In reply to Siddartha P from comment #8) > > - Can we rename 'nsIIccCallback' interface to 'nsIIccChannelCallback' or > > some such (because 'Icc' is much broader in that sense) ? > > Naming nsIIccChannelCallback sounds good to me. However, I'd like to see if > Edgar has concerns, especially considering our further plan bug 864489. I am ok with `nsIIccChannelCallback` if it makes more sense. ((For bug 864489, we can have another callback for the other icc requests.)) Thank you.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #9) > (In reply to Anshul from comment #5) > > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #1) > > > I believe this issue could be fixed in Bug 864489, which is huge work. > > > Considering the schedule, I realize that we need to have a *transient* > > > solution for SE and that would be refining only "iccOpenChannel", > > > "iccExchangeAPDU" and "iccCloseChannel" first IMO. > > > > > > We'd like to avoid jsval as much as possible; however, as this is a > > > transient solution, I'd also like to keep this bug as simple as possible, > > > i.e. I suggest we keep use jsval in "iccExchangeAPDU" here. jsval usage will > > > be discarded eventually in bug 864489. > > > > > > A possible solution would look like: > > > > > > /* New interface */ > > > interface nsIIccCallback : nsISupports > > > { > > > void notifySuccess(); > > > void notifyError(in AString error); > > > void notifyExchangeAPDUSuccess(in long sw1, in long sw2, in AString > > > response); > > > }; > > > > > > partial interface nsIIccProvider > > > { > > > void iccOpenChannel(in unsigned long clientId, > > > in AString aid, > > > in nsIIccCallback callback); > > > void iccExchangeAPDU(in unsigned long clientId, > > > in long channel, > > > in jsval apdu, > > > in nsIIccCallback callback); > > > void iccCloseChannel(in unsigned long clientId, > > > in long channel, > > > in nsIIccCallback callback); > > > }; > > > > > > How does that sound? > > Hsin-Yi, even though this is a transient solution the use of JSVAL just adds > > unnecessary complexity in our implementation. > > I had this proposal because I tried to look for good balance b/w the tight > schedule for SE and technical solution. I thought we have been used jsval > for exchangeAPDU for a long time, and you already had that implementation. > Since we didn't change the basic behaviour of exchangeAPDU() I reason it > could create barely overhead for you, and this could be acceptable for all > of us, no? Please let us know if I didn't get your situation clear. Hsin-Yi, I understand your concern. My point simply is that you are getting away with JSVAL in bug 864489. Once this bug lands, we are expecting no more JSVAls in the ICC interface including SEEK APIs. So seems like we would have to modify the NFC APIs again once bug 864489 lands. This reworking on the same APIs again and again is frankly very wasteful for both you and us. Therefore I was suggesting to come up with a final API design as part of this bug as the cost of doing a simple solution now and a redesign later is far more than coming up with one final design right now. > > > Since apdu is sort of a > > structure of fixed data types, can we please either define a new strongly > > type interface that we pass as apdu or expand apdu inline in the exchange > > APDU? > > Could you provide a complete data structure here? > > > > > Freezing of RIL Interface is still far from completion so we shouldn't > > really wait for that before making this change. Ideally we make the change > > as soon as Moz has resources to do so and QC will adapt to it ASAP.

(In reply to Anshul from comment #12) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #9) > > (In reply to Anshul from comment #5) > > > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #1) > > > > I believe this issue could be fixed in Bug 864489, which is huge work. > > > > Considering the schedule, I realize that we need to have a *transient* > > > > solution for SE and that would be refining only "iccOpenChannel", > > > > "iccExchangeAPDU" and "iccCloseChannel" first IMO. > > > > > > > > We'd like to avoid jsval as much as possible; however, as this is a > > > > transient solution, I'd also like to keep this bug as simple as possible, > > > > i.e. I suggest we keep use jsval in "iccExchangeAPDU" here. jsval usage will > > > > be discarded eventually in bug 864489. > > > > > > > > A possible solution would look like: > > > > > > > > /* New interface */ > > > > interface nsIIccCallback : nsISupports > > > > { > > > > void notifySuccess(); > > > > void notifyError(in AString error); > > > > void notifyExchangeAPDUSuccess(in long sw1, in long sw2, in AString > > > > response); > > > > }; > > > > > > > > partial interface nsIIccProvider > > > > { > > > > void iccOpenChannel(in unsigned long clientId, > > > > in AString aid, > > > > in nsIIccCallback callback); > > > > void iccExchangeAPDU(in unsigned long clientId, > > > > in long channel, > > > > in jsval apdu, > > > > in nsIIccCallback callback); > > > > void iccCloseChannel(in unsigned long clientId, > > > > in long channel, > > > > in nsIIccCallback callback); > > > > }; > > > > > > > > How does that sound? > > > Hsin-Yi, even though this is a transient solution the use of JSVAL just adds > > > unnecessary complexity in our implementation. > > > > I had this proposal because I tried to look for good balance b/w the tight > > schedule for SE and technical solution. I thought we have been used jsval > > for exchangeAPDU for a long time, and you already had that implementation. > > Since we didn't change the basic behaviour of exchangeAPDU() I reason it > > could create barely overhead for you, and this could be acceptable for all > > of us, no? Please let us know if I didn't get your situation clear. > Hsin-Yi, I understand your concern. My point simply is that you are getting > away with JSVAL in bug 864489. > Once this bug lands, we are expecting no more JSVAls in the ICC interface > including SEEK APIs. So seems like we would have to modify the NFC APIs > again once bug 864489 lands. This reworking on the same APIs again and again > is frankly very wasteful for both you and us. Therefore I was suggesting to > come up with a final API design as part of this bug as the cost of doing a > simple solution now and a redesign later is far more than coming up with one > final design right now. > That's indeed a valid concern. I am fine with this suggestion. But note that it might take more time to finish this bug. According to [1], the adpu structure contains "cla (int32), command (int32), channel (int32), path (string), data (string), data2 (string), p1 (int32), p2 (int32), p3 (int32)." Are these the apdu interface needs? Anything missing, Vincent/Kamil/Siddartha ? Thank you! [1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#1480

Security element command and response interface are defined in Bug 879861 [1]. But I think we could define more generic interface for APDU because it may be used in cases other than security element as well. [1] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=879861&attachment=8491514 interface SECommand { attribute octet cla; // 1 Byte : Class Byte attribute octet ins; // 1 Byte : Instruction Byte attribute octet p1; // 1 Byte : First Octet of Parameters Byte attribute octet p2; // 1 Byte : Second Octet of Parameters Byte [Cached, Pure] attribute sequence<octet> data; // Sequence of octets attribute short le; // The length of the expected // response data or -1 if none is expected } interface SEResponse { // Response received on this 'channel' object. readonly attribute SEChannel channel; // First octet of response's status word readonly attribute octet sw1; // Second octet of response's status word readonly attribute octet sw2; // The response's data field bytes [Cached, Pure] readonly attribute sequence<octet> data; };

To be compliant with Open Mobile API we also need to able to retrieve the response to SELECT applet command which is currently done automatically when the channel is opened. I think that the success callback for opening channel should also carry the response to applet selection.

Maybe we don't have to change the interfaces described in 'nsIIccProvider' interface and still support 'openResponse'. For SecureElement parent process, it will be translated into two calls. (Since anyways the baseband creates the two APDU commands 1) Implicit selection , MANAGE CHANNEL [OPEN] , 2) Explicit Selection, SELECT [with AID] for iccOpenLogicalChannel() command) 1) iccOpenChannel() : returns the channel id , say XX 2) iccExchangeAPDU() : { cla: xx, ins: 0xC0, 0x00 , 0x00, 0x00 } // 0xCO is the INS for 'GET RESPONSE command APDU'. The result of this operation 'GET RESPONSE response APDU' ('SEResponse') then used to update 'openResponse' attribute in SEChannel interface.

Hi sid, do you think you can take the bug?

Ok Vincent. Sure!

Thanks for your great help.

Moving this bug forward. Add a 'needInfo' on a few. (Infact please consider this a 'needinfo' on all). I have basically collated all the comments from the bug and uploaded this patch. (However please note that I have now renamed callbacks to 'onIccOpenChannelDone' , etc.. from notifySuccess(..) , etc) - Please note that TMobile Poland (Wallet developers using SE API) are in need of 'selectResponse' that comes from opening a logical channel successfully. This patch considers this request. @Anshul, Is it possible for FxOS QCOM RIL to implement the callback 'onOpenChannelDone(in long channel, in nsIAPDUResponse selectResponse)' ? It implies that QCOM RIL should perform a two step process in order to retrieve before calling this callback. After getting the 'channel number / handle (say 'XX')' , QCOM RIL (instead of immediately notifying upper layers of the 'channel number / handle' - This is what happens currently) should also send "XX C0 00 00 00" APDU again to the baseband to retrieve 'select response' and then notify upper layers (Secure Element parent process) using the callback 'onOpenChannelDone( xx, selectResponse-apdu)'.

Also forgot to add this comment. I am not sure if we can use keywords such as 'partial interface' 'sequence<octet>' etc in an idl (*.idl). These should be ok in a webidl though. Again please correct me / provide feedback if that's not the case . This patch modifies earlier 3 definitions in nsIIccProvider.idl inline.

(In reply to Siddartha P from comment #20) > Created attachment 8515406 [details] [diff] [review] > 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch > > Moving this bug forward. Add a 'needInfo' on a few. (Infact please consider > this a 'needinfo' on all). I have basically collated all the comments from > the bug and uploaded this patch. (However please note that I have now > renamed callbacks to 'onIccOpenChannelDone' , etc.. from notifySuccess(..) , > etc) > > - Please note that TMobile Poland (Wallet developers using SE API) are in > need of 'selectResponse' that comes from opening a logical channel > successfully. > This patch considers this request. > Hi Siddartha, Thanks for the patch! I am really new to SE so please forgive my trivial questions :) If I understood the SE WebAPI correctly, there will be another asyn operation to retrieve APDUResponse after openChannel. Then why cannot we also have the same design in this internal interface? Why do we need to have "selectResponse" along with iccOpenChannel callback, especially in AOSP [2] I don't see it's possible to retrieve the response in one-step request? Another question is, does it always apply that "selectResponse" necessarily come with iccOpenChannel, or is it simply specific to TMobile Poland? Thank you :) [1] SE API current draft https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=879861&attachment=8491514 [2] https://github.com/android/platform_hardware_ril/blob/master/include/telephony/ril.h > @Anshul, Is it possible for FxOS QCOM RIL to implement the callback > 'onOpenChannelDone(in long channel, in nsIAPDUResponse selectResponse)' ? > It implies that QCOM RIL should perform a two step process in order to > retrieve before calling this callback. > After getting the 'channel number / handle (say 'XX')' , QCOM RIL (instead > of immediately notifying upper layers of the 'channel number / handle' - > This is what happens currently) should also send "XX C0 00 00 00" APDU again > to the baseband to retrieve 'select response' and then notify upper layers > (Secure Element parent process) using the callback 'onOpenChannelDone( xx, > selectResponse-apdu)'.

Comment on attachment 8515406 [details] [diff] [review] 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch Review of attachment 8515406 [details] [diff] [review]: ----------------------------------------------------------------- Comments in-line in addition to my question comment 22, thank you very much :) ::: dom/icc/interfaces/nsIIccProvider.idl @@ +57,5 @@ > + * The Channel Number/Handle that is successfully opened. > + * @param selectResponse > + * 'SelectResponse' (GET RESPONSE) result on openening a channel. > + */ > + void onOpenChannelDone(in long channel, in nsIAPDUResponse selectResponse); I'd prefer this follows RIL interface convention. Let's name it "notifyOpenChannelSuccess". @@ +63,5 @@ > + /** > + * Callback function to notify on successfully closing the logical channel. > + * > + */ > + void onCloseChannelDone(); ditto: s/onCloseChannelDone/notifyCloseChannelSuccess/ @@ +72,5 @@ > + * @param response > + * APDU response containing status bytes and the optional data. > + * > + */ > + void onExchangeAPDUResponse(in nsIAPDUResponse response); ditto: s/onExchangeAPDUResponse/notifyExchangeAPDUResponseSuccess/ @@ +75,5 @@ > + */ > + void onExchangeAPDUResponse(in nsIAPDUResponse response); > + > + /** > + * Callback function to notify error (Applicable only to iccOpenChannel & iccCloseChannel) I don't get it ... Why is onError not applicable to iccExchangeAPDU? Then, which callback should be called when iccExchangeAPDU failed? @@ +78,5 @@ > + /** > + * Callback function to notify error (Applicable only to iccOpenChannel & iccCloseChannel) > + * > + */ > + void onError(in AString error); ditto: s/onError/notifyError/

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #23) > Comment on attachment 8515406 [details] [diff] [review] > 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch > > Review of attachment 8515406 [details] [diff] [review]: > ----------------------------------------------------------------- > > Comments in-line in addition to my question comment 22, thank you very much > :) > > ::: dom/icc/interfaces/nsIIccProvider.idl > @@ +57,5 @@ > > + * The Channel Number/Handle that is successfully opened. > > + * @param selectResponse > > + * 'SelectResponse' (GET RESPONSE) result on openening a channel. > > + */ > > + void onOpenChannelDone(in long channel, in nsIAPDUResponse selectResponse); > > I'd prefer this follows RIL interface convention. Let's name it > "notifyOpenChannelSuccess". > > @@ +63,5 @@ > > + /** > > + * Callback function to notify on successfully closing the logical channel. > > + * > > + */ > > + void onCloseChannelDone(); > > ditto: s/onCloseChannelDone/notifyCloseChannelSuccess/ > > @@ +72,5 @@ > > + * @param response > > + * APDU response containing status bytes and the optional data. > > + * > > + */ > > + void onExchangeAPDUResponse(in nsIAPDUResponse response); > > ditto: s/onExchangeAPDUResponse/notifyExchangeAPDUResponseSuccess/ Sorry. I recalled comment 10. This callback could be called in error cases as well. so, my suggestion is s/onExchangeAPDUResponse/notifyExchangeAPDUResponse/ :) > > @@ +75,5 @@ > > + */ > > + void onExchangeAPDUResponse(in nsIAPDUResponse response); > > + > > + /** > > + * Callback function to notify error (Applicable only to iccOpenChannel & iccCloseChannel) > > I don't get it ... Why is onError not applicable to iccExchangeAPDU? Then, > which callback should be called when iccExchangeAPDU failed? > To be clear, my question is what if modem simply informs the exchange fails through a REQUEST error instead of sw1/sw2? Are you going to call |onExchangeAPDUResponse| with some manual specified sw1/sw2? > @@ +78,5 @@ > > + /** > > + * Callback function to notify error (Applicable only to iccOpenChannel & iccCloseChannel) > > + * > > + */ > > + void onError(in AString error); > > ditto: s/onError/notifyError/

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #22) > (In reply to Siddartha P from comment #20) > > Created attachment 8515406 [details] [diff] [review] > > 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch > > > > Moving this bug forward. Add a 'needInfo' on a few. (Infact please consider > > this a 'needinfo' on all). I have basically collated all the comments from > > the bug and uploaded this patch. (However please note that I have now > > renamed callbacks to 'onIccOpenChannelDone' , etc.. from notifySuccess(..) , > > etc) > > > > - Please note that TMobile Poland (Wallet developers using SE API) are in > > need of 'selectResponse' that comes from opening a logical channel > > successfully. > > This patch considers this request. > > > > Hi Siddartha, > > Thanks for the patch! I am really new to SE so please forgive my trivial > questions :) > > If I understood the SE WebAPI correctly, there will be another asyn > operation to retrieve APDUResponse after openChannel. Then why cannot we > also have the same design in this internal interface? Why do we need to have > "selectResponse" along with iccOpenChannel callback, especially in AOSP [2] > I don't see it's possible to retrieve the response in one-step request? > > Another question is, does it always apply that "selectResponse" necessarily > come with iccOpenChannel, or is it simply specific to TMobile Poland? > Looks like comment 15 has answer the question. So the question would be should we comply with open mobile API?

Comment on attachment 8515406 [details] [diff] [review] 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch Review of attachment 8515406 [details] [diff] [review]: ----------------------------------------------------------------- Looks good for me. Thank you.

(In reply to Vincent Chang[:vchang] from comment #25) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #22) > > (In reply to Siddartha P from comment #20) > > > Created attachment 8515406 [details] [diff] [review] > > > 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch > > > > > > Moving this bug forward. Add a 'needInfo' on a few. (Infact please consider > > > this a 'needinfo' on all). I have basically collated all the comments from > > > the bug and uploaded this patch. (However please note that I have now > > > renamed callbacks to 'onIccOpenChannelDone' , etc.. from notifySuccess(..) , > > > etc) > > > > > > - Please note that TMobile Poland (Wallet developers using SE API) are in > > > need of 'selectResponse' that comes from opening a logical channel > > > successfully. > > > This patch considers this request. > > > > > > > Hi Siddartha, > > > > Thanks for the patch! I am really new to SE so please forgive my trivial > > questions :) > > > > If I understood the SE WebAPI correctly, there will be another asyn > > operation to retrieve APDUResponse after openChannel. Then why cannot we > > also have the same design in this internal interface? Why do we need to have > > "selectResponse" along with iccOpenChannel callback, especially in AOSP [2] > > I don't see it's possible to retrieve the response in one-step request? > > > > Another question is, does it always apply that "selectResponse" necessarily > > come with iccOpenChannel, or is it simply specific to TMobile Poland? > > > Looks like comment 15 has answer the question. So the question would be > should we comply with open mobile API? As the earlier comments says, the Global platform specification states that every time openLogicalChannel AT command is issued to the baseband, it results in two commands mentioned below: 1) MANAGE (OPEN) CHANNEL : This command's response fetches the 'channel number' 2) GET RESPONSE : This command shall fetch the openChannel's response. Earlier API exposed ( notifySuccess(long channelId) ), only retrieves the channel number. Hence the change request to the API. Please note that 'select response (response to command 2)' is as per the specification and this is not a custom implementation. @Vincent, As you said, open mobile API / seek-for-android and W3C system group's SE API proposal expose a high level API to retrieve 'select response'. Our proposal seems to be inline with it. Moreover TMobile Poland (First consumers of SE API) have a definitive use-case to handle 'select response' at Gaia level.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #24) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #23) > > Comment on attachment 8515406 [details] [diff] [review] > > 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch > > > > Review of attachment 8515406 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Comments in-line in addition to my question comment 22, thank you very much > > :) > > > > ::: dom/icc/interfaces/nsIIccProvider.idl > > @@ +57,5 @@ > > > + * The Channel Number/Handle that is successfully opened. > > > + * @param selectResponse > > > + * 'SelectResponse' (GET RESPONSE) result on openening a channel. > > > + */ > > > + void onOpenChannelDone(in long channel, in nsIAPDUResponse selectResponse); > > > > I'd prefer this follows RIL interface convention. Let's name it > > "notifyOpenChannelSuccess". Ok. > > > > @@ +63,5 @@ > > > + /** > > > + * Callback function to notify on successfully closing the logical channel. > > > + * > > > + */ > > > + void onCloseChannelDone(); > > > > ditto: s/onCloseChannelDone/notifyCloseChannelSuccess/ Ok > > > > @@ +72,5 @@ > > > + * @param response > > > + * APDU response containing status bytes and the optional data. > > > + * > > > + */ > > > + void onExchangeAPDUResponse(in nsIAPDUResponse response); > > > > ditto: s/onExchangeAPDUResponse/notifyExchangeAPDUResponseSuccess/ Ok > > Sorry. I recalled comment 10. This callback could be called in error cases > as well. > so, my suggestion is s/onExchangeAPDUResponse/notifyExchangeAPDUResponse/ :) > > > > > @@ +75,5 @@ > > > + */ > > > + void onExchangeAPDUResponse(in nsIAPDUResponse response); > > > + > > > + /** > > > + * Callback function to notify error (Applicable only to iccOpenChannel & iccCloseChannel) > > > > I don't get it ... Why is onError not applicable to iccExchangeAPDU? Then, > > which callback should be called when iccExchangeAPDU failed? > > > > To be clear, my question is what if modem simply informs the exchange fails > through a REQUEST error instead of sw1/sw2? > Are you going to call |onExchangeAPDUResponse| with some manual specified > sw1/sw2? > This callback 'onExchangeAPDUResponse' shall be called for both success & failure scenarios for the given IccChannel command iccExchangeAPDU(..). Following are some of the examples of sw1 , sw2 scenarios SW1 SW2 DESCP 90 00 Command successfully executed (OK). 92 10 Insufficient memory. No more storage available. 97 89 Service not available 99 00 PIN try left 6A 82 File not found 68 81 Logical channel not supported . . . . As you may noted, the above list is exhaustive and combination of sw1 & sw2 values determine if it is a success or failure for a given APDU command. SE stack relays this information (sw1 & sw2) all the way to the Gaia Apps. > > @@ +78,5 @@ > > > + /** > > > + * Callback function to notify error (Applicable only to iccOpenChannel & iccCloseChannel) > > > + * > > > + */ > > > + void onError(in AString error); > > > > ditto: s/onError/notifyError/ Ok

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #22) > (In reply to Siddartha P from comment #20) > > Created attachment 8515406 [details] [diff] [review] > > 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch > > > > Moving this bug forward. Add a 'needInfo' on a few. (Infact please consider > > this a 'needinfo' on all). I have basically collated all the comments from > > the bug and uploaded this patch. (However please note that I have now > > renamed callbacks to 'onIccOpenChannelDone' , etc.. from notifySuccess(..) , > > etc) > > > > - Please note that TMobile Poland (Wallet developers using SE API) are in > > need of 'selectResponse' that comes from opening a logical channel > > successfully. > > This patch considers this request. > > > > Hi Siddartha, > > Thanks for the patch! I am really new to SE so please forgive my trivial > questions :) > No, Thank You for your time and feedback. > If I understood the SE WebAPI correctly, there will be another asyn > operation to retrieve APDUResponse after openChannel. Then why cannot we > also have the same design in this internal interface? Why do we need to have > "selectResponse" along with iccOpenChannel callback, especially in AOSP [2] > I don't see it's possible to retrieve the response in one-step request? > Yes you are right. This is not a one-step request! Consider 'GET RESPONSE' (Select response a.k.a openResponse) is simply another APDU command that could be sent by the SE stack using iccExchangeAPDU(..) . Its response could be simply interpreted as 'selectResponse'. But since nsIIccProvider.idl is an internal contract , (and one level above ril contract - ril.h), it'd be nicer (from the users of iccChannel APIs point of view) if the contract supports both (channel id & selectResponse) in single callback. Hope I have addressed your concern. > Another question is, does it always apply that "selectResponse" necessarily > come with iccOpenChannel, or is it simply specific to TMobile Poland? > This is in line with the specifications and not specific to TMobile Poland! > > [1] SE API current draft > https://bugzilla.mozilla.org/page.cgi?id=splinter. > html&bug=879861&attachment=8491514 > [2] > https://github.com/android/platform_hardware_ril/blob/master/include/ > telephony/ril.h >

Uploading the idl's second version! Difference between this version and earlier is renaming of callback functions from 'onIccxxxx' to 'notifyIccxxx'. Thanks Anshul and Vincent for your quick feedback. I assume that this change should be ok to you guys.

Hi Vincent , HsinYi and All, Once idl is locked down, I assume that we would also have to make changes to dom/icc , RILContentHelper & RadioInterfaceLayer on Mozilla RIL side of things in order for these components to comply with the latest idl changes.

Comment on attachment 8516917 [details] [diff] [review] (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch Review of attachment 8516917 [details] [diff] [review]: ----------------------------------------------------------------- hi Sid, Thanks for the explanation that sounds reasonable to me. :) Just some minor comments for convention. ::: dom/icc/interfaces/nsIIccProvider.idl @@ +19,5 @@ > > +[scriptable, uuid(7a93578d-b389-4bec-9ea3-5ee4cf048710)] > +interface nsIAPDUCommand : nsISupports > +{ > + // 1 Byte : Class Byte Let's follow the convention we used for RIL. Comment applies here and below. e.g. /** * 1 Byte: Instrunction Byte */ @@ +20,5 @@ > +[scriptable, uuid(7a93578d-b389-4bec-9ea3-5ee4cf048710)] > +interface nsIAPDUCommand : nsISupports > +{ > + // 1 Byte : Class Byte > + readonly attribute octet cla; Please add an extra empty line. Comment applies here and below.

(In reply to Siddartha P from comment #31) > Hi Vincent , HsinYi and All, > > Once idl is locked down, I assume that we would also have to make changes to > dom/icc , RILContentHelper & RadioInterfaceLayer on Mozilla RIL side of > things in order for these components to comply with the latest idl changes. Hi Sid, As we chat on IRC, you need to have new files for IccChannelCallback and APDUCommand. Changes to RILContentHelper and RadioInterfaceLayer are required. In addition, changes to ril_worker.js are needed for 1) correct data format in [1], and 2) to support 'selectResponse' coming along with iccOpenChannel Let me know if it isn't clear enough. :) [1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js#1481

Sure, you are going to continue that patch, right? To speed up the SE development, I think QC can start to develop new QC Ril once interface is locked down and get r+.

Comment on attachment 8516917 [details] [diff] [review] (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch Review of attachment 8516917 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/interfaces/nsIIccProvider.idl @@ +32,5 @@ > + readonly attribute octet p3; > + // bytes of data > + readonly attribute AString data; > + // The length of the expected response data or -1 if none is expected > + readonly attribute short le; Hi Sid, I have a question about "le". I don't see this attribute is defined in AOSP ril.h [1]. Can you explain how it is going to be used? I am wondering if this is needed in this interface. Thank you! [1] https://github.com/android/platform_hardware_ril/blob/master/include/telephony/ril.h#L400

http://en.wikipedia.org/wiki/Smart_card_application_protocol_data_unit According to ISO/IEC 7816-4, there are 4 cases of APDU commands. Command APDU: Field name Length (bytes) Description ----------------------------------------------------------------------------------------------------- CLA 1 Instruction class - indicates the type of command INS 1 Instruction code - indicates the specific command, e.g. "write data" P1-P2 2 Instruction parameters for the command, e.g. offset into file at which to write the data Lc 0, 1 or 3 Encodes the number (Nc) of bytes of command data to follow Data N N bytes of data Le 0,1,2 or 3 Encodes the maximum number (Ne) of response bytes expected Case 1 - Command: CLA|INS|P1|P2 (Case 1 is a simple command without command data and don't expect response data.) Case 2 - Command: CLA|INS|P1|P2|Le (In Case 2, the command has no command data, but expected response data.) Case 3 - Command: CLA|INS|P1|P2|Lc|Data (Case 3 describes a command with command data, the expected no response data) Case 4 - Command: CLA|INS|P1|P2|Lc|Data|Le (A Case 4 command has both command and response data) All these 4 cases could be required by NFC payments.

Comment on attachment 8516917 [details] [diff] [review] (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch Review of attachment 8516917 [details] [diff] [review]: ----------------------------------------------------------------- I think Hsinyi's question is if the instruction byte is SELECT_FILE or SELECT, which lc and le both could have values. How does Android RIL address this since it only has 1 byte (p3) to do this. ::: dom/icc/interfaces/nsIIccProvider.idl @@ +28,5 @@ > + readonly attribute octet p1; > + // 1 Byte : Second Octet of Parameters Byte > + readonly attribute octet p2; > + // 1 Byte : Third Octet of Parameters Byte > + readonly attribute octet p3; Should be lc @@ +30,5 @@ > + readonly attribute octet p2; > + // 1 Byte : Third Octet of Parameters Byte > + readonly attribute octet p3; > + // bytes of data > + readonly attribute AString data; Should be some byte array. Each AString stores 2 bytes.

(In reply to Yoshi Huang[:allstars.chh] from comment #37) > Comment on attachment 8516917 [details] [diff] [review] > (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch > > Review of attachment 8516917 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think Hsinyi's question is if the instruction byte is SELECT_FILE or > SELECT, which lc and le both could have values. > How does Android RIL address this since it only has 1 byte (p3) to do this. > Thank you, Yoshi! This is what I tried to ask!

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #38) > Thanks Ming for the information. > > If my understanding is correct, I wonder if it sounds more reasonable to > replace "octet p3" with "short le" ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ typo, should be |replace "octet p3" with short *lc*| > which indicates the length of the command > data? Having "p3" here along with "le" really confuses. > > And do we really need to use "-1" to indicate the case that no response data > is expected? Can't we just use 0 for that?

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #35) > Comment on attachment 8516917 [details] [diff] [review] > (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch > > Review of attachment 8516917 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/icc/interfaces/nsIIccProvider.idl > @@ +32,5 @@ > > + readonly attribute octet p3; > > + // bytes of data > > + readonly attribute AString data; > > + // The length of the expected response data or -1 if none is expected > > + readonly attribute short le; > > Hi Sid, > I have a question about "le". I don't see this attribute is defined in AOSP > ril.h [1]. Can you explain how it is going to be used? I am wondering if > this is needed in this interface. Thank you! > > [1] > https://github.com/android/platform_hardware_ril/blob/master/include/ > telephony/ril.h#L400 Hi HsinYi, Here are my thoughts on this one. APDU command is header + body . At the lowest level when reader sends APDU commands to the smart card, here is how it will be interpreted CLA, INS (command) , p1, p2 : header [Note this is as per T1 Thanks Sid(In reply to Hsin-Yi Tsai [:hsinyi] from comment #32) > Comment on attachment 8516917 [details] [diff] [review] > (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch > > Review of attachment 8516917 [details] [diff] [review]: > ----------------------------------------------------------------- > > hi Sid, > > Thanks for the explanation that sounds reasonable to me. :) > > Just some minor comments for convention. > > ::: dom/icc/interfaces/nsIIccProvider.idl > @@ +19,5 @@ > > > > +[scriptable, uuid(7a93578d-b389-4bec-9ea3-5ee4cf048710)] > > +interface nsIAPDUCommand : nsISupports > > +{ > > + // 1 Byte : Class Byte > > Let's follow the convention we used for RIL. Comment applies here and below. > > e.g. > /** > * 1 Byte: Instrunction Byte > */ > > @@ +20,5 @@ > > +[scriptable, uuid(7a93578d-b389-4bec-9ea3-5ee4cf048710)] > > +interface nsIAPDUCommand : nsISupports > > +{ > > + // 1 Byte : Class Byte > > + readonly attribute octet cla; > > Please add an extra empty line. Comment applies here and below. Btw , I noted these comments

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #35) > Comment on attachment 8516917 [details] [diff] [review] > (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch > > Review of attachment 8516917 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/icc/interfaces/nsIIccProvider.idl > @@ +32,5 @@ > > + readonly attribute octet p3; > > + // bytes of data > > + readonly attribute AString data; > > + // The length of the expected response data or -1 if none is expected > > + readonly attribute short le; > > Hi Sid, > I have a question about "le". I don't see this attribute is defined in AOSP > ril.h [1]. Can you explain how it is going to be used? I am wondering if > this is needed in this interface. Thank you! > > [1] > https://github.com/android/platform_hardware_ril/blob/master/include/ > telephony/ril.h#L400 Hi HsinYi, Here are my thoughts on this one. APDU command is header + body . At the lowest level when reader sends APDU commands to the smart card, here is how it will be interpreted CLA, INS (command) , p1, p2 : header [Note this is as per T1 Thanks Sid(In reply to Hsin-Yi Tsai [:hsinyi] from comment #32) > Comment on attachment 8516917 [details] [diff] [review] > (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch > > Review of attachment 8516917 [details] [diff] [review]: > ----------------------------------------------------------------- > > hi Sid, > > Thanks for the explanation that sounds reasonable to me. :) > > Just some minor comments for convention. > > ::: dom/icc/interfaces/nsIIccProvider.idl > @@ +19,5 @@ > > > > +[scriptable, uuid(7a93578d-b389-4bec-9ea3-5ee4cf048710)] > > +interface nsIAPDUCommand : nsISupports > > +{ > > + // 1 Byte : Class Byte > > Let's follow the convention we used for RIL. Comment applies here and below. > > e.g. > /** > * 1 Byte: Instrunction Byte > */ > > @@ +20,5 @@ > > +[scriptable, uuid(7a93578d-b389-4bec-9ea3-5ee4cf048710)] > > +interface nsIAPDUCommand : nsISupports > > +{ > > + // 1 Byte : Class Byte > > + readonly attribute octet cla; > > Please add an extra empty line. Comment applies here and below. Btw , I noted these comments

(In reply to Vincent Chang[:vchang] from comment #34) > Sure, you are going to continue that patch, right? To speed up the SE > development, I think QC can start to develop new QC Ril once interface is > locked down and get r+. Sure Vincent , I have noted your comment as well. I am going to post my update very soon on this. I would need RIL experts opinion followed by Qualcomm's. Thanks

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #35) > Comment on attachment 8516917 [details] [diff] [review] > (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch > > Review of attachment 8516917 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/icc/interfaces/nsIIccProvider.idl > @@ +32,5 @@ > > + readonly attribute octet p3; > > + // bytes of data > > + readonly attribute AString data; > > + // The length of the expected response data or -1 if none is expected > > + readonly attribute short le; > > Hi Sid, > I have a question about "le". I don't see this attribute is defined in AOSP > ril.h [1]. Can you explain how it is going to be used? I am wondering if > this is needed in this interface. Thank you! > > [1] > https://github.com/android/platform_hardware_ril/blob/master/include/ > telephony/ril.h#L400 Hi HsinYi, I have started replying to your questions in my earlier comments. But I submitted the reply too soon. So correcting my response here for your comment. ------------------------------------------------------------------ I must confess that the usage of 'Le' is not super clear / obvious. According to ISO 7816 standards, there are different transmission protocols such T=0, T=1 etc etc. According to T=1 protocol, here is how APDU command is interpreted at IC card level (smart card) HEADER : CLA, INS , P1, P2 (Note that there is no P3 here). P3 can be also reffered to as 'Lc' for historical reasons, I guess. BODY : P3, data & Le FYI, according to T=0 protocol, HEADER comprised of 'P3' as well (Header in T= 0, CLA, INS , P1, P2, P3) But from T=1, the first byte of body is 'P3'. This way it was upward compatible within different Last byte of the body is 'Le'. (in our case, last byte of of 'data' field in nsIAPDUCommand) i;e; if 'n' is the length of data, data[n] will contain Le. To the smart card, apdu will be dispatched as bytes[] in the end. At the smart card side of things, it does the following marshalling Read 'header' : CLA + INS + P1 + P2 Now for the body : Body Length (len) = 1 (Lc) + N (num of bytes of 'data') + 1 (Le); Read the first byte which is length of data. body[0..'len -1'] is what will be sent as 'data' in Android RIL as well as in FxOS RIL. To make it explicit from vendor RIL , the presence of extra byte (Le), hence the added in nsIApduCommand interface) Following are valid APDU commands Case 1 - Only Header : CLA, INS, P1, P2 Case 2 - Header & Body (w/o Le) CLA, INS, P1, P2 || Lc , data Lc is encoded on the fifth byte of the T=0 T-APDU Case 3 - Header & Only Le in body CLA, INS, P1, P2 || Le In this case as you may have noted, Le is encoded on the fifth byte of T-APDU, Case 4 - Header , Data with P3 (Lc) & Le CLA, INS, P1, P2 || Lc, Data, Le So in my opinion 'Le' is part of 'data' and is appended. Adding the explicit Le value, say Le=n bytes to the APDU perhaps needed, if the user of trasmit API expect any output / response bytes. Disclaimer: I am no expert in this area, but from whatever I have read thus far, I am sharing some of my thoughts :)

(In reply to Yoshi Huang[:allstars.chh] from comment #37) > Comment on attachment 8516917 [details] [diff] [review] > (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch > > Review of attachment 8516917 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think Hsinyi's question is if the instruction byte is SELECT_FILE or > SELECT, which lc and le both could have values. > How does Android RIL address this since it only has 1 byte (p3) to do this. > > ::: dom/icc/interfaces/nsIIccProvider.idl > @@ +28,5 @@ > > + readonly attribute octet p1; > > + // 1 Byte : Second Octet of Parameters Byte > > + readonly attribute octet p2; > > + // 1 Byte : Third Octet of Parameters Byte > > + readonly attribute octet p3; > > Should be lc Ok. I think either 'Lc' or 'P3' have the same significance given the smart card spec history. That said, I will rename it to 'Lc' in the next iteration! > > @@ +30,5 @@ > > + readonly attribute octet p2; > > + // 1 Byte : Third Octet of Parameters Byte > > + readonly attribute octet p3; > > + // bytes of data > > + readonly attribute AString data; > > Should be some byte array. > Each AString stores 2 bytes. Oh! Yes we would need a byte array. I will fix this as well in the next iteration!

All, Quick update from my side. I have been in the process of making few changes in Moz-RIL side of things and I think, I have managed to successfully communicate with already released version of Qualcomm build. So my pitch at this point (Proposal 2) that we may not need another Qualcomm release as it may be a time consuming process (because of release procedure between Qualcomm <-> T2M <-> DT ). So what are my changes ? 1. First apply these changes https://bugzilla.mozilla.org/show_bug.cgi?id=1030602 that are already landed in master, I suppose. These are gecko/dom/icc related changes which is not needed as we are exposing any 'channel apis' to content through webIDL 2. Modified RILContentHelper.js for everyone's feedback. Please note that this is not a final code to be submitted and only demonstration of how RILContentHelper is to be modified to make things work. << Refer to patch - proposal1_RILContentHellper.patch >> 3. Modified nsIIccProvider.idl to redefine the 3 APIs (Remove the 'window' argument so that it is accessible to other modules namely secure element parent process) << Refer to patch - proposal1_0001-Bug-1081789-Redefine-IccOpenChannel-IccCloseChannel-.patch >> With the above changes (Lets call them Proposal 2) SE stack (parent process) is able to access all the three APIs and communicate with SIM and get the responses! Benefits of Proposal 2 - No extra build release needed from T2M as this can be time consuming process (in the order of 3 - 4 weeks at-least after we land RIL changes). This will push the initial SE stack landing around mid-dec further. - Minimal set of changes and everything work as-is - Since we are not modifying anything other RILContentHelper, we do not need another release from Qualcomm. Disadvantages of Proposal 2 - The APIs in nsIIccProvider.idl still use 'jsval' as against proper XPCOM objs such as 'nsIAPDUCommand' & 'nsIAPDUResponse' as identified. (In my opinion, this should be ok for now. Requesting Anshul to chime in and voice his opinion) - Select Response will be a two step process. (Again in my opinion, this is not a deal breaker as we have already validated the stack and noted that Qualcomm baseband already supports GET RESPONSE. This will just be another extra call in SE parent process) Future work of Proposal 2 - We can migrate / port these APIs along with other RILContentHelper functions from 'jsval' to proper XPCOM objs. If needed, DT can still own this task for three identified channel interfaces in future as well. Proposal 1 (Original one with proper XPCOM objs + new Release from Qualcomm through T2M) can delay the deliverables overall. Doing a needinfo on few, but please treat this as needInfo on all. Adding Edgar to the discussion as well as I was in really helpful discussions with him on IRC over the last week.

For the completion sake, I was able to access the APIs and receive the callbacks from SecureElement Parent Process using the following code snippet. Sharing it for everyone's feedback _openChannel: function(data, callback) { let clientId = 1; let status = FAILURE; // TBD: Validate the AID 'data.aid' with ACE before opening a channel! iccProvider.iccOpenChannel(clientId, data.aid , { notifyOpenChannelSuccess: function(channel) { let token = null; if (((this.cardState === CARDSTATE_READY)) && (data.type === 'uicc')) { let token = UUIDGenerator.generateUUID().toString(); status = gSEMessageManager._addChannelToSession(channel, token, data); } if (callback) { callback({ status: status, token : token }); } }, notifyCloseChannelSuccess: function() {}, notifyExchangeAPDUResponse: function(response) {}, notifyError: function(error) { callback({ status: status, error: error }); } }); },

It looks like I have mixed up names of the patches. I meant "proposal2_RILContentHellper.patch" instead of 'proposal1_RILContentHellper.patch' and "proposal2_0001-Bug-1081789-Redefine-IccOpenChannel-IccCloseChannel-.patch" instead of "proposal1_0001-Bug-1081789-Redefine-IccOpenChannel-IccCloseChannel-.patch"

It looks like I have mixed up names of the patches. I meant "proposal2_RILContentHellper.patch" instead of 'proposal1_RILContentHellper.patch' and "proposal2_0001-Bug-1081789-Redefine-IccOpenChannel-IccCloseChannel-.patch" instead of "proposal1_0001-Bug-1081789-Redefine-IccOpenChannel-IccCloseChannel-.patch"

Hi Sid, thanks for your hard work. One question about nsIIccProvider.idl change, my understanding is that QCM ril implements nsIIccProvider.idl interface, I am wondering how can nsIIccProvider.idl APIs interface change works without modification of QCM ril?

Hello Sid, Thanks for the proposal. I am willing to support a way to help avoid any extra comril release. As a result, "Select Response will be a two step process" is acceptable to me. However, as we talked on irc, let's try our best to get rid of 'jsval.' We also don't really need the interfaces of nsIAPDUCommand and nsIAPDUResponse, while with these two new interfaces, we just create tedious steps of un-wrapping a object to retrieve some parameters and wrapping those values. Instead, we could simply spread those attributes. For example, nsIIccChannelCallback.notifyExchangeAPDUResponse(sw1, sw2, data);

Comment on attachment 8520384 [details] [diff] [review] proposal1_0001-Bug-1081789-Redefine-IccOpenChannel-IccCloseChannel-.patch Review of attachment 8520384 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/interfaces/nsIIccProvider.idl @@ +40,5 @@ > + * @param response > + * APDU response containing status bytes and the optional data. > + * > + */ > + void notifyExchangeAPDUResponse(in jsval response); let's just spread arguments up: notifyExchangeAPDUResponse(in octet sw1, in octet sw2, in unsigned long length, [array, size_is(length)] in octet data); @@ +128,5 @@ > + in DOMString aid, > + in nsIIccChannelCallback callback); > + void iccExchangeAPDU(in unsigned long clientId, > + in long channel, > + in jsval apdu, ditto: spread "apdu" this argument list is a bit longer but I am fine with it, especially considering repeated wrap-unwrap steps.

Comment on attachment 8520383 [details] [diff] [review] proposal1_RILContentHellper.patch Review of attachment 8520383 [details] [diff] [review]: ----------------------------------------------------------------- The overall direction looks right :) ::: dom/system/gonk/RILContentHelper.js @@ +1060,4 @@ > }); > }, > > + iccOpenChannel: function(clientId, aid, callbackInterface) { s/callbackInterface/callback @@ +1074,3 @@ > }, > > + iccExchangeAPDU: function(clientId, channel, apdu, callbackInterface) { ditto. @@ +1089,3 @@ > }, > > + iccCloseChannel: function(clientId, channel, callbackInterface) { ditto. @@ +1517,4 @@ > } > }, > > + addIccChannelCallback: function(requestId, channelCb) { Please rename it _addIccChannelCallback. The _ prefix means it's a private or internal member. @@ +1526,5 @@ > + > + if (DEBUG) debug("Unable to add channelCbInterface for requestId : " + requestId); > + }, > + > + getIccChannelCallback: function(requestId) { ditto. @@ +1932,5 @@ > + handleIccOpenChannel: function(message) { > + let requestId = message.requestId; > + let channel = message.channel; > + > + let callbackInterface = this.getIccChannelCallback(requestId); We could just call it "callback." @@ +1941,5 @@ > + }, > + > + handleIccCloseChannel: function(message) { > + let requestId = message.requestId; > + let callbackInterface = this.getIccChannelCallback(requestId); ditto. @@ +1954,5 @@ > + let requestId = message.requestId; > + let callbackInterface = this.getIccChannelCallback(requestId); > + > + if (callbackInterface) { > + let result = [message.sw1, message.sw2, message.simResponse, message.errorMsg]; Wondering if message.errorMsg is actually needed. Per my understanding of comment 28, combination of sw1 and sw2 is able to provide error messages.

Sid and Hsin-Yi, I am fine with the Sid's proposal #2 for now. Perhaps the cleanup of JSVALs can be done as part of moving the ICC APIs to IPDL in bug 864489.

Hi HsinYi, I think I have addressed all your comments in this patch for your review. I have one question though. For this patch to compile and function as expected, https://bugzilla.mozilla.org/show_bug.cgi?id=1030602 , we would need this change as well. (This is already landed on the main line :Bug#1030602). Since for SE stack development, we are based off Qualcomm's release on v2.1 branch. So two things should happen 1) On REL 2.1 branch, Bug#1030602 change should be landed along with the current changes / patch of redefining the interfaces. 2) The current patch should land on the mainline. Any suggestions?

(In reply to Anshul from comment #56) > Sid and Hsin-Yi, I am fine with the Sid's proposal #2 for now. Perhaps the > cleanup of JSVALs can be done as part of moving the ICC APIs to IPDL in bug > 864489. Thanks Anshul

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #55) > Comment on attachment 8520383 [details] [diff] [review] > proposal1_RILContentHellper.patch > > Review of attachment 8520383 [details] [diff] [review]: > ----------------------------------------------------------------- > > The overall direction looks right :) > > ::: dom/system/gonk/RILContentHelper.js > @@ +1060,4 @@ > > }); > > }, > > > > + iccOpenChannel: function(clientId, aid, callbackInterface) { > > s/callbackInterface/callback > > @@ +1074,3 @@ > > }, > > > > + iccExchangeAPDU: function(clientId, channel, apdu, callbackInterface) { > > ditto. > > @@ +1089,3 @@ > > }, > > > > + iccCloseChannel: function(clientId, channel, callbackInterface) { > > ditto. > > @@ +1517,4 @@ > > } > > }, > > > > + addIccChannelCallback: function(requestId, channelCb) { > > Please rename it _addIccChannelCallback. The _ prefix means it's a private > or internal member. > > @@ +1526,5 @@ > > + > > + if (DEBUG) debug("Unable to add channelCbInterface for requestId : " + requestId); > > + }, > > + > > + getIccChannelCallback: function(requestId) { > > ditto. > > @@ +1932,5 @@ > > + handleIccOpenChannel: function(message) { > > + let requestId = message.requestId; > > + let channel = message.channel; > > + > > + let callbackInterface = this.getIccChannelCallback(requestId); > > We could just call it "callback." > > @@ +1941,5 @@ > > + }, > > + > > + handleIccCloseChannel: function(message) { > > + let requestId = message.requestId; > > + let callbackInterface = this.getIccChannelCallback(requestId); > > ditto. > > @@ +1954,5 @@ > > + let requestId = message.requestId; > > + let callbackInterface = this.getIccChannelCallback(requestId); > > + > > + if (callbackInterface) { > > + let result = [message.sw1, message.sw2, message.simResponse, message.errorMsg]; > > Wondering if message.errorMsg is actually needed. Per my understanding of > comment 28, combination of sw1 and sw2 is able to provide error messages. It looks like even in failed transmit APDU scenarios, (say sw1 & sw2 is 6x 00 and not 9x 00 - which is a success ), RILContentHelper receives these values with 'errorMsg being null'. So I am yet to see a transmit apdu usecase with an errorMsg. But that said, since this value is being sent by lower layers, I made use of it just as how it was (just in case).

Hi Sid, since we don't have Le in iccEchangeAPDU, I don't see any way to send GET RESPONSE [1] or READ BINARY [2] commands with number of response bytes specified. I still can get the whole response if I just set Lc to 0. Am I missing something here? I think that requesting specific number of bytes in the response should be also supported. [1] http://www.cardwerk.com/smartcards/smartcard_standard_ISO7816-4_7_transmission_interindustry_commands.aspx#chap7_1 [2] http://www.cardwerk.com/smartcards/smartcard_standard_ISO7816-4_6_basic_interindustry_commands.aspx#chap6_1

Hi Krzysztof, Here are my thoughts! As you may have noted, CLA+INS+P1+P2 make up the header. For the body part, Its generally Lc , Lc Bytes and / of Le The meaning of P3 could be either 'Lc/Le' depending on which case we are dealing with coupled with Transmission protocol (T=0 or T=1). > I still can get the whole response if I just set Lc to 0 I think, firstly we should fix my patch, and rename Lc to simply 'P3' to avoid all confusion. This use-case of yours is interpreted as : Case 3 - Header & Only Le in body CLA, INS, P1, P2 || Le Since you have set Lc to zero, it is the last byte of body. So at the lowest level when terminal (baseband) is interacting with UICC, it will be interpreted as 'P3 = Le'. As I said, I will simply rename the iccExchaneAPDU arg from 'Lc' to 'P3' to avoid confusion and be in-consistent with the spec. High Level SE APIs can have both Lc and Le though. Now coming back to other case where, you want to specify both Lc and Le (Case 4, as stated above). a) Application Layer (C-APDU) : CLA INS P1 P2 Lc [Lc bytes of DATA] Le - The C-APDU is mapped onto the C-TPDU by cutting off the last byte (Le) of the body. b) Transport layer (C-TPDU) : CLA INS P1 P2 P3=Lc [Lc bytes of DATA] c) Terminal will receive SW1 and SW2 d) Upon receipt if values are (9x 00) , then Terminal issues [Get Response, P3 = Le ] to the UICC again e) This response, R_APDU : resp bytes, SW1, SW2 will be delivered to SE parent process eventually What this means , in short is, if you want to specify 'Le' bytes explicitly, make sure it is the last byte of body (data).

changing feature flag to 2.2? since we are still clarifying how we can verify/test this patch for 2.2.

(In reply to Siddartha P from comment #61) > I think, firstly we should fix my patch, and rename Lc to simply 'P3' to > avoid all confusion. I agree to to that. We also need to modify data argument attributes in iccExchangeAPDU. Right now we have [array, size_is(lc)], this does not allow me to set data to null and Lc/P3 to the expected response length and I'm getting the following error: >JavaScript Array does not have as many elements as indicated by size argument arg 7 [nsIIccProvider.iccExchangeAPDU So data should be nullable and independent of Lc/P3 value. At least in the idl, we could do APDU validation later on in the code. If I set p3 to expected response length and data to null directly in RILContentHelper the response is ok.

Comment on attachment 8523376 [details] [diff] [review] (v1.1) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl Review of attachment 8523376 [details] [diff] [review]: ----------------------------------------------------------------- Hi Sid, .idl part looks really good though comment 61 is a valid concern and a revision is required. I believe the approach for either m-c or 2.1 branch is almost the same, except some rebasing effort. I am more than happy to see this landing on m-c as long as we test the patch as we usually do. Thank you :) ::: dom/icc/interfaces/nsIIccProvider.idl @@ +141,5 @@ > + in octet cla, > + in octet ins, > + in octet p1, > + in octet p2, > + in unsigned long lc, Agree with comment 61. Let's rename it "p3" to avoid confusion. ::: dom/system/gonk/RILContentHelper.js @@ +1074,3 @@ > }, > > + iccExchangeAPDU: function(clientId, channel, cla, ins, p1, p2, lc, data, callback) { s/lc/p3 @@ +1082,5 @@ > + cla: cla, > + command: ins, > + p1: p1, > + p2: p2, > + p3: lc, s/lc/p3/ @@ +1536,5 @@ > + > + if (DEBUG) debug("Unable to add channelCbInterface for requestId : " + requestId); > + }, > + > + _getIccChannelCallback: function(requestId) { We should also delete the callback when the request is done. @@ +1685,5 @@ > + > + return bytes; > + }, > + > + _byte2hexString: function(array) { s/_byte2hexString/_byteToHexString/ @@ +1690,5 @@ > + let hexString = ""; > + let hex; > + > + for (let i = 0; i < array.length; i++) { > + hex = array[i].toString(16).toUpperCase(); Declare a variable when it's really needed, so let's declare "hex" here. @@ +1970,5 @@ > + let channel = message.channel; > + > + let callback = this._getIccChannelCallback(requestId); > + if (callback) { > + let notify = (!message.errorMsg) ? callback.notifyOpenChannelSuccess(channel) : nit: parenthesis around message.errorMsg not necessary @@ +1980,5 @@ > + let requestId = message.requestId; > + let callback = this._getIccChannelCallback(requestId); > + > + if (callback) { > + let notify = (!message.errorMsg) ? callback.notifyCloseChannelSuccess() : ditto @@ +1989,3 @@ > handleIccExchangeAPDU: function(message) { > + let requestId = message.requestId; > + let callback = this._getIccChannelCallback(requestId); Bail out earlier, please do: if (!callback) { return; } @@ +1989,5 @@ > handleIccExchangeAPDU: function(message) { > + let requestId = message.requestId; > + let callback = this._getIccChannelCallback(requestId); > + > + let responseBytes = (message.simResponse !== null && message.simResponse.length > 0) ? wrap at 80th character @@ +1991,5 @@ > + let callback = this._getIccChannelCallback(requestId); > + > + let responseBytes = (message.simResponse !== null && message.simResponse.length > 0) ? > + this._hexStringToBytes(message.simResponse) : null; > + let responseLength = (responseBytes !== null ) ? responseBytes.length : 0; It looks we check the very similar thing repeatedly. How about this: let responseBytes = null; let responseLength = 0; if (message.simResponse) { // responseBytes = this._hexStringToBytes(message.simResponse); responseLength = responseBytes.length; } @@ +1992,5 @@ > + > + let responseBytes = (message.simResponse !== null && message.simResponse.length > 0) ? > + this._hexStringToBytes(message.simResponse) : null; > + let responseLength = (responseBytes !== null ) ? responseBytes.length : 0; > + if (callback) { As we bail out earlier, we don't need this if-sentence.

Hi Hsinyi, Thanks for your comments. I have addressed all of them in v1.2 patch. However I also made another important change to the idl (specifically to one of iccExchangeAPDU arguments, changed the argument to 'in DOMString data,' instead of '[array, size_is(lc)] in octet data'. ) "[array, size_is(lc)] in octet data" : This may have multiple issues. Since we are locking the size of 'data' to 'lc' (now 'P3' in the current patch), we may not be able to support usecases like --> lc is set, data (lc bytes) is present and le is also present Lets says lc = 0x04, 'data' will contain 4 bytes. There is no way to send 'le' with the old proposed API. --> Now another usecase, consider only Le is present (say 0x02). Effectively 'p3' is 'Le' and data will be not set or say 'null'. Again we may have issues I think there are few such cases that old proposed API may not be extensible. With the current one, I just changed it to 'string' and therefore not worry about any specific length. Anyways Qualcomm RIL expects , a string only. So there is no need to convert in RILConetenHelper using byte2HexString() helpers. ------------------------------------------------------------------------- There is also another way of supporting all the 'le' and 'lc' usecases if we change the iccExchangeAPDU() args in the following way ... ... in unsigned long lc, [array, size_is(lc)] in octet data, in unsigned short le, callback); This ensures that we send all the required info to RILContentHelper and the onus is on RILContentHelper to compute 'P3' roughly something in this order: + iccExchangeAPDU: function(clientId, channel, cla, ins, p1, p2, lc, data, le, callback) { + let requestId = UUIDGenerator.generateUUID().toString(); + this._addIccChannelCallback(requestId, callback); + let apdu = ""; + let p3 = 0; + + // First check if lc is set and data is present + if (lc > 0 && !!data) { + p3 = lc; + apdu = this._byte2hexString(data); + } + + // check if le is set + if (le !== -1) { + // If 'p3' already set, then append 'le' to 'apdu' + if (p3 > 0) { + apdu += this._byte2hexString(le); + } else { // p3 is 'le' in case 'lc' is not set + p3 = le; + apdu = null; + } + } + + if (DEBUG) debug('P3 : ' + p3 + ', apdu: ' + apdu); + let apdu = { + cla: cla, + command: ins, + p1: p1, + p2: p2, + p3: p3, + data: apdu + }; + + //Potentially you need serialization here and can't pass the jsval through + cpmm.sendAsyncMessage("RIL:IccExchangeAPDU", { + clientId: clientId, + data: { + requestId: requestId, + channel: channel, + apdu: apdu + } + }); + }, I simply chose 'DOMString' over option 2. Please let me know your preference. Anyways I have uploaded for your review. Thanks

Comment on attachment 8528762 [details] [diff] [review] (v1.2) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl Review of attachment 8528762 [details] [diff] [review]: ----------------------------------------------------------------- Hello Sid, Your new change to iccExchangeAPDU looks good to me. Here is a further idea based on your new proposal - Can we just replace all current 'octet' type with 'long' as I've seen that's the type ril implementation uses, details inline, thank you! ::: dom/icc/interfaces/nsIIccProvider.idl @@ +45,5 @@ > + * Length of response's data field > + * @param data > + * Response's data field bytes > + */ > + void notifyExchangeAPDUResponse(in octet sw1, Sorry for the second thought: Per ril.h, the type of sw1 is Int. How about we use 'long'? @@ +46,5 @@ > + * @param data > + * Response's data field bytes > + */ > + void notifyExchangeAPDUResponse(in octet sw1, > + in octet sw2, ditto. @@ +48,5 @@ > + */ > + void notifyExchangeAPDUResponse(in octet sw1, > + in octet sw2, > + in unsigned long length, > + [array, size_is(length)] in octet data); Wondering can't we have the type DOMString instead of octect for 'data' in order to make the interface more consistent. @@ +138,4 @@ > > + void iccExchangeAPDU(in unsigned long clientId, > + in long channel, > + in octet cla, ditto. @@ +138,5 @@ > > + void iccExchangeAPDU(in unsigned long clientId, > + in long channel, > + in octet cla, > + in octet ins, ditto. @@ +139,5 @@ > + void iccExchangeAPDU(in unsigned long clientId, > + in long channel, > + in octet cla, > + in octet ins, > + in octet p1, ditto. @@ +140,5 @@ > + in long channel, > + in octet cla, > + in octet ins, > + in octet p1, > + in octet p2, ditto. @@ +142,5 @@ > + in octet ins, > + in octet p1, > + in octet p2, > + in unsigned long p3, > + in DOMString data, I am fine with DOMString, thank you. ::: dom/system/gonk/RILContentHelper.js @@ +1957,5 @@ > + let channel = message.channel; > + > + let callback = this._getIccChannelCallback(requestId); > + if (callback) { > + delete this._iccChannelCallback[requestId]; Can we move the delete operation into _getIccChannelCallback()? Or have you foreseen possible cases that we'd like to get callback but not to delete it?

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #66) > Comment on attachment 8528762 [details] [diff] [review] > (v1.2) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces > in nsIIccProvider.idl > > Review of attachment 8528762 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hello Sid, > Your new change to iccExchangeAPDU looks good to me. Here is a further idea > based on your new proposal - Can we just replace all current 'octet' type > with 'long' as I've seen that's the type ril implementation uses, details > inline, thank you! > > ::: dom/icc/interfaces/nsIIccProvider.idl > @@ +45,5 @@ > > + * Length of response's data field > > + * @param data > > + * Response's data field bytes > > + */ > > + void notifyExchangeAPDUResponse(in octet sw1, > > Sorry for the second thought: Per ril.h, the type of sw1 is Int. How about > we use 'long'? > Though I don't have an answer why ril.h have an 'ints' while say a char would have sufficed. Please refer to this spec: http://www.3gpp.org/ftp/tsg_t/TSG_T/TSGT_05/docs/PDFs/TP-99185.pdf, section 10.2 Command APDU Structure. All the specs say that 'cla' , 'ins' , 'p1' , 'p2' are 1 byte sized. In any case, this idl interfaces are one level above ril exchange, maybe it is still ok to leave them as 'octet' including status bytes sw1 & sw2. Just my opinion. What do you think ? > @@ +46,5 @@ > > + * @param data > > + * Response's data field bytes > > + */ > > + void notifyExchangeAPDUResponse(in octet sw1, > > + in octet sw2, > > ditto. > > @@ +48,5 @@ > > + */ > > + void notifyExchangeAPDUResponse(in octet sw1, > > + in octet sw2, > > + in unsigned long length, > > + [array, size_is(length)] in octet data); > > Wondering can't we have the type DOMString instead of octect for 'data' in > order to make the interface more consistent. > Agreed! My thoughts too. > @@ +138,4 @@ > > > > + void iccExchangeAPDU(in unsigned long clientId, > > + in long channel, > > + in octet cla, > > ditto. > > @@ +138,5 @@ > > > > + void iccExchangeAPDU(in unsigned long clientId, > > + in long channel, > > + in octet cla, > > + in octet ins, > > ditto. > > @@ +139,5 @@ > > + void iccExchangeAPDU(in unsigned long clientId, > > + in long channel, > > + in octet cla, > > + in octet ins, > > + in octet p1, > > ditto. > > @@ +140,5 @@ > > + in long channel, > > + in octet cla, > > + in octet ins, > > + in octet p1, > > + in octet p2, > > ditto. > > @@ +142,5 @@ > > + in octet ins, > > + in octet p1, > > + in octet p2, > > + in unsigned long p3, > > + in DOMString data, > > I am fine with DOMString, thank you. > > ::: dom/system/gonk/RILContentHelper.js > @@ +1957,5 @@ > > + let channel = message.channel; > > + > > + let callback = this._getIccChannelCallback(requestId); > > + if (callback) { > > + delete this._iccChannelCallback[requestId]; > > Can we move the delete operation into _getIccChannelCallback()? Or have you > foreseen possible cases that we'd like to get callback but not to delete it? Actually no such use-cases I have in mind. I have made the change suggested by you in the latest patch

Comment on attachment 8532993 [details] [diff] [review] (v1.3) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl Review of attachment 8532993 [details] [diff] [review]: ----------------------------------------------------------------- Hi Edgar, can you help to review the patches at the moment?

Comment on attachment 8532993 [details] [diff] [review] (v1.3) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl Review of attachment 8532993 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/interfaces/nsIIccProvider.idl @@ +44,5 @@ > + * @param data > + * Response's data > + */ > + void notifyExchangeAPDUResponse(in octet sw1, > + in octet sw2, Since specs say sw1 and sw2 are both 1 bytes size, I am ok with octet. @@ +137,5 @@ > + in long channel, > + in octet cla, > + in octet ins, > + in octet p1, > + in octet p2, ditto, I am fine with octet. @@ +138,5 @@ > + in octet cla, > + in octet ins, > + in octet p1, > + in octet p2, > + in unsigned long p3, I am wondering how does idl handle the case that APDU contains only header (CLA, INS, P1, P2). From the ril.h [1], it seems it could be achieved in ril level by assigning p3 to a negative value. Wondering should we use `short` for p3 [2] in idl, instead of `unsigned long`. [1] https://github.com/android/platform_hardware_ril/blob/master/include/telephony/ril.h#L410 [2] From the spec, http://www.3gpp.org/ftp/tsg_t/TSG_T/TSGT_05/docs/PDFs/TP-99185.pdf, table 10.1: Contents of Command, the lc and le are both 1 bytes if present, it seems short is enough.

Comment on attachment 8532993 [details] [diff] [review] (v1.3) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl Review of attachment 8532993 [details] [diff] [review]: ----------------------------------------------------------------- Hi Sid, The overall looks good, but there are still few questions need to be clarified. Please see comment #70. Thanks for your work. ::: dom/system/gonk/RILContentHelper.js @@ +1079,2 @@ > > + if (!data) if (!data) { ... } even if only one line in `if` block.

(In reply to Edgar Chen [:edgar][:echen] from comment #70) > Comment on attachment 8532993 [details] [diff] [review] > (v1.3) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces > in nsIIccProvider.idl > > Review of attachment 8532993 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/icc/interfaces/nsIIccProvider.idl > @@ +44,5 @@ > > + * @param data > > + * Response's data > > + */ > > + void notifyExchangeAPDUResponse(in octet sw1, > > + in octet sw2, > > Since specs say sw1 and sw2 are both 1 bytes size, I am ok with octet. > Ok > @@ +137,5 @@ > > + in long channel, > > + in octet cla, > > + in octet ins, > > + in octet p1, > > + in octet p2, > > ditto, I am fine with octet. > > @@ +138,5 @@ > > + in octet cla, > > + in octet ins, > > + in octet p1, > > + in octet p2, > > + in unsigned long p3, > > I am wondering how does idl handle the case that APDU contains only header > (CLA, INS, P1, P2). > From the ril.h [1], it seems it could be achieved in ril level by assigning > p3 to a negative value. As far as C-APDU is concerned this is Case 1: This is interpreted at T-APDU as only header i;e; Both command data ('data' & 'lc') & command response (le) are absent. Using our idl , we could do the following iccProvider.iccExchangeAPDU(clientId, channel,cla, ins, p1, p2, -1, null , callback); Not sure if I have addressed your concern. > Wondering should we use `short` for p3 [2] in idl, instead of `unsigned > long`. Agreed. Changed 'p3' to short > > [1] > https://github.com/android/platform_hardware_ril/blob/master/include/ > telephony/ril.h#L410 > [2] From the spec, > http://www.3gpp.org/ftp/tsg_t/TSG_T/TSGT_05/docs/PDFs/TP-99185.pdf, table > 10.1: Contents of Command, the lc and le are both 1 bytes if present, it > seems short is enough.

Comment on attachment 8534077 [details] [diff] [review] (v1.4) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl Review of attachment 8534077 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Siddartha P from comment #73) > As far as C-APDU is concerned this is Case 1: This is interpreted at T-APDU > as only header i;e; Both command data ('data' & 'lc') & command response > (le) are absent. Using our idl , we could do the following > > iccProvider.iccExchangeAPDU(clientId, channel,cla, ins, p1, p2, -1, null , > callback); Not sure if I have addressed your concern. Thanks for explanation, it's clear to me. :) Since p3 could be a negative value for this case, it should not be `unsigned` in idl. Please help to address this. And if you could add some comments for it, that will be really great. ::: dom/icc/interfaces/nsIIccProvider.idl @@ +138,5 @@ > + in octet cla, > + in octet ins, > + in octet p1, > + in octet p2, > + in unsigned short p3, r=me with `unsigned` removed.

Hi Edgar, Thanks for your comments. In order to land this patch on REL 2.1 branch, there is another dependency : https://bugzilla.mozilla.org/show_bug.cgi?id=1030602 , we would need this change as well. (Note that this is already landed on the main line ). For SE stack development, we are based off Qualcomm's release on v2.1 branch. Could you please let us know how we can proceed with these changes / landing ?

Thanks for catching the review up, Edgar :)

(In reply to Siddartha P from comment #76) > Hi Edgar, Thanks for your comments. In order to land this patch on REL 2.1 > branch, there is another dependency : > https://bugzilla.mozilla.org/show_bug.cgi?id=1030602 , we would need this > change as well. (Note that this is already landed on the main line ). For SE > stack development, we are based off Qualcomm's release on v2.1 branch. > Could you please let us know how we can proceed with these changes / landing > ? Hey, v2.1 branch is now code complete. We wouldn't uplift patches unless for critical fixes. (Purpose is to maintain stability.) Can we focus the SE implementation on current master/central (v2.2)?

Hi Sid, let commit the patch to m-c first. The patch is ready to go right?

Yes this patch is ready to go now. I will get my tryServer builds going and update the results shortly!

Let's assume we can have this landed after new year holiday :) Feel free to correct me.

Uploading a new patch for m-c. As is the case, RILContentHelper.js and nsIIccProvider.idl are different on m-c from v2.1. Also try server is in progress here : https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2930bf6e3e2f Do we have to write any test cases for the try server results to be successful ?

Hi Wesley , Vincent Could you tell me how to go about landing these changes on m-c ? I have also uploaded the rebased patch 'New rebased patch for m-c ' for feedback ?

Hi Sid, I think you can run that test case manually in your local environment. It looks like nsIIccChannelCallback is missing when you push the patch to try server. One of the main reason that we need to run try server is to prevent breaking m-c tree when submit a new patch.

Uploaded the re-based patch. I guess this needs another look by the reviewers. Here are try results: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=04ae57c980f3 ( Mostly green)

(In reply to Vincent Chang[:vchang] from comment #84) > Hi Sid, I think you can run that test case manually in your local > environment. It looks like nsIIccChannelCallback is missing when you push > the patch to try server. One of the main reason that we need to run try > server is to prevent breaking m-c tree when submit a new patch. Ok. I uploaded the rebased patch for another feedback (just in case), since I have not tested this patch on m-c for the obvious reason.

Comment on attachment 8538931 [details] [diff] [review] New rebased patch for m-c. Forwarding f? to me, I can help to take a look.

Comment on attachment 8538931 [details] [diff] [review] New rebased patch for m-c. Review of attachment 8538931 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thank you.

Hi Edgar, I assume that this patch is good to go now. Since try results are also good, should I go ahead and do 'check-in' needed for this ? Thanks!

(In reply to Siddartha P from comment #89) > Hi Edgar, > I assume that this patch is good to go now. Since try results are also good, > should I go ahead and do 'check-in' needed for this ? Thanks! Go ahead. Thanks for your work.

Thanks Edgar and Sid!

Hi , this patch failed to apply: patching file dom/icc/interfaces/nsIIccProvider.idl Hunk #1 FAILED at 11 Hunk #2 succeeded at 161 with fuzz 2 (offset 5 lines). 1 out of 2 hunks FAILED -- saving rejects to file dom/icc/interfaces/nsIIccProvider.idl.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh 0001-Bug-1081789-Redefine-IccOpenChannel-IccCloseChannel.patch could you take a look, thanks!

Hi , I assume that the patch that is used on try is: "New rebased patch for m-c." and not the patch '(v1.4a)...' one. If yes, I will recheck and provide an update. Thanks

(In reply to Siddartha P from comment #93) > Hi , I assume that the patch that is used on try is: "New rebased patch for > m-c." and not the patch '(v1.4a)...' one. If yes, I will recheck and provide > an update. > Thanks yes was trying to land on b2g-inbound

Will update try results shortly!

Hi Edgar, One more time, could you please quickly skim through the latest patch rebased for in-bound. Also try results look ok for most except 'B2G ICS Emulator opt' and 'B2G ICS Emulator debug' https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc0eaa66b46c. Could you please suggest if I am missing anything here ?

Comment on attachment 8542403 [details] [diff] [review] New re-based patch for Inbound Review of attachment 8542403 [details] [diff] [review]: ----------------------------------------------------------------- I guess the try result in B2G ICS Emulator was busted due to below error. ::: dom/system/gonk/RILContentHelper.js @@ +410,5 @@ > }); > }, > > + iccOpenChannel: function(clientId, aid, callback) { > + let requestId = UUIDGenerator.generateUUID().toString(); You forget to define the lazy getter for `UUIDGenerator`. XPCOMUtils.defineLazyServiceGetter(this, "UUIDGenerator", "@mozilla.org/uuid-generator;1", "nsIUUIDGenerator"); @@ +566,5 @@ > if (DEBUG) debug("Unregistered listener: " + listener); > } > }, > > +_ addIccChannelCallback: function(requestId, channelCb) { Should be `_addIccChangelCallback`.

Hi Edgar, Hopefully one last time, does these try results look promising now ? https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b0f30c0bbf88 Thanks for your time

Comment on attachment 8543538 [details] [diff] [review] New re-based patch for Inbound Review of attachment 8543538 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thank you. And I will help to merge this patch into inbound once bug 1098210 is re-landed.

Thanks, Edgar.

(In reply to Edgar Chen [:edgar][:echen] from comment #101) > https://hg.mozilla.org/integration/b2g-inbound/rev/ab9fb3092d9f Thanks Edgar