nrfcloud coap: use downloader library #19908

maxd-nordic · 2025-01-14T15:14:33Z

Rework the implementation for nRF Cloud CoAP downloads to use downloader instead of a custom implementation. This also adds some missing features to downloader and cleans up the previous code from fota_download and nrf_cloud_download.
The old implementation had issues with resumption which should be resolved by this. Also, by reducing custom code, maintainability should be improved.

NordicBuilder · 2025-01-14T15:16:49Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 20

Inputs:

Sources:

sdk-nrf: PR head: 09b000c8cdaf47c17ac88e22b205d52c8da11329

more details

sdk-nrf:

PR head: 09b000c8cdaf47c17ac88e22b205d52c8da11329
merge base: 82d3d2677adf85c5b9645b34f69e787fb25ff2f3
target head (main): 1e301e40ae1d342703b1226979dce15e8690716c
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (20)

doc
│  ├── nrf
│  │  ├── releases_and_maturity
│  │  │  ├── releases
│  │  │  │  │ release-notes-changelog.rst
include
│  ├── net
│  │  ├── downloader.h
│  │  │ fota_download.h
subsys
│  ├── net
│  │  ├── lib
│  │  │  ├── downloader
│  │  │  │  ├── Kconfig
│  │  │  │  ├── src
│  │  │  │  │  ├── transports
│  │  │  │  │  │  ├── coap.c
│  │  │  │  │  │  │ http.c
│  │  │  ├── fota_download
│  │  │  │  ├── Kconfig
│  │  │  │  ├── src
│  │  │  │  │  │ fota_download.c
│  │  │  ├── nrf_cloud
│  │  │  │  ├── Kconfig.nrf_cloud_coap
│  │  │  │  ├── Kconfig.nrf_cloud_fota
│  │  │  │  ├── coap
│  │  │  │  │  ├── include
│  │  │  │  │  │  │ nrf_cloud_coap_transport.h
│  │  │  │  │  ├── src
│  │  │  │  │  │  │ nrf_cloud_coap_transport.c
│  │  │  │  ├── include
│  │  │  │  │  │ nrf_cloud_download.h
│  │  │  │  ├── src
│  │  │  │  │  ├── nrf_cloud_download.c
│  │  │  │  │  │ nrf_cloud_fota_poll.c
tests
│  ├── subsys
│  │  ├── net
│  │  │  ├── lib
│  │  │  │  ├── downloader
│  │  │  │  │  │ CMakeLists.txt
│  │  │  │  ├── fota_download
│  │  │  │  │  │ CMakeLists.txt
│  │  │  │  ├── lwm2m_client_utils
│  │  │  │  │  │ CMakeLists.txt
│  │  │  │  ├── lwm2m_fota_utils
│  │  │  │  │  │ CMakeLists.txt
│  │  │  │  ├── mcumgr_smp_client
│  │  │  │  │  │ CMakeLists.txt

Outputs:

Toolchain

Version: 342151af73
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:342151af73_912848a074

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
- sdk-nrf test count: 345
✅ Integration tests
- ✅ test-fw-nrfconnect-boot
- ✅ test-fw-nrfconnect-nrf-iot_serial_lte_modem
- ✅ test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- ✅ test-fw-nrfconnect-nrf-iot_samples
- ✅ test-fw-nrfconnect-nrf-iot_lwm2m
- ✅ test-fw-nrfconnect-nrf-iot_thingy91
- ✅ test-fw-nrfconnect-nrf-iot_mosh
- ✅ test-fw-nrfconnect-nrf-iot_positioning
- ✅ test-sdk-mcuboot
- ⚠️ test-fw-nrfconnect-fw-update
- ⚠️ test-fw-nrfconnect-nrf-iot_cloud

Disabled integration tests

- desktop52_verification
- doc-internal
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-chip
- test-fw-nrfconnect-fem
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_libmodem-nrf
- test-fw-nrfconnect-nrf_crypto
- test-fw-nrfconnect-ps
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-tfm
- test-fw-nrfconnect-thread
- test-fw-nrfconnect-zigbee
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-find-my
- test-sdk-pmic-samples
- test-sdk-sidewalk
- test-sdk-wifi
- test-secdom-samples-public

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

NordicBuilder · 2025-01-14T15:32:23Z

You can find the documentation preview for this PR at this link.

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

maxd-nordic · 2025-01-17T13:04:18Z

note: CONFIG_DOWNLOADER_STACK_SIZE=4096 is needed when integrating this, the normal stack size is too small.

maxd-nordic · 2025-01-17T15:20:16Z

~~reliability issue: https://github.com/hello-nrfcloud/firmware/actions/runs/12831259647/job/35781981043~~
RESOLVED: https://github.com/hello-nrfcloud/firmware/actions/runs/12868170261

subsys/net/lib/nrf_cloud/src/nrf_cloud_download.c

include/net/downloader.h

subsys/net/lib/downloader/src/transports/coap.c

subsys/net/lib/fota_download/src/fota_download.c

eivindj-nordic · 2025-01-20T13:18:56Z

include/net/fota_download.h

+int fota_download_with_host_cfg(const char *host, const char *file,
+	int sec_tag, uint8_t pdn_id, size_t fragment_size,
+	const enum dfu_target_image_type expected_type, const struct downloader_host_cfg *host_cfg);


I wonder if we should define a struct for the host config in fota_download.h instead of reusing it from the downloader. On one hand it saves a struct definition and copy of fields when calling the downloader. On the other hand it makes it harder to get the overview of the interface. Now one has to navigate both this header, downloader.h and dfu_target.h to know what arguments to fill in. Perhaps this is a bigger task than what is in the scope of this PR.

Replace some magic numbers in downloader:coap with macros. This also makes it look more similar to the http transport code. Signed-off-by: Maximilian Deubel <[email protected]>

doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst

include/net/downloader.h

include/net/fota_download.h

doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst

subsys/net/lib/downloader/src/transports/coap.c

subsys/net/lib/nrf_cloud/src/nrf_cloud_download.c

subsys/net/lib/downloader/src/transports/coap.c

nordicjm · 2025-01-21T11:30:56Z

include/net/fota_download.h

@@ -145,13 +138,38 @@ int fota_download_init(fota_download_callback_t client_callback);
 * @retval 0	     If download has started successfully.
 * @retval -EALREADY If download is already ongoing.
 * @retval -E2BIG    If sec_tag_count is larger than
- *		     @kconfig{CONFIG_FOTA_DOWNLOAD_SEC_TAG_LIST_SIZE_MAX}
+ *		     `CONFIG_FOTA_DOWNLOAD_SEC_TAG_LIST_SIZE_MAX`


why the change? The old way should create a direct link in the generated documentation which goes to the Kconfig page, why would you want to remove that?

according to @peknis these are not supported anymore in the docs

Comment from Gerard:

the Doxygen macro still exists and should be used. It now renders a verbatim block, but that may change.

With Breathe now removed, this notation does not produce a link in doxygen docs anymore and renders weirdly. Therefore, suggesting to dump that notation and use single backticks only to make it look cleaner.

(from Gerard's comment in one of the JIRAs): breathe, the extension we used, had a poor performance (with removal CI went from ~40min to ~15min in NCS) and was not maintained properly. Upstream Zephyr proceeded with the removal of it and so NCS followed.

Keeping the macro has some advantages:

When reading code, it's clear this is a Kconfig

Allows us to tweak it in the future to, maybe, link somewhere

We can also tweak the style

So please, keep using @kconfig even if it just renders a verbatim block. Upstream Zephyr also does this, so let's be consistent.

It looks like the docs are broken and IMO we should not continue making weird-looking docs just because upstream does it for whatever reason.

that's because of that hideous zoom thing though, it looks absolutely fine without it https://ncsdoc.z6.web.core.windows.net/latest/nrf/doxygen/html/group__lte__lc_ga4c137ae6b3b1c7006dd730458958fb77.html#ga4c137ae6b3b1c7006dd730458958fb77

I'll open a PR just for that change.

That sounds like an argument that we can't adopt everything from upstream and need to make adaptations in NCS

@peknis @nordicjm @gmarull Please continue the discussion here: #20022

include/net/fota_download.h

subsys/net/lib/fota_download/src/fota_download.c

subsys/net/lib/nrf_cloud/coap/src/nrf_cloud_coap_transport.c

subsys/net/lib/nrf_cloud/include/nrf_cloud_codec_internal.h

subsys/net/lib/nrf_cloud/src/nrf_cloud_download.c

Add support for Proxy-URI options to downloader. This is defined in RFC7252 and we need it for nRF Cloud. Signed-off-by: Maximilian Deubel <[email protected]>

Add support for an authentication callback to the downloader library. The callback is called after connecting and allows to authenticate the current socket. nRF Cloud uses authentication in this way for their CoAP backend. Signed-off-by: Maximilian Deubel <[email protected]>

CoAP URLs were parsed twice, leading to errors. Now, the backend doesn't try to treat the file name as a URL anymore. Signed-off-by: Maximilian Deubel <[email protected]>

Integrate the improved downloader library into nRF Cloud. Instead of using a custom downloader backend, use downloader for coap downloads. Also, remove external_download_client support since it isn't used anymore. Signed-off-by: Maximilian Deubel <[email protected]>

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 14, 2025

maxd-nordic force-pushed the downloader-nrfcloud-coap branch 2 times, most recently from ff29a72 to 48a0dae Compare January 15, 2025 14:10

maxd-nordic force-pushed the downloader-nrfcloud-coap branch 2 times, most recently from 575a09a to c6ee5da Compare January 17, 2025 14:10

maxd-nordic marked this pull request as ready for review January 17, 2025 14:14

maxd-nordic requested review from a team as code owners January 17, 2025 14:14

maxd-nordic changed the title ~~Downloader nrfcloud coap~~ nrfcloud coap: use downloader library Jan 17, 2025

maxd-nordic requested review from plskeggs, jtguggedal and simensrostad January 17, 2025 14:18

maxd-nordic force-pushed the downloader-nrfcloud-coap branch from c6ee5da to 2b2c4d8 Compare January 17, 2025 15:00

maxd-nordic force-pushed the downloader-nrfcloud-coap branch from 2b2c4d8 to f9b7ac7 Compare January 17, 2025 16:05

plskeggs approved these changes Jan 17, 2025

View reviewed changes

subsys/net/lib/nrf_cloud/src/nrf_cloud_download.c Show resolved Hide resolved

jtguggedal reviewed Jan 20, 2025

View reviewed changes

maxd-nordic force-pushed the downloader-nrfcloud-coap branch 2 times, most recently from 44bc70a to 41264ac Compare January 20, 2025 12:14

jtguggedal approved these changes Jan 20, 2025

View reviewed changes

eivindj-nordic approved these changes Jan 20, 2025

View reviewed changes

net: downloader: coap: style fixes

e3cb088

Replace some magic numbers in downloader:coap with macros. This also makes it look more similar to the http transport code. Signed-off-by: Maximilian Deubel <[email protected]>

maxd-nordic force-pushed the downloader-nrfcloud-coap branch from 41264ac to cbe25ae Compare January 20, 2025 16:07

maxd-nordic requested a review from a team as a code owner January 20, 2025 16:07

maxd-nordic requested a review from divipillai January 20, 2025 16:07

github-actions bot added doc-required PR must not be merged without tech writer approval. and removed changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Jan 20, 2025

peknis requested changes Jan 21, 2025

View reviewed changes

doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst Outdated Show resolved Hide resolved

peknis requested changes Jan 21, 2025

View reviewed changes

include/net/downloader.h Outdated Show resolved Hide resolved

include/net/fota_download.h Show resolved Hide resolved

maxd-nordic force-pushed the downloader-nrfcloud-coap branch from cbe25ae to 1aae103 Compare January 21, 2025 08:57

maxd-nordic requested a review from peknis January 21, 2025 08:58

peknis requested changes Jan 21, 2025

View reviewed changes

doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst Outdated Show resolved Hide resolved

rlubos approved these changes Jan 21, 2025

View reviewed changes

maxd-nordic force-pushed the downloader-nrfcloud-coap branch from 1aae103 to 13eb271 Compare January 21, 2025 09:05

maxd-nordic requested a review from peknis January 21, 2025 09:45

peknis requested changes Jan 21, 2025

View reviewed changes

doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst Outdated Show resolved Hide resolved

maxd-nordic force-pushed the downloader-nrfcloud-coap branch from fc60568 to 920fc62 Compare January 21, 2025 11:02

maxd-nordic requested a review from peknis January 21, 2025 11:04

peknis approved these changes Jan 21, 2025

View reviewed changes

maxd-nordic force-pushed the downloader-nrfcloud-coap branch from 920fc62 to f3fa476 Compare January 21, 2025 11:08

jukkar approved these changes Jan 21, 2025

View reviewed changes

nordicjm reviewed Jan 21, 2025

View reviewed changes

maxd-nordic force-pushed the downloader-nrfcloud-coap branch from f3fa476 to 12b3157 Compare January 21, 2025 11:44

maxd-nordic requested a review from nordicjm January 21, 2025 11:45

maxd-nordic force-pushed the downloader-nrfcloud-coap branch from 12b3157 to 9b2c66a Compare January 21, 2025 11:54

maxd-nordic added 4 commits January 21, 2025 13:04

net: downloader: add coap proxy-uri support

54d625d

Add support for Proxy-URI options to downloader. This is defined in RFC7252 and we need it for nRF Cloud. Signed-off-by: Maximilian Deubel <[email protected]>

net: downloader: fix coap url parsing

38b1ef5

CoAP URLs were parsed twice, leading to errors. Now, the backend doesn't try to treat the file name as a URL anymore. Signed-off-by: Maximilian Deubel <[email protected]>

maxd-nordic force-pushed the downloader-nrfcloud-coap branch from 9b2c66a to 09b000c Compare January 21, 2025 12:04

nordicjm approved these changes Jan 21, 2025

View reviewed changes

rlubos merged commit fbec239 into nrfconnect:main Jan 21, 2025
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nrfcloud coap: use downloader library #19908

nrfcloud coap: use downloader library #19908

maxd-nordic commented Jan 14, 2025 •

edited

Loading

NordicBuilder commented Jan 14, 2025 •

edited

Loading

NordicBuilder commented Jan 14, 2025

maxd-nordic commented Jan 17, 2025

maxd-nordic commented Jan 17, 2025 •

edited

Loading

eivindj-nordic Jan 20, 2025

nordicjm Jan 21, 2025

maxd-nordic Jan 21, 2025

nordicjm Jan 21, 2025

peknis Jan 21, 2025 •

edited

Loading

gmarull Jan 21, 2025

jtguggedal Jan 21, 2025

nordicjm Jan 21, 2025

maxd-nordic Jan 21, 2025

jtguggedal Jan 21, 2025

maxd-nordic Jan 21, 2025

nrfcloud coap: use downloader library #19908

nrfcloud coap: use downloader library #19908

Conversation

maxd-nordic commented Jan 14, 2025 • edited Loading

NordicBuilder commented Jan 14, 2025 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

NordicBuilder commented Jan 14, 2025

maxd-nordic commented Jan 17, 2025

maxd-nordic commented Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peknis Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxd-nordic commented Jan 14, 2025 •

edited

Loading

NordicBuilder commented Jan 14, 2025 •

edited

Loading

maxd-nordic commented Jan 17, 2025 •

edited

Loading

peknis Jan 21, 2025 •

edited

Loading