Closed Bug 975778 Opened 6 years ago Closed 4 years ago

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

Categories

(Firefox OS Graveyard :: Emulator, defect)

x86_64
Linux
defect
Not set

Tracking

(feature-b2g:2.5+, firefox46 fixed)

RESOLVED FIXED
FxOS-S5 (21Aug)
feature-b2g 2.5+
Tracking Status
firefox46 --- fixed

People

(Reporter: hsinyi, Assigned: bhsu)

References

Details

(Whiteboard: [ft:ril] [p=8])

Attachments

(4 files, 6 obsolete files)

Support REQUEST_CDMA_FLASH on reference-ril
Blocks: 975779
Assignee: nobody → htsai
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"
(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
As this bug requires CNAP, set the dependency with bug 986362.
Depends on: 986362
Whiteboard: [ft:ril] [p=8]
Target Milestone: --- → 1.4 S6 (25apr)
blocking-b2g: --- → backlog
Summary: [B2G] [Emulator] Support CDMA_FLASH command for Cdma call waiting and 3way calling senario → [B2G] [Emulator] Support the request RIL_UNSOL_CDMA_CALL_WAITING for Cdma call waiting
blocking-b2g: backlog → ---
I haven't been here for a while.
Assignee: htsai → nobody
Assignee: nobody → bhsu
Depends on: 891707
No longer depends on: 986362
Blocks: 1181009
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.
No longer blocks: 1181009
Attached file WIP testcase (obsolete) —
This is the current testcase to test both the related WebAPIs and the emulator and open to any suggestion.
Hi Ben,
Could you help to revise the target milestone?
Thanks!
Status: NEW → ASSIGNED
Flags: needinfo?(bhsu)
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
blocking-b2g: --- → 2.5?
Flags: needinfo?(bhsu)
blocking-b2g: 2.5? → 2.5+
Hi Ben,
Thanks for the help! feel free to update the target milestone.
Flags: needinfo?(bhsu)
Target Milestone: 1.4 S6 (25apr) → FxOS-S5 (21Aug)
Oh, thank you. I am so embarassed for misunderstanding your meaning before :P
Flags: needinfo?(bhsu)
blocking-b2g: 2.5+ → ---
feature-b2g: --- → 2.5+
Attachment #8630313 - Attachment is obsolete: true
Attachment #8661085 - Attachment is obsolete: true
Attachment #8661086 - Attachment is obsolete: true
Attachment #8661098 - Flags: review?(btseng)
Attachment #8661099 - Attachment description: Part 1: Modify some helper functions (head.js) → Part 2: Add a new testcase for CDMA call waiting (Testcase)
Attachment #8661099 - Flags: review?(btseng)
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
Attachment #8661098 - Flags: review?(btseng) → review+
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.
Attachment #8661099 - Flags: review?(btseng)
Comment improved
Attachment #8661098 - Attachment is obsolete: true
Attachment #8671271 - Flags: review+
Attachment #8661099 - Attachment is obsolete: true
Attachment #8671273 - Flags: review?(btseng)
Attachment #8671273 - Flags: review?(btseng)
Attachment #8671271 - Attachment description: Part 1 (V2): Modify some helper functions (head.js) → Part 1 (V2): Modify some helper functions (head.js). r=btseng
> 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.
Attachment #8671281 - Flags: review?(btseng)
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!
Attachment #8671281 - Flags: review?(btseng) → review+
Attachment #8672537 - Flags: review?(echen)
Comment on attachment 8672537 [details] [review]
[hardware/ril] Pull request 68 (for Emulator-ICS)

Please see my comments on github, thank you.
Attachment #8672537 - Flags: review?(echen)
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.
Attachment #8672539 - Flags: review?(echen)
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.
Attachment #8672537 - Flags: review?(echen)
Attachment #8672539 - Flags: review?(echen)
Comment on attachment 8672537 [details] [review]
[hardware/ril] Pull request 68 (for Emulator-ICS)

Looks good to me.
Attachment #8672537 - Flags: review?(echen) → review+
Comment on attachment 8672539 [details] [review]
[external/qemu] pull request 166 (for Emulator-ICS)

Just left some comments on github, thank you.
Attachment #8672539 - Flags: review?(echen)
Comment on attachment 8672539 [details] [review]
[external/qemu] pull request 166 (for Emulator-ICS)

Updated
Attachment #8672539 - Flags: review?(echen)
Comment on attachment 8672539 [details] [review]
[external/qemu] pull request 166 (for Emulator-ICS)

Thank you.
Attachment #8672539 - Flags: review?(echen) → review+
Keywords: checkin-needed
I will help to check-in the patches, ni? to myself.
Flags: needinfo?(echen)
Keywords: checkin-needed
Flags: needinfo?(echen)
Attachment #8671281 - Attachment description: Part 2 (V3): Add a new testcase for CDMA call waiting (Testcase) → Part 2 (V3): Add a new testcase for CDMA call waiting (Testcase). r=btseng
https://hg.mozilla.org/mozilla-central/rev/9bb256876625
https://hg.mozilla.org/mozilla-central/rev/153a1790611f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
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
Duplicate of this bug: 975779
You need to log in before you can comment on or make changes to this bug.