Closed
Bug 714973
Opened 13 years ago
Closed 12 years ago
Emergency calls on B2G
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: qdot, Assigned: jaoo)
References
Details
Attachments
(1 file, 8 obsolete files)
10.63 KB,
patch
|
philikon
:
review+
|
Details | Diff | Splinter Review |
We need to be able to make emergency calls without a SIM card on B2G. I believe this is a legal regulation in quite a few nations for cell phones.
Reporter | ||
Updated•13 years ago
|
Blocks: b2g-telephony
Assignee | ||
Comment 1•13 years ago
|
||
I would like to take this bug and start working on it. Is anyone already working on it?
Assignee | ||
Comment 3•13 years ago
|
||
This patch adds the possibility of making emergency calls within RIL's state machine implemented between RadioLayerInterface and ril_worker workers. Emergency calls are enabled (without SIM) when the registration state is equal to 10, 11, 12, or 14. The emergency call could be established by using the RIL_REQUEST_DIAL_EMERGENCY_CALL parcel. It seems there is a set of hidden parcels that do not appears in the RIL interface definition (ril.h). I discovered that parcel by looking into the Android logs during an emergency call in a S2 running a stock ROM. This funtionality could be tested through the Gaia/OWD dialer by dialing an emergency number (It could be different in every countries).
Attachment #604972 -
Flags: feedback?(philipp)
Assignee | ||
Comment 4•13 years ago
|
||
This patch add the 'emergencyCallsPossible' information to existing exposed information through the WebMobileConnection interface.
Attachment #604974 -
Flags: feedback?(philipp)
Assignee | ||
Comment 5•13 years ago
|
||
Previous attachments are work-in-progress patches. Further work is needed to test it on Akami (especially the 'hidden' RIL_REQUEST_DIAL_EMERGENCY_CALL parcel). My idea is to deal with emergency calls through current Telephony API without any addition or modification. Emergency calls will be allowed at any time (with SIM present and ready) and handled as an usual call. Emergency calls are possible when the device is not connected (SIM absent, pin puk required, etc.) and registration state is equal to 10, 11, 12 or 14. This information will be exposed through the WebMobileConnection interface (emergencyCallsPossible). In this situation any call will be handled as an emergency call but always by using the dial function of WebTelephony. Thoughts? Android saves a list of emergency numbers in a system property. Typical names for this property are: - ro.ril.oem.nosim.ecclist - ril.ecclist - ro.ril.ecclist - ro.ril.domestic_ecclist - persist.ril.ecclist - ro.ril.oem.ecclist So the dial number used in any emergency call has to be any of those included in this list. Note these patches apply on top of cgjones/m-c git fork. Waiting to someone willing to upstream everything that was landed to cgjones/m-c during the work week. Once done I will be happy to change the pacthes to hg-mq ones.
Version: unspecified → Trunk
Assignee | ||
Updated•13 years ago
|
Blocks: webmobileconnection
Updated•13 years ago
|
Summary: [b2g] Emergency calls on B2G → Emergency calls on B2G
Comment 6•13 years ago
|
||
Thanks for the patches, José! I will take a look. Formally we will incorporate the emergencyCallsPossible flag into the final WebMobileConnection implementation, so Part 2 will not be done here, nor does this bug block WebMobileConnection in any way.
No longer blocks: webmobileconnection
Comment 7•13 years ago
|
||
Comment on attachment 604972 [details] [diff] [review] Part 1 (WIP v1) - Add emergency calls to RIL's state machine. Review of attachment 604972 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/b2g/RadioInterfaceLayer.js @@ +205,5 @@ > + } > + this.radioState.emergencyCallsPossible = > + (state.regState >= RIL.NETWORK_CREG_STATE_NOT_SEARCHING_EMERGENCY_CALLS && > + state.regState <= RIL.NETWORK_CREG_STATE_UNKNOWN_EMERGENCY_CALLS) ? > + true : false; This might be easier to read with by initializing `state.regState` to a value and then setting it to the other based on the condition in an `if` statement. I don't think the logic you're implementing here is quite correct. Unless the `emergencyCallsPossible` flag should only ever be looked at when `connected` is false? This is something we should discuss on dev-webapi because it has implications for WebMobileConnection. ::: dom/system/b2g/ril_consts.js @@ +141,4 @@ > const REQUEST_SET_SMSC_ADDRESS = 101; > const REQUEST_REPORT_SMS_MEMORY_STATUS = 102; > const REQUEST_REPORT_STK_SERVICE_IS_RUNNING = 103; > +const REQUEST_DIAL_EMERGENCY_CALL = 10016; Please retain the separating line before RESPONSE_TYPE_SOLICITED. Also, it seems like this is a non-standard addition to the RIL... which devices support it? ::: dom/system/b2g/ril_worker.js @@ +67,4 @@ > // We leave this as 'undefined' instead of setting it to 'false'. That > // way an outer scope can define it to 'true' (e.g. for testing purposes) > // without us overriding that here. > +let DEBUG = true; Please revert this. :) @@ +1317,5 @@ > RIL[REQUEST_REPORT_SMS_MEMORY_STATUS] = null; > RIL[REQUEST_REPORT_STK_SERVICE_IS_RUNNING] = null; > +RIL[REQUEST_DIAL_EMERGENCY_CALL] = function REQUEST_DIAL_EMERGENCY_CALL(length) { > + Phone.onDialEmergencyCall(); > +}; Let's not add empty methods anymore. This was kind of anti-pattern that we developed. I am working on cleaning these up. @@ +2053,5 @@ > RIL.getDataCallList(); > }, > > + onDialEmergencyCall: function onDialEmergencyCall() { > + }, See above. Please remove this. @@ +2298,5 @@ > + */ > + _isEmergencyNumber: function _isEmergencyNumber(number) { > + // TODO. > + return true; > + }, Obviously that still needs to be done, e.g. in a second part to this patch, or in a follow-up... Do you know yet how we can check? Also, if we have to ask rild, how can this be a synchronous call? -- Would like to know more about the details here!
Attachment #604972 -
Flags: feedback?(philipp)
Updated•13 years ago
|
Attachment #604974 -
Flags: feedback?(philipp)
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #7) > Comment on attachment 604972 [details] [diff] [review] > Part 1 (WIP v1) - Add emergency calls to RIL's state machine. > > Review of attachment 604972 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/b2g/RadioInterfaceLayer.js > @@ +205,5 @@ > > + } > > + this.radioState.emergencyCallsPossible = > > + (state.regState >= RIL.NETWORK_CREG_STATE_NOT_SEARCHING_EMERGENCY_CALLS && > > + state.regState <= RIL.NETWORK_CREG_STATE_UNKNOWN_EMERGENCY_CALLS) ? > > + true : false; > > This might be easier to read with by initializing `state.regState` to a > value and then setting it to the other based on the condition in an `if` > statement. Yes, you are right. I have changed that to: + this.radioState.emergencyCallsOnly = + (state.regState != RIL.NETWORK_CREG_STATE_REGISTERED_HOME); This is because to be able to dial emergency numbers -without SIM- the phone has to reach a registration state 10, 12, 13, or 14. That is the case for S2 (it reaches inmediately state 13 without SIM) but not for Akami. On akami device we never reach any of them. Our friends at Qualcomm have told me that they really don't differentiate between states like REGISTRATION_STATE 2 and 12. The modem also defaults to emergency mode until the SIM is validated at boot. Thoughts? > > I don't think the logic you're implementing here is quite correct. Unless > the `emergencyCallsPossible` flag should only ever be looked at when > `connected` is false? This is something we should discuss on dev-webapi > because it has implications for WebMobileConnection. Discussed at https://bugzilla.mozilla.org/show_bug.cgi?id=729173#c20. > > ::: dom/system/b2g/ril_consts.js > @@ +141,4 @@ > > const REQUEST_SET_SMSC_ADDRESS = 101; > > const REQUEST_REPORT_SMS_MEMORY_STATUS = 102; > > const REQUEST_REPORT_STK_SERVICE_IS_RUNNING = 103; > > +const REQUEST_DIAL_EMERGENCY_CALL = 10016; > > Please retain the separating line before RESPONSE_TYPE_SOLICITED. Done! > > Also, it seems like this is a non-standard addition to the RIL... which > devices support it? AFAIK only on Samsung Galaxy S2. This is a problem for us. There are different behaviours in every devices tested. On S2 there is an special RIL request type for emergency calls. This is a non-standard addition to the RIL (see [1]). On akami we can deal with emergency calls by using the REQUEST_DIAL RIL request type. IMHO the best choice for implementing this is to deal with it as a quirk. > > ::: dom/system/b2g/ril_worker.js > @@ +67,4 @@ > > // We leave this as 'undefined' instead of setting it to 'false'. That > > // way an outer scope can define it to 'true' (e.g. for testing purposes) > > // without us overriding that here. > > +let DEBUG = true; > > Please revert this. :) Done! > > @@ +1317,5 @@ > > RIL[REQUEST_REPORT_SMS_MEMORY_STATUS] = null; > > RIL[REQUEST_REPORT_STK_SERVICE_IS_RUNNING] = null; > > +RIL[REQUEST_DIAL_EMERGENCY_CALL] = function REQUEST_DIAL_EMERGENCY_CALL(length) { > > + Phone.onDialEmergencyCall(); > > +}; > > Let's not add empty methods anymore. This was kind of anti-pattern that we > developed. I am working on cleaning these up. > > @@ +2053,5 @@ > > RIL.getDataCallList(); > > }, > > > > + onDialEmergencyCall: function onDialEmergencyCall() { > > + }, > > See above. Please remove this. Ok, done! > > @@ +2298,5 @@ > > + */ > > + _isEmergencyNumber: function _isEmergencyNumber(number) { > > + // TODO. > > + return true; > > + }, > > Obviously that still needs to be done, e.g. in a second part to this patch, > or in a follow-up... Do you know yet how we can check? Also, if we have to > ask rild, how can this be a synchronous call? -- Would like to know more > about the details here! The ril.ecclist property is updated by rild with the current list of emergency numbers. The idea is to retrieve that list and check if the given number matches with any of those contained within the list. [1] http://groups.google.com/group/android-porting/browse_thread/thread/d426ccefa561c419
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #604972 -
Attachment is obsolete: true
Attachment #604974 -
Attachment is obsolete: true
Attachment #610071 -
Flags: review?(philipp)
Comment 10•12 years ago
|
||
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #8) > + this.radioState.emergencyCallsOnly = > + (state.regState != RIL.NETWORK_CREG_STATE_REGISTERED_HOME); > > This is because to be able to dial emergency numbers -without SIM- the phone > has to reach a registration state 10, 12, 13, or 14. That is the case for S2 > (it reaches inmediately state 13 without SIM) but not for Akami. On akami > device we never reach any of them. Our friends at Qualcomm have told me that > they really don't differentiate between states like REGISTRATION_STATE 2 and > 12. The modem also defaults to emergency mode until the SIM is validated at > boot. Thoughts? That means the Akami does its own thing. We should cover this in a RILQUIRK constant and explicitly call it out. Otherwise what's the point of the emergencyCallsOnly at all? We should look at the 1x states on all other spec-compliant devices. > > Also, it seems like this is a non-standard addition to the RIL... which > > devices support it? > > AFAIK only on Samsung Galaxy S2. This is a problem for us. There are > different behaviours in every devices tested. On S2 there is an special RIL > request type for emergency calls. This is a non-standard addition to the RIL > (see [1]). On akami we can deal with emergency calls by using the > REQUEST_DIAL RIL request type. IMHO the best choice for implementing this is > to deal with it as a quirk. I agree. So please do that and add a RILQUIRK constant.
Comment 11•12 years ago
|
||
Comment on attachment 610071 [details] [diff] [review] (WIP v2) - Add emergency calls to RIL's state machine. Review of attachment 610071 [details] [diff] [review]: ----------------------------------------------------------------- This patch has a lot of unrelated whitespace changes. Please remove them. ::: dom/system/gonk/RadioInterfaceLayer.js @@ +203,5 @@ > + return; > + } > + > + this.radioState.emergencyCallsOnly = > + (state.regState != RIL.NETWORK_CREG_STATE_REGISTERED_HOME); See comment 10 about making this dependent on a RILQUIRK. For that reason, we probably want to compute emergencyCallsOnly in ril_worker.js. Perhaps even all of radioState. @@ +264,5 @@ > this.radioState.cardState = message.cardState; > if (!message.cardState || message.cardState == "absent") { > this.resetRadioState(); > } > + this.radioState.emergencyCallsOnly = emergencyCallsOnly; Those two lines can move into the "if" block, no? @@ +505,5 @@ > + // Note: If 'this.radioState.emergencyCallsOnly' is true it is because > + // 'connected' is false and emergency calls are possible. > + this.worker.postMessage({type: "dial", > + number: number, > + emergencyCallsOnly: this.radioState.emergencyCallsOnly}); If radioState, in particular emergencyCallsOnly, is computed in ril_worker.js, then we don't need to channel it back and forth. ::: dom/system/gonk/ril_worker.js @@ +76,5 @@ > const PARCEL_SIZE_SIZE = UINT32_SIZE; > > let RILQUIRKS_CALLSTATE_EXTRA_UINT32 = false; > let RILQUIRKS_DATACALLSTATE_DOWN_IS_UP = false; > +let RILQUIRKS_REQUEST_DIAL_IS_DIAL_EMERGENCY_CALL = false; Better: RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL
Attachment #610071 -
Flags: review?(philipp)
Assignee | ||
Comment 12•12 years ago
|
||
It adds two RIL quirks (RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL and RILQUIRKS_MODEM_DEFAULTS_TO_EMERGENCY_MODE) and computes the emergency call only flag in ril_worker.js. BTW, on ICS emergency calls use the REQUEST_DIAL RIL request type and the device reaches inmediately state 13 without SIM so current implementation should work on it as well.
Attachment #610071 -
Attachment is obsolete: true
Attachment #611239 -
Flags: review?(philipp)
Comment 13•12 years ago
|
||
Comment on attachment 611239 [details] [diff] [review] (WIP v3) - Add emergency calls to RIL's state machine. Review of attachment 611239 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RadioInterfaceLayer.js @@ +150,5 @@ > + bars: null, > + operator: null, > + type: null, > + msisdn: null, > + emergencyCallsOnly: null, So, in bug 729173 I'm revamping this code a bit. I will include your handling of emergencyCallsOnly, so I will not review any changes to RadioInterfaceLayer.js in this patch. Maybe you can remove them from the patch so that we don't get conflicts? ::: dom/system/gonk/ril_worker.js @@ +665,5 @@ > } > RILQUIRKS_DATACALLSTATE_DOWN_IS_UP = true; > } > + let ril_impl = libcutils.property_get("gsm.version.ril-impl"); > + if (ril_impl == "Qualcomm RIL 1.0") { Interesting. They don't use ril.model_id? @@ +930,5 @@ > + if (!this._isEmergencyNumber(options.number)) { > + if (DEBUG) { > + debug(options.number + " is not a valid emergency number."); > + } > + return; We should probably notify an error here so that the DOM will see an error event. Except we don't have errors for telephony yet, so we can't do that. But please add a // TODO comment for that so we don't forget. @@ +1205,5 @@ > + * @param number > + * The number to look up. > + */ > + _isEmergencyNumber: function _isEmergencyNumber(number) { > + //Check read-write ecclist property first Nit: space after // and period at the end of the sentence (also in other comments below) @@ +1207,5 @@ > + */ > + _isEmergencyNumber: function _isEmergencyNumber(number) { > + //Check read-write ecclist property first > + let numbers = libcutils.property_get("ril.ecclist"); > + if (numbers === "") { if (!numbers) @@ +1219,5 @@ > + return true; > + } > + } > + return false; > + } Much simpler and faster: if (numbers) { numbers = numbers.split(","); } else { numbers = DEFAULT_EMERGENCY_NUMBERS; } return numbers.indexOf(number) != -1; @@ +1222,5 @@ > + return false; > + } > + > + //No ecclist system property, so use our own list. > + return ((number === "112") || (number === "911")); Please const these values, e.g. using the name from the example above: const DEFAULT_EMERGENCY_NUMBERS = ["112", "911"]; @@ +1323,5 @@ > }, > > _processVoiceRegistrationState: function _processVoiceRegistrationState(state) { > + this.initRILQuirks(); > + This is an interesting case. We need to be very careful about not initializing the RIL quirks too soon. Because the RIL needs to be "warm" for the SGS2 quirks to be detectable, which in this case means there needs to be an active voice or data call. We might need to split the "call quirks" from the other quirks. I would suggest doing that in order not to jeopardize the SGS2 quirk detection.
Attachment #611239 -
Flags: review?(philipp)
Assignee | ||
Comment 14•12 years ago
|
||
This new patch addresses all the comments above except what Philikon suggests about splitting the emergency call quirk detection from the other S2 RIL quirks. This is reason because I do not request review right now at anyone until I figure out the best solution for that problem.
Attachment #611239 -
Attachment is obsolete: true
Assignee | ||
Comment 15•12 years ago
|
||
New WIP patch. This one updates remaining issues still open regarding how/when to detect the RIL quirks. The 'gsm.version.ril-impl' (see [1]) and 'ril.model_id' system properties are set by rild through libsecril-client.so at booting time. I have tested that on both Samsung Galasy S2 and akami devices. I am pretty sure the system property value 'ril.model_id' will be never empty if we read it when processing the voice registration state as we commentend on IRC discussion last week. I touched a bit the initRILQuirks() function in order to read first the 'gsm.version.ril-impl' property because it also defined in SGS2 devices and is useful to diferenciate between both devices. This pacth is fully tested on both SGS2 and akami devices. Emergency calls work on decices with SIM as well. [1] https://github.com/mozilla-b2g/android-hardware-ril/blob/b2g-gingerbread/libril/ril.cpp#L2240
Attachment #613575 -
Attachment is obsolete: true
Attachment #615737 -
Flags: review?(philipp)
Assignee | ||
Comment 16•12 years ago
|
||
Comment on attachment 615737 [details] [diff] [review] (WIP v5) - Add emergency calls to RIL's state machine. Need to rebase this pacth. I'll request review at philikon again.
Attachment #615737 -
Flags: review?(philipp)
Assignee | ||
Comment 17•12 years ago
|
||
Patch rebased. Need feedback about the initRILQuirks function.
Attachment #615737 -
Attachment is obsolete: true
Attachment #618654 -
Flags: review?(philipp)
Comment 18•12 years ago
|
||
Comment on attachment 618654 [details] [diff] [review] (WIP v6) - Add emergency calls to RIL's state machine. Review of attachment 618654 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! I can verify what you said in comment 15, so this approach is definitely a good one. The only piece that's missing is updating the info in the mainthread/content process. This would be just a line or two in RadioInterfaceLayer.js and should perhaps just be part of this patch. ::: dom/system/gonk/ril_worker.js @@ +82,4 @@ > let RILQUIRKS_CALLSTATE_EXTRA_UINT32 = false; > let RILQUIRKS_DATACALLSTATE_DOWN_IS_UP = false; > // This flag defaults to true since on RIL v6 and later, we get the > +// version number via the UNSOLICITED_RIL_CONNECTED Parcel. Unrelated change, please remove. @@ +1149,5 @@ > + let dial_request_type = REQUEST_DIAL; > + if (this.voiceRegistrationState.emergencyCallsOnly) { > + if (!this._isEmergencyNumber(options.number)) { > + if (DEBUG) { > + // TODO: Notify an error here so that the DOM will see an error event. Please file a follow-up bug for this so that we can track that and work on it once bug 712944 has landed, and refer to the bug in this comment.
Attachment #618654 -
Flags: review?(philipp) → feedback+
Assignee | ||
Comment 19•12 years ago
|
||
Updated emergencyCallsOnly info in the main/content process. Please, take a look at it. Is that what you mean?
Attachment #618654 -
Attachment is obsolete: true
Attachment #619633 -
Flags: review?(philipp)
Comment 20•12 years ago
|
||
Comment on attachment 619633 [details] [diff] [review] (WIP v7) - Add emergency calls to RIL's state machine. Review of attachment 619633 [details] [diff] [review]: ----------------------------------------------------------------- Perfecto
Attachment #619633 -
Flags: review?(philipp)
Attachment #619633 -
Flags: review+
Comment 21•12 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #18) > @@ +1149,5 @@ > > + let dial_request_type = REQUEST_DIAL; > > + if (this.voiceRegistrationState.emergencyCallsOnly) { > > + if (!this._isEmergencyNumber(options.number)) { > > + if (DEBUG) { > > + // TODO: Notify an error here so that the DOM will see an error event. > > Please file a follow-up bug for this so that we can track that and work on > it once bug 712944 has landed, and refer to the bug in this comment. Please don't forget to file this bug!
Comment 22•12 years ago
|
||
Comment on attachment 619633 [details] [diff] [review] (WIP v7) - Add emergency calls to RIL's state machine. Review of attachment 619633 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +698,5 @@ > + // The Samsung Galaxy S2 I-9100 radio sends an extra Uint32 in the > + // call state. > + let model_id = libcutils.property_get("ril.model_id"); > + if (DEBUG) debug("Detected RIL model " + model_id); > + if (model_id == "I9100") { Bad indention @@ +709,5 @@ > + RILQUIRKS_CALLSTATE_EXTRA_UINT32 = true; > + RILQUIRKS_DATACALLSTATE_DOWN_IS_UP = true; > + RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL = true; > + } > + } Extra } (due to bad indention above). This manifests as Syntax Error and completely breaks the RIL. Please don't upload patches you haven't tested. It wastes everybody's time.
Attachment #619633 -
Flags: review+ → review-
Comment 23•12 years ago
|
||
(PS: I highly recommend an editor that understands JS syntax and tells you when you create invalid syntax. Also helps with indentation.)
Comment 24•12 years ago
|
||
fixed version (only because I'm nice... you might not get so lucky next time ;))
Attachment #619633 -
Attachment is obsolete: true
Attachment #619816 -
Flags: review+
Assignee | ||
Comment 25•12 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #22) > Comment on attachment 619633 [details] [diff] [review] > (WIP v7) - Add emergency calls to RIL's state machine. > > Review of attachment 619633 [details] [diff] [review]: > ----------------------------------------------------------------- > > Extra } (due to bad indention above). > > This manifests as Syntax Error and completely breaks the RIL. Please don't > upload patches you haven't tested. It wastes everybody's time. Oh, crap! My fault, I made a mistake when requesting review in the last two versions of the patch. I wanted to request feedback about the two last changes before building and testing again but I selected 'review' in the form. You went ahead too fast and I have not had time to let you know about that. Anyway I admit it is my fault. Sorry, sorry, sorry. Gracias.
Assignee | ||
Comment 26•12 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #24) > Created attachment 619816 [details] [diff] [review] > v8 > > fixed version (only because I'm nice... you might not get so lucky next time > ;)) Tested on Samsung Galaxy S2. Need to test it on Akami, I'll do it on Thursday because I have the device in the office. Please, let me know if you tested it on Akami.
Comment 27•12 years ago
|
||
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #26) > Tested on Samsung Galaxy S2. Need to test it on Akami, I'll do it on > Thursday because I have the device in the office. Please, let me know if you > tested it on Akami. Did a rudimentary test on the Akami, seems to work fine!
Comment 28•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dbafb657a59f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in
before you can comment on or make changes to this bug.
Description
•