Re: [PATCH 1/2] network: allow status' and notifications on eutran networks

Hi Sean,

On 10/8/24 4:11 AM, Sean Nyekjaer wrote:
> SIMCom A7672E-FASE shows attached on LTE with +CREG <stat> either
> 6 registered for "SMS only", home network (applicable only when E-UTRAN)
> 7 registered for "SMS only", roaming (applicable only when <AcT> indicates E-UTRAN)
> +COPS supplies the <AcT> = EUTRAN

Do you know the distinction between registered/roaming and "SMS only" variants 
in practical terms?  Are data calls available on "SMS only"?  Voicecalls?

> ---
>   src/network.c | 58 +++++++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 47 insertions(+), 11 deletions(-)
> 

CI complains:

Checkpatch Output
=================
0001-network-allow-status-and-notifications-on-eutran-net.patch:9: WARNING: 
Prefer a maximum 75 chars per line (possible unwrapped commit description?)
0001-network-allow-status-and-notifications-on-eutran-net.patch:36: ERROR: 
switch and case should be at the same indent
0001-network-allow-status-and-notifications-on-eutran-net.patch:63: ERROR: 
switch and case should be at the same indent
0001-network-allow-status-and-notifications-on-eutran-net.patch:92: ERROR: 
switch and case should be at the same indent
0001-network-allow-status-and-notifications-on-eutran-net.patch:114: ERROR: 
switch and case should be at the same indent
total: 4 errors, 1 warnings, 100 lines checked
0002-gprs-allow-attached-updates-and-status-on-eutran-net.patch:9: WARNING: 
Prefer a maximum 75 chars per line (possible unwrapped commit description?)
total: 0 errors, 1 warnings, 47 lines checked

> diff --git a/src/network.c b/src/network.c
> index 40626179..02853e23 100644
> --- a/src/network.c
> +++ b/src/network.c
> @@ -376,7 +376,8 @@ static char *get_operator_display_name(struct ofono_netreg *netreg)
>   		return name;
>   	}
>   
> -	if (netreg->status == NETWORK_REGISTRATION_STATUS_REGISTERED)
> +	if (netreg->status == NETWORK_REGISTRATION_STATUS_REGISTERED ||
> +		netreg->status == NETWORK_REGISTRATION_STATUS_REGISTERED_SMS_EUTRAN)
>   		home_or_spdi = TRUE;
>   	else
>   		home_or_spdi = sim_spdi_lookup(netreg->spdi,
> @@ -1205,9 +1206,15 @@ static void current_operator_callback(const struct ofono_error *error,
>   	 * in which case the operator information frequently comes in bogus.
>   	 * We ignore it here
>   	 */
> -	if (netreg->status != NETWORK_REGISTRATION_STATUS_REGISTERED &&
> -			netreg->status != NETWORK_REGISTRATION_STATUS_ROAMING)
> -		current = NULL;
> +	switch (netreg->status) {
> +		case NETWORK_REGISTRATION_STATUS_REGISTERED:
> +		case NETWORK_REGISTRATION_STATUS_REGISTERED_SMS_EUTRAN:
> +		case NETWORK_REGISTRATION_STATUS_ROAMING:
> +		case NETWORK_REGISTRATION_STATUS_ROAMING_SMS_EUTRAN:
> +			break;
> +		default:
> +			current = NULL;
> +	}
>   
>   	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>   		DBG("Error during current operator");
> @@ -1324,6 +1331,8 @@ static void notify_emulator_status(struct ofono_atom *atom, void *data)
>   void ofono_netreg_status_notify(struct ofono_netreg *netreg, int status,
>   			int lac, int ci, int tech)
>   {
> +	ofono_bool_t netreg_status;
> +
>   	if (netreg == NULL)
>   		return;
>   
> @@ -1351,8 +1360,18 @@ void ofono_netreg_status_notify(struct ofono_netreg *netreg, int status,
>   	if (netreg->technology != tech)
>   		set_registration_technology(netreg, tech);
>   
> -	if (netreg->status == NETWORK_REGISTRATION_STATUS_REGISTERED ||
> -		netreg->status == NETWORK_REGISTRATION_STATUS_ROAMING) {
> +	switch (netreg->status) {
> +		case NETWORK_REGISTRATION_STATUS_REGISTERED:
> +		case NETWORK_REGISTRATION_STATUS_REGISTERED_SMS_EUTRAN:
> +		case NETWORK_REGISTRATION_STATUS_ROAMING:
> +		case NETWORK_REGISTRATION_STATUS_ROAMING_SMS_EUTRAN:
> +			netreg_status = true;
> +			break;
> +		default:
> +			netreg_status = false;
> +	}
> +
> +	if (netreg_status) {
>   		if (netreg->driver->current_operator != NULL)
>   			netreg->driver->current_operator(netreg,
>   					current_operator_callback, netreg);
> @@ -1448,6 +1467,7 @@ static void init_registration_status(const struct ofono_error *error,
>   					void *data)
>   {
>   	struct ofono_netreg *netreg = data;
> +	ofono_bool_t netreg_status;

Simple bool is fine.  The plan is to remove ofono_bool_t entirely.

>   
>   	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>   		DBG("Error during registration status query");
> @@ -1460,8 +1480,18 @@ static void init_registration_status(const struct ofono_error *error,
>   	 * Bootstrap our signal strength value without waiting for the
>   	 * stack to report it
>   	 */
> -	if (netreg->status == NETWORK_REGISTRATION_STATUS_REGISTERED ||
> -		netreg->status == NETWORK_REGISTRATION_STATUS_ROAMING) {
> +	switch (netreg->status) {
> +		case NETWORK_REGISTRATION_STATUS_REGISTERED:
> +		case NETWORK_REGISTRATION_STATUS_REGISTERED_SMS_EUTRAN:
> +		case NETWORK_REGISTRATION_STATUS_ROAMING:
> +		case NETWORK_REGISTRATION_STATUS_ROAMING_SMS_EUTRAN:
> +			netreg_status = true;
> +			break;
> +		default:
> +			netreg_status = false;
> +	}
> +
> +	if (netreg_status) {
>   		if (netreg->driver->strength != NULL)
>   			netreg->driver->strength(netreg,
>   					signal_strength_callback, netreg);
> @@ -1516,9 +1546,15 @@ void ofono_netreg_strength_notify(struct ofono_netreg *netreg, int strength)
>   	 * Theoretically we can get signal strength even when not registered
>   	 * to any network.  However, what do we do with it in that case?
>   	 */
> -	if (netreg->status != NETWORK_REGISTRATION_STATUS_REGISTERED &&
> -			netreg->status != NETWORK_REGISTRATION_STATUS_ROAMING)
> -		return;
> +	switch (netreg->status) {
> +		case NETWORK_REGISTRATION_STATUS_REGISTERED:
> +		case NETWORK_REGISTRATION_STATUS_REGISTERED_SMS_EUTRAN:
> +		case NETWORK_REGISTRATION_STATUS_ROAMING:
> +		case NETWORK_REGISTRATION_STATUS_ROAMING_SMS_EUTRAN:
> +			break;
> +		default:
> +			return;
> +	}

Might be nicer to do:

if (!L_IN_SET(netreg->status, NETWORK_REGISTRATION_STATUS_REGISTERED,
			NETWORK_REGISTRATION_STATUS_ROAMING,
			...))
	return;

>   
>   	DBG("strength %d", strength);
>   

Regards,
-Denis