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

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

[Denis Kenzior]

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

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

[Slava Monich]

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

Hi Anirudh, Denis at al.

> @@ -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";

What's would be the difference between "roaming lte" and "roaming" (or 
"registered lte" vs "registered") from the viewpoint of D-Bus API 
clients? In other words, what would e.g. dialer do differently depending 
on whether registration status is "registered lte" or just "registered"? 
Is it worth the API break? The existing clients won't know how to handle 
the new values until they (clients) get updated.

Cheers,
-Slava

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

[Denis Kenzior]

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

Hi Slava,

On 09/10/2018 01:26 PM, Slava Monich wrote:
> Hi Anirudh, Denis at al.
> 
>> @@ -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";
> 
> What's would be the difference between "roaming lte" and "roaming" (or 
> "registered lte" vs "registered") from the viewpoint of D-Bus API 
> clients? In other words, what would e.g. dialer do differently depending 
> on whether registration status is "registered lte" or just "registered"? 
> Is it worth the API break? The existing clients won't know how to handle 
> the new values until they (clients) get updated.
> 

I agree.  That is why I suggested a separate Property for this (and the 
CSFB case from 27.007) instead of modifying the Status property directly.

E.g. REGISTERED_SMS_EUTRAN would map to Status="registered" and 
SmsOnly=False.  ROAMING_SMS_EUTRAN would map to Status="roaming" and 
SmsOnly=True.

I assume that the Dialer can ignore this as the modem will fall back to 
3G in case of a call?  Someone feel free to educate me.

Regards,
-Denis

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

[Slava Monich]

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

Hi Denis,
> Hi Slava,
>
> On 09/10/2018 01:26 PM, Slava Monich wrote:
>> Hi Anirudh, Denis at al.
>>
>>> @@ -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";
>>
>> What would be the difference between "roaming lte" and "roaming" (or 
>> "registered lte" vs "registered") from the viewpoint of D-Bus API 
>> clients? In other words, what would e.g. dialer do differently 
>> depending on whether registration status is "registered lte" or just 
>> "registered"? Is it worth the API break? The existing clients won't 
>> know how to handle the new values until they (clients) get updated.
>>
>
> I agree.  That is why I suggested a separate Property for this (and 
> the CSFB case from 27.007) instead of modifying the Status property 
> directly.
>
> E.g. REGISTERED_SMS_EUTRAN would map to Status="registered" and 
> SmsOnly=False.  ROAMING_SMS_EUTRAN would map to Status="roaming" and 
> SmsOnly=True.
>
> I assume that the Dialer can ignore this as the modem will fall back 
> to 3G in case of a call?  Someone feel free to educate me.

The modems I dealt with do drop to 3G for voice calls. Once voice call 
terminates, they switch back to LTE (well, most of the time). Dialler 
(at least our dialler) doesn't care about access technology.

Just a thought - could the presence of org.ofono.VoiceCallManager and 
org.ofono.MessageManager interfaces be used to indicate whether voice 
calls and/or messaging is available? That wouldn't require any D-Bus API 
changes at all.

Cheers,
-Slava

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

[Denis Kenzior]

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

Hi Slava,

On 09/10/2018 02:56 PM, Slava Monich wrote:
> Hi Denis,
>> Hi Slava,
>>
>> On 09/10/2018 01:26 PM, Slava Monich wrote:
>>> Hi Anirudh, Denis at al.
>>>
>>>> @@ -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";
>>>
>>> What would be the difference between "roaming lte" and "roaming" (or 
>>> "registered lte" vs "registered") from the viewpoint of D-Bus API 
>>> clients? In other words, what would e.g. dialer do differently 
>>> depending on whether registration status is "registered lte" or just 
>>> "registered"? Is it worth the API break? The existing clients won't 
>>> know how to handle the new values until they (clients) get updated.
>>>
>>
>> I agree.  That is why I suggested a separate Property for this (and 
>> the CSFB case from 27.007) instead of modifying the Status property 
>> directly.
>>
>> E.g. REGISTERED_SMS_EUTRAN would map to Status="registered" and 
>> SmsOnly=False.  ROAMING_SMS_EUTRAN would map to Status="roaming" and 
>> SmsOnly=True.
>>
>> I assume that the Dialer can ignore this as the modem will fall back 
>> to 3G in case of a call?  Someone feel free to educate me.
> 
> The modems I dealt with do drop to 3G for voice calls. Once voice call 
> terminates, they switch back to LTE (well, most of the time). Dialler 
> (at least our dialler) doesn't care about access technology.

That is what I've seen as well.  But these are pre-VoLTE devices most 
likely.  VoLTE might act differently.

> 
> Just a thought - could the presence of org.ofono.VoiceCallManager and 
> org.ofono.MessageManager interfaces be used to indicate whether voice 
> calls and/or messaging is available? That wouldn't require any D-Bus API 
> changes at all.
> 

VoiceCallManager should always be available since you still need to make 
Emergency Calls.  There might be some really weird legacy devices which 
were Data-Only, but those aren't really the focus of oFono anyway.

What I assume is happening in 27.007 is that registration states 6/9 & 
7/10 allows one to discern whether Circuit Switched Fallback would be 
triggered.  I assume if the device (& network) is VoLTE capable, then 
you'd see state 9/10, 6/7 otherwise.  Perhaps all 4 states should be 
combined into a single CircuitSwitchedFallback property instead...

Regards,
-Denis