[PATCHv2 1/2] hfpmodem: Retry AT+CLCC request after outgoing callsetup

[PATCHv2 1/2] hfpmodem: Retry AT+CLCC request after outgoing callsetup

[Timo Mueller]

[-- Attachment #1: Type: text/plain, Size: 2215 bytes --]

From: Timo Mueller <timo.mueller@bmw-carit.de>

Currently the list of current calls is requested right after an
outgoing callsetup has been reported by the AG device. If the AG
device updates the list after reporting the callsetup the +CLCC
response may not contain the new call and it is not registered
with oFono.

In this case AT+CLCC should be requested again once after a small
delay.

AT sequence that exhibited the failure (AG device was a Nokia N9
placing an outgoing call)

< \r\n+CIEV: 5,2\r\n
> AT+CLCC\r
< \r\nOK\r\n
< \r\n+VGS=7\r\n
< \r\n+VGM=7\r\n
< \r\n+CIEV: 5,3\r\n
< \r\n+CIEV: 4,1\r\n
< \r\n+CIEV: 5,0\r\n
---
v2: Request CLCC immediately and schedule a second request if the
call was not found in the response. Recheck CLCC when callheld=2 was
received after a callsetup=<2 or 3>.

 drivers/hfpmodem/voicecall.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/hfpmodem/voicecall.c b/drivers/hfpmodem/voicecall.c
index 33dd05e..18ed54a 100644
--- a/drivers/hfpmodem/voicecall.c
+++ b/drivers/hfpmodem/voicecall.c
@@ -295,6 +295,31 @@ static void clcc_poll_cb(gboolean ok, GAtResult *result, gpointer user_data)
 							vc);
 }
 
+static void clcc_poll_dialing_cb(gboolean ok, GAtResult *result,
+			gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	GSList *calls, *dialing;
+
+	if (!ok)
+		return;
+
+	calls = at_util_parse_clcc(result);
+	dialing = find_dialing(calls);
+
+	if (dialing == NULL) {
+		if (vd->clcc_source)
+			g_source_remove(vd->clcc_source);
+
+		vd->clcc_source = g_timeout_add(POLL_CLCC_DELAY,
+												poll_clcc, vc);
+		return;
+	}
+
+	clcc_poll_cb(ok, result, user_data);
+}
+
 static gboolean poll_clcc(gpointer user_data)
 {
 	struct ofono_voicecall *vc = user_data;
@@ -977,7 +1002,8 @@ static void ciev_callsetup_notify(struct ofono_voicecall *vc,
 		 * from AG: query and create call.
 		 */
 		g_at_chat_send(vd->chat, "AT+CLCC", clcc_prefix,
-				clcc_poll_cb, vc, NULL);
+				clcc_poll_dialing_cb, vc, NULL);
+
 		break;
 
 	case 3:
-- 
1.7.11.7

[PATCHv2 2/2] hfpmodem: Request AT+CLCC again after callheld=<2 or 3>

[Timo Mueller]

[-- Attachment #1: Type: text/plain, Size: 2095 bytes --]

From: Timo Mueller <timo.mueller@bmw-carit.de>

With a call being active, placing a second call on the AG device will
lead to the active call being put on hold. During the transition some
phones do not update the list of current calls. This leads to an +CLCC
response that does not contain the new outgoing call, even though an
outgoing callsetup was reported.

The list will be updated once the active call was put on hold and
callheld=2 is reported. In this case the list has to be requested
again (AT+CLCC).

AT sequence that exhibited the failure (AG device was a Nokia N9
having an active outgoing call and placing a second outgoing call)

< \r\n+CIEV: 5,2\r\n
> AT+CLCC\r
< \r\n+CLCC: 1,0,0,0,0,"+49xxx1",145\r\n
< \r\nOK\r\n
< \r\n+CIEV: 6,2\r\n
< \r\n+CIEV: 5,3\r\n
< \r\n+CIEV: 6,1\r\n
> AT+CLCC\r
< \r\n+CIEV: 5,0\r\n
< \r\n+CLCC: 1,0,1,0,0,"+49xxx1",145\r\n
< \r\n+CLCC: 2,0,0,0,0,"+49xxx2",145\r\n
< \r\nOK\r\n
---
 drivers/hfpmodem/voicecall.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/hfpmodem/voicecall.c b/drivers/hfpmodem/voicecall.c
index 18ed54a..482b325 100644
--- a/drivers/hfpmodem/voicecall.c
+++ b/drivers/hfpmodem/voicecall.c
@@ -1035,8 +1035,10 @@ static void ciev_callheld_notify(struct ofono_voicecall *vc,
 {
 	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
 	GSList *l;
+	GSList *dialing;
 	struct ofono_call *call;
 	unsigned int callheld = vd->cind_val[HFP_INDICATOR_CALLHELD];
+	unsigned int callsetup = vd->cind_val[HFP_INDICATOR_CALLSETUP];
 
 	switch (value) {
 	case 0:
@@ -1072,6 +1074,14 @@ static void ciev_callheld_notify(struct ofono_voicecall *vc,
 				call->status = CALL_STATUS_HELD;
 				ofono_voicecall_notify(vc, call);
 			}
+
+			if (callsetup == 2 || callsetup == 3) {
+				dialing = find_dialing(vd->calls);
+
+				if (dialing == NULL)
+					g_at_chat_send(vd->chat, "AT+CLCC", clcc_prefix,
+						clcc_poll_cb, vc, NULL);
+			}
 		} else if (callheld == 1) {
 			if (vd->clcc_source)
 				g_source_remove(vd->clcc_source);
-- 
1.7.11.7

Re: [PATCHv2 2/2] hfpmodem: Request AT+CLCC again after callheld=<2 or 3>

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2279 bytes --]

Hi Timo,

On 01/28/2013 04:13 AM, Timo Mueller wrote:
> From: Timo Mueller<timo.mueller@bmw-carit.de>
>
> With a call being active, placing a second call on the AG device will
> lead to the active call being put on hold. During the transition some
> phones do not update the list of current calls. This leads to an +CLCC
> response that does not contain the new outgoing call, even though an
> outgoing callsetup was reported.
>
> The list will be updated once the active call was put on hold and
> callheld=2 is reported. In this case the list has to be requested
> again (AT+CLCC).
>
> AT sequence that exhibited the failure (AG device was a Nokia N9
> having an active outgoing call and placing a second outgoing call)
>

Ugh.

<snip>

> @@ -1072,6 +1074,14 @@ static void ciev_callheld_notify(struct ofono_voicecall *vc,
>   				call->status = CALL_STATUS_HELD;
>   				ofono_voicecall_notify(vc, call);
>   			}
> +
> +			if (callsetup == 2 || callsetup == 3) {
> +				dialing = find_dialing(vd->calls);
> +
> +				if (dialing == NULL)
> +					g_at_chat_send(vd->chat, "AT+CLCC", clcc_prefix,
> +						clcc_poll_cb, vc, NULL);
> +			}
>   		} else if (callheld == 1) {
>   			if (vd->clcc_source)
>   				g_source_remove(vd->clcc_source);

I'm seeing reports of too many devices doing too many weird things, and 
if we change the core logic for each and every one then the code becomes 
a huge mess quickly.  It would be hard to validate or regression test it 
without massive investment in automated interoperability testing.

Marcel and I spoke briefly about this and our belief is that we need to 
introduce manufacturer specific quirks to HFP drivers instead of trying 
to solve this generically.

BlueZ should be able to provide us enough information about the remote 
device for us to make a fairly good guess as to the manufacturer / 
version.  Additional details can be queried by manufacturer specific 
commands (e.g. in the case of iOS).  That should be enough for us to 
simply set the vendor / quirk ID and take appropriate action in the driver.

We can always fall back to strict spec compliant behavior by default if 
the manufacturer quirks cannot be determined.  What do you think?

Regards,
-Denis

Re: [PATCHv2 2/2] hfpmodem: Request AT+CLCC again after callheld=<2 or 3>

[Timo =?unknown-8bit?q?M=C3=BCller?=]

[-- Attachment #1: Type: text/plain, Size: 3756 bytes --]

Hi Denis,

Denis Kenzior wrote, On 31.01.2013 05:09:
> Hi Timo,
>
> On 01/28/2013 04:13 AM, Timo Mueller wrote:
>> From: Timo Mueller<timo.mueller@bmw-carit.de>
>>
>> With a call being active, placing a second call on the AG device will
>> lead to the active call being put on hold. During the transition some
>> phones do not update the list of current calls. This leads to an +CLCC
>> response that does not contain the new outgoing call, even though an
>> outgoing callsetup was reported.
>>
>> The list will be updated once the active call was put on hold and
>> callheld=2 is reported. In this case the list has to be requested
>> again (AT+CLCC).
>>
>> AT sequence that exhibited the failure (AG device was a Nokia N9
>> having an active outgoing call and placing a second outgoing call)
>>
>
> Ugh.
>
> <snip>
>
>> @@ -1072,6 +1074,14 @@ static void ciev_callheld_notify(struct
>> ofono_voicecall *vc,
>>                   call->status = CALL_STATUS_HELD;
>>                   ofono_voicecall_notify(vc, call);
>>               }
>> +
>> +            if (callsetup == 2 || callsetup == 3) {
>> +                dialing = find_dialing(vd->calls);
>> +
>> +                if (dialing == NULL)
>> +                    g_at_chat_send(vd->chat, "AT+CLCC", clcc_prefix,
>> +                        clcc_poll_cb, vc, NULL);
>> +            }
>>           } else if (callheld == 1) {
>>               if (vd->clcc_source)
>>                   g_source_remove(vd->clcc_source);
>
> I'm seeing reports of too many devices doing too many weird things, and
> if we change the core logic for each and every one then the code becomes
> a huge mess quickly.  It would be hard to validate or regression test it
> without massive investment in automated interoperability testing.
>
> Marcel and I spoke briefly about this and our belief is that we need to
> introduce manufacturer specific quirks to HFP drivers instead of trying
> to solve this generically.
>
> BlueZ should be able to provide us enough information about the remote
> device for us to make a fairly good guess as to the manufacturer /
> version.  Additional details can be queried by manufacturer specific
> commands (e.g. in the case of iOS).  That should be enough for us to
> simply set the vendor / quirk ID and take appropriate action in the driver.
>
> We can always fall back to strict spec compliant behavior by default if
> the manufacturer quirks cannot be determined.  What do you think?

I agree, that the core logic shouldn't be polluted with phone specific 
workarounds. This is especially true when the quirks somehow interfere 
or modify the standard path. Having said this, I'd consider this fix 
general enough to justify it's integration in the standard driver, since 
the policy makes sense for any phone (mis)behaving this way, keeping the 
phone specific quirks to an absolute minimum.

I know that the N9 is the only phone that has seen this problem so far, 
but the problem will occur with every phone that doesn't "atomically" 
change the CLCC list the moment it reports an indicator change (+CIEV).

As described in the commit message the phone already told the HF that it 
has set up an outgoing call (or that it's already alerting). The  only 
thing missing is the detailed information about the call, which is 
retrieved using the AT+CLCC request.

So in this case I think it's valid to generally try to fill in the 
missing information by issuing another AT+CLCC request. Otherwise the 
call will be alerting and although the phone has reported all indicator 
changes to oFono, we won't be able to reasonably control it from HF side.

>
> Regards,
> -Denis

Best regards,
Timo

Re: [PATCHv2 2/2] hfpmodem: Request AT+CLCC again after callheld=<2 or 3>

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]

Hi Timo,

> I agree, that the core logic shouldn't be polluted with phone specific
> workarounds. This is especially true when the quirks somehow interfere
> or modify the standard path. Having said this, I'd consider this fix
> general enough to justify it's integration in the standard driver, since
> the policy makes sense for any phone (mis)behaving this way, keeping the
> phone specific quirks to an absolute minimum.
>

You can extend this justification logic to any mis-behavior.  We have to 
draw a line somewhere.

> I know that the N9 is the only phone that has seen this problem so far,
> but the problem will occur with every phone that doesn't "atomically"
> change the CLCC list the moment it reports an indicator change (+CIEV).
>

Our motto here is 'We do not deal with hypothetical situations'.  So 
unless you find another phone that behaves in this manner, lets try not 
to speculate ;)

> As described in the commit message the phone already told the HF that it
> has set up an outgoing call (or that it's already alerting). The only
> thing missing is the detailed information about the call, which is
> retrieved using the AT+CLCC request.
>
> So in this case I think it's valid to generally try to fill in the
> missing information by issuing another AT+CLCC request. Otherwise the
> call will be alerting and although the phone has reported all indicator
> changes to oFono, we won't be able to reasonably control it from HF side.
>

I disagree.  However, I do think we need to get started on the HFP 
quirks framework.

Regards,
-Denis

Re: [PATCHv2 1/2] hfpmodem: Retry AT+CLCC request after outgoing callsetup

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1269 bytes --]

Hi Timo,

<snip>

> +static void clcc_poll_dialing_cb(gboolean ok, GAtResult *result,
> +			gpointer user_data)
> +{
> +	struct ofono_voicecall *vc = user_data;
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	GSList *calls, *dialing;
> +
> +	if (!ok)
> +		return;
> +
> +	calls = at_util_parse_clcc(result);
> +	dialing = find_dialing(calls);
> +
> +	if (dialing == NULL) {
> +		if (vd->clcc_source)
> +			g_source_remove(vd->clcc_source);
> +
> +		vd->clcc_source = g_timeout_add(POLL_CLCC_DELAY,
> +												poll_clcc, vc);
> +		return;
> +	}
> +
> +	clcc_poll_cb(ok, result, user_data);

I'm not entirely happy that we parse the CLCC list twice here, once in 
this function and once in the clcc_poll_cb.  Would setting a flag in 
ciev_callsetup_notify and checking it in clcc_poll_cb be a better idea?

e.g.
if (num_active > 1 || num_held > 1)
	vd->clcc_source = g_timeout_add(POLL_CLCC_INTERVAL, poll_clcc,
                                                         vc);

would go into something like:

poll_again = num_active > 1 || num_held > 1;
if (vd->flags & FLAG_NOKIA_N9_BROKEN_DIAL) {
	poll_again = TRUE;
	vd->flags &= FLAG_NOKIA_N9_BROKEN_DIAL;
}

if (poll_again)
	....

Regards,
-Denis