869902 - [Bluetooth] Unable to accept MT call by PCM CK

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect, P1)

Description

[leo.bugzilla.gecko]

Attachment: porsche_call_air_log

1. Title : [BT/ BT CAR KIT] - Unable to accept MT call by PCM CK
2. Precondition : Pair and connect to Porsche PCM
3. Tester's Action : Receive MT call. Try to accept it by CK
4. Detailed Symptom (ENG.) : Unable to accept MT call using CK button
5. Reproducibility: Y
6. Frequency Rate : 100%

Dear Mozilla Team,

Incoming call cannot be accepted by Porsche PCM Carkit.
If accept button click, incoming call screen appears again after about one second.
In the air log, carkit send CHLD=1 to b2g phone.
I attached the air log and repoduced movie, please check it.

Thanks.

Comment 1

[leo.bugzilla.gecko]

Attachment: porsche_call_reproduced_movie

Comment 2

[Alex Keybl [:akeybl]]

Not blocking until it's clear this will impact a large number of CKs and it's also clear that we're doing something wrong (as opposed to a car issue).

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

From my observation, we sent "+CLLC:1,1,5,0,0, "582513", 129. We are somehow makes things incorrect here. We shall send indicator of status is "4" (incoming call) not "5" (waiting call). This looks like really bad if car kit actually checks +CLCC parameter, which decides to switch on incoming call/waiting call mode.
I guess this is why car kit tries to send "CHLD=1", because "5" means one call held!

This also impact v1.0.1. I'm surprised doing certification, this incorrect parameter.
Unless mCurrentCallIndex is not 1.
        case nsIRadioInterfaceLayer::CALL_STATE_INCOMING:
          message.AppendInt((i == mCurrentCallIndex) ? 4 : 5);

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Ok, it looks like mCurrentCallIndex will be assigned unless the 1st call changes to call connected. So we're fine with 3way call situation, however, in 2 way call, we probably screw up CLCC. Since we don't have PCM car kit, can leo help to give it a quick try to prove this theory is correct?

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I cannot enumerate how many car kits get involved but it shall be fixed into v.1.1.

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

> This also impact v1.0.1. I'm surprised doing certification, this incorrect
> parameter.
I'm surprised when doing certification, this incorrect parameter problem was not been identified.

Comment 7

[leo.bugzilla.gecko]

Dear Mozilla Team,

I'll test with PCM Carkit in our side and discuss with you again.

Thanks.

Comment 8

[leo.bugzilla.gecko]

Dear Mozilla Team,

I have tested it with PCM carkit.
As your comment, 

==> message.AppendInt((i == mCurrentCallIndex) ? 4 : 5);

this code is always assigned 5. So carkit send the CHLD=1 to phone.
So, I fixed this as below.

==> message.AppendInt((i == 1) ? 4 : 5);

And then carkit send the ATA command to phone normally and I can receive an incoming call by answer button of carkit.

Good to here. But in this state, when second incoming call is received, carkit does not have any behavior but the second call is displayed on phone.

I attached the log, please check this.
Thanks.

Comment 9

[leo.bugzilla.gecko]

Attachment: fix_CLCC_log

Comment 10

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Thanks again for trying with PCM CK! I got your point, and this is a shameful bug. The second call CLCC with +CLCC:2,1,5,0,0, "",129. How come the 2nd call number is missing!

Comment 11

[Alex Keybl [:akeybl]]

Do you need any more information here from partners/QA to help your investigation?

Comment 12

[Gina Yeh [:gyeh] [:ginayeh]]

> ==> message.AppendInt((i == 1) ? 4 : 5); 

I don't think this would be a good idea. Please refer my attachment in Bug 875719. That patch should fix the issue of wrong status in CLCC. Please let me know if we still fail to pass the test case. Thanks.

Comment 13

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Shawn Huang from comment #10)
> Thanks again for trying with PCM CK! I got your point, and this is a
> shameful bug. The second call CLCC with +CLCC:2,1,5,0,0, "",129. How come
> the 2nd call number is missing!

It seems like we failed to get call number from RIL?

05-21 08:23:15.429 I/Gecko   (  153): -*- RILContentHelper: Received message 'RIL:CallStateChanged': {"state":11,"callIndex":2,"toa":129,"isMpty":false,"isMT":true,"als":0,"isVoice":true,"isVoicePrivacy":false,"number":null,"numberPresentation":1,"name":null,"namePresentation":1,"uusInfo":null,"isActive":false}

05-21 08:23:15.439 V/BluetoothHfpManager(  153): HandleCallStateChanged:1238 [LGBT_gecko] aCallIndex = 2, aCallState = 11, aSend = 1

Comment 14

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Gina is right, I only checked air trace, so I missed something and left stupid comments on Comment 10. As Comment 13 shows exactly what happened, I also checked mozilla ril, it seems to work fine. So please help to check your RIL code first.
I set this bug dependency on Bug 875719 for CLCC call status.
Thanks.

Comment 15

[leo.bugzilla.gecko]

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #12)
> > ==> message.AppendInt((i == 1) ? 4 : 5); 
> 
> I don't think this would be a good idea. Please refer my attachment in Bug
> 875719. That patch should fix the issue of wrong status in CLCC. Please let
> me know if we still fail to pass the test case. Thanks.

I have fixed this code as temporary for right working. bug 875719 is right.
However, this code does not seem enough.
Because it does not consider the incoming situation.

So, patch is needed as below

case nsIRadioInterfaceLayer::CALL_STATE_INCOMING:
     mCurrentCallIndex = aCallIndex; => added

     if (mCurrentCallIndex)     { => //before.
     if (mCurrentCallIndex > 1) { => //after. When mCurrentCallIndex is 2, CCWA is worked.

And in CLCC command should be fixed as below,

    case nsIRadioInterfaceLayer::CALL_STATE_INCOMING:
        message.AppendInt((i == mCurrentCallIndex) ? 4 : 5);  //before
        message.AppendInt((mCurrentCallIndex == 1) ? 4 : 5);  //after

Because i == mCurrentCallIndex is always same value, so if second call is receiving, CLCC command send the 4 value always.

Please review this point.
Thanks.

Comment 16

[Gina Yeh [:gyeh] [:ginayeh]]

Is there any specific case for the proposed changes in comment 15?

The value of |mCurrentCallIndex| means the index of current "active" call object in |mCurrentCallArray|, and that's why I didn't assign a new value to |mCurrentCallIndex| when there's an incoming call.

However, I would like to make sure that I didn't miss any case. So, please let me know if the changes in comment 15 is necessary. Thanks.

Comment 17

[leo.bugzilla.gecko]

I understand your comment about |mCurrentCallIndex|.
But if value of |mCurrentCallIndex| means the index of current "active" call, CALL_STATE_DIALING seems to be a non-active state. 
I think that if outgoing is active, incoming is same, too.
I wonder that you have not assigned a this value when incoming call.

And I have modified that code is for bluetooth CLCC command.
If first call is incoming, parameter value should set the 4 and when a second call set the 5. This is defined in the Bluetooth specification.
If it does not correct, the car kit does not work normally.

Please discuss with your bluetooth team and fix to good solution. :)
Thanks.

Comment 18

[Gina Yeh [:gyeh] [:ginayeh]]

For CLCC part, I'm afraid that we couldn't modify our code like this way since it would generate side effect.

Let's say we have two calls now, the first is active, and the second is on hold.
Then, we hang up the first call and answer the second call.
After that, there's incoming call, which is the third call.

In this case, the value mCurrentCallIndex should be 2, and we'll fail to set correct status for CLCC response code.

Comment 19

[Gina Yeh [:gyeh] [:ginayeh]]

As for mCurrentCallIndex, please see my new patch in Bug 875719. In order to clarify the meaning of variable, it is renamed as mActiveCallIndex which is re-assigned whenever a SCO is set up.

Thanks for your suggestion. We'll check whether the new solution works or not.

Comment 20

[Eric Chou [:ericchou] [:echou]]

This problem should be fixed if the patch of bug 875719 is applied. The patch has just landed in birch and we still need to wait for triage to decide if it's a tef+ or a leo+. Please retry after it's merged into b2g18.

Thank you.

Comment 21

[Wayne Chang [:wchang]]

bug 875719 has just been tef+'ed and should get uplifted to b2g18 in the next day or so as well. please retest per comment 20.

Comment 22

[leo.bugzilla.gecko]

Dear Mozilla Team,

This bug is fixed on leo device after bug 875719 is applied.
Thanks for your help.

Comment 23

[Eric Chou [:ericchou] [:echou]]

Hi Leo,

Thanks.

Close this per comment 22.