lib: nrf_modem: lte_connectivity -> lte_net_if

[lemrey]

• Renamed; this isn't really a library per-se but rather a component of the integration layer that adds support for Zephyr's API.
  lte_connectivity was not a good name, because LTE connectivity is possible regardless of this component.
• Removed (deprecated) NRF_MODEM_LIB_NET_IF_AUTO_START; this shouldn't be supported any longer in NCS.
• Removed header lte_connectivity.h. I will add its contents to libmodem's documentation as part of this PR.

Missing doc & changelog updates.

[NordicBuilder]

Test specification

CI/Jenkins/NRF

• Integration Platforms

CI/Jenkins/integration

Test Module	File based changes	Manually selected	West overwrite
test-fw-nrfconnect-nrf-iot_lwm2m	X
test-fw-nrfconnect-nrf-iot_mosh	X
test-fw-nrfconnect-nrf-iot_samples	X
test-fw-nrfconnect-nrf-iot_serial_lte_modem	X
test-fw-nrfconnect-nrf-iot_zephyr_lwm2m	X

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

[eivindj-nordic]

nit:

Suggested change

	/* Bind L2 connectity APIs.
	/* Bind L2 connectivity APIs.

[glarsennordic]

In fact I would go so far as to suggest

/* Bind conn_mgr APIs */

L2 connectivity is I think a mistaken reference to an old name

[lemrey]

ok, will update

[eivindj-nordic]

I'm wondering if this struct, as included as extern, should have a more explicit name, e.g. nrf_modem_lib_netif_conn_api etc.?

[lemrey]

good point

[eivindj-nordic]

nit: the folder with these tests should be renamed.

[glarsennordic]

What are your thoughts on naming it lte_netif just to emphasize that it is specifically integrating the two?

I know you have placed it in the modem_lib folder, but I still suspect that in the long run lte_netif will make communication easier than just nrf_modem_lib/netif -- especially since we have net_if to contend with in Zephyr

alternatively: modem_netif

Also, since nrf91_sockets.c is also implementing a lot of stuff related to Zephyr's API, should this be moved under the umbrella as well? I guess it does stick mostly to the socket provider implementation, so maybe not

[NordicBuilder]

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

[glarsennordic]

In fact I would go so far as to suggest

/* Bind conn_mgr APIs */

L2 connectivity is I think a mistaken reference to an old name

[glarsennordic]

nit: I personally find these closure comments /* CONFIG_NRF_MODEM_LIB_NETIF */ helpful, even for short #if spans

[lemrey]

ok, I can add it back.

[glarsennordic]

Also, just heads up, I'm not sure which of ours will merge first:

#11424

[glarsennordic]

Ah, one final thing to consider: It would be nice if both this and its Wi-Fi counterpart ended up with consistent naming:

#10434

Currently it is named l2_wifi_conn or L2_WIFI_CONNECTIVITY

[lemrey]

What are your thoughts on naming it lte_netif just to emphasize that it is specifically integrating the two?'

That's a good idea.

Also, since nrf91_sockets.c is also implementing a lot of stuff related to Zephyr's API, should this be moved under the umbrella as well? I guess it does stick mostly to the socket provider implementation, so maybe not

I think it's good to keep the two things separated, socket offloading and net_if wrapper; If possible I'd put the connection manager macros in the netif file, but the linker complains and I haven't been able to untangle the macros yet :)

[lemrey]

Ah, one final thing to consider: It would be nice if both this and its Wi-Fi counterpart ended up with consistent naming:

#10434

Currently it is named l2_wifi_conn or L2_WIFI_CONNECTIVITY

yes, definitely

[rlubos]

Shouldn't the actual Zephyr net_if definition for nRF91 also be moved here, if we want to abstract Zephyr integration stuff? Currently done in https://github.com/nrfconnect/sdk-nrf/blob/main/lib/nrf_modem_lib/nrf91_sockets.c#L1129

[lemrey]

It could be done but we'd need to extern all the nrf91 socket APIs, right?

[lemrey]

Actually not; I'll look into this but ideally we should keep the two things separate, socket offloading and net_if wrapper.

[simensrostad]

Looks good to me

[lemrey]

Some thoughts.

1. Right now lte_if_enable (net_if_up) will turn on modem and initialize the LTE link controller.
   However, the LTE link controller has hooks for libmodem initialization where it sets up several things and finally calls lte_lc_init_and_connect if the Kconfig option CONFIG_LTE_AUTO_INIT_AND_CONNECT is enabled; now that that option is deprecated we should add an unconditional call to lte_lc_init there instead, which would let us decouple this component from lte_lc and make lte_net_if_enable just call nrf_modem_lib_init.

2. Now onto _IF_DOWN_DISCONNECT and _IF_DOWN_SHUTDOWN; these options change the behavior of net_if_down, in a way that makes it no longer the opposite of what net_if_up does, which instead always just turns on the modem. I argue that we can remove that option altogether, and let net_if_up and net_if_down be symmetrical in all cases and just manipulate the modem power status.

[glarsennordic]

now that that option is deprecated we should add an unconditional call to lte_lc_init there instead, which would let us decouple this component from lte_lc and make lte_net_if_enable just call nrf_modem_lib_init

That seems reasonable to me -- @simensrostad do you have thoughts?

Now onto _IF_DOWN_DISCONNECT and _IF_DOWN_SHUTDOWN; these options change the behavior of net_if_down, in a way that makes it no longer the opposite of what net_if_up does, which instead always just turns on the modem. I argue that we can remove that option altogether, and let net_if_up and net_if_down be symmetrical in all cases and just manipulate the modem power status.

I'm not sure we should get rid of this flag -- it arises from a fundamental ambiguity about what the nature of net_if_up and net_if_down should be for the LTE iface

Perspective 1: IF_DOWN_DISCONNECT user (net_if_xxx purist)

From this perspective, net_if_up and net_if_down should affect only whether the modem is "ready" to try to connect to LTE. It should not affect anything else -- namely, GNSS.

In that case, _IF_DOWN_SHUTDOWN isn't acceptable because it also shuts down GNSS.

Furthermore, from this perspective, the call to modem_init is technically not quite appropriate, but included out of necessity / convenience. And so, from this perspective, net_if_up and net_if_down are already effectively symmetric (when using IF_DOWN_DISCONNECT) because their main purpose was never anything to do with modem_init in the first place -- they are effectively no-op functions, other than the convenience init.

Perspective 2: IF_DOWN_SHUTDOWN user (functionality maximalist / symmetry purist)

From this perspective, net_if_up and net_if_down ought to do something, so why not make them affect whether the modem as a whole is initialized? This violates the principle that net_if_up/net_if_down should only affect networking state, but some customers may not care about this, and are happy to gain the extra functionality.

In that case, the call to modem_init is not an incidental necessity, but the main purpose of net_if_up, and so it makes sense to have net_if_down call the corresponding shutdown function.

Because both of these perspectives have their merits, I think it is probably reasonable to keep both options

I'd be open to marking one option or the other as experimental, though, (assuming that is possible) until this solution becomes more mature. Maybe we will find that there really is no demand for one perspective or the other

[lemrey]

I'm not sure we should get rid of this flag -- it arises from a fundamental ambiguity about what the nature of net_if_up and net_if_down should be for the LTE iface

I think it's bad to have ambiguity as to what an API does; we should settle on what we want those to do for the LTE modem, and remove flags that modify the API behavior.

If net_if_up and net_if_down should just let the modem be "ready" to connect to the LTE network without affecting GNSS, then they should do nothing beyond setting the UP flag if the modem is on (or return an error otherwise), and we should accept that the power state of the modem can't be managed with those API.

Otherwise, we accept that they manage the power status (always).

Because both of these perspectives have their merits, I think it is probably reasonable to keep both options

I do not agree with this, we should settle on what semantics we want for net_if_up and net_if_down when we talk about the LTE modem. If we let API change their behavior based on flags, it will be confusing both by itself, since we don't know what the API does anymore, and together with the other APIs that already exist since the user won't know what to use.

[lemrey]

I think the last point is important, let me clarify what I mean about not knowing what APIs to use.
This is a problem both for NCS maintainers and for NCS users.

These are all the different ways to turn on the modem (perhaps):

• do nothing (NRF_MODEM_LIB_SYS_INIT (deprecated))
• do nothing (NET_IF_AUTO_START (deprecated by this PR))
• call nrf_modem_lib_init()
• call net_if_up()

To turn off the modem:

• do nothing (fail to attach, NET_IF_AUTO_DOWN, with _MODEM_SHUTDOWN flag set)
• call nrf_modem_lib_shutdown()
• call net_if_down() (with _MODEM_SHUTDOWN flag set)

To attach to the LTE network:

• do nothing (do any of the 4 actions in "turn on modem", with LTE_AUTO_INIT_AND_CONNECT (deprecated))
• call net_if_up() (with NET_IF_AUTO_CONNECT)
• call nrf_modem_at_printf("AT+CFUN=1")
• call lte_lc_init_and_connect() (and lte_lc_connect())
• call conn_mgr_connect()

To detach from the LTE network:

• call nrf_modem_at_printf("AT+CFUN=4")
• call lte_lc_offline()
• call net_if_down() (with _LTE_DISCONNECT flag)
• call conn_mgr_disconnect()

I might have missed some, you can see why.

The issue I see here is that there are too many APIs/ways to do the same thing; this is only further exacerbated by APIs that change behavior based on flags.

[glarsennordic]

Sorry I missed this in my earlier reviews, I was confused -- Do not remove this, it has nothing to do with NRF_MODEM_LIB_SYS_INIT

It is a convenience KConfig that controls the value of NET_IF_NO_AUTO_START for the LTE iface. We definitely don't want to get rid of it.

The description in KConfig is a bit misleading -- It's technically correct because net_if_up does initialize the modem, and NET_IF_NO_AUTO_START causes net_if_up to be called by Zephyr on boot, but the setting is really about controlling NET_IF_NO_AUTO_START.

If anything I think the description just needs to be updated to talk directly about net_if_up and NET_IF_NO_AUTO_START rather than the implicit consequences of that flag being set/unset

[glarsennordic]

See this line for where it is used:

sdk-nrf/lib/nrf_modem_lib/nrf91_sockets.c

Line 1106 in b88446c

if (!IS_ENABLED(CONFIG_NRF_MODEM_LIB_NET_IF_AUTO_START)) {

[lemrey]

It's technically correct because net_if_up does initialize the modem, and NET_IF_NO_AUTO_START causes net_if_up to be called by Zephyr on boot

There is a big problem with that. I have tried to explain it in the comment below.

[glarsennordic]

I am not really convinced that it is detrimental to allow the net_if_down behavior of the integration to be customizable. Nor am I personally convinced we are suffering from an over-abundance of ways to do things.

In my eyes, we have three pretty nice abstraction levels, and the user is free to choose which level of abstraction is appropriate for their application:

1. Direct modem interaction (nrf_modem_at_printf and nrf_modem_lib_xxx)
2. lte_lc
3. conn_mgr/Zephyr (net_if_up, net_if_down, net_if_connect, net_if_disconnect)

Everything else you list are just flags which cause some combination of net_if_up, net_if_down, net_if_connect, net_if_disconnect to be fired automatically when conditions are met.

I personally feel that as long as the descriptions are written clearly (IE: Always in terms of which net_if_xxx function they automatically call), that shouldn't be too confusing. Admittedly, the description for NRF_MODEM_LIB_NET_IF_AUTO_START currently violates this, so we should definitely update that.

I also strongly hesitate to conclude on a single behavior from among the two options. Generally the samples that the Portland team is responsible for don't actually need net_if_down to ever be called, so it's not something that currently affects us.

But I would not want to choose IF_DOWN_SHUTDOWN and then be caught in a situation that both requires GNSS and necessitates using conn_mgr_all_if_down. (For instance, an application wanting to sleep all connectivity options, but keep tracking GNSS)

I would also not want to choose IF_DOWN_DISCONNECT and then be caught in a situation where net_if_down shutting down the modem allows conn_mgr_all_if_down to be the only call made. (for instance, an application wanting to support some kind of deep sleep)

It isn't clear to me which of the above two scenarios is more likely. Arguably, both are quite likely, and so we cannot remove one of the two options without harming a reasonable use-case.

But if we really must choose, I am inclined to suggest that we postpone that decision until later, when use cases are potentially more clear

[lemrey]

Automatically turning on the modem in SYS_INIT, is bad idea.

Turning on the modem is an operation that blocks for up to two minutes due to the time it takes to execute the update.
In a real product (DFU-enabled), it is not acceptable to block that long before even getting a chance to run any code, and that is it.
Imagine your device getting a firmware a update and then, boom, screen goes black for 2 minutes.
No blinking LEDs, no messages on screen, no sound; the device appears dead for minutes.

I cannot see a good reason why a product developer would want that.
What have they achieved, beyond avoid calling one parameter-less function?
Even that minuscule "advantage" is immediately defeated by the fact that the application needs to know the result of the initialization, in case it failed, and so the user would need to add code for that purpose.

Finally, if the user is really inclined, they can just declare a one-line SYS_INIT routine to do this themselves anyway.
Certainly we shouldn't encourage that, given all the above reasons.

Automatically initializing the modem in SYS_INIT is counterproductive, and not really usable in a real product, which is why we had just deprecated in NCS v2.4.0...

To conclude, if net_if_up and net_if_down turn on/off the modem then NET_IF_NO_AUTO_START should always be set (and we can remove the Kconfig option to control that).

Then onto the flags (shutdown/lte-disconnect) and Kconfig (auto-connect, auto-down).

we have three pretty nice abstraction levels, and the user is free to choose which level of abstraction is appropriate for their application

That is what I am trying to achieve with this PR, currently, like I have shown above, there are 4 ways to do most things, for example attach/detach from the network:

1. libmodem native (AT command)
2. ncs native (lte_lc)
3. Zephyr native (conn_mgr)
4. net_if_up with Kconfig / net_if_down with flag

The fourth way consist of net_if_up with a Kconfig and net_if_down with a flag (which is in addition to the Zephyr APIs conn_mgr_if_connect and conn_mgr_if_disconnect, right?). Note that these calls may be explicit in the application, or not, depending on more Kconfig options (AUTO_INIT, AUTO_DOWN). Furthermore, net_if_up and net_if_down can also be used to turn on the modem (without Kconfig/flags), and to turn off the modem (with a flag).

As you can see, the net_if_ APIs can basically be used to do anything based on a Kconfig/flag combination. There is no clear semantic to what they do, and I see a problem with that. If you were to read net_if_up in the code you'd have no idea what that call does. Is it turning on the modem? Maybe, if it's not already initialized. Is it attaching to LTE? maybe, if you have a Kconfig set. And net_if_down, will it turn off the modem? Maybe, if you have set a flag. Will it detach from LTE? Maybe, if you set the other flag.

This is not at all a clear abstraction; it creates confusion as to what these functions do each time they are called.

I think we should at the very least remove the flags to change the behavior of net_if_down.
If the user wants to turn off the modem they can call net_if_down, if they want to detach they can call conn_mgr_if_disconnect.

There is a dedicated API to detach, so what do we achieve by extending another API with a flag to let it do the same thing?

This would at least remove the ambiguity as to what net_if_down does (and consequently NET_IF_AUTO_DOWN).
Parallelly, we can have net_if_up turn on the modem and conn_mgr_if_connect attach to the network.

Again, there is a dedicate API to attach, so what do we achieve by extending net_if_up with a KConfig to let it do the same thing? NET_IF_AUTO_CONNECT makes net_if_up ambiguous, so I would like to understand one thing: what are the trying to achieve/solve with this Kconfig? Why should we support NET_IF_AUTO_CONNECT for this specific network interface?

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[glarsennordic]

I can unblock this if you remove the changes that aren't related to the rename

We discussed offline that the other changes require a group meeting before moving forward

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.