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

[Denis Kenzior <denkenz@gmail.com>]

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

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.

>  	}
>  }

Regards,
-Denis