B2G Emulator: update voice registration after switching radio technology

RESOLVED FIXED in Firefox 42

Status

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: edgar, Assigned: bhsu)

Tracking

unspecified
FxOS-S4 (07Aug)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(5 attachments, 10 obsolete attachments)

923 bytes, patch
aknow
: review+
Details | Diff | Splinter Review
2.37 KB, patch
aknow
: review+
Details | Diff | Splinter Review
60 bytes, text/x-github-pull-request
edgar
: review+
Details | Review
62 bytes, text/x-github-pull-request
edgar
: review+
Details | Review
3.38 KB, patch
bhsu
: review+
Details | Diff | Splinter Review
In bug 876396, we add a console command, 'modem tech', to switch radio technology. So that we can use this command to switch emulator to cdma mode and run test cases.

But when I try to write a marionette tests, I meet some problems as below:

- Currently the preferred Network Type is GSM/WCDMA, so it is failed to switch to cdma by using 'modem tech' console command.
To fix this, maybe we could consider to make 'modem tech' can also support switching preferred network type.

- There is no way to know the radio switching process is finished. If we start test before it is ready, we may got some unexpected timing issue. My idea is using voice registration event [1], when radio technology changes, qemu should report voice registration change with the current radio technology [2]. But this mechanism seems not work properly now.

[1] https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/telephony/ril.h#L3354
[2] https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/telephony/ril.h#L1248
(In reply to Edgar Chen [:edgar][:echen] from comment #0)
> - Currently the preferred Network Type is GSM/WCDMA, so it is failed to
> switch to cdma by using 'modem tech' console command.
> To fix this, maybe we could consider to make 'modem tech' can also support
> switching preferred network type.
File a separated bug for this.
(In reply to Edgar Chen [:edgar][:echen] from comment #4)
> (In reply to Edgar Chen [:edgar][:echen] from comment #0)
> > - Currently the preferred Network Type is GSM/WCDMA, so it is failed to
> > switch to cdma by using 'modem tech' console command.
> > To fix this, maybe we could consider to make 'modem tech' can also support
> > switching preferred network type.
> File a separated bug for this.

Bug 897015. Thanks
No longer blocks: 887690
Do you think these PRs are ready? Looks good for me.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #6)
> Do you think these PRs are ready? Looks good for me.

It is almost ready, but I would like to do more test before request review.
Thanks.
(In reply to Edgar Chen [:edgar][:echen] from comment #7)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #6)
> > Do you think these PRs are ready? Looks good for me.
> 
> It is almost ready, but I would like to do more test before request review.
> Thanks.

I found voice registration does not update correctly if the switch event is triggered by setting preferred network type from gecko.
(In reply to Edgar Chen [:edgar][:echen] from comment #8)
> (In reply to Edgar Chen [:edgar][:echen] from comment #7)
> > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #6)
> > > Do you think these PRs are ready? Looks good for me.
> > 
> > It is almost ready, but I would like to do more test before request review.
> > Thanks.
> 
> I found voice registration does not update correctly if the switch event is
> triggered by setting preferred network type from gecko.

Provide the solution in part1 patch of qemu pull request.
Attachment #774441 - Flags: review?(vyang)
Attachment #774450 - Flags: review?(vyang)
Attachment #774441 - Flags: review?(vyang) → review+
Attachment #774450 - Flags: review?(vyang)
Component: General → Emulator
No longer blocks: 899004
Attachment #774441 - Attachment is obsolete: true
Attachment #774450 - Attachment is obsolete: true
Depends on: 979169
Blocks: 975778
Attachment #8617777 - Flags: review?(szchen)
Comment on attachment 8617776 [details] [diff] [review]
Part 1: Waiting for |UNSOLICITED_RESPONSE_VOICE_NETWORK_STATE_CHANGED| when changing modem tech

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

::: dom/telephony/test/marionette/head.js
@@ +281,5 @@
>     */
>    function changeModemTech(aTech, aPreferredMask) {
> +    let mobileConn = navigator.mozMobileConnections[0];
> +
> +    function checkTech(aMobileConnection) {

Use closure for both |mobileConn| and |aTech| or passing them as function parameters.
Let's apply the same way for these two variables.

@@ +282,5 @@
>    function changeModemTech(aTech, aPreferredMask) {
> +    let mobileConn = navigator.mozMobileConnections[0];
> +
> +    function checkTech(aMobileConnection) {
> +      switch(mobileConn.voice.type) {

not |mobileConn|, should be |aMobileConnection|

@@ +314,5 @@
> +          return false;
> +      }
> +    }
> +
> +    let promise1 = checkTech(mobileConn) ? Promise.resolve()

The naming of checkTech is not good enough. By reading the line
  let promise1 = checkTech(mobileConn) ... 
You have no idea what will be done in the function.

The function mixed two purposes, one is converting voice type to tech, another one is comparing the tech. I'll suggest something like

function voiceTypeToTech(aType) {
  case "...":
    return "gsm";
  ...
}

function isCompleted() {
  return aTech === voiceTypeToTech(mobileConn.voice.type);
}
Attachment #8617776 - Flags: review?(szchen) → review-
Attachment #8617777 - Flags: review?(szchen) → review+
Comment on attachment 8622180 [details] [diff] [review]
Part 1: Waiting for |UNSOLICITED_RESPONSE_VOICE_NETWORK_STATE_CHANGED| when changing modem tech (V2). r=aknow

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

::: dom/telephony/test/marionette/head.js
@@ +277,5 @@
>    }
>  
>    /**
> +   * @param aVoiceType
> +   *        The voice type of a mobileConnection, which can be obtained from

This line is too long.
Attachment #8622180 - Flags: review?(szchen) → review+
Hi Edgar,

Can you help review these patches? They are made from simply rebasing previous works.
Attachment #8342282 - Attachment is obsolete: true
Attachment #8624053 - Flags: review?(echen)
Attachment #8342284 - Attachment is obsolete: true
Attachment #8624054 - Flags: review?(echen)
Since you are working on this bug now, I change the assignee to you. Thank you, Ben.
Assignee: echen → bhsu
Attachment #8624054 - Attachment mime type: text/plain → text/x-github-pull-request
Comment on attachment 8624054 [details]
[external/qemu] pull request 150 (for Emulator-KK)

The link of pull request seems incorrect.
Attachment #8624054 - Flags: review?(echen)
Comment on attachment 8628143 [details] [review]
[external/qemu] pull request 149 (for Emulator-ICS)

Sorry for being late. Per comments on github, could you please file a follow-up bug to find a better solution for the registration state. Thank you.
Attachment #8628143 - Flags: review?(echen) → review+
(In reply to Edgar Chen [:edgar][:echen] from comment #23)
> Comment on attachment 8628142 [details] [review]
> [hardware/ril] pull request 60 (for Emulator-ICS)
> 
> Sorry for being late. Per comments on github, could you please file a
> follow-up bug to find a better solution for the registration state. Thank
> you.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(Please ignore my previous comment, It should belong to the pull request of external/qemu. :P)

r=me with comments on github addressed. Thank you.
Link updated :P
Attachment #8624054 - Attachment is obsolete: true
Attachment #8640887 - Flags: review?(echen)
See Also: → 1189153
Posted patch Part 3: Disable some tests (obsolete) — Splinter Review
Since there are some defects in current gecko, some of the test will fail after applying other patches for this issue. As a result, I suggest first disable some test here, and re-enable them in Bug 1190274.
Attachment #8642270 - Flags: review?(echen)
Comment on attachment 8642270 [details] [diff] [review]
Part 3: Disable some tests

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

r=me with comment addressed, thank you.

::: dom/mobileconnection/tests/marionette/test_mobile_voice_state.js
@@ -125,5 @@
>  
>  startTestCommon(function() {
>    log("Test initial voice connection info");
>  
> -  verifyVoiceInfo(INITIAL_STATES);

Please also remove |INITIAL_STATES|.
Attachment #8642270 - Flags: review?(echen) → review+
Attachment #8642270 - Attachment is obsolete: true
Attachment #8642923 - Flags: review+
Attachment #8617777 - Attachment description: Part 2: Enable a testcase → Part 2: Enable a testcase. r=aknow
Attachment #8622180 - Attachment description: Part 1: Waiting for |UNSOLICITED_RESPONSE_VOICE_NETWORK_STATE_CHANGED| when changing modem tech (V2) → Part 1: Waiting for |UNSOLICITED_RESPONSE_VOICE_NETWORK_STATE_CHANGED| when changing modem tech (V2). r=aknow
Attachment #8624053 - Attachment is obsolete: true
Attachment #8624053 - Flags: review?(echen)
Attachment #8640887 - Attachment is obsolete: true
Attachment #8640887 - Flags: review?(echen)
You need to log in before you can comment on or make changes to this bug.