1093014 - [B2G][RIL] handle in-call MMI codes correctly

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

Now gecko just sends the in-call MMI code dirrectly to carrier that is wrong. We should instead parse the code then ask for corresponding supplementary requests, AT+CHLD.

See  TS 22.030 Clause 6.5.5 http://www.etsi.org/deliver/etsi_ts/122000_122099/122030/10.00.00_60/ts_122030v100000p.pdf

Comment 1

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

Hey Aknow,
Would you mind taking this bug? Thank you.

Comment 2

[Szu-Yu Chen [:aknow] (inactive)]

Are we going to implement all the codes described in the spec or just part of them for the user story of bug 890912 ?

During a call:
1 SEND: retrieve a call previously put on hold
2 SEND: put on hold an active call
3 SEND: put the active call and the held one on a multiconference (merge)

Comment 3

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1: Refactoring

Comment 4

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2: Test in call MMI

Comment 5

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3: Implement in call MMI

Comment 6

[Szu-Yu Chen [:aknow] (inactive)]

Support in call MMI: 1, 2, 3
Let me know if we really need to support 1X, 2X

Comment 7

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

(In reply to Szu-Yu Chen [:aknow] from comment #6)
> Support in call MMI: 1, 2, 3
> Let me know if we really need to support 1X, 2X

Yes, we need them :( And code 0, but I noticed you already considered that.

B.t.w., thank you for the quick patches!

Comment 8

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#2: Test in call MMI

Comment 9

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3#2: Implement in call MMI

Comment 10

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

Comment on attachment 8522121 [details] [diff] [review]
Part 1: Refactoring

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

::: dom/system/gonk/ril_worker.js
@@ +1905,5 @@
> +      return;
> +    }
> +
> +    if (this.currentConferenceState === CALL_STATE_ACTIVE) {
> +      this.hangUpForeground(options);

This reminds me that we forgot to set 'hangupLocal' for conference participants. Please file a follow-up bug for the fix, thank you.

Comment 11

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

Comment on attachment 8522159 [details] [diff] [review]
Part 2#2: Test in call MMI

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

A main concern is about the potential timing issue, please see comments inline, thank you!

::: dom/telephony/test/marionette/test_incall_mmi_call_hold.js
@@ +32,5 @@
> +    .then(calls => [call1, call2] = calls)
> +    // Hangup held call.
> +    .then(() => gSendMMI("0"))
> +    .then(() => gWaitForNamedStateEvent(call1, "disconnected"))
> +    .then(() => gCheckAll(call2, [call2], "", [], [call2Info.active]))

Is there any chance that call1.disconnected is received but call2 state isn't ready? I may be too cautious about this type of timing problems...

@@ +33,5 @@
> +    // Hangup held call.
> +    .then(() => gSendMMI("0"))
> +    .then(() => gWaitForNamedStateEvent(call1, "disconnected"))
> +    .then(() => gCheckAll(call2, [call2], "", [], [call2Info.active]))
> +    .then(() => gRemoteHangUpCalls([call2]));

nit: gRemoteHangUp is also working

@@ +44,5 @@
> +    .then(calls => [call1, call2] = calls)
> +    // Hangup current call, resume held call.
> +    .then(() => gSendMMI("1"))
> +    .then(() => gWaitForNamedStateEvent(call1, "connected"))
> +    .then(() => gCheckAll(call1, [call1], "", [], [call1Info.active]))

ditto: timing b/w call1.connected and call2.disconnected

@@ +45,5 @@
> +    // Hangup current call, resume held call.
> +    .then(() => gSendMMI("1"))
> +    .then(() => gWaitForNamedStateEvent(call1, "connected"))
> +    .then(() => gCheckAll(call1, [call1], "", [], [call1Info.active]))
> +    .then(() => gRemoteHangUpCalls([call1]));

ditto

@@ +55,5 @@
> +  return setupTwoCalls()
> +    .then(calls => [call1, call2] = calls)
> +    // Hold current call, resume held call.
> +    .then(() => gSendMMI("2"))
> +    .then(() => gWaitForNamedStateEvent(call1, "connected"))

ditto: timing

@@ +60,5 @@
> +    .then(() => gCheckAll(call1, [call1, call2], "", [],
> +                          [call1Info.active, call2Info.held]))
> +    // Hold current call, resume held call.
> +    .then(() => gSendMMI("2"))
> +    .then(() => gWaitForNamedStateEvent(call2, "connected"))

ditto: timing

::: dom/telephony/test/marionette/test_incall_mmi_call_waiting.js
@@ +28,5 @@
> +    .then(calls => [outCall, inCall] = calls)
> +    // Hangup waiting call.
> +    .then(() => gSendMMI("0"))
> +    .then(() => gWaitForNamedStateEvent(inCall, "disconnected"))
> +    .then(() => gCheckAll(outCall, [outCall], "", [], [outInfo.active]))

ditto: timing

@@ +40,5 @@
> +    .then(calls => [outCall, inCall] = calls)
> +    // Hangup current call, accept waiting call.
> +    .then(() => gSendMMI("1"))
> +    .then(() => gWaitForNamedStateEvent(inCall, "connected"))
> +    .then(() => gCheckAll(inCall, [inCall], "", [], [inInfo.active]))

ditto: timing

@@ +53,5 @@
> +    // Hold current call, accept waiting call.
> +    .then(() => gSendMMI("2"))
> +    .then(() => gWaitForNamedStateEvent(inCall, "connected"))
> +    .then(() => gCheckAll(inCall, [outCall, inCall], "", [],
> +                          [outInfo.held, inInfo.active]))

ditto: timing

Comment 12

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

Comment on attachment 8522160 [details] [diff] [review]
Part 3#2: Implement in call MMI

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

Looks good to me, thank you.

Comment 13

[Szu-Yu Chen [:aknow] (inactive)]

> A main concern is about the potential timing issue, please see comments
> inline, thank you!
> 
> ::: dom/telephony/test/marionette/test_incall_mmi_call_hold.js
> @@ +32,5 @@
> > +    .then(calls => [call1, call2] = calls)
> > +    // Hangup held call.
> > +    .then(() => gSendMMI("0"))
> > +    .then(() => gWaitForNamedStateEvent(call1, "disconnected"))
> > +    .then(() => gCheckAll(call2, [call2], "", [], [call2Info.active]))
> 
> Is there any chance that call1.disconnected is received but call2 state
> isn't ready? I may be too cautious about this type of timing problems...

This one is okay. There won't be any state change for call2 (remained active).
 
> @@ +33,5 @@
> > +    // Hangup held call.
> > +    .then(() => gSendMMI("0"))
> > +    .then(() => gWaitForNamedStateEvent(call1, "disconnected"))
> > +    .then(() => gCheckAll(call2, [call2], "", [], [call2Info.active]))
> > +    .then(() => gRemoteHangUpCalls([call2]));
> 
> nit: gRemoteHangUp is also working

I'd like to use gRemoteHangUpCalls. Using this, it provides some kinds of consistency among all sub tests.

> @@ +44,5 @@
> > +    .then(calls => [call1, call2] = calls)
> > +    // Hangup current call, resume held call.
> > +    .then(() => gSendMMI("1"))
> > +    .then(() => gWaitForNamedStateEvent(call1, "connected"))
> > +    .then(() => gCheckAll(call1, [call1], "", [], [call1Info.active]))
> 
> ditto: timing b/w call1.connected and call2.disconnected

Currently, no problem for this one. Our implementation (processCall) guarantees all disconnected events are fired before statechanges. However, make it safer is better.

> 
> @@ +45,5 @@
> > +    // Hangup current call, resume held call.
> > +    .then(() => gSendMMI("1"))
> > +    .then(() => gWaitForNamedStateEvent(call1, "connected"))
> > +    .then(() => gCheckAll(call1, [call1], "", [], [call1Info.active]))
> > +    .then(() => gRemoteHangUpCalls([call1]));
> 
> ditto
> 
> @@ +55,5 @@
> > +  return setupTwoCalls()
> > +    .then(calls => [call1, call2] = calls)
> > +    // Hold current call, resume held call.
> > +    .then(() => gSendMMI("2"))
> > +    .then(() => gWaitForNamedStateEvent(call1, "connected"))
> 
> ditto: timing

Agree with you. Need to fix.

> 
> @@ +60,5 @@
> > +    .then(() => gCheckAll(call1, [call1, call2], "", [],
> > +                          [call1Info.active, call2Info.held]))
> > +    // Hold current call, resume held call.
> > +    .then(() => gSendMMI("2"))
> > +    .then(() => gWaitForNamedStateEvent(call2, "connected"))
> 
> ditto: timing

Agree with you. Need to fix.

> ::: dom/telephony/test/marionette/test_incall_mmi_call_waiting.js
> @@ +28,5 @@
> > +    .then(calls => [outCall, inCall] = calls)
> > +    // Hangup waiting call.
> > +    .then(() => gSendMMI("0"))
> > +    .then(() => gWaitForNamedStateEvent(inCall, "disconnected"))
> > +    .then(() => gCheckAll(outCall, [outCall], "", [], [outInfo.active]))
> 
> ditto: timing

no problem

> 
> @@ +40,5 @@
> > +    .then(calls => [outCall, inCall] = calls)
> > +    // Hangup current call, accept waiting call.
> > +    .then(() => gSendMMI("1"))
> > +    .then(() => gWaitForNamedStateEvent(inCall, "connected"))
> > +    .then(() => gCheckAll(inCall, [inCall], "", [], [inInfo.active]))
> 
> ditto: timing

Agree with you. Need to fix.

> @@ +53,5 @@
> > +    // Hold current call, accept waiting call.
> > +    .then(() => gSendMMI("2"))
> > +    .then(() => gWaitForNamedStateEvent(inCall, "connected"))
> > +    .then(() => gCheckAll(inCall, [outCall, inCall], "", [],
> > +                          [outInfo.held, inInfo.active]))
> 
> ditto: timing

Agree with you. Need to fix.

Comment 14

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#3: Test in call MMI

Comment 15

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

Comment on attachment 8526561 [details] [diff] [review]
Part 2#3: Test in call MMI

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

Looks really good, thank you.

Comment 16

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 1: Refactoring. r=hsinyi

Comment 17

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 2: Test in call MMI. r=hsinyi

Comment 18

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 3: Implement in call MMI. r=hsinyi

Comment 19

[Szu-Yu Chen [:aknow] (inactive)]

try looks good
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c4e31a6fa0f1

Comment 20

[Szu-Yu Chen [:aknow] (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/d7462aafbd1a
https://hg.mozilla.org/integration/b2g-inbound/rev/838ed7eb8560
https://hg.mozilla.org/integration/b2g-inbound/rev/c06f2c1e0d40

Comment 21

[Carsten Book [:Tomcat]]

(In reply to Szu-Yu Chen [:aknow] from comment #19)
> try looks good
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c4e31a6fa0f1

yeah but sorry had to back this out for the X1 Test failures https://treeherder.mozilla.org/logviewer.html#?job_id=969075&repo=b2g-inbound that became permanent

Comment 22

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final#2] Part 3: Implement in call MMI. r=hsinyi

Comment 23

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Carsten Book [:Tomcat] from comment #21)
> (In reply to Szu-Yu Chen [:aknow] from comment #19)
> > try looks good
> > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c4e31a6fa0f1
> 
> yeah but sorry had to back this out for the X1 Test failures
> https://treeherder.mozilla.org/logviewer.html#?job_id=969075&repo=b2g-
> inbound that became permanent

Sorry. It's my fault.

The problem is fixed
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d1ae4be2c8a4

Comment 24

[Szu-Yu Chen [:aknow] (inactive)]

land again

https://hg.mozilla.org/integration/b2g-inbound/rev/95e8b9ca4957
https://hg.mozilla.org/integration/b2g-inbound/rev/a9e9218619c9
https://hg.mozilla.org/integration/b2g-inbound/rev/65de6bc63a10

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/95e8b9ca4957
https://hg.mozilla.org/mozilla-central/rev/a9e9218619c9
https://hg.mozilla.org/mozilla-central/rev/65de6bc63a10