Closed
Bug 772357
Opened 12 years ago
Closed 12 years ago
B2G RIL: Support network depersonalization
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
People
(Reporter: jaoo, Assigned: jaoo)
References
Details
(Keywords: feature, Whiteboard: [LOE:S][WebAPI:P0])
Attachments
(4 files, 5 obsolete files)
61.65 KB,
text/plain
|
Details | |
1.17 KB,
patch
|
philikon
:
review+
|
Details | Diff | Splinter Review |
3.64 KB,
patch
|
philikon
:
review+
|
Details | Diff | Splinter Review |
278.19 KB,
text/plain
|
Details |
This capability is about allowing the user to deactivate that the phone is personalised to a particular network (carrier). The network personalisation feature is controlled by a Network Control Key, (NCK) which has to be entered into the mobile equipment (ME) in order to network de-personalise it. For more information please take a look at 3GPP TS 22.022 document (section 5) [1] and [2]. [1] http://www.3gpp.org/ftp/Specs/html-info/22022.htm [2] http://en.wikipedia.org/wiki/SIM_lock
Assignee | ||
Comment 1•12 years ago
|
||
Here is a WIP for this capability. The implementation is pretty easy and looks similar -for example- to the SIM PIN lock one. I just ask for feedback especially from philikon and mvines. The tricky part here is how to test it because we need a phone with that network personalisation enabled.
Attachment #640530 -
Flags: feedback?(philipp)
Attachment #640530 -
Flags: feedback?(mvines)
Comment 2•12 years ago
|
||
Comment on attachment 640530 [details] [diff] [review] WIP v1 Review of attachment 640530 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +902,5 @@ > + * > + * @param requestId > + * Request Id from RadioInterfaceLayer. > + */ > + getICCNetworkControlKey: function getICCNetworkControlKey(options) { Not sure I like the naming of this method. AFAICT, this fetches the *state* of the network control key, not the actual network control key, right? @@ +2851,5 @@ > + this.sendDOMMessage({type: "iccunlockcardlock", > + lockType: "nck", > + result: options.rilRequestError == 0 ? true : false, > + retryCount: length ? Buf.readUint32List()[0] : -1, > + requestId: options.requestId}); Please reuse the `options` object, that way you don't have to copy over stuff like `requestId` and `lockType`. Also, I suggest renaming s/result/success/. Speaking of the success flag, I realize that error handling for REQUEST_QUERY_FACILITY_LOCK is pretty much non-existent. We should deal with that in a follow-up. Just silently dropping errors is not a good idea, we should dispatch the error event on the DOMRequest.
Attachment #640530 -
Flags: feedback?(philipp) → feedback+
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #2) > > + getICCNetworkControlKey: function getICCNetworkControlKey(options) { > > Not sure I like the naming of this method. AFAICT, this fetches the *state* > of the network control key, not the actual network control key, right? Yes, you are right. The reason of that name is because for fetching the state of the SIM PIN lock the naming of that function is similar 'getICCPinLock'. It does not retrieve the value of that PIN but the *state*. Should I change the name of the function to something like 'getICCNetworkControlKeyState'? > @@ +2851,5 @@ > > + this.sendDOMMessage({type: "iccunlockcardlock", > > + lockType: "nck", > > + result: options.rilRequestError == 0 ? true : false, > > + retryCount: length ? Buf.readUint32List()[0] : -1, > > + requestId: options.requestId}); > > Please reuse the `options` object, that way you don't have to copy over > stuff like `requestId` and `lockType`. I did the same here that what is done in functions REQUEST_ENTER_* for maintaining the same way of handling the response. Is that what you suggest? +RIL[REQUEST_ENTER_NETWORK_DEPERSONALIZATION] = + function REQUEST_ENTER_NETWORK_DEPERSONALIZATION(length, options) { + options.type = "iccunlockcardlock"; + options.result = options.rilRequestError == 0 ? true : false; + options.retryCount = length ? Buf.readUint32List()[0] : -1; + this.sendDOMMessage(options); +};
Comment 4•12 years ago
|
||
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #3) > (In reply to Philipp von Weitershausen [:philikon] from comment #2) > > > + getICCNetworkControlKey: function getICCNetworkControlKey(options) { > > > > Not sure I like the naming of this method. AFAICT, this fetches the *state* > > of the network control key, not the actual network control key, right? > > Yes, you are right. The reason of that name is because for fetching the > state of the SIM PIN lock the naming of that function is similar > 'getICCPinLock'. It does not retrieve the value of that PIN but the *state*. > Should I change the name of the function to something like > 'getICCNetworkControlKeyState'? Hmm, I see. It's probably fine. I would almost want to rename `getICCPinLock` to `getICCPinLockState`, but it's probably not worth the trouble. > I did the same here that what is done in functions REQUEST_ENTER_* for > maintaining the same way of handling the response. Sure. They should be fixed, too, then. (In a follow-up bug. Probably the one I was talking about in comment 2 regarding error handling. Can you file that please?) > Is that what you suggest? > > +RIL[REQUEST_ENTER_NETWORK_DEPERSONALIZATION] = > + function REQUEST_ENTER_NETWORK_DEPERSONALIZATION(length, options) { > + options.type = "iccunlockcardlock"; > + options.result = options.rilRequestError == 0 ? true : false; > + options.retryCount = length ? Buf.readUint32List()[0] : -1; > + this.sendDOMMessage(options); > +}; Yes, although s/result/success/ and error handling, please!
Assignee | ||
Comment 5•12 years ago
|
||
This patch depends on bug 774114. This is still a WIP patch. It needs to be tested on a phone. Just need some feedback before continuing.
Attachment #640530 -
Attachment is obsolete: true
Attachment #640530 -
Flags: feedback?(mvines)
Comment 6•12 years ago
|
||
Confirmed with jaoo. This one should be a MUST have for v1.
Comment 7•12 years ago
|
||
(In reply to khu from comment #6) > Confirmed with jaoo. > This one should be a MUST have for v1. Nom'ing for basecamp then.
blocking-basecamp: --- → ?
Updated•12 years ago
|
blocking-basecamp: ? → +
Assignee | ||
Comment 8•12 years ago
|
||
I'll spend some time on this bug and bug 774114 during the rest of the week.
Assignee | ||
Comment 9•12 years ago
|
||
Yoshi, Michael: need some feedback from you. After testing this patch on otoro I have noticed that It does not work at all. I believe the logic implemented here for fetching the state of the network control key and for disabling the SIM-lock capability built into a phone is right but it does not work since that capability is not built into the otoro by its manufacturer. Is that possible? Every time I test I get the same error, generic failure.
Attachment #642422 -
Attachment is obsolete: true
Attachment #650581 -
Flags: feedback?(mvines)
Attachment #650581 -
Flags: feedback?(allstars.chh)
Comment on attachment 650581 [details] [diff] [review] WIP v3 Review of attachment 650581 [details] [diff] [review]: ----------------------------------------------------------------- Hi jaoo, I think your parcel is wrong here. Could you check that in ril.h again? Also you can take the reference code from codeaurora [1]. [1]: http://bit.ly/Nr4h7c Thanks ::: dom/system/gonk/ril_worker.js @@ +857,5 @@ > + * Requests a network personalization be deactivated. > + * > + * @param depersonalizationType > + * Integer indicating the network personalization be deactivated. > + * @param code code or pin? @@ +864,5 @@ > + enterICCDepersonalization: function enterICCDepersonalization(options) { > + Buf.newParcel(REQUEST_ENTER_NETWORK_DEPERSONALIZATION, options); > + Buf.writeUint32(RILQUIRKS_V5_LEGACY ? 1 : 2); > + Buf.writeUint32(options.depersonalizationType); > + Buf.writeString(options.pin); Your parcel is completely wrong, I think. Could you check the ril.h for otoro ? you should take a look at RIL_Depersonalization. it's a string and an int. Also even you're writing for galaxy-s2 or ril v5, the data is a **char, it is also wrong here. So you don't have to supply depersonalizationType either. @@ +866,5 @@ > + Buf.writeUint32(RILQUIRKS_V5_LEGACY ? 1 : 2); > + Buf.writeUint32(options.depersonalizationType); > + Buf.writeString(options.pin); > + if (!RILQUIRKS_V5_LEGACY) { > + Buf.writeString(options.aid ? options.aid : this.aid); DEPERSONALIZATION doesn't have aid. You don't have to provide it here.
Attachment #650581 -
Flags: feedback?(allstars.chh) → feedback-
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #10) > Comment on attachment 650581 [details] [diff] [review] > WIP v3 > > Review of attachment 650581 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hi jaoo, > I think your parcel is wrong here. Thanks for comments, I'll poke around how to send the parcel properly. Actually that part of code hasn't been tested yet. I have started my test fetching the state of the network control key and in that test is where I get the generic failure error. So the earlier question that was "The logic does not work since that capability is not built into the otoro by its manufacturer. Is that possible?" is still not clear to me. Thanks in advance if you have further comments.
Comment on attachment 650581 [details] [diff] [review] WIP v3 Review of attachment 650581 [details] [diff] [review]: ----------------------------------------------------------------- Hi, jaoo Sorry I didn't read your comment carefully. I've applied your patch and I got the same result with you. still don't know why. from 'adb logcat -b radio' I also see "received invalid paramets in RIL_REQUEST_QUERY_FACILITY_LOCK" BTW, recently the message passing between RadioInterfaceLayer and ril_worker has been changed, so you need to change to 'type' to 'rilMessageType' when sending those msgs. thanks ::: dom/system/gonk/ril_worker.js @@ +998,5 @@ > options.facility = ICC_CB_FACILITY_SIM; > options.password = ""; // For query no need to provide pin. > options.serviceClass = ICC_SERVICE_CLASS_VOICE | > ICC_SERVICE_CLASS_DATA | > ICC_SERVICE_CLASS_FAX, Can you help to correct this for me? It should end with semicolon. Sorry for my typo @@ +1014,5 @@ > + options.facility = ICC_CB_FACILITY_NETWORK_PERSONALIZATION; > + options.password = ""; // For query no need to provide pin. > + options.serviceClass = ICC_SERVICE_CLASS_VOICE | > + ICC_SERVICE_CLASS_DATA | > + ICC_SERVICE_CLASS_FAX, also here
Attachment #650581 -
Flags: feedback- → feedback+
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #12) > Comment on attachment 650581 [details] [diff] [review] > WIP v3 > > Review of attachment 650581 [details] [diff] [review]: > ----------------------------------------------------------------- > Can you help to correct this for me? > It should end with semicolon. > Sorry for my typo > > @@ +1014,5 @@ > > + options.facility = ICC_CB_FACILITY_NETWORK_PERSONALIZATION; > > + options.password = ""; // For query no need to provide pin. > > + options.serviceClass = ICC_SERVICE_CLASS_VOICE | > > + ICC_SERVICE_CLASS_DATA | > > + ICC_SERVICE_CLASS_FAX, > > also here Sure, done in bug 774114 since that bug is about refactoring and such stuff.
Assignee | ||
Comment 14•12 years ago
|
||
I believe the logic implemented here is right but tests report "Generic Failure" every time I try to fetch the state of the lock. I have requested a sample to my local contact for testing on a otoro with the SIM-lock capability enabled on the device. I'll keep you updated.
Attachment #650581 -
Attachment is obsolete: true
Attachment #650581 -
Flags: feedback?(mvines)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [LOE:S]
Updated•12 years ago
|
Whiteboard: [LOE:S] → [LOE:S][WebAPI:P0]
Assignee | ||
Comment 15•12 years ago
|
||
Just received an otoro with a network personalization enabled. Here is a logcat radio output from a SIM-Locked phone (otoro) running Android. It could give us some hints about the behavior of the phone in such circumstances and could be useful for testing and finishing this work.
Assignee | ||
Comment 16•12 years ago
|
||
New WIP. If the phone is personalised to a particular network (carrier) and the user inserts an ICC card from another carrier `nsIDOMMozMobileConnection.cardState` indicates that the state of the device's ICC card is `networkLocked`. This patch allows the user to supply the depersonalization code through nsIDOMMozMobileConnection by using the `unlockCardLock` function.
Attachment #651341 -
Attachment is obsolete: true
Assignee | ||
Comment 17•12 years ago
|
||
This patch updates the nsIDOMMozMobileConnection specs with some useful information about how to supply the code for disabling the lock from a phone that is personalised to a particular network (carrier).
Assignee | ||
Updated•12 years ago
|
Attachment #662591 -
Attachment description: WIP v5 - Part 1: RIL implementation. → Part 1: RIL implementation. - WIP v5.
Comment 18•12 years ago
|
||
Do the patches here need reviewers? Will this work make the feature freeze?
Assignee | ||
Comment 19•12 years ago
|
||
I need to test these patches on a phone or on the emulator before requesting review. We have a problem here because we need a SIM-locked phone for testing and this capability is not built into the otoro by the phone manufacturer. In that situation is difficult to test current patches, request review and resolve this bug. On the other hand AFAIK the emulator lacks of this support :(. A few days ago I received a SIM-locked GB-based otoro phone. Tomorrow I'll try to build B2G for that device and test the patches but since we are now ICS I have to recover the old GB-based build environment for B2G and I could run into build problems and won't be able to test them. Let me do that tomorrow and I'll update you with my progress.
Comment 20•12 years ago
|
||
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #19) > I need to test these patches on a phone or on the emulator before requesting > review. We have a problem here because we need a SIM-locked phone for > testing and this capability is not built into the otoro by the phone > manufacturer. In that situation is difficult to test current patches, > request review and resolve this bug. On the other hand AFAIK the emulator > lacks of this support :(. How about we land what you have now and deal with any bugfixes later? So long as the feature doesn't introduce any risk (I would say it doesn't introduce much risk), there should be no reason not to land this IMHO. (I do wonder if we're going to ship with a SIM lock enabled, btw. We said this is a must-have for v1, but is it really? Just want to double-check!) > A few days ago I received a SIM-locked GB-based otoro phone. Tomorrow I'll > try to build B2G for that device and test the patches but since we are now > ICS I have to recover the old GB-based build environment for B2G and I could > run into build problems and won't be able to test them. Let me do that > tomorrow and I'll update you with my progress. Is it an Otoro with GB android or just the GB modem firmware? The current build system works just fine with the GB modem firmware, you just need to swap out your backup-otoro directory for one with the GB-based blobs and reconfigure. Please ping me if you need any help!
Comment 21•12 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #20) > (I do wonder if we're going to ship with a SIM lock enabled, btw. We said > this is a must-have for v1, but is it really? Just want to double-check!) > The activation of this feature will depend on the country and operator we want to launch. ie. Brasil Vivo do not ask for SIM lock while any other country in LATAM with Telefonica will ask for it. We should prepare the platform for its support.
Assignee | ||
Comment 22•12 years ago
|
||
Attachment #662591 -
Attachment is obsolete: true
Attachment #665405 -
Flags: review?(philipp)
Assignee | ||
Updated•12 years ago
|
Attachment #662593 -
Flags: review?(philipp)
Assignee | ||
Comment 23•12 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #20) > How about we land what you have now and deal with any bugfixes later? So > long as the feature doesn't introduce any risk (I would say it doesn't > introduce much risk), there should be no reason not to land this IMHO. SGTM, I'm afraid we only have that solution for this issue. You're right this code doesn't introduce any risk. Doesn't broke the RIL at all and just reports `ERROR_GENERIC_FAILURE` if we try to unlock the lock. > Is it an Otoro with GB android or just the GB modem firmware? The current > build system works just fine with the GB modem firmware, you just need to > swap out your backup-otoro directory for one with the GB-based blobs and > reconfigure. Please ping me if you need any help! Thanks for your help! It's an Otoro with GB Android (SIM-locked for -a Spanish Carrier- called YOIGO). The current build system works but B2G doesn't boot properly and it causes I cannot test. It's a pain to waste more time on this and IMHO it doesn't maks sense to test it for GB. I've requested an ICS-based SIM-locked phone to my local contact from otoro but It'll take some time. Landing what I have now and deal with any bugfixes later sounds good to me too.
Assignee | ||
Comment 24•12 years ago
|
||
Current build system works for GB but B2G doesn't boot properly and it causes I cannot test. Please, find attached the logcat output if you guys are curious.
Updated•12 years ago
|
Attachment #665405 -
Flags: review?(philipp) → review+
Comment 25•12 years ago
|
||
Comment on attachment 662593 [details] [diff] [review] Part 2: Update nsIDOMMozMobileConnection specs. - v1. Review of attachment 662593 [details] [diff] [review]: ----------------------------------------------------------------- Probably want to collapse this with the other patch.
Attachment #662593 -
Flags: review?(philipp) → review+
Assignee | ||
Comment 26•12 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #25) > Comment on attachment 662593 [details] [diff] [review] > Part 2: Update nsIDOMMozMobileConnection specs. - v1. > > Review of attachment 662593 [details] [diff] [review]: > ----------------------------------------------------------------- > > Probably want to collapse this with the other patch. Sure, done.
Assignee | ||
Comment 27•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3d7c4fc731f
Comment 28•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c3d7c4fc731f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in
before you can comment on or make changes to this bug.
Description
•