modem on nrf91 - #146
Conversation
91ab905 to
8b1acec
Compare
Drop controller/ application, as that is no longer used. Signed-off-by: Marcin Niestroj <marcin.niestroj@canonical.com>
This comes with new Zephyr (v4.3.0) and NCS (v3.2.3) versions. Signed-off-by: Marcin Niestroj <marcin.niestroj@canonical.com>
Disable bootloader by default for now, so that there is more flash space available on the application side. DFU was not part of gateway application, so removing bootloader has no functional changes. Signed-off-by: Marcin Niestroj <marcin.niestroj@canonical.com>
e766a93 to
473cd23
Compare
trond-snekvik
left a comment
There was a problem hiding this comment.
Long term, what should we do here? We'll have to migrate all of this every time we want to bump the NCS version we target, and maintaining this copy, but with an increasing set of patches seems daunting, especially considering Nordic deliberately removed support for the nRF9160dk in October 2025. They won't accept patches to add it back in, as they were probably very relieved to reduce the number of platforms they had to test on.
We can fork the serial modem and pull that in through west, but porting to newer versions of NCS will be an ongoing resource drain, and the set of changes we apply will probably keep growing.
If we want to use the serial modem in this way, my suggestion would be that we fork a version of the serial modem that we're happy with, but treat it as a standalone project, and provide pre-built binaries for the hardware we want demos to work on. This would allow us to disconnect the serial modem release cycle from our pouch release cycle, and we wouldn't actually have to migrate it to newer versions of NCS -- maybe ever. We'd also be able to more clearly separate our changes and additions from the existing functionality.
The problem with pre-built binaries is that it only really works for demos, as users would have to build their own versions for their own hardware. This means they have to go through the same deep dive that you have done here, and it's going to require a skill set that not all engineering teams have access to.
Long term, I think we should consider reinstating the nRF91 as the primary controller, and use the nRF52 as a Bluetooth-UART bridge, leaving the nRF91 disconnected from the Bluetooth transport. In this configuration, we would create a serial transport for pouch, and use that to communicate between the two devices. If the Bluetooth device is just a bridge that speaks pouch over Bluetooth on one end and pouch over UART on the other, we could maintain our pouch library paradigm on the nRF91, and with proper transport abstraction, it could be the same app that we run on other internet connected devices. This would force us to make design and architecture decisions that would be very healthy for the portability and usability of Pouch in the long term, as we'd actually have a second, very versatile transport to design our transport abstraction for.
| # nRF Cloud | ||
| CONFIG_NRF_CLOUD=y | ||
| CONFIG_NRF_CLOUD_MQTT=y | ||
| CONFIG_NRF_CLOUD_FOTA=n | ||
| CONFIG_NRF_CLOUD_AGNSS=y | ||
| CONFIG_NRF_CLOUD_AGNSS_FILTERED=n | ||
| CONFIG_NRF_CLOUD_PGPS=n | ||
| CONFIG_NRF_CLOUD_LOCATION=y | ||
| CONFIG_NRF_CLOUD_LOG_LEVEL_INF=y | ||
| CONFIG_NRF_CLOUD_GPS_LOG_LEVEL_INF=y | ||
| CONFIG_NRF_CLOUD_CLIENT_ID_SRC_INTERNAL_UUID=y | ||
| CONFIG_NRF_CLOUD_WIFI_LOCATION_ENCODE_OPT_MAC_RSSI=y | ||
| CONFIG_DATE_TIME=y | ||
| CONFIG_MODEM_INFO=y | ||
| CONFIG_MODEM_INFO_ADD_DATE_TIME=n |
There was a problem hiding this comment.
Thoughts on disabling nRF Cloud? Replacing this with CONFIG_SM_NRF_CLOUD=n builds, and removes 50k of flash. It increases the diff between this build and the original though.
There was a problem hiding this comment.
We can definitely remove (and should) NRF_CLOUD from here. This file is just a copy paste of the ncs-serial-modem prj.conf (which I am sure you already figured out).
| # | ||
| # PPP | ||
| # | ||
| CONFIG_SM_PPP=y | ||
|
|
||
| # NCS modem libraries | ||
| CONFIG_AT_CMD_CUSTOM=y | ||
|
|
||
| CONFIG_NRF_MODEM_LIB_SHMEM_TX_SIZE=22528 | ||
|
|
||
| # Zephyr PPP support | ||
| CONFIG_NET_NATIVE=y | ||
| CONFIG_NET_L2_PPP=y | ||
| CONFIG_MODEM_MODULES=y | ||
| CONFIG_MODEM_PPP=y | ||
|
|
||
| # L2 protocol | ||
| CONFIG_NET_L2_PPP_MGMT=y | ||
| CONFIG_NET_L2_PPP_OPTION_MRU=y | ||
| CONFIG_NET_L2_PPP_OPTION_SERVE_IP=y | ||
| CONFIG_NET_L2_PPP_OPTION_SERVE_DNS=y | ||
| CONFIG_NET_L2_PPP_TIMEOUT=5000 | ||
| CONFIG_NET_L2_PPP_MAX_CONFIGURE_REQ_RETRANSMITS=20 | ||
|
|
||
| # IP stack | ||
| CONFIG_NET_IP_ADDR_CHECK=n | ||
| CONFIG_NET_SOCKETS_PACKET=y | ||
|
|
||
| # network buffering | ||
| CONFIG_NET_BUF=y | ||
| CONFIG_NET_BUF_POOL_USAGE=y | ||
| CONFIG_NET_PKT_RX_COUNT=44 | ||
| CONFIG_NET_BUF_RX_COUNT=88 | ||
| CONFIG_NET_PKT_TX_COUNT=44 | ||
| CONFIG_NET_BUF_TX_COUNT=88 | ||
| CONFIG_NET_TC_RX_COUNT=0 | ||
| CONFIG_NET_CONN_PACKET_CLONE_TIMEOUT=1000 |
There was a problem hiding this comment.
AFAICT, this part is unchanged from the ppp overlay file. Could we just add that to the overlays instead?
The same goes for the CMUX settings, except for the buffer sizes - we could do the same there, but add our changes to a separate overlay that gets applied at the end (along with the UART changes at the top of this file too).
Determining what changed here compared to the original gets pretty hard when the files are merged, as diffing becomes less effective.
There was a problem hiding this comment.
We can definitely apply the overlay. There are two options I see how that can happen:
-
First option is that we will apply the overlay using west build command. This however implies longer west build command in the docs, so that the user can copy-paste it. This part is something we need to potentially maintain and let users know whenever something changes (ncs-serial-modem repo might reorganize things).
-
The second option is to apply that implicitly from CMakeLists. This seems counterintuitive.
I would rather try to optimize that config to be minimal (so remove any unused features or non-default options that do not make sense in our context). This means that it will still be kind of a "merge" of configs from ncs-serial-modem repo, but strictly for our use-case (running with another Zephyr-powered MCU, which is always pouch-gateway).
| modem_backend_uart_isr_flush(backend); | ||
| if (backend->dtr_gpio) { | ||
| gpio_pin_set_dt(backend->dtr_gpio, 1); | ||
| + k_sleep(K_MSEC(50)); |
There was a problem hiding this comment.
These make me very uncomfortable. What is the context for these?
|
|
||
| zephyr_include_directories(${ZEPHYR_NCS_SERIAL_MODEM_MODULE_DIR}/app/src) | ||
|
|
||
| include(${ZEPHYR_NCS_SERIAL_MODEM_MODULE_DIR}/app/cmake/sm_version.cmake) |
There was a problem hiding this comment.
Do you think it'd be possible to instead provide a set of DTS and Kconfig overlays that can be applied over the NCS serial modem app, and still have it build? We're doing a lot of fire juggling in this directory to replicate the serial modem build system contents, with some changes sprinkled in, and I'm worried about maintenance.
I'm sure I've missed something, but we have the following changes to the serial modem app:
- CMUX and PPP overlays are enabled
- UART buffer sizes are changed
- Reintroduce DTS overlays for nRF9160dk
- Thingy91x overlays connect to the nRF52 UART port instead of the USB UART port
The rest of this directory is here to allow us to override these parts. It took me an hour to find which parts were copies and which were additions, and I'm going to forget which is which by Monday, and it worries me that we're making our users consider what's in here too.
There was a problem hiding this comment.
Do you think it'd be possible to instead provide a set of DTS and Kconfig overlays that can be applied over the NCS serial modem app, and still have it build?
That is mainly how I developed this "modem support in pouch-gateway". This was not easy, as each version bump there were changes in ncs-serial-modem configuration that I needed to apply. Each target (nrf9160dk and thingy91x) required a bit different set of overlays (e.g. nrf9160dk required using overlay-nrf9160dk-nrf52840.overlay and previously also .conf changes as well due to some configuration being there).
The main reasons why I created separate application were:
- flatten configuration (so reduce number of overlays) for each hw platform
- easier build, i.e. simple
west buildcommand, as we do not need to support unlimited number of configurations (hence overlays for different use-cases) and can just "optimize" for pouch-gateway
Forking seems a good idea. Or alternatively creating a separate project that will point to specific version of |
Add ncs-serial-modem application, which is based on ncs-serial-modem (v1.0.0) upstream project. This application builds upstream sources. Provide overlay configurations that are pouch-gateway specific. This includes configuration for nrf9160dk and thingy91x boards. Disable thingy91x board-specific sysbuild overrides, so that there is one common configuration for all Nordic development boards (DK and Thingy boards). Copy 'write_sm_version_header.cmake' from upstream project as-is. This is a workaound for cmake include searching path (relative to application directory). Signed-off-by: Marcin Niestroj <marcin.niestroj@canonical.com>
Add 'cjson' to west-ncs.yml, as that is needed for successfully building nrf9160dk/nrf9160 firmware (nrf/application/serial_lte_modem). Workaround no buttons attached to nrf9160dk, by enabling automatic BT bonding and confirmation. Note that this should be only used for development and not for production. Signed-off-by: Marcin Niestroj <marcin.niestroj@canonical.com>
Enable HCI netcore image by default, so that Bluetooth is working with split core architecture, with HCI over IPC. Provide sysbuild defaults, that override/disable all the board specific defaults. This is needed to have more generic support. Add libmetal and open-amp to west-ncs.yml, as that is a requirement for nrf5340 Bluetooth split core achitecture with HCI over IPC. Signed-off-by: Marcin Niestroj <marcin.niestroj@canonical.com>
This snippet can be used to expose console and shell on Expansion Board for thingy91x. This is useful for having nrf9151 logs on this UART interface and nrf5340 logs on RTT interface. Signed-off-by: Marcin Niestroj <marcin.niestroj@canonical.com>
|
This was merged to https://github.com/golioth/pouch |
Major changes:
controller/ncs-serial-modem/