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

[Denis Kenzior <denkenz@gmail.com>]

[-- 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