Re: [PATCH 7/8] netreg: adapt CMER and CIEV for telit

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Christopher,

On 08/13/2012 08:23 AM, Christopher Vogl wrote:
> Telit uses a 2 to enable indicator event reporting and
> indicators in a +CIEV URC are identified by strings, not numbers.

Yikes, can you include a sample AT trace?  It sounds like something is 
going terribly wrong with CIEVs.

> ---
>   drivers/atmodem/network-registration.c |   46 +++++++++++++++++++++++++-------
>   1 files changed, 36 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
> index 3d09913..7083efe 100644
> --- a/drivers/atmodem/network-registration.c
> +++ b/drivers/atmodem/network-registration.c
> @@ -54,6 +54,7 @@ struct netreg_data {
>   	GAtChat *chat;
>   	char mcc[OFONO_MAX_MCC_LENGTH + 1];
>   	char mnc[OFONO_MAX_MNC_LENGTH + 1];
> +	const char *signal_identifier;
>   	int signal_index; /* If strength is reported via CIND */
>   	int signal_min; /* min strength reported via CIND */
>   	int signal_max; /* max strength reported via CIND */
> @@ -734,6 +735,7 @@ static void ciev_notify(GAtResult *result, gpointer user_data)
>   	struct ofono_netreg *netreg = user_data;
>   	struct netreg_data *nd = ofono_netreg_get_data(netreg);
>   	int strength, ind;
> +	const char *ind_str;
>   	GAtResultIter iter;
>
>   	g_at_result_iter_init(&iter, result);
> @@ -741,11 +743,23 @@ static void ciev_notify(GAtResult *result, gpointer user_data)
>   	if (!g_at_result_iter_next(&iter, "+CIEV:"))
>   		return;
>
> -	if (!g_at_result_iter_next_number(&iter,&ind))
> -		return;
> +	/*
> +	 * Telit uses strings to identify indicators.
> +	 */
> +	if (nd->vendor == OFONO_VENDOR_TELIT) {
> +		if (!g_at_result_iter_next_unquoted_string(&iter,&ind_str))
> +			return;
>
> -	if (ind != nd->signal_index)
> -		return;
> +		if (!g_str_equal(nd->signal_identifier, ind_str))
> +			return;
> +	}
> +	else {
> +		if (!g_at_result_iter_next_number(&iter,&ind))
> +			return;
> +		
> +		if (ind != nd->signal_index)
> +			return;
> +	}

If Telit is indeed this broken, then likely you can simply create a 
telit version of this function.  Then you can skip storing 
nd->signal_identifier and the like.  It would also make things much more 
readable.

>
>   	if (!g_at_result_iter_next_number(&iter,&strength))
>   		return;
> @@ -754,6 +768,8 @@ static void ciev_notify(GAtResult *result, gpointer user_data)
>   		strength = -1;
>   	else
>   		strength = (strength * 100) / (nd->signal_max - nd->signal_min);
> +	
> +	DBG("Strength: %d", strength);

This might belong in a separate patch.

>
>   	ofono_netreg_strength_notify(netreg, strength);
>   }
> @@ -1401,12 +1417,12 @@ static void cind_support_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   	struct netreg_data *nd = ofono_netreg_get_data(netreg);
>   	GAtResultIter iter;
>   	const char *str;
> -	char *signal_identifier = "signal";
> +	const char *cmd;
>   	int index;
>   	int min = 0;
>   	int max = 0;
>   	int tmp_min, tmp_max, invalid;
> -
> +	
>   	if (!ok)
>   		goto error;
>
> @@ -1422,8 +1438,10 @@ static void cind_support_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   	 */
>   	if (nd->vendor == OFONO_VENDOR_TELIT) {
>   		g_at_result_iter_open_list(&iter);
> -		signal_identifier = "rssi";
> +		nd->signal_identifier = "rssi";
>   	}
> +	else
> +		nd->signal_identifier = "signal";
>
>   	while (g_at_result_iter_open_list(&iter)) {
>   		/* Reset invalid default value for every token */
> @@ -1449,7 +1467,7 @@ static void cind_support_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   		if (!g_at_result_iter_close_list(&iter))
>   			goto error;
>
> -		if (g_str_equal(signal_identifier, str) == TRUE) {
> +		if (g_str_equal(nd->signal_identifier, str) == TRUE) {
>   			nd->signal_index = index;
>   			nd->signal_min = min;
>   			nd->signal_max = max;
> @@ -1464,8 +1482,16 @@ static void cind_support_cb(gboolean ok, GAtResult *result, gpointer user_data)
>
>   	if (nd->signal_index == 0)
>   		goto error;
> -
> -	g_at_chat_send(nd->chat, "AT+CMER=3,0,0,1", NULL,
> +	
> +	/*
> +	 * Telit uses a 2 to enable indicator event reporting, 1 is undefined.
> +	 */
> +	if (nd->vendor == OFONO_VENDOR_TELIT)
> +		cmd = "AT+CMER=3,0,0,2";
> +	else
> +		cmd = "AT+CMER=3,0,0,1";
> +	
> +	g_at_chat_send(nd->chat, cmd, NULL,

This looks fine, but we might be better off with something similar to 
how the CNMI string is being built in drivers/atmodem/sms.c.  E.g. query 
which CMER modes are supported and select these intelligently.

>   			NULL, NULL, NULL);
>   	g_at_chat_register(nd->chat, "+CIEV:",
>   				ciev_notify, FALSE, netreg, NULL);

Regards,
-Denis