[PATCH] voicecall: fix callheld indicator for PTS

[PATCH] voicecall: fix callheld indicator for PTS

[=?unknown-8bit?q?Fr=C3=A9d=C3=A9ric?= Danis]

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

Fix PTS test TP/TWC/BV-03-I [Call Waiting- Hold Active/Retrieve
Waiting Call or Held].

PTS test fails after receiving intermediate update of callheld indicator
with value 0 (no held call) before it receives update to value 1
(active and held calls). This is due to the non-atomic update of calls
status after call swap (moving first call to active state before moving
second one to hold state).

HFP 1.5 spec specifies that an update of callheld indicator to 1 should
be sent after AT+CHLD=2 command.
As oFono emulator sends +CIEV only if the indicator value changes, we
need to use an intermediate state for callheld indicator (2, all calls on
hold).

So, in case of multiple active calls, or an active call with an active
mutiparty call, force update of callheld indicator to 2.
---
 src/voicecall.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/src/voicecall.c b/src/voicecall.c
index 3646951..4b8373e 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -733,7 +733,8 @@ static void emulator_callheld_status_cb(struct ofono_atom *atom, void *data)
 static void notify_emulator_call_status(struct ofono_voicecall *vc)
 {
 	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
-	gboolean call = FALSE;
+	unsigned int non_mpty = 0;
+	gboolean multiparty = FALSE;
 	gboolean held = FALSE;
 	gboolean incoming = FALSE;
 	gboolean dialing = FALSE;
@@ -750,7 +751,12 @@ static void notify_emulator_call_status(struct ofono_voicecall *vc)
 
 		switch (v->call->status) {
 		case CALL_STATUS_ACTIVE:
-			call = TRUE;
+			if (g_slist_find_custom(vc->multiparty_list,
+						GINT_TO_POINTER(v->call->id),
+						call_compare_by_id))
+				multiparty = TRUE;
+			else
+				non_mpty++;
 			break;
 
 		case CALL_STATUS_HELD:
@@ -775,7 +781,8 @@ static void notify_emulator_call_status(struct ofono_voicecall *vc)
 		}
 	}
 
-	data.status = call || held ? OFONO_EMULATOR_CALL_ACTIVE :
+	data.status = non_mpty || multiparty || held ?
+					OFONO_EMULATOR_CALL_ACTIVE :
 					OFONO_EMULATOR_CALL_INACTIVE;
 
 	__ofono_modem_foreach_registered_atom(modem,
@@ -799,8 +806,11 @@ static void notify_emulator_call_status(struct ofono_voicecall *vc)
 						&data);
 
 	if (held)
-		data.status = call ? OFONO_EMULATOR_CALLHELD_MULTIPLE :
+		data.status = (non_mpty || multiparty) ?
+					OFONO_EMULATOR_CALLHELD_MULTIPLE :
 					OFONO_EMULATOR_CALLHELD_ON_HOLD;
+	else if (non_mpty > 1 || (non_mpty && multiparty))
+		data.status = OFONO_EMULATOR_CALLHELD_ON_HOLD;
 	else
 		data.status = OFONO_EMULATOR_CALLHELD_NONE;
 
-- 
1.7.1

Re: [PATCH] voicecall: fix callheld indicator for PTS

[Denis Kenzior]

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

Hi Frédéric,

On 07/18/2011 09:33 AM, Frédéric Danis wrote:
> Fix PTS test TP/TWC/BV-03-I [Call Waiting- Hold Active/Retrieve
> Waiting Call or Held].
> 
> PTS test fails after receiving intermediate update of callheld indicator
> with value 0 (no held call) before it receives update to value 1
> (active and held calls). This is due to the non-atomic update of calls
> status after call swap (moving first call to active state before moving
> second one to hold state).

Which scenario fails exactly?
- <1-N> Active + Waiting -> Active + Held
- <1-N> Active + 1 Held -> 1 Active + <1-N> Held
- 1 Active + <1-N> Held -> <1-N> Active + 1 Held

Also note that you can't really count on the order of the updates, these
are completely up to the implementation.

> 
> HFP 1.5 spec specifies that an update of callheld indicator to 1 should
> be sent after AT+CHLD=2 command.
> As oFono emulator sends +CIEV only if the indicator value changes, we
> need to use an intermediate state for callheld indicator (2, all calls on
> hold).
> 
> So, in case of multiple active calls, or an active call with an active
> mutiparty call, force update of callheld indicator to 2.
> ---
>  src/voicecall.c |   18 ++++++++++++++----
>  1 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/src/voicecall.c b/src/voicecall.c
> index 3646951..4b8373e 100644
> --- a/src/voicecall.c
> +++ b/src/voicecall.c
> @@ -733,7 +733,8 @@ static void emulator_callheld_status_cb(struct ofono_atom *atom, void *data)
>  static void notify_emulator_call_status(struct ofono_voicecall *vc)
>  {
>  	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
> -	gboolean call = FALSE;
> +	unsigned int non_mpty = 0;
> +	gboolean multiparty = FALSE;
>  	gboolean held = FALSE;
>  	gboolean incoming = FALSE;
>  	gboolean dialing = FALSE;
> @@ -750,7 +751,12 @@ static void notify_emulator_call_status(struct ofono_voicecall *vc)
>  
>  		switch (v->call->status) {
>  		case CALL_STATUS_ACTIVE:
> -			call = TRUE;
> +			if (g_slist_find_custom(vc->multiparty_list,
> +						GINT_TO_POINTER(v->call->id),
> +						call_compare_by_id))
> +				multiparty = TRUE;
> +			else
> +				non_mpty++;
>  			break;

What is this trying to achieve? You cannot have multiple calls in active
/ held state unless they are part of the mpty.  So if you have 3 active
calls, all 3 must be on the mpty list.

>  
>  		case CALL_STATUS_HELD:
> @@ -775,7 +781,8 @@ static void notify_emulator_call_status(struct ofono_voicecall *vc)
>  		}
>  	}
>  
> -	data.status = call || held ? OFONO_EMULATOR_CALL_ACTIVE :
> +	data.status = non_mpty || multiparty || held ?
> +					OFONO_EMULATOR_CALL_ACTIVE :
>  					OFONO_EMULATOR_CALL_INACTIVE;
>  
>  	__ofono_modem_foreach_registered_atom(modem,
> @@ -799,8 +806,11 @@ static void notify_emulator_call_status(struct ofono_voicecall *vc)
>  						&data);
>  
>  	if (held)
> -		data.status = call ? OFONO_EMULATOR_CALLHELD_MULTIPLE :
> +		data.status = (non_mpty || multiparty) ?
> +					OFONO_EMULATOR_CALLHELD_MULTIPLE :
>  					OFONO_EMULATOR_CALLHELD_ON_HOLD;
> +	else if (non_mpty > 1 || (non_mpty && multiparty))
> +		data.status = OFONO_EMULATOR_CALLHELD_ON_HOLD;
>  	else
>  		data.status = OFONO_EMULATOR_CALLHELD_NONE;
>  

I think a different approach is needed.  One idea off the top of my
head:  Can we mark calls which we foresee being as part of the
transaction and only send the status updates once all of them have
entered the desired state.

Regards,
-Denis

Re: [PATCH] voicecall: fix callheld indicator for PTS

[Frederic Danis]

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

Hello Denis,

Le 18/07/2011 18:40, Denis Kenzior a écrit :
> Hi Frédéric,
>
> On 07/18/2011 09:33 AM, Frédéric Danis wrote:
>> Fix PTS test TP/TWC/BV-03-I [Call Waiting- Hold Active/Retrieve
>> Waiting Call or Held].
>>
>> PTS test fails after receiving intermediate update of callheld indicator
>> with value 0 (no held call) before it receives update to value 1
>> (active and held calls). This is due to the non-atomic update of calls
>> status after call swap (moving first call to active state before moving
>> second one to hold state).
>
> Which scenario fails exactly?
> -<1-N>  Active + Waiting ->  Active + Held
> -<1-N>  Active + 1 Held ->  1 Active +<1-N>  Held
> - 1 Active +<1-N>  Held ->  <1-N>  Active + 1 Held
>
> Also note that you can't really count on the order of the updates, these
> are completely up to the implementation.
>

This test fails for (tested with phonesim) :
- 1st call held + 2nd call active -> 1st call active + 2nd call held

I understand that order of update for calls will be implementation 
dependent, this problem occurs when 1st call to be updated was held and 
become active.

>>
>> HFP 1.5 spec specifies that an update of callheld indicator to 1 should
>> be sent after AT+CHLD=2 command.
>> As oFono emulator sends +CIEV only if the indicator value changes, we
>> need to use an intermediate state for callheld indicator (2, all calls on
>> hold).
>>
>> So, in case of multiple active calls, or an active call with an active
>> mutiparty call, force update of callheld indicator to 2.
>> ---
>>   src/voicecall.c |   18 ++++++++++++++----
>>   1 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/voicecall.c b/src/voicecall.c
>> index 3646951..4b8373e 100644
>> --- a/src/voicecall.c
>> +++ b/src/voicecall.c
>> @@ -733,7 +733,8 @@ static void emulator_callheld_status_cb(struct ofono_atom *atom, void *data)
>>   static void notify_emulator_call_status(struct ofono_voicecall *vc)
>>   {
>>   	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>> -	gboolean call = FALSE;
>> +	unsigned int non_mpty = 0;
>> +	gboolean multiparty = FALSE;
>>   	gboolean held = FALSE;
>>   	gboolean incoming = FALSE;
>>   	gboolean dialing = FALSE;
>> @@ -750,7 +751,12 @@ static void notify_emulator_call_status(struct ofono_voicecall *vc)
>>
>>   		switch (v->call->status) {
>>   		case CALL_STATUS_ACTIVE:
>> -			call = TRUE;
>> +			if (g_slist_find_custom(vc->multiparty_list,
>> +						GINT_TO_POINTER(v->call->id),
>> +						call_compare_by_id))
>> +				multiparty = TRUE;
>> +			else
>> +				non_mpty++;
>>   			break;
>
> What is this trying to achieve? You cannot have multiple calls in active
> / held state unless they are part of the mpty.  So if you have 3 active
> calls, all 3 must be on the mpty list.
>

As calls status are updated separately, you can have multiple active 
calls (for a short time) which are not part of a multi-party list :

  - 1st call held + 2nd call active (callheld indicator = 1)
      -> 1st call active + 2nd call active (intermediate state,
                                            callheld indicator = 0)
      -> 1st call active + 2nd call held (callheld indicator = 1)

>>
>>   		case CALL_STATUS_HELD:
>> @@ -775,7 +781,8 @@ static void notify_emulator_call_status(struct ofono_voicecall *vc)
>>   		}
>>   	}
>>
>> -	data.status = call || held ? OFONO_EMULATOR_CALL_ACTIVE :
>> +	data.status = non_mpty || multiparty || held ?
>> +					OFONO_EMULATOR_CALL_ACTIVE :
>>   					OFONO_EMULATOR_CALL_INACTIVE;
>>
>>   	__ofono_modem_foreach_registered_atom(modem,
>> @@ -799,8 +806,11 @@ static void notify_emulator_call_status(struct ofono_voicecall *vc)
>>   						&data);
>>
>>   	if (held)
>> -		data.status = call ? OFONO_EMULATOR_CALLHELD_MULTIPLE :
>> +		data.status = (non_mpty || multiparty) ?
>> +					OFONO_EMULATOR_CALLHELD_MULTIPLE :
>>   					OFONO_EMULATOR_CALLHELD_ON_HOLD;
>> +	else if (non_mpty>  1 || (non_mpty&&  multiparty))
>> +		data.status = OFONO_EMULATOR_CALLHELD_ON_HOLD;
>>   	else
>>   		data.status = OFONO_EMULATOR_CALLHELD_NONE;
>>
>
> I think a different approach is needed.  One idea off the top of my
> head:  Can we mark calls which we foresee being as part of the
> transaction and only send the status updates once all of them have
> entered the desired state.
>

I agree with you this should be the best way, but:

- currently in voicecall atom during call swap, and as far as I 
understand, the calls are updated separately from the modem driver and 
there is no way to know when this update ends.
I do not found a way to perform this with certainty that it will not 
break actual behavior (regarding DBus), This is why I proposed this 
hack, limited to emulator related code.

- currently HFP indicators are only sent when their value changes, we 
need to move through an intermediate state (callheld = 2), or 
add/change API to be able to force the sending of the indicator.

> Regards,
> -Denis

Regards

Fred


-- 
Frederic Danis                            Open Source Technology Centre
frederic.danis(a)intel.com                              Intel Corporation

Re: [PATCH] voicecall: fix callheld indicator for PTS

[Denis Kenzior]

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

Hi Frederic,

On 07/19/2011 08:50 AM, Frederic Danis wrote:
> Hello Denis,
> 
> Le 18/07/2011 18:40, Denis Kenzior a écrit :
>> Hi Frédéric,
>>
>> On 07/18/2011 09:33 AM, Frédéric Danis wrote:
>>> Fix PTS test TP/TWC/BV-03-I [Call Waiting- Hold Active/Retrieve
>>> Waiting Call or Held].
>>>
>>> PTS test fails after receiving intermediate update of callheld indicator
>>> with value 0 (no held call) before it receives update to value 1
>>> (active and held calls). This is due to the non-atomic update of calls
>>> status after call swap (moving first call to active state before moving
>>> second one to hold state).
>>
>> Which scenario fails exactly?
>> -<1-N>  Active + Waiting ->  Active + Held
>> -<1-N>  Active + 1 Held ->  1 Active +<1-N>  Held
>> - 1 Active +<1-N>  Held ->  <1-N>  Active + 1 Held
>>
>> Also note that you can't really count on the order of the updates, these
>> are completely up to the implementation.
>>
> 
> This test fails for (tested with phonesim) :
> - 1st call held + 2nd call active -> 1st call active + 2nd call held
> 
> I understand that order of update for calls will be implementation
> dependent, this problem occurs when 1st call to be updated was held and
> become active.
> 
>>>
>>> HFP 1.5 spec specifies that an update of callheld indicator to 1 should
>>> be sent after AT+CHLD=2 command.
>>> As oFono emulator sends +CIEV only if the indicator value changes, we
>>> need to use an intermediate state for callheld indicator (2, all
>>> calls on
>>> hold).
>>>
>>> So, in case of multiple active calls, or an active call with an active
>>> mutiparty call, force update of callheld indicator to 2.
>>> ---
>>>   src/voicecall.c |   18 ++++++++++++++----
>>>   1 files changed, 14 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/voicecall.c b/src/voicecall.c
>>> index 3646951..4b8373e 100644
>>> --- a/src/voicecall.c
>>> +++ b/src/voicecall.c
>>> @@ -733,7 +733,8 @@ static void emulator_callheld_status_cb(struct
>>> ofono_atom *atom, void *data)
>>>   static void notify_emulator_call_status(struct ofono_voicecall *vc)
>>>   {
>>>       struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>>> -    gboolean call = FALSE;
>>> +    unsigned int non_mpty = 0;
>>> +    gboolean multiparty = FALSE;
>>>       gboolean held = FALSE;
>>>       gboolean incoming = FALSE;
>>>       gboolean dialing = FALSE;
>>> @@ -750,7 +751,12 @@ static void notify_emulator_call_status(struct
>>> ofono_voicecall *vc)
>>>
>>>           switch (v->call->status) {
>>>           case CALL_STATUS_ACTIVE:
>>> -            call = TRUE;
>>> +            if (g_slist_find_custom(vc->multiparty_list,
>>> +                        GINT_TO_POINTER(v->call->id),
>>> +                        call_compare_by_id))
>>> +                multiparty = TRUE;
>>> +            else
>>> +                non_mpty++;
>>>               break;
>>
>> What is this trying to achieve? You cannot have multiple calls in active
>> / held state unless they are part of the mpty.  So if you have 3 active
>> calls, all 3 must be on the mpty list.
>>
> 
> As calls status are updated separately, you can have multiple active
> calls (for a short time) which are not part of a multi-party list :
> 
>  - 1st call held + 2nd call active (callheld indicator = 1)
>      -> 1st call active + 2nd call active (intermediate state,
>                                            callheld indicator = 0)
>      -> 1st call active + 2nd call held (callheld indicator = 1)
> 

I still don't see how this helps you if the active call is transitioned
first.

So call A held + call B active (call = 1, callheld = 1)
call B -> held (call = 0, callheld = 2)
call A -> active (call = 1, callheld = 1)

This scenario is also wrong...

>>>
>>>           case CALL_STATUS_HELD:
>>> @@ -775,7 +781,8 @@ static void notify_emulator_call_status(struct
>>> ofono_voicecall *vc)
>>>           }
>>>       }
>>>
>>> -    data.status = call || held ? OFONO_EMULATOR_CALL_ACTIVE :
>>> +    data.status = non_mpty || multiparty || held ?
>>> +                    OFONO_EMULATOR_CALL_ACTIVE :
>>>                       OFONO_EMULATOR_CALL_INACTIVE;
>>>
>>>       __ofono_modem_foreach_registered_atom(modem,
>>> @@ -799,8 +806,11 @@ static void notify_emulator_call_status(struct
>>> ofono_voicecall *vc)
>>>                           &data);
>>>
>>>       if (held)
>>> -        data.status = call ? OFONO_EMULATOR_CALLHELD_MULTIPLE :
>>> +        data.status = (non_mpty || multiparty) ?
>>> +                    OFONO_EMULATOR_CALLHELD_MULTIPLE :
>>>                       OFONO_EMULATOR_CALLHELD_ON_HOLD;
>>> +    else if (non_mpty>  1 || (non_mpty&&  multiparty))
>>> +        data.status = OFONO_EMULATOR_CALLHELD_ON_HOLD;
>>>       else
>>>           data.status = OFONO_EMULATOR_CALLHELD_NONE;
>>>
>>
>> I think a different approach is needed.  One idea off the top of my
>> head:  Can we mark calls which we foresee being as part of the
>> transaction and only send the status updates once all of them have
>> entered the desired state.
>>
> 
> I agree with you this should be the best way, but:
> 
> - currently in voicecall atom during call swap, and as far as I
> understand, the calls are updated separately from the modem driver and
> there is no way to know when this update ends.
> I do not found a way to perform this with certainty that it will not
> break actual behavior (regarding DBus), This is why I proposed this
> hack, limited to emulator related code.
> 

Not sure I'm following, what do you mean updated separately from the
modem driver?  The modem driver can be stateless, but it is the first to
know of the state switch.

> - currently HFP indicators are only sent when their value changes, we
> need to move through an intermediate state (callheld = 2), or add/change
> API to be able to force the sending of the indicator.

Again, not really sure how this will help you...  Lets discuss on IRC if
needed.

Regards,
-Denis