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

[Frederic Danis <frederic.danis@linux.intel.com>]

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