B2G RIL: Support emergency callback mode

RESOLVED FIXED

Status

Firefox OS
RIL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aknow, Assigned: aknow)

Tracking

unspecified
All
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 fixed)

Details

(Whiteboard: [ETA:8/2], [FT:RIL], [Sprint:2])

Attachments

(4 attachments, 24 obsolete attachments)

9.76 KB, patch
Details | Diff | Splinter Review
4.70 KB, patch
Details | Diff | Splinter Review
17.31 KB, patch
Details | Diff | Splinter Review
5.89 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=881863#c5

- handle unsolicited enter ECM from ril
- handle unsolicited exit ECM from ril
- send request exit ECM to ril
  * when user dial an non-emergency call
  * when timeout (already stay in ECM for a given period)
- send notification to DOM for mode change
(Assignee)

Updated

5 years ago
Assignee: nobody → szchen
(Assignee)

Updated

5 years ago
Blocks: 881863
(Assignee)

Comment 2

5 years ago
CDMA EMERGENCY_CALLBACK_MODE related ril command is supported in latest reference-ril (b2g/master) but not in current revision set in repo (m/v1-train)

However I got some problem with latest ril version in emulator. It always show search network after power-on.

So for testing in marionette, I use the patch in Bug 887656.
(Assignee)

Comment 3

5 years ago
Created attachment 768864 [details] [diff] [review]
Part 1: Add ecmchange event (idl)
(Assignee)

Comment 4

5 years ago
Created attachment 768866 [details] [diff] [review]
Part 2: Add ecmchange event (dom)
(Assignee)

Comment 5

5 years ago
Created attachment 768867 [details] [diff] [review]
Part 3: Support emergency callback mode in ril
(Assignee)

Comment 6

5 years ago
Created attachment 768868 [details] [diff] [review]
Part 4: Test emergency callback mode (xpcshell-test)
(Assignee)

Comment 7

5 years ago
Created attachment 768870 [details] [diff] [review]
Part 5: Test emergency callback mode (marionette)

As I mentioned in Comment 2, the test need RIL's support.
It will not be added into the manifest now.

The purpose of this test is to simulate the whole scenario of entering emergency callback mode, which could only be triggered by make an emergency call on real network.
(Assignee)

Updated

5 years ago
Attachment #768867 - Flags: review?(htsai)
(Assignee)

Updated

5 years ago
Attachment #768868 - Flags: review?(htsai)
(Assignee)

Updated

5 years ago
Attachment #768870 - Flags: review?(htsai)
(Assignee)

Comment 8

5 years ago
Hi Hsinyi,

The API is not confirmed and still under discussion (Ref to Bug 881863).
This is just a WIP but workable patch. Please give me some opinions. Thank you.
Comment on attachment 768867 [details] [diff] [review]
Part 3: Support emergency callback mode in ril

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

I would like to hole the formal review until we decide how the API should look like.

::: dom/system/gonk/ril_worker.js
@@ +718,5 @@
>  
> +// Initialize buffers. This is a separate function so that unit tests can
> +// re-initialize the buffers at will.
> +// RIL uses Buf. Make sure we init Buf first.
> +Buf.init();

Why moving this? Any problems in jshint?

@@ +793,5 @@
> +    this._isInECM = libcutils.property_get("ril.cdma.inecmmode", "false") !== "false";
> +
> +    // Always ensure that we are not in emergency callback mode when init. The
> +    // code assumes that we have only one ril_worker in the system.
> +    if (this._isInECM) this._requestExitECM();

nit: don't forget {} even only one line in the body.

@@ +799,5 @@
> +    /**
> +     * Timeout value for emergency callback mode (default 5 mins). After the
> +     * period, we will request ril to exit the mode.
> +     */
> +    this._ecmTimeoutMs = parseInt(libcutils.property_get("ro.cdma.ecmexittimer",

Can't we simply have a constant in RIL? Any reason that you would like to have this property?

@@ +1712,5 @@
>      }
>  
> +    // Exit emergency callback mode when user dial a non-emergency call.
> +    if (this._isInECM) {
> +      this._requestExitECM();

Wouldn't modem/service provider take care of this?

@@ +3509,5 @@
> +  },
> +
> +  _handleChangedECM: function _handleChangedECM(inECM) {
> +    // Nothing changed.
> +    if (this._isInECM === inECM) return;

nit: coding style - remember {}.

@@ +5267,5 @@
>    this.ESN = result[2];
>    this.MEID = result[3];
>  };
> +RIL[REQUEST_EXIT_EMERGENCY_CALLBACK_MODE] = function REQUEST_EXIT_EMERGENCY_CALLBACK_MODE(length, options) {
> +  if (options.rilRequestError) {

Curious: what happens if we request exiting emergency callback mode while we haven't been in the mode already? Request succeeds or fails?

@@ +5271,5 @@
> +  if (options.rilRequestError) {
> +    return;
> +  }
> +
> +  this._handleChangedECM(false);

Why do we need this here? Shouldn't the ECM value be updated until UNSOLICITED_EXIT_EMERGENCY_CALLBACK_MODE?
Attachment #768867 - Flags: review?(htsai)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #9)
> Comment on attachment 768867 [details] [diff] [review]
> Part 3: Support emergency callback mode in ril
> 
> Review of attachment 768867 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would like to hole the formal review until we decide how the API should
> look like.
> 

Typo: I would like to *hold* the formal review ...  :)
Comment on attachment 768867 [details] [diff] [review]
Part 3: Support emergency callback mode in ril

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

::: dom/system/gonk/RILContentHelper.js
@@ +338,4 @@
>  function RILContentHelper() {
>    this.rilContext = {
>      cardState:            RIL.GECKO_CARDSTATE_UNKNOWN,
> +    ecm:                  false,

I would say I usually don't like abbreviation much. I don't see it really helps readability...
(Assignee)

Comment 12

5 years ago
Comment on attachment 768867 [details] [diff] [review]
Part 3: Support emergency callback mode in ril

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

::: dom/system/gonk/RILContentHelper.js
@@ +338,4 @@
>  function RILContentHelper() {
>    this.rilContext = {
>      cardState:            RIL.GECKO_CARDSTATE_UNKNOWN,
> +    ecm:                  false,

I agree with you.
Any suggestion? If not, I will use 'emergencyCallbackMode' throughout all files.

::: dom/system/gonk/ril_worker.js
@@ +723,1 @@
>  

In RIL.initState(), I write
>> if (this._isInECM) this._requestExitECM();

The purpose is to force modem to exit the ECM mode when ril worker init.
The function _requestExitECM() need a ready Buf to send out the parcel.
So we should do Buf.init() before RIL.initState.

@@ +799,5 @@
> +    /**
> +     * Timeout value for emergency callback mode (default 5 mins). After the
> +     * period, we will request ril to exit the mode.
> +     */
> +    this._ecmTimeoutMs = parseInt(libcutils.property_get("ro.cdma.ecmexittimer",

I got some document mentioned that the timeout value is default to 5 mins but the actual value used is depended on the operator.

Now I set it as a ro property. (android do the same thing)
If the value is the same from phone to phone, simple constant is better.
However, I am not sure about this. We could confirm it later.

@@ +1713,5 @@
>  
> +    // Exit emergency callback mode when user dial a non-emergency call.
> +    if (this._isInECM) {
> +      this._requestExitECM();
> +    }

Not sure.
Try to do it ourself to mimic the behavior in the worst case.

Check this, search +WSOS (not spec, just one of the reference design)
http://www.multitech.com/en_US/DOCUMENTS/Collateral/manuals/S000478D.pdf

Modem has two mode: (1) timer reset, (2) basic.
In (1). modem take cares of this part, but the command to request exit explicitly is disabled. However, our RIL does expose the 'exit mode' request. So I guess the configuration of our modem might be something like case (2).

@@ +5267,5 @@
>    this.ESN = result[2];
>    this.MEID = result[3];
>  };
> +RIL[REQUEST_EXIT_EMERGENCY_CALLBACK_MODE] = function REQUEST_EXIT_EMERGENCY_CALLBACK_MODE(length, options) {
> +  if (options.rilRequestError) {

Request fail: this is the behavior in emulator (android_modem)

@@ +5272,5 @@
> +    return;
> +  }
> +
> +  this._handleChangedECM(false);
> +};

Also, I am not sure of the real behavior.
No matter the cases, call this._handleChangedECM(false) twice is ok.
We only fire one mode change event to DOM.
(In reply to Szu-Yu Chen [:aknow] from comment #12)
> Comment on attachment 768867 [details] [diff] [review]
> Part 3: Support emergency callback mode in ril
> 
> Review of attachment 768867 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +338,4 @@
> >  function RILContentHelper() {
> >    this.rilContext = {
> >      cardState:            RIL.GECKO_CARDSTATE_UNKNOWN,
> > +    ecm:                  false,
> 
> I agree with you.
> Any suggestion? If not, I will use 'emergencyCallbackMode' throughout all
> files.
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +723,1 @@
> >  
> 
> In RIL.initState(), I write
> >> if (this._isInECM) this._requestExitECM();
> 
> The purpose is to force modem to exit the ECM mode when ril worker init.
> The function _requestExitECM() need a ready Buf to send out the parcel.
> So we should do Buf.init() before RIL.initState.
> 
> @@ +799,5 @@
> > +    /**
> > +     * Timeout value for emergency callback mode (default 5 mins). After the
> > +     * period, we will request ril to exit the mode.
> > +     */
> > +    this._ecmTimeoutMs = parseInt(libcutils.property_get("ro.cdma.ecmexittimer",
> 
> I got some document mentioned that the timeout value is default to 5 mins
> but the actual value used is depended on the operator.
> 
> Now I set it as a ro property. (android do the same thing)
> If the value is the same from phone to phone, simple constant is better.
> However, I am not sure about this. We could confirm it later.
> 
> @@ +1713,5 @@
> >  
> > +    // Exit emergency callback mode when user dial a non-emergency call.
> > +    if (this._isInECM) {
> > +      this._requestExitECM();
> > +    }
> 
> Not sure.
> Try to do it ourself to mimic the behavior in the worst case.
> 
> Check this, search +WSOS (not spec, just one of the reference design)
> http://www.multitech.com/en_US/DOCUMENTS/Collateral/manuals/S000478D.pdf
> 
> Modem has two mode: (1) timer reset, (2) basic.
> In (1). modem take cares of this part, but the command to request exit
> explicitly is disabled. However, our RIL does expose the 'exit mode'
> request. So I guess the configuration of our modem might be something like
> case (2).
> 
> @@ +5267,5 @@
> >    this.ESN = result[2];
> >    this.MEID = result[3];
> >  };
> > +RIL[REQUEST_EXIT_EMERGENCY_CALLBACK_MODE] = function REQUEST_EXIT_EMERGENCY_CALLBACK_MODE(length, options) {
> > +  if (options.rilRequestError) {
> 
> Request fail: this is the behavior in emulator (android_modem)
> 

If that's the case, how are we going to make sure the property value is sync with modem?
(Assignee)

Updated

5 years ago
Blocks: 890825

Updated

5 years ago
blocking-b2g: --- → koi+
(Assignee)

Updated

5 years ago
Depends on: 891707, 897015
(Assignee)

Comment 14

5 years ago
Created attachment 780309 [details] [diff] [review]
Part 1#2: Add emergency callback mode support (idl, webidl)
Attachment #768864 - Attachment is obsolete: true
Attachment #780309 - Flags: review?(htsai)
(Assignee)

Comment 15

5 years ago
Created attachment 780310 [details] [diff] [review]
Part 2#2: Add emergency callback mode support (dom)
Attachment #768866 - Attachment is obsolete: true
Attachment #780310 - Flags: review?(htsai)
(Assignee)

Comment 16

5 years ago
Created attachment 780311 [details] [diff] [review]
Part 3#2: Add emergency callback mode support (ril)
Attachment #768867 - Attachment is obsolete: true
Attachment #780311 - Flags: review?(htsai)
(Assignee)

Comment 17

5 years ago
Created attachment 780312 [details] [diff] [review]
Part 4#2: Test emergency callback mode (xpcshell-test)
Attachment #768868 - Attachment is obsolete: true
Attachment #768868 - Flags: review?(htsai)
Attachment #780312 - Flags: review?(htsai)
(Assignee)

Comment 18

5 years ago
Created attachment 780313 [details] [diff] [review]
Part 5#2: Test emergency callback mode (marionette)
Attachment #768870 - Attachment is obsolete: true
Attachment #768870 - Flags: review?(htsai)
Attachment #780313 - Flags: review?(htsai)
(Assignee)

Comment 19

5 years ago
Created attachment 780317 [details] [diff] [review]
Part 4#3: Test emergency callback mode (xpcshell-test)
Attachment #780312 - Attachment is obsolete: true
Attachment #780312 - Flags: review?(htsai)
Attachment #780317 - Flags: review?(htsai)
(Assignee)

Comment 20

5 years ago
Created attachment 780903 [details] [diff] [review]
Part 1#3: Add emergency callback mode support (idl, webidl)
Attachment #780309 - Attachment is obsolete: true
Attachment #780309 - Flags: review?(htsai)
Attachment #780903 - Flags: review?(htsai)
(Assignee)

Comment 21

5 years ago
Created attachment 780905 [details] [diff] [review]
Part 2#3: Add emergency callback mode support (dom)
Attachment #780310 - Attachment is obsolete: true
Attachment #780310 - Flags: review?(htsai)
Attachment #780905 - Flags: review?(htsai)
(Assignee)

Comment 22

5 years ago
Created attachment 780906 [details] [diff] [review]
Part 3#3: Add emergency callback mode support (ril)
Attachment #780311 - Attachment is obsolete: true
Attachment #780311 - Flags: review?(htsai)
Attachment #780906 - Flags: review?(htsai)
(Assignee)

Comment 23

5 years ago
Created attachment 780907 [details] [diff] [review]
Part 4#4: Test emergency callback mode (xpcshell-test)
Attachment #780317 - Attachment is obsolete: true
Attachment #780317 - Flags: review?(htsai)
Attachment #780907 - Flags: review?(htsai)
(Assignee)

Comment 24

5 years ago
Created attachment 780908 [details] [diff] [review]
Part 5#3: Test emergency callback mode (marionette)
Attachment #780313 - Attachment is obsolete: true
Attachment #780313 - Flags: review?(htsai)
Attachment #780908 - Flags: review?(htsai)
(Assignee)

Updated

5 years ago
Attachment #780903 - Flags: superreview?(mounir)
(Assignee)

Updated

5 years ago
Attachment #780905 - Flags: review?(bent.mozilla)
(Assignee)

Comment 25

5 years ago
Comment on attachment 780908 [details] [diff] [review]
Part 5#3: Test emergency callback mode (marionette)

Hi edgar:

Please help for "switch technology" part. Thx.
Attachment #780908 - Flags: review?(echen)

Updated

5 years ago
Attachment #780903 - Flags: review?(htsai) → review+
Comment on attachment 780905 [details] [diff] [review]
Part 2#3: Add emergency callback mode support (dom)

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

Leave this part to DOM peer ;)
Attachment #780905 - Flags: review?(htsai)

Updated

5 years ago
Whiteboard: [ETA:7/26]
Comment on attachment 780906 [details] [diff] [review]
Part 3#3: Add emergency callback mode support (ril)

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

::: dom/system/gonk/ril_worker.js
@@ +3724,5 @@
> +    }
> +  },
> +
> +  _handleChangedEmergencyCbMode: function _handleChangedEmergencyCbMode(active) {
> +    // Nothing changed.

nit: How about removing the comment as the code is very straightforward?

@@ +3735,5 @@
> +      let ril = this;
> +      this._cancelEmergencyCbModeTimeout();
> +      this._exitEmergencyCbModeTimeoutID = setTimeout(function() {
> +          ril.exitEmergencyCbMode(); },
> +          EMERGENCY_CB_MODE_TIMEOUT_MS);

nit: coding style

this._exitEmergencyCbModeTimeoutID = setTimeout(function() {
   ril.exitEmergencyCbMode();
}, EMERGENCY_CB_MODE_TIMEOUT_MS);

@@ +5651,5 @@
>    this.ESN = result[2];
>    this.MEID = result[3];
>  };
> +RIL[REQUEST_EXIT_EMERGENCY_CALLBACK_MODE] = function REQUEST_EXIT_EMERGENCY_CALLBACK_MODE(length, options) {
> +  debug("[aknow]: request exit ecm " + JSON.stringify(options));

Please ~ ;)
Attachment #780906 - Flags: review?(htsai) → review+

Updated

5 years ago
Attachment #780907 - Flags: review?(htsai) → review+
Comment on attachment 780908 [details] [diff] [review]
Part 5#3: Test emergency callback mode (marionette)

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

Thank you for the marionette tests which look good. However, I was told that we still have some problems in emulator technology switch that some network states might not be correctly updated. That problem isn't obvious in the separate telephony tests, but I am concerned some unexpected behaviours occurs when we do a bunch of tests in sequence. So, I would suggest move the marionette tests to a follow-up bug. Let's land them later.
Attachment #780908 - Flags: review?(htsai)

Comment 29

5 years ago
Comment on attachment 780908 [details] [diff] [review]
Part 5#3: Test emergency callback mode (marionette)

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

Agree with Hsinyi's comment :)
Attachment #780908 - Flags: review?(echen)
(Assignee)

Updated

5 years ago
Blocks: 899004

Updated

5 years ago
No longer depends on: 891707, 897015
(Assignee)

Updated

5 years ago
Attachment #780908 - Attachment is obsolete: true
Comment on attachment 780903 [details] [diff] [review]
Part 1#3: Add emergency callback mode support (idl, webidl)

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

Hsin-Yi, can you do that sr?
Attachment #780903 - Flags: superreview?(mounir) → superreview?(htsai)

Updated

5 years ago
Whiteboard: [ETA:7/26] → [ETA:8/2]
(In reply to Mounir Lamouri (:mounir) from comment #30)
> Comment on attachment 780903 [details] [diff] [review]
> Part 1#3: Add emergency callback mode support (idl, webidl)
> 
> Review of attachment 780903 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hsin-Yi, can you do that sr?

Sure!
Comment on attachment 780903 [details] [diff] [review]
Part 1#3: Add emergency callback mode support (idl, webidl)

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

Looks good to me.
Attachment #780903 - Flags: superreview?(htsai) → superreview+
Comment on attachment 780903 [details] [diff] [review]
Part 1#3: Add emergency callback mode support (idl, webidl)

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

Mounir, the api design looks good to me. It supports what the user story needs. 

However, per my understanding, we want this MozEmergencyCbModeEvent being created and fired by MobileConnection, not created by |new MozEmergencyCbModeEvent()| on web pages. So I thought it's good no [constructor] here, but I was just told that all event interfaces are supposed to have [constructor]. I am delivering the sr? back to you since I am not sure the part of event webidl, and would like to have your comment on it. Thank you!
Attachment #780903 - Flags: superreview+ → superreview?(mounir)
Comment on attachment 780903 [details] [diff] [review]
Part 1#3: Add emergency callback mode support (idl, webidl)

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

Yes, events should have a constructor, feel free to ping smaug for more information.
Attachment #780903 - Flags: superreview?(mounir)
Comment on attachment 780903 [details] [diff] [review]
Part 1#3: Add emergency callback mode support (idl, webidl)

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

A lot of news things to learn about webidl. I am sorry that we need to have [constructor] for all events. r- for that.
And we would need another DOM/webidl peer for the webidl review. bent or smaug could be nice candidates. :)
Attachment #780903 - Flags: review+ → review-

Updated

5 years ago
Whiteboard: [ETA:8/2] → [ETA:8/2], [FT:RIL], [Sprint:2]
(Assignee)

Comment 36

5 years ago
Created attachment 784902 [details] [diff] [review]
Part 1#4: Add emergency callback mode support (idl, webidl)
Attachment #780903 - Attachment is obsolete: true
Attachment #784902 - Flags: superreview?(mounir)
Attachment #784902 - Flags: review?(htsai)
(Assignee)

Comment 37

5 years ago
Created attachment 784903 [details] [diff] [review]
Part 2#4: Add emergency callback mode support (dom)
Attachment #780905 - Attachment is obsolete: true
Attachment #780905 - Flags: review?(bent.mozilla)
Attachment #784903 - Flags: review?(bent.mozilla)
Comment on attachment 784902 [details] [diff] [review]
Part 1#4: Add emergency callback mode support (idl, webidl)

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

Bouncing to smaug, to make sure the event is properly defined.
Attachment #784902 - Flags: superreview?(mounir) → superreview?(bugs)

Updated

5 years ago
Attachment #784902 - Flags: review?(htsai) → review+
Comment on attachment 784902 [details] [diff] [review]
Part 1#4: Add emergency callback mode support (idl, webidl)

Could you please use event codegen for the event.
I know it requires .idl still, but at least we wouldn't have .h and .cpp for the
event.
Attachment #784902 - Flags: superreview?(bugs) → superreview-
(Assignee)

Comment 40

5 years ago
(In reply to Olli Pettay [:smaug] from comment #39)
> Comment on attachment 784902 [details] [diff] [review]
> Part 1#4: Add emergency callback mode support (idl, webidl)
> 
> Could you please use event codegen for the event.
> I know it requires .idl still, but at least we wouldn't have .h and .cpp for
> the
> event.

What's the policy of idl? I thought we were going to replace all the idl by webidl in the future.
Flags: needinfo?(bugs)
No. We're not going to remove all .idl. We're trying to remove most of the .idl which
define interfaces for web content. And yes, that means also these events, but for
now events should use event codegenerator whenever possible. That makes code reviewing easier, 
mistakes harder and more importantly, changes to all the event implementations a lot easier.

(I'm hoping to have a code generator for .webidl only events early this autumn.)
Flags: needinfo?(bugs)
(Assignee)

Comment 42

5 years ago
Created attachment 786141 [details] [diff] [review]
Part 1#5: Add emergency callback mode support (idl, webidl)
Attachment #784902 - Attachment is obsolete: true
Attachment #786141 - Flags: superreview?(bugs)
(Assignee)

Comment 43

5 years ago
Created attachment 786142 [details] [diff] [review]
Part 2#5: Add emergency callback mode support (dom)
Attachment #784903 - Attachment is obsolete: true
Attachment #784903 - Flags: review?(bent.mozilla)
Attachment #786142 - Flags: review?(bent.mozilla)

Updated

5 years ago
Attachment #786141 - Flags: superreview?(bugs) → superreview+

Updated

5 years ago
Blocks: 726098
Comment on attachment 786142 [details] [diff] [review]
Part 2#5: Add emergency callback mode support (dom)

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

::: dom/network/src/MobileConnection.cpp
@@ +549,5 @@
> +  }
> +
> +  nsCOMPtr<nsIDOMEvent> event;
> +  NS_NewDOMMozEmergencyCbModeEvent(getter_AddRefs(event), this, nullptr,
> +                                   nullptr);

Please MOZ_ASSERT(event) here.
Attachment #786142 - Flags: review?(bent.mozilla) → review+
(Assignee)

Updated

5 years ago
Attachment #786141 - Flags: review?(htsai)

Updated

5 years ago
Attachment #786141 - Flags: review?(htsai) → review+
(Assignee)

Comment 46

5 years ago
Created attachment 788794 [details] [diff] [review]
[Final] Part 1: Add emergency callback mode support (idl, webidl). r=hsinyi. sr=bugs
Attachment #786141 - Attachment is obsolete: true
(Assignee)

Comment 47

5 years ago
Created attachment 788795 [details] [diff] [review]
[Final] Part 2: Add emergency callback mode support (dom). r=bent
Attachment #786142 - Attachment is obsolete: true
(Assignee)

Comment 48

5 years ago
Created attachment 788796 [details] [diff] [review]
[Final] Part 3: Add emergency callback mode support (ril). r=hsinyi
Attachment #780906 - Attachment is obsolete: true
(Assignee)

Comment 49

5 years ago
Created attachment 788797 [details] [diff] [review]
[Final] Part 4: Test emergency callback mode (xpcshell-test). r=hsinyi
Attachment #780907 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed

Comment 50

5 years ago
Hi Aknow,

Can it enter ECM even without UIM card?
Flags: needinfo?(szchen)
(Assignee)

Comment 51

5 years ago
Actually, entering ECM is controlled by modem. Gecko just pass the event from modem to gaia, so we could not control the behavior.

However, I think that it should got the entering event from modem if you dial an emergency call.
Flags: needinfo?(szchen)
Backed out for causing pretty frequent B2G mochitest-2 failures.
https://hg.mozilla.org/integration/b2g-inbound/rev/c146d402a55f

https://tbpl.mozilla.org/php/getParsedLog.php?id=26446516&tree=B2g-Inbound

12:41:01     INFO -  pushing /system/bin/busybox
12:41:01     INFO -  waiting for system-message-listener-ready...
12:41:01     INFO -  done
12:41:01     INFO -  args: [u'/builds/slave/test/build/xre/bin/xpcshell', '-g', '/builds/slave/test/build/xre/bin', '-v', '170', '-f', '/builds/slave/test/build/xre/bin/components/httpd.js', '-e', "const _PROFILE_PATH = '/tmp/tmpUaMZZW';const _SERVER_PORT = '8888'; const _SERVER_ADDR = '10.0.2.2';\n                     const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = false;", '-f', './server.js']
12:41:01     INFO -  1376336428770	Marionette	INFO	MarionetteComponent loaded
12:41:01     INFO -  1376336429059	Marionette	INFO	marionette enabled via build flag and pref
12:41:01     INFO -  -*- RadioInterfaceLayer: 1 interfaces
12:41:01     INFO -  1376336435753	Marionette	INFO	marionette-server.js loaded
12:41:01     INFO -  1376336437073	Marionette	INFO	B2G emulator: yes
12:41:01     INFO -  1376336437085	Marionette	INFO	Device detected is generic
12:41:01     INFO -  1376336437095	Marionette	INFO	Bypassing offline status.
12:41:01     INFO -  1376336437113	Marionette	INFO	Listening on port 2828
12:41:01     INFO -  1376336437130	Marionette	INFO	Marionette server ready
12:41:01     INFO -  1376336438657	Marionette	DEBUG	accepted connection on 127.0.0.1:38559
12:41:01     INFO -  PermissionsTable.jsm: expandPermissions: Unknown Permission: network-tcpPermissionsTable.jsm: expandPermissions: Unknown Permission: network-httpPermissionsInstaller.jsm: 'network-tcp' is not a valid Webapps permission name.PermissionsInstaller.jsm: 'network-http' is not a valid Webapps permission name.1376336455139	Marionette	DEBUG	accepted connection on 127.0.0.1:38560
12:41:01     INFO -  1376336455352	Marionette	TRACE	Got: {"to": "root", "type": "getMarionetteID"}
12:41:01     INFO -  1376336456512	Marionette	TRACE	Got: {"to": "0", "type": "newSession"}
12:41:01     INFO -  System JS : ERROR jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js:1148
12:41:01     INFO -                       Error: Don't know about this message type: undefined
12:41:01     INFO -  System JS : ERROR chrome://satchel/content/formSubmitListener.js:15
12:41:01     INFO -                       ReferenceError: XPCOMUtils is not defined

You can ignore all the warnings about forms.js - that's a different issue being tracked.
(Assignee)

Comment 54

5 years ago
22:57:02     INFO -  System JS : ERROR jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js:1118
22:57:02     INFO -                       Error: Don't know about this message type: undefined

This is not the reason that cause test fail. We could also found this line in other pass log.

12:41:01     INFO -  System JS : ERROR chrome://satchel/content/formSubmitListener.js:15
12:41:01     INFO -                       ReferenceError: XPCOMUtils is not defined

Also appears in Bug 904224
https://tbpl.mozilla.org/php/getParsedLog.php?id=26445003&tree=B2g-Inbound
(Assignee)

Comment 55

5 years ago
Hi Ryan,

Test on try without any modification.
mochitest-2 seems good
https://tbpl.mozilla.org/?tree=Try&rev=483793a4689e
(Assignee)

Updated

5 years ago
Flags: needinfo?(ryanvm)
(In reply to Szu-Yu Chen [:aknow] from comment #54)
> Also appears in Bug 904224
> https://tbpl.mozilla.org/php/getParsedLog.php?id=26445003&tree=B2g-Inbound

Because I used the same log link. There were also many instances of bug 904224 that occurred in non-failing runs and didn't have that error.

I've retriggered mochitest-2 on your Try push (it was an intermittent failure, one run won't really tell us if it was a spurious issue or not).
Flags: needinfo?(ryanvm)
But I actually do see that error elsewhere, so maybe it was something spurious going on yesterday. The retriggers on Try look good, so I will re-land. Sorry for the churn.
Except part 3 doesn't apply anymore. Please rebase :(
(Assignee)

Comment 59

5 years ago
Created attachment 790142 [details] [diff] [review]
[Final] Part 1#2: Add emergency callback mode support (idl, webidl). r=hsinyi. sr=bugs
(Assignee)

Comment 60

5 years ago
Created attachment 790143 [details] [diff] [review]
[Final] Part 2#2: Add emergency callback mode support (dom). r=bent
Attachment #788794 - Attachment is obsolete: true
Attachment #788795 - Attachment is obsolete: true
(Assignee)

Comment 61

5 years ago
Created attachment 790144 [details] [diff] [review]
[Final] Part 3#2: Add emergency callback mode support (ril). r=hsinyi
Attachment #788796 - Attachment is obsolete: true
(Assignee)

Comment 62

5 years ago
Created attachment 790145 [details] [diff] [review]
[Final] Part 4#2: Test emergency callback mode (xpcshell-test). r=hsinyi
Attachment #788797 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed

Updated

5 years ago
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Target Milestone: mozilla26 → ---
Version: Trunk → unspecified
status-firefox26: --- → fixed

Comment 66

5 years ago
HI Chen,
In gaia at [1] we already uses timer to track ECM mode timeout value (5min),which we displays ti UI, then why does the ril_worker.js at [2] needs to maintain a separate timer in Emergency callback mode.

Can't gaia send the Exit Emergency callback mode request to RIL whenever timer expires?

This query is because sometime possibly the UI can go out of sync if for some reason RIL didn't exit the callback mode but UI removed that information.

[1] https://github.com/steveck-chung/gaia/commit/5e4f51cdaf4581301b1834fc584d78e5ea6e1604
[2] https://hg.mozilla.org/mozilla-central/rev/ca56d6ec8474#l3.124

Thanks,
Pooja
(Assignee)

Comment 67

5 years ago
> Can't gaia send the Exit Emergency callback mode request to RIL whenever
> timer expires?
> 
> This query is because sometime possibly the UI can go out of sync if for
> some reason RIL didn't exit the callback mode but UI removed that
> information.

It is a protection in case gaia doesn't send the exit request. Thus, even without the gaia (correctly handle the related event), all the functionality works well. We don't have to worry about being lock in emergency callback mode and cannot get out of it.

It should be OK if gecko exit the mode first and then gaia send the exit request when timeout. If it cause the problem, we should fix it. My idea is that no matter gaia send the request or not, everything should be fine.

Exit request might fail. UI should not directly remove the information when timeout occurs or just after it send out the exit request. The better way is to check 'emergencycbmodechange' event. Whenever RIL changes the mode, gaia will receive that event. Based on the event, we could know the entering/leaving time and then show the correct info.

Did you find any problem or risk in current implementation?

Comment 68

5 years ago
(In reply to Szu-Yu Chen [:aknow] from comment #67)
> > Can't gaia send the Exit Emergency callback mode request to RIL whenever
> > timer expires?
> > 
> > This query is because sometime possibly the UI can go out of sync if for
> > some reason RIL didn't exit the callback mode but UI removed that
> > information.
> 
> It is a protection in case gaia doesn't send the exit request. Thus, even
> without the gaia (correctly handle the related event), all the functionality
> works well. We don't have to worry about being lock in emergency callback
> mode and cannot get out of it.
> 
> It should be OK if gecko exit the mode first and then gaia send the exit
> request when timeout. If it cause the problem, we should fix it. My idea is
> that no matter gaia send the request or not, everything should be fine.
> 
> Exit request might fail. UI should not directly remove the information when
> timeout occurs or just after it send out the exit request. The better way is
> to check 'emergencycbmodechange' event. Whenever RIL changes the mode, gaia
> will receive that event. Based on the event, we could know the
> entering/leaving time and then show the correct info.
Szu-Yu, I now understand what you are going for. However there is a potential for race condition between gaia and gecko. Since gecko already manages the timer and report the emergencycbmodechange event the timer in gaia seems unnecessary. 

> 
> Did you find any problem or risk in current implementation?
(Assignee)

Comment 69

5 years ago
(In reply to Anshul from comment #68)
> (In reply to Szu-Yu Chen [:aknow] from comment #67)
> > > Can't gaia send the Exit Emergency callback mode request to RIL whenever
> > > timer expires?
> > > 
> > > This query is because sometime possibly the UI can go out of sync if for
> > > some reason RIL didn't exit the callback mode but UI removed that
> > > information.
> > 
> > It is a protection in case gaia doesn't send the exit request. Thus, even
> > without the gaia (correctly handle the related event), all the functionality
> > works well. We don't have to worry about being lock in emergency callback
> > mode and cannot get out of it.
> > 
> > It should be OK if gecko exit the mode first and then gaia send the exit
> > request when timeout. If it cause the problem, we should fix it. My idea is
> > that no matter gaia send the request or not, everything should be fine.
> > 
> > Exit request might fail. UI should not directly remove the information when
> > timeout occurs or just after it send out the exit request. The better way is
> > to check 'emergencycbmodechange' event. Whenever RIL changes the mode, gaia
> > will receive that event. Based on the event, we could know the
> > entering/leaving time and then show the correct info.
> Szu-Yu, I now understand what you are going for. However there is a
> potential for race condition between gaia and gecko. Since gecko already
> manages the timer and report the emergencycbmodechange event the timer in
> gaia seems unnecessary. 

Gaia side needs a timer to show the countdown value on UI. So I think the timer could not be removed.

What's the problem for this race condition? I have consider following two cases. Both of them looks good to me.
1. If gaia gets timeout first and then sends the exit request, gecko will stop its timer when handling the request.
2. If gecko gets timeout first and send the exit request by itself, gaia should receive the mode change event during its countdown. Whenever gaia receives a leaving event, it should clear the related UI. The same situation occurs when user dialing a non-emergency call during the emergency callback mode.

Comment 70

5 years ago
(In reply to Szu-Yu Chen [:aknow] from comment #69)
> (In reply to Anshul from comment #68)
> > (In reply to Szu-Yu Chen [:aknow] from comment #67)
> > > > Can't gaia send the Exit Emergency callback mode request to RIL whenever
> > > > timer expires?
> > > > 
> > > > This query is because sometime possibly the UI can go out of sync if for
> > > > some reason RIL didn't exit the callback mode but UI removed that
> > > > information.
> > > 
> > > It is a protection in case gaia doesn't send the exit request. Thus, even
> > > without the gaia (correctly handle the related event), all the functionality
> > > works well. We don't have to worry about being lock in emergency callback
> > > mode and cannot get out of it.
> > > 
> > > It should be OK if gecko exit the mode first and then gaia send the exit
> > > request when timeout. If it cause the problem, we should fix it. My idea is
> > > that no matter gaia send the request or not, everything should be fine.
> > > 
> > > Exit request might fail. UI should not directly remove the information when
> > > timeout occurs or just after it send out the exit request. The better way is
> > > to check 'emergencycbmodechange' event. Whenever RIL changes the mode, gaia
> > > will receive that event. Based on the event, we could know the
> > > entering/leaving time and then show the correct info.
> > Szu-Yu, I now understand what you are going for. However there is a
> > potential for race condition between gaia and gecko. Since gecko already
> > manages the timer and report the emergencycbmodechange event the timer in
> > gaia seems unnecessary. 
> 
> Gaia side needs a timer to show the countdown value on UI. So I think the
> timer could not be removed.
> 
> What's the problem for this race condition? I have consider following two
> cases. Both of them looks good to me.
> 1. If gaia gets timeout first and then sends the exit request, gecko will
> stop its timer when handling the request.
> 2. If gecko gets timeout first and send the exit request by itself, gaia
> should receive the mode change event during its countdown. Whenever gaia
> receives a leaving event, it should clear the related UI. The same situation
> occurs when user dialing a non-emergency call during the emergency callback
> mode.
When gecko timesout and send the request to RIL to exit the emergency callback mode gecko doesn't notify gaia that emergency mode is exited http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#1323. Also sine gecko originated the request explictly most likely RIL won't respond with unsol emergency callback mode and therefore no callback mode change event would be sent to gaia.
(Assignee)

Comment 71

5 years ago
(In reply to Anshul from comment #70)
> (In reply to Szu-Yu Chen [:aknow] from comment #69)
> > (In reply to Anshul from comment #68)
> > > (In reply to Szu-Yu Chen [:aknow] from comment #67)
> > > > > Can't gaia send the Exit Emergency callback mode request to RIL whenever
> > > > > timer expires?
> > > > > 
> > > > > This query is because sometime possibly the UI can go out of sync if for
> > > > > some reason RIL didn't exit the callback mode but UI removed that
> > > > > information.
> > > > 
> > > > It is a protection in case gaia doesn't send the exit request. Thus, even
> > > > without the gaia (correctly handle the related event), all the functionality
> > > > works well. We don't have to worry about being lock in emergency callback
> > > > mode and cannot get out of it.
> > > > 
> > > > It should be OK if gecko exit the mode first and then gaia send the exit
> > > > request when timeout. If it cause the problem, we should fix it. My idea is
> > > > that no matter gaia send the request or not, everything should be fine.
> > > > 
> > > > Exit request might fail. UI should not directly remove the information when
> > > > timeout occurs or just after it send out the exit request. The better way is
> > > > to check 'emergencycbmodechange' event. Whenever RIL changes the mode, gaia
> > > > will receive that event. Based on the event, we could know the
> > > > entering/leaving time and then show the correct info.
> > > Szu-Yu, I now understand what you are going for. However there is a
> > > potential for race condition between gaia and gecko. Since gecko already
> > > manages the timer and report the emergencycbmodechange event the timer in
> > > gaia seems unnecessary. 
> > 
> > Gaia side needs a timer to show the countdown value on UI. So I think the
> > timer could not be removed.
> > 
> > What's the problem for this race condition? I have consider following two
> > cases. Both of them looks good to me.
> > 1. If gaia gets timeout first and then sends the exit request, gecko will
> > stop its timer when handling the request.
> > 2. If gecko gets timeout first and send the exit request by itself, gaia
> > should receive the mode change event during its countdown. Whenever gaia
> > receives a leaving event, it should clear the related UI. The same situation
> > occurs when user dialing a non-emergency call during the emergency callback
> > mode.
> When gecko timesout and send the request to RIL to exit the emergency
> callback mode gecko doesn't notify gaia that emergency mode is exited
> http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.
> js#1323. Also sine gecko originated the request explictly most likely RIL
> won't respond with unsol emergency callback mode and therefore no callback
> mode change event would be sent to gaia.

Anshul,

Did you mean when we send out the exit request in the emergency callback mode, there will not be an unsolicited response of "callback mode change" from ril. We should determine whether the mode changed by success/error result of the exit request.

If so, this is a flaw in current design. We should fix it. Really thank you for pointing out the problem.

Comment 72

5 years ago
Szu-Yu Chen, I have to test this to confirm but I would suppose RIL won't send an unsol as gecko explicitly requested for it. If my hunch is true (and I will confirm that) then gaia sending an exit emergency mode after timer expires as per my original suggestion seems like a solution.
(Assignee)

Comment 73

5 years ago
(In reply to Anshul from comment #72)
> Szu-Yu Chen, I have to test this to confirm but I would suppose RIL won't
> send an unsol as gecko explicitly requested for it. If my hunch is true (and
> I will confirm that) then gaia sending an exit emergency mode after timer
> expires as per my original suggestion seems like a solution.

Yes, it could also be achieved by gecko. Gecko could notify gaia that emergency mode is exited after its exiting request is successful.

Another question is dialing a non-emergency call during the emergency callback mode. Currently, gecko also explicitly request the mode exit when it found the case happened. We need a way to notify gaia about the mode change if ril doesn't send the unsol response. Using above solution could also resolve this problem. Otherwise, we should move all the logic to gaia side.
You need to log in before you can comment on or make changes to this bug.