Re: [PATCH] atmodem: add LTE state for AT+CREG

* Re: [PATCH] atmodem: add LTE state for AT+CREG
       [not found] <1536556520-24462-1-git-send-email-anirudh.gargi@intel.com>
@ 2018-09-10 16:23 ` Denis Kenzior
  2018-09-10 18:26   ` Slava Monich
  0 siblings, 1 reply; 5+ messages in thread
From: Denis Kenzior @ 2018-09-10 16:23 UTC (permalink / raw)
  To: ofono

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

Hi Anirudh,

On 09/10/2018 12:15 AM, "Anirudh Gargi anirudh.gargi"@intel.com wrote:

Something is funny with your email address here..

> From: Anirudh Gargi <anirudh.gargi@intel.com>
> 
> CREG reports 6 and 7 for SMS only registration on E-UTRAN. Fix
> sms atom to consider the new states while sending SMS.
> ---
>   drivers/atmodem/network-registration.c | 11 +++++++++--
>   src/common.c                           |  4 ++++
>   src/common.h                           |  2 ++
>   src/sms.c                              |  2 ++
>   4 files changed, 17 insertions(+), 2 deletions(-)

This needs to be broken up into two patches per 'HACKING', 'Submitting 
patches' section.

> 
> diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
> index 0854cd1..5945e1c 100644
> --- a/drivers/atmodem/network-registration.c
> +++ b/drivers/atmodem/network-registration.c
> @@ -228,6 +228,10 @@ static void at_creg_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   	if ((status == 1 || status == 5) && (tech == -1))
>   		tech = nd->tech;
>   
> +	/* Handle EUTRAN cases */
> +	if ((status == 6 || status == 7) && (tech == -1))
> +		tech = nd->tech;
> +

Should tech be hardcoded to EUTRAN here?

>   	cb(&error, status, lac, ci, tech, cbd->data);
>   }
>   
> @@ -1522,8 +1526,11 @@ static void creg_notify(GAtResult *result, gpointer user_data)
>   				&lac, &ci, &tech, nd->vendor) == FALSE)
>   		return;
>   
> -	if (status != 1 && status != 5)
> -		goto notify;
> +	if (status != 1 && status != 5) {
> +		/* Check EUTRAN cases */
> +		if (status != 6 && status != 7)
> +			goto notify;
> +	}

So I would do something like:

if (status == 6 || status == 7) {
	tech = EUTRAN;
	goto notify;
}

if (status != 1 && status != 5)
	goto notify;

>   
>   	tq = g_try_new0(struct tech_query, 1);
>   	if (tq == NULL)
> diff --git a/src/common.c b/src/common.c
> index 3ccaf7c..79eed3e 100644
> --- a/src/common.c
> +++ b/src/common.c
> @@ -669,6 +669,10 @@ const char *registration_status_to_string(int status)
>   		return "unknown";
>   	case NETWORK_REGISTRATION_STATUS_ROAMING:
>   		return "roaming";
> +	case NETWORK_REGISTRATION_STATUS_REGISTERED_EUTRAN:
> +		return "registered lte";
> +	case NETWORK_REGISTRATION_STATUS_ROAMING_EUTRAN:
> +		return "roaming lte";

No spaces in our property values.  Also you're changing the D-Bus API, 
so you might want to add a separate commit to doc/network-api.txt.

And another thing, this is an SMS-only registration according to 27.007. 
  So 'registered lte' makes no sense.   Perhaps we need a separate 
property for this and the CSFB cases.  E.g.
	boolean SmsOnly [readonly] - Current registration is only valid for SMS.

>   	}
>   
>   	return "";
> diff --git a/src/common.h b/src/common.h
> index 1b6b01d..44d7c6f 100644
> --- a/src/common.h
> +++ b/src/common.h
> @@ -43,6 +43,8 @@ enum network_registration_status {
>   	NETWORK_REGISTRATION_STATUS_DENIED =		3,
>   	NETWORK_REGISTRATION_STATUS_UNKNOWN =		4,
>   	NETWORK_REGISTRATION_STATUS_ROAMING =		5,
> +	NETWORK_REGISTRATION_STATUS_REGISTERED_EUTRAN =	6,
> +	NETWORK_REGISTRATION_STATUS_ROAMING_EUTRAN = 	7,
REGISTERED_SMS_EUTRAN?
>   };
>   
>   /* 27.007 Section 7.3 <stat> */
> diff --git a/src/sms.c b/src/sms.c
> index b86158e..e229425 100644
> --- a/src/sms.c
> +++ b/src/sms.c
> @@ -782,6 +782,8 @@ static void netreg_status_watch(int status, int lac, int ci, int tech,
>   	switch (status) {
>   	case NETWORK_REGISTRATION_STATUS_REGISTERED:
>   	case NETWORK_REGISTRATION_STATUS_ROAMING:
> +	case NETWORK_REGISTRATION_STATUS_REGISTERED_EUTRAN:
> +	case NETWORK_REGISTRATION_STATUS_ROAMING_EUTRAN:
>   		sms->registered = TRUE;
>   		break;
>   	default:
> 

Regards,
-Denis

^ permalink raw reply	[flat|nested] 5+ messages in thread