Re: [PATCH v2 3/8] emulator: add RING for HFP AG

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

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

Hello Denis,

Le 11/04/2011 17:51, Denis Kenzior a écrit :
> Hi Frédéric,
>
> On 04/07/2011 11:33 AM, Frédéric Danis wrote:
>> ---
>>   src/emulator.c |   86 +++++++++++++++++++++++++++++++++++++++++++++++--------
>>   1 files changed, 73 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/emulator.c b/src/emulator.c
>> index 2707592..f1ccfb3 100644
>> --- a/src/emulator.c
>> +++ b/src/emulator.c
>> @@ -37,6 +37,8 @@
>>   #define DUN_DNS_SERVER_1       "10.10.10.10"
>>   #define DUN_DNS_SERVER_2       "10.10.10.11"
>>
>> +#define RING_TIMEOUT 3
>> +
>>   struct ofono_emulator {
>>   	struct ofono_atom *atom;
>>   	enum ofono_emulator_type type;
>> @@ -49,6 +51,7 @@ struct ofono_emulator {
>>   	int events_mode;
>>   	gboolean events_ind;
>>   	GSList *indicators;
>> +	guint ring;
>
> Please name this callsetup_source.
>
>>   };
>>
>>   struct indicator {
>> @@ -177,6 +180,42 @@ error:
>>   	g_at_server_send_final(em->server, G_AT_SERVER_RESULT_ERROR);
>>   }
>>
>> +static struct indicator *find_indicator(struct ofono_emulator *em,
>> +						const char *name, int *index)
>> +{
>> +	GSList *l;
>> +	int i;
>> +
>> +	for (i = 1, l = em->indicators; l; l = l->next, i++) {
>> +		struct indicator *ind = l->data;
>> +
>> +		if (g_str_equal(ind->name, name) == TRUE) {
>> +			if (index)
>> +				*index = i;
>> +
>> +			return ind;
>> +		}
>
> To avoid nesting, this might be better written as:
>
> if (g_str_equal(ind->name, name) == FALSE)
> 	continue;
>
> if (index)
> 	...
>
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static gboolean ring_cb(gpointer user_data)
>
> Please name this something like: send_callsetup_indicators
>
>> +{
>> +	struct ofono_emulator *em = user_data;
>> +	struct indicator *call_ind;
>> +
>> +	if (em->type == OFONO_EMULATOR_TYPE_HFP&&  em->slc == FALSE)
>> +		return TRUE;
>> +
>> +	call_ind = find_indicator(em, OFONO_EMULATOR_IND_CALL, NULL);
>> +
>> +	if (call_ind->value == OFONO_EMULATOR_CALL_INACTIVE)
>> +		g_at_server_send_unsolicited(em->server, "RING");
>> +
>> +	return TRUE;
>> +}
>> +
>>   static void brsf_cb(GAtServer *server, GAtServerRequestType type,
>>   			GAtResult *result, gpointer user_data)
>>   {
>> @@ -418,6 +457,11 @@ static void emulator_unregister(struct ofono_atom *atom)
>>   		em->source = 0;
>>   	}
>>
>> +	if (em->ring) {
>> +		g_source_remove(em->ring);
>> +		em->ring = 0;
>> +	}
>> +
>>   	for (l = em->indicators; l; l = l->next) {
>>   		struct indicator *ind = l->data;
>>
>> @@ -681,27 +725,43 @@ enum ofono_emulator_request_type ofono_emulator_request_get_type(
>>   void ofono_emulator_set_indicator(struct ofono_emulator *em,
>>   					const char *name, int value)
>>   {
>> -	GSList *l;
>>   	int i;
>>   	char buf[20];
>> +	struct indicator *ind;
>> +	struct indicator *call_ind;
>>
>> -	for (i = 1, l = em->indicators; l; l = l->next, i++) {
>> -		struct indicator *ind = l->data;
>> +	ind = find_indicator(em, name,&i);
>>
>> -		if (g_str_equal(ind->name, name) == FALSE)
>> -			continue;
>> +	if (ind == NULL || ind->value == value || value<  ind->min
>> +			|| value>  ind->max)
>> +		return;
>>
>> -		if (ind->value == value || value<  ind->min
>> -					|| value>  ind->max)
>> -			return;
>> +	ind->value = value;
>>
>> -		ind->value = value;
>> +	call_ind = find_indicator(em, OFONO_EMULATOR_IND_CALL, NULL);
>>
>> -		if (em->events_mode == 3&&  em->events_ind&&  em->slc) {
>> -			sprintf(buf, "+CIEV: %d,%d", i, ind->value);
>> -			g_at_server_send_info(em->server, buf, TRUE);
>> -		}
>> +	if (em->events_mode == 3&&  em->events_ind&&  em->slc) {
>> +		sprintf(buf, "+CIEV: %d,%d", i, ind->value);
>> +		g_at_server_send_info(em->server, buf, TRUE);
>
> Reminds me, this should be g_at_server_send_unsolicited
>
>> +	}
>>
>> +	/*
>> +	 * Ring timer should be started when callsetup indicator is set to
>> +	 *   Incoming
>> +	 * If there is no active call, a first RING should be sent just after
>> +	 *   the +CIEV
>> +	 * It should be stopped for all other values of callsetup
>> +	 */
>> +	if (g_str_equal(name, OFONO_EMULATOR_IND_CALLSETUP) == FALSE)
>>   		return;
>> +
>> +	if (value == OFONO_EMULATOR_CALLSETUP_INCOMING&&  em->ring == 0) {
>> +		if (call_ind->value == OFONO_EMULATOR_CALL_INACTIVE)
>> +			ring_cb(em);
>> +
>> +		em->ring = g_timeout_add_seconds(RING_TIMEOUT, ring_cb, em);
>> +	} else if (value != OFONO_EMULATOR_CALLSETUP_INCOMING&&  em->ring) {
>> +		g_source_remove(em->ring);
>> +		em->ring = 0;
>
> Do you really need to check for em->ring here?  You already return if
> the value of the indicator is already the same.  Also, it sounds like
> you should just start a timer and always call ring_cb when callsetup ==
> callsetup_incoming.  The CCWA/RING determination should be done in that
> function.
>
I agree with you that check of em->ring when callsetup becomes INCOMING 
is useless.
I thought that the second one, when callsetup is no more INCOMING, 
should be kept to stop the timer as soon as callsetup change states.
>>   	}
>>   }
>
> Regards,
> -Denis

Regards

Fred

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

---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.