975778 - [B2G] [Emulator] Support the request RIL_UNSOL_CDMA_CALL_WAITING for Cdma call waiting

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Emulator, defect)

Description

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Support REQUEST_CDMA_FLASH on reference-ril

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

AOSP ril.h defines

typedef struct {
    char *          number;             /* Remote party number */
    int             numberPresentation; /* 0=Allowed, 1=Restricted, 2=Not Specified/Unknown */
    char *          name;               /* Remote party name */
    RIL_CDMA_SignalInfoRecord signalInfoRecord;
    /* Number type/Number plan required to support International Call Waiting */
    int             number_type;        /* 0=Unknown, 1=International, 2=National,
                                           3=Network specific, 4=subscriber */
    int             number_plan;        /* 0=Unknown, 1=ISDN, 3=Data, 4=Telex, 8=Nat'l, 9=Private */
} RIL_CDMA_CallWaiting_v6;

rild needs to respond this structure to gecko.

To retrieve these attributes (communication b/w rild and modem), we should follow AT command defined in TS 27.007
-- "+CCWA:" is an unsolicited event for a waiting call. Per TS 27.007, the format is: +CCWA: <number>,<type>,<class>,[<alpha>][,<CLI validity>[,<subaddr>,<satype>[,<priority>]]]. Now we get "number" "numberPresentation" and "number_type".

-- "+CNAP:" 27.007 says, +CNAP: <name>[,<CNI validity>] response is returned after every *RING* result code sent from TA to TE. But we need "CNAP" comes after "CCWA", not after "RING." Maybe we should have a flow like "RING->CNAP->CCWA"

-- Keep getting a document specifying "number_plan"

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #1)
> AOSP ril.h defines
> 
> typedef struct {
>     char *          number;             /* Remote party number */
>     int             numberPresentation; /* 0=Allowed, 1=Restricted, 2=Not
> Specified/Unknown */
>     char *          name;               /* Remote party name */
>     RIL_CDMA_SignalInfoRecord signalInfoRecord;
>     /* Number type/Number plan required to support International Call
> Waiting */
>     int             number_type;        /* 0=Unknown, 1=International,
> 2=National,
>                                            3=Network specific, 4=subscriber
> */
>     int             number_plan;        /* 0=Unknown, 1=ISDN, 3=Data,
> 4=Telex, 8=Nat'l, 9=Private */
> } RIL_CDMA_CallWaiting_v6;
> 
> rild needs to respond this structure to gecko.
> 
> To retrieve these attributes (communication b/w rild and modem), we should
> follow AT command defined in TS 27.007
> -- "+CCWA:" is an unsolicited event for a waiting call. Per TS 27.007, the
> format is: +CCWA: <number>,<type>,<class>,[<alpha>][,<CLI
> validity>[,<subaddr>,<satype>[,<priority>]]]. Now we get "number"
> "numberPresentation" and "number_type".
> 
> -- "+CNAP:" 27.007 says, +CNAP: <name>[,<CNI validity>] response is returned
> after every *RING* result code sent from TA to TE. But we need "CNAP" comes
> after "CCWA", not after "RING." Maybe we should have a flow like
> "RING->CNAP->CCWA"
> 
> -- Keep getting a document specifying "number_plan"

-- Keep trying to get a document specifying "CDMA_SignalInfoRecord" as well

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

As this bug requires CNAP, set the dependency with bug 986362.

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

I haven't been here for a while.

Comment 5

[Ben Hsu [:HoPang]]

Now, we can make a waiting call to the emulator, and accepting the waiting call by the newly implemented CDMA flash handler. However, there are still many things to be implemented, including filling the attribute defined in |RIL_CDMA_CallWaiting_v6| as hsinyi mentioned in comment 1 and turning a waiting call into an incoming call when needed as filed in bug 1181009.

Comment 6

[Ben Hsu [:HoPang]]

Attachment: WIP testcase

This is the current testcase to test both the related WebAPIs and the emulator and open to any suggestion.

Comment 7

[Josh Cheng [:josh]]

Hi Ben,
Could you help to revise the target milestone?
Thanks!

Comment 8

[Ben Hsu [:HoPang]]

Sure(In reply to Josh Cheng [:josh] from comment #7)
> Could you help to revise the target milestone?

Sure, glad to help.

[Blocking Requested - why for this release]: Stabilty enhancement

Comment 9

[Josh Cheng [:josh]]

Hi Ben,
Thanks for the help! feel free to update the target milestone.

Comment 10

[Ben Hsu [:HoPang]]

Oh, thank you. I am so embarassed for misunderstanding your meaning before :P

Comment 11

[Ben Hsu [:HoPang]]

Attachment: Part 1: Modify some helper functions (head.js)

Comment 12

[Ben Hsu [:HoPang]]

Attachment: Part 2: Add a new testcase for CDMA call waiting (Testcase)

Comment 13

[Ben Hsu [:HoPang]]

Attachment: Part 1: Modify some helper functions (head.js)

Comment 14

[Ben Hsu [:HoPang]]

Attachment: Part 2: Add a new testcase for CDMA call waiting (Testcase)

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8661098 [details] [diff] [review]
Part 1: Modify some helper functions (head.js)

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

r=me after the nits are addressed.

thanks!

::: dom/telephony/test/marionette/head.js
@@ +980,1 @@
>        .then(event => {

nit: s/event/aResults

@@ +980,2 @@
>        .then(event => {
> +        let call = event[0].call;

ditto

@@ +1022,3 @@
>     * @return Promise<TelephonyCall>
>     */
> +  function remoteHangUp(aIdeniftier, aWaitForEvent = true) {

s/aIdeniftier/aIdentifier

@@ +1023,5 @@
>     */
> +  function remoteHangUp(aIdeniftier, aWaitForEvent = true) {
> +    let call;
> +    let number;
> +    if (typeof aIdeniftier === "string") { // A number is passed in.

ditto

@@ +1024,5 @@
> +  function remoteHangUp(aIdeniftier, aWaitForEvent = true) {
> +    let call;
> +    let number;
> +    if (typeof aIdeniftier === "string") { // A number is passed in.
> +      number = aIdeniftier;

ditto

@@ +1030,5 @@
> +        return aCall.id.number === number ||
> +               aCall.secondId && aCall.secondId.number === number;
> +      });
> +    } else { // A Telephony call is passed in.
> +      call = aIdeniftier;

ditto

@@ +1031,5 @@
> +               aCall.secondId && aCall.secondId.number === number;
> +      });
> +    } else { // A Telephony call is passed in.
> +      call = aIdeniftier;
> +      number = aIdeniftier.id.number;

ditto

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8661099 [details] [diff] [review]
Part 2: Add a new testcase for CDMA call waiting (Testcase)

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

Please see my comments below.

Thanks!

::: dom/telephony/test/marionette/test_cdma_call_waiting.js
@@ +13,5 @@
> +
> +let Numbers = ["0911111111",
> +               "0922222222"];
> +
> +function exptectedCall(aCall, aIndex, aState, aEmulatorState = null) {

nit: s/aIndex/aNumIndex

@@ +31,5 @@
> +
> +// First incoming, Second call waiting
> +function incoming(aNumber) {
> +  return Remote.dial(aNumber);
> +}

We have TelephonyHelper, Remote, Modem to represent different roles.
This wrapper makes this naming meaningless. :(

@@ +35,5 @@
> +}
> +
> +function answer(aCall) {
> +  return TelephonyHelper.answer(aCall);
> +}

Ditto

@@ +39,5 @@
> +}
> +
> +function hangUp(aCall) {
> +  return TelephonyHelper.hangUp(aCall);
> +}

Ditto

@@ +43,5 @@
> +}
> +
> +function remoteHangUp(aNumber, aWaitForEvent) {
> +  return Remote.hangUp(aNumber, aWaitForEvent);
> +}

Ditto

@@ +46,5 @@
> +  return Remote.hangUp(aNumber, aWaitForEvent);
> +}
> +
> +function flash(aCall) {
> +  return TelephonyHelper.hold(aCall, false);

I didn't see there is a boolean argument required in TelephonyHelper.hold()

@@ +58,5 @@
> +   --------------------------------+----------------+------------------------
> +   2-Way                           | N/A            | [connected]
> +   --------------------------------+----------------+------------------------
> +   2-Way Call Waiting Notification | CWN            | [connected, waiting],
> +                                   |                | [waiting, connected]

What's the difference of [connected, waiting] & [waiting, connected] here?
Will it be possible to switch these 2 calls during CWN?
If no, then we could remove the 2nd one.

@@ +63,5 @@
> +   --------------------------------+----------------+------------------------
> +   2-Way Call Waiting              | CW             | [connected, held],
> +                                   |                | [held, connected]       */
> +
> +let Opening = (function(){

nit: add <sp> between ){

@@ +101,5 @@
> +      // Return the call
> +      .then(() => call);
> +  }
> +
> +  function CW_Flash() {

CW_1_Flash

@@ +116,5 @@
> +      // Return the call
> +      .then(() => call);
> +  }
> +
> +  function CW_Flash_Flash() {

CW_2_Flashes

@@ +131,5 @@
> +      // Return the call
> +      .then(() => call);
> +  }
> +
> +  function CW_Flash_Flash_Flash() {

CW_3_Flashes

@@ +155,5 @@
> +    CW_Flash_Flash_Flash: CW_Flash_Flash_Flash
> +  };
> +})();
> +
> +let Ending = (function(){

nit: add <sp> between ){

@@ +221,5 @@
> +
> +    BHangUp_AHangUp: BHangUp_AHangUp,
> +    BHangUp_CHangUp: BHangUp_CHangUp,
> +    CHangUp_AHangUp: CHangUp_AHangUp,
> +    CHangUp_BHangUp: CHangUp_BHangUp

Please have a prefix of CWN_ or CW for these hangup functions and have
more explanation on the expected behavior after each hangup function is executed.

Comment 17

[Ben Hsu [:HoPang]]

Attachment: Part 1 (V2): Modify some helper functions (head.js). r=btseng

Comment improved

Comment 18

[Ben Hsu [:HoPang]]

Attachment: Part 2 (V2): Add a new testcase for CDMA call waiting (Testcase)

Comment 19

[Ben Hsu [:HoPang]]

Attachment: Part 2 (V3): Add a new testcase for CDMA call waiting (Testcase). r=btseng

Comment 20

[Ben Hsu [:HoPang]]

> Please have a prefix of CWN_ or CW for these hangup functions and have
> more explanation on the expected behavior after each hangup function is
> executed.

Since some of the ending functions are shared by both CWN and CW testcases, I think we should not separate them. To mitigate ambiguity, new comments are added.

Comment 21

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8671281 [details] [diff] [review]
Part 2 (V3): Add a new testcase for CDMA call waiting (Testcase). r=btseng

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

LGTM, thanks!

Comment 22

[Ben Hsu [:HoPang]]

Attachment: [hardware/ril] Pull request 68 (for Emulator-ICS)

Comment 23

[Ben Hsu [:HoPang]]

Attachment: [external/qemu] pull request 166 (for Emulator-ICS)

Comment 24

[Edgar Chen [:edgar]]

Comment on attachment 8672537 [details] [review]
[hardware/ril] Pull request 68 (for Emulator-ICS)

Please see my comments on github, thank you.

Comment 25

[Edgar Chen [:edgar]]

Comment on attachment 8672539 [details] [review]
[external/qemu] pull request 166 (for Emulator-ICS)

Per offline sync, cancel the review first.
Feel free to r? me again when you are ready. Thank you.

Comment 26

[Ben Hsu [:HoPang]]

Comment on attachment 8672537 [details] [review]
[hardware/ril] Pull request 68 (for Emulator-ICS)

Updated :P

----------------------------------------------------------------

Since CDMA doesn't use AT commands as the communication protocol between RIL daemon and modem, I think it's better to create a customized AT command to utilize the existing testing architecture without intruding the well-tested GSM functionalities, while the customized AT command can be extended for future need.

Comment 27

[Edgar Chen [:edgar]]

Comment on attachment 8672537 [details] [review]
[hardware/ril] Pull request 68 (for Emulator-ICS)

Looks good to me.

Comment 28

[Edgar Chen [:edgar]]

Comment on attachment 8672539 [details] [review]
[external/qemu] pull request 166 (for Emulator-ICS)

Just left some comments on github, thank you.

Comment 29

[Ben Hsu [:HoPang]]

Comment on attachment 8672539 [details] [review]
[external/qemu] pull request 166 (for Emulator-ICS)

Updated

Comment 30

[Edgar Chen [:edgar]]

Comment on attachment 8672539 [details] [review]
[external/qemu] pull request 166 (for Emulator-ICS)

Thank you.

Comment 31

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4186e2977a25&exclusion_profile=false

Comment 32

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d05d874a0ed4&exclusion_profile=false

Comment 33

[Edgar Chen [:edgar]]

I will help to check-in the patches, ni? to myself.

Comment 34

[Edgar Chen [:edgar]]

b2g-ril_v7:
https://github.com/mozilla-b2g/platform_hardware_ril/commit/5fa1b11a01e7e90dbe9681e690020d10795ec1a9

b2g-ics:
https://github.com/mozilla-b2g/platform_external_qemu/commit/af8b2b782387209cc5b5803926f86bdbc31570ca

b2g-kitkat:
https://github.com/mozilla-b2g/platform_hardware_ril/commit/3865e5be2dd7cb5007ca521922c25d301610d2f3
https://github.com/mozilla-b2g/platform_external_qemu/commit/5232a86eda9a6cc5edb51a355229aff65afec933
https://github.com/mozilla-b2g/platform_external_qemu/commit/da7e1df1e5f9c8a47935ce21272972687d1407b4
https://github.com/mozilla-b2g/platform_external_qemu/commit/2d2d29b5de14ce82f22a1a2d31d8d24e9f60357b
https://github.com/mozilla-b2g/platform_external_qemu/commit/2432fb9c0b8438b08b9fc12311e2074f9af6ec10
https://github.com/mozilla-b2g/platform_external_qemu/commit/30809c2bb8448d670490afad6b20bfc1719478c1

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/9bb256876625
https://hg.mozilla.org/integration/b2g-inbound/rev/153a1790611f

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9bb256876625
https://hg.mozilla.org/mozilla-central/rev/153a1790611f

Comment 37

[Edgar Chen [:edgar]]

Now the test is almost always timeout on ICS emulator [1]. Although we plans to shutdown ics tests, but I would still like to fix it on ICS. And it can be fixed by increasing the timeout value [2].

[1] https://treeherder.allizom.org/#/jobs?repo=b2g-inbound&filter-searchStr=mnw&fromchange=a7cf51ca566d&group_state=expanded&exclusion_profile=false
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3647a0ae9c90&group_state=expanded&exclusion_profile=false

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/d8d8ce02431c