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

Hi Denis,

On Wed, Oct 09, 2024 at 10:35:47AM +0100, Denis Kenzior wrote:
> 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?

To my limited knowledge; E-UTRAN(let us call it LTE) doesn't have voicecalls,
that why we have VoLTE.
With the SIMCom modem on LTE in NETWORK_REGISTRATION_STATUS_REGISTERED_SMS_EUTRAN
mode we have data and legacy sms available.

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

Can I run checkpatch myself?

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

OK


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

Will do that for V2.

> 
> >   	DBG("strength %d", strength);
> 
> Regards,
> -Denis

/Sean