Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

subsys: net: download_client rework #17481

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eivindj-nordic
Copy link
Contributor

@eivindj-nordic eivindj-nordic commented Sep 25, 2024

  • Deprecate current download client
  • Download client restructuring:
    • Moved to new library: downloader
    • Move socket features to a separate file.
    • Restructuring of socket functions.
    • Update internal states.
    • Replace is_idle(), is_connecting, is_downloading(), is closed() and is_finished() with is_state(). This reduces the code size a bit.
    • Align download client variable name.
    • Add interface for transport (http, coap, ...). Goal is to make the library easier to maintain and ease the work to add more transports (mqtt, ...).
  • Use range requests for nRF91 TLS only
  • Add option to use URI instead of separate host and filename as input parameters.
  • Let application provide client buffer. This allows for multiple download clients with different buffer sizes.
  • Parse HTTP header line for line. This reduces the size requirement for the download client recv buffer.
  • Add new field for requesting new data to dlc.
  • Change TLS range override logic.
  • Update libraries and samples.
  • Update tests.
  • Migration guide
  • Fixes to align behavior with new (internal) tests.

@eivindj-nordic eivindj-nordic self-assigned this Sep 25, 2024
@github-actions github-actions bot added doc-required PR must not be merged without tech writer approval. changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Sep 25, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Sep 25, 2024

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 175

Inputs:

Sources:

sdk-nrf: PR head: b4f24a78b92e61b9931858cb4e5543e685169544

more details

sdk-nrf:

PR head: b4f24a78b92e61b9931858cb4e5543e685169544
merge base: d9500b94b05c776294fda96ebb0baaedb553a47e
target head (main): 64064df7382b14849617e8b0d7675382ecf0cb5a
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 (121)
.github
│  │ test-spec.yml
CODEOWNERS
applications
│  ├── asset_tracker_v2
│  │  ├── boards
│  │  │  │ native_sim.conf
│  │  ├── doc
│  │  │  │ asset_tracker_v2_description.rst
│  │  ├── overlay-carrier.conf
│  │  │ prj.conf
│  ├── serial_lte_modem
│  │  ├── doc
│  │  │  │ slm_description.rst
│  │  ├── overlay-carrier.conf
│  │  ├── prj.conf
│  │  ├── src
│  │  │  │ slm_at_fota.c
doc
│  ├── nrf
│  │  ├── app_dev
│  │  │  ├── device_guides
│  │  │  │  ├── nrf91
│  │  │  │  │  │ nrf91_building.rst
│  │  ├── libraries
│  │  │  ├── bin
│  │  │  │  ├── lwm2m_carrier
│  │  │  │  │  ├── app_integration.rst
│  │  │  │  │  │ requirements.rst
│  │  │  ├── networking
│  │  │  │  ├── aws_fota.rst
│  │  │  │  ├── azure_fota.rst
│  │  │  │  ├── download_client.rst
│  │  │  │  ├── downloader.rst
│  │  │  │  ├── fota_download.rst
│  │  │  │  ├── nrf_cloud.rst
│  │  │  │  │ nrf_cloud_pgps.rst
│  │  ├── links.txt
│  │  ├── releases_and_maturity
│  │  │  ├── known_issues.rst
│  │  │  ├── migration
│  │  │  │  │ migration_guide_3.0.rst
│  │  │  ├── migration_guides.rst
│  │  │  ├── releases
│  │  │  │  ├── release-notes-2.4.0.rst
│  │  │  │  │ release-notes-changelog.rst
drivers
│  ├── sensor
│  │  ├── sensor_sim
│  │  │  │ sensor_sim.c
include
│  ├── net
│  │  ├── download_client.h
│  │  ├── downloader.h
│  │  ├── downloader_transport.h
│  │  ├── downloader_transport_coap.h
│  │  ├── downloader_transport_http.h
│  │  ├── fota_download.h
│  │  │ nrf_cloud_pgps.h
lib
│  ├── bin
│  │  ├── lwm2m_carrier
│  │  │  ├── Kconfig
│  │  │  ├── include
│  │  │  │  │ lwm2m_os.h
│  │  │  ├── os
│  │  │  │  │ lwm2m_os.c
samples
│  ├── cellular
│  │  ├── http_update
│  │  │  ├── application_update
│  │  │  │  ├── overlay-carrier.conf
│  │  │  │  ├── prj.conf
│  │  │  │  ├── src
│  │  │  │  │  │ main.c
│  │  │  ├── modem_delta_update
│  │  │  │  ├── prj.conf
│  │  │  │  ├── src
│  │  │  │  │  │ main.c
│  │  │  ├── modem_full_update
│  │  │  │  ├── prj.conf
│  │  │  │  ├── src
│  │  │  │  │  │ main.c
│  │  ├── location
│  │  │  │ overlay-pgps.conf
│  │  ├── lwm2m_carrier
│  │  │  │ prj.conf
│  │  ├── lwm2m_client
│  │  │  ├── prj.conf
│  │  │  │ sample_description.rst
│  │  ├── modem_shell
│  │  │  ├── overlay-carrier.conf
│  │  │  ├── overlay-modem_fota_full.conf
│  │  │  ├── prj.conf
│  │  │  ├── src
│  │  │  │  ├── fota
│  │  │  │  │  │ fota_shell.c
│  │  │  │  ├── gnss
│  │  │  │  │  │ gnss.c
│  │  ├── nrf_cloud_multi_service
│  │  │  ├── Kconfig
│  │  │  │ prj.conf
│  │  ├── nrf_cloud_rest_fota
│  │  │  │ prj.conf
│  ├── net
│  │  ├── aws_iot
│  │  │  ├── boards
│  │  │  │  ├── nrf7002dk_nrf5340_cpuapp_ns.conf
│  │  │  │  ├── nrf9151dk_nrf9151_ns.conf
│  │  │  │  ├── nrf9160dk_nrf9160_ns.conf
│  │  │  │  ├── nrf9161dk_nrf9161_ns.conf
│  │  │  │  ├── thingy91_nrf9160_ns.conf
│  │  │  │  │ thingy91x_nrf9151_ns.conf
│  │  ├── azure_iot_hub
│  │  │  ├── README.rst
│  │  │  ├── boards
│  │  │  │  ├── nrf7002dk_nrf5340_cpuapp_ns.conf
│  │  │  │  ├── nrf9151dk_nrf9151_ns.conf
│  │  │  │  ├── nrf9160dk_nrf9160_ns.conf
│  │  │  │  │ nrf9161dk_nrf9161_ns.conf
│  │  ├── download
│  │  │  ├── README.rst
│  │  │  ├── prj.conf
│  │  │  ├── sample.yaml
│  │  │  ├── src
│  │  │  │  │ main.c
scripts
│  │ quarantine_integration.yaml
subsys
│  ├── dfu
│  │  ├── dfu_target
│  │  │  │ Kconfig
│  ├── net
│  │  ├── lib
│  │  │  ├── CMakeLists.txt
│  │  │  ├── Kconfig
│  │  │  ├── aws_fota
│  │  │  │  ├── src
│  │  │  │  │  │ aws_fota.c
│  │  │  ├── azure_fota
│  │  │  │  │ azure_fota.c
│  │  │  ├── download_client
│  │  │  │  ├── Kconfig
│  │  │  │  ├── src
│  │  │  │  │  │ download_client.c
│  │  │  ├── downloader
│  │  │  │  ├── CMakeLists.txt
│  │  │  │  ├── Kconfig
│  │  │  │  ├── dl_transports.ld
│  │  │  │  ├── include
│  │  │  │  │  ├── dl_parse.h
│  │  │  │  │  │ dl_socket.h
│  │  │  │  ├── src
│  │  │  │  │  ├── dl_parse.c
│  │  │  │  │  ├── dl_socket.c
│  │  │  │  │  ├── downloader.c
│  │  │  │  │  ├── sanity.c
│  │  │  │  │  ├── shell.c
│  │  │  │  │  ├── transports
│  │  │  │  │  │  ├── coap.c
│  │  │  │  │  │  │ http.c
│  │  │  ├── fota_download
│  │  │  │  ├── CMakeLists.txt
│  │  │  │  ├── Kconfig
│  │  │  │  ├── src
│  │  │  │  │  ├── fota_download.c
│  │  │  │  │  ├── util
│  │  │  │  │  │  ├── fota_download_delta_modem.c
│  │  │  │  │  │  ├── fota_download_full_modem.c
│  │  │  │  │  │  │ fota_download_util.c
│  │  │  ├── lwm2m_client_utils
│  │  │  │  ├── lwm2m
│  │  │  │  │  │ lwm2m_firmware.c
│  │  │  ├── mcumgr_smp_client
│  │  │  │  ├── Kconfig
│  │  │  │  ├── src
│  │  │  │  │  │ mcumgr_smp_client_shell.c
│  │  │  ├── nrf_cloud
│  │  │  │  ├── Kconfig.nrf_cloud_fota
│  │  │  │  ├── Kconfig.nrf_cloud_pgps
│  │  │  │  ├── include
│  │  │  │  │  ├── nrf_cloud_download.h
│  │  │  │  │  │ nrf_cloud_pgps_utils.h
│  │  │  │  ├── src
│  │  │  │  │  ├── nrf_cloud_codec_internal.c
│  │  │  │  │  ├── nrf_cloud_download.c
│  │  │  │  │  ├── nrf_cloud_fota.c
│  │  │  │  │  ├── nrf_cloud_fota_poll.c
│  │  │  │  │  ├── nrf_cloud_pgps.c
│  │  │  │  │  │ nrf_cloud_pgps_utils.c
tests
│  ├── subsys
│  │  ├── net
│  │  │  ├── lib
│  │  │  │  ├── aws_fota
│  │  │  │  │  ├── aws_fota_json
│  │  │  │  │  │  │ CMakeLists.txt
│  │  │  │  ├── downloader
│  │  │  │  │  ├── CMakeLists.txt
│  │  │  │  │  ├── prj.conf
│  │  │  │  │  ├── src
│  │  │  │  │  │  │ main.c
│  │  │  │  │  │ testcase.yaml
│  │  │  │  ├── fota_download
│  │  │  │  │  ├── CMakeLists.txt
│  │  │  │  │  ├── src
│  │  │  │  │  │  │ test_fota_download.c
│  │  │  │  ├── lwm2m_client_utils
│  │  │  │  │  │ CMakeLists.txt
│  │  │  │  ├── lwm2m_fota_utils
│  │  │  │  │  │ CMakeLists.txt
│  │  │  │  ├── mcumgr_smp_client
│  │  │  │  │  │ CMakeLists.txt
│  │  │  │  ├── nrf_cloud
│  │  │  │  │  ├── cloud
│  │  │  │  │  │  │ CMakeLists.txt

Outputs:

Toolchain

Version: b77d8c1312
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:b77d8c1312_912848a074

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

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister
    • sdk-nrf test count: 672
  • ❌ Integration tests
    • ✅ test-fw-nrfconnect-boot
    • ✅ test-fw-nrfconnect-chip
    • ✅ 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-zigbee
    • ✅ test-sdk-find-my
    • ✅ test-fw-nrfconnect-nrf-iot_mosh
    • ✅ test-fw-nrfconnect-nrf-iot_positioning
    • ❌ test-sdk-sidewalk
    • ✅ 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-fem
    • test-fw-nrfconnect-nfc
    • test-fw-nrfconnect-nrf-iot_libmodem-nrf
    • test-fw-nrfconnect-nrf-iot_nrf_provisioning
    • 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-low-level
    • test-sdk-audio
    • test-sdk-dfu
    • test-sdk-pmic-samples
    • test-sdk-wifi
    • test-secdom-samples-public

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

Comment on lines 133 to 137
struct download_client_host_cfg {
/** Server hosting the file, null-terminated.
* The host name must be kept in scope while download is going on.
*/
const char *hostname;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are so heavily refactoring, I would propose that we get rid of this separation of host and path.
Just use URI on the API.
It is actually very rare that any interfaces would give us host and path separately. I only know nRF Cloud to do so on some REST APIs. It is more common that we receive full URI where to fetch the data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh.. and also now host is provided inside a struct and path is provided as a function parameter. This emphasizes the separation even further.

It feels like cleanest API would just take URI as one parameter, or field inside this struct. Let the HTTP parser to split protocol, host, port and path.

@eivindj-nordic eivindj-nordic force-pushed the 25700_download_client_rework branch from 3af4cbb to 3e9412e Compare October 17, 2024 07:53
@github-actions github-actions bot removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 17, 2024
@eivindj-nordic eivindj-nordic force-pushed the 25700_download_client_rework branch 6 times, most recently from 6573c1d to 2666f2d Compare October 24, 2024 13:07
@eivindj-nordic eivindj-nordic force-pushed the 25700_download_client_rework branch 9 times, most recently from 272b2de to c058985 Compare November 1, 2024 09:41
@eivindj-nordic eivindj-nordic force-pushed the 25700_download_client_rework branch from c058985 to 49e7000 Compare November 1, 2024 09:51
@eivindj-nordic
Copy link
Contributor Author

Marking ready for review to trigger CI.

@eivindj-nordic eivindj-nordic marked this pull request as ready for review November 1, 2024 12:21
@eivindj-nordic eivindj-nordic requested a review from a team as a code owner November 1, 2024 12:21
Copy link
Contributor

@kacperradoszewski kacperradoszewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

@eivindj-nordic eivindj-nordic force-pushed the 25700_download_client_rework branch 3 times, most recently from 1e839d8 to 4710ce7 Compare December 11, 2024 13:44
@eivindj-nordic
Copy link
Contributor Author

@nrfconnect/ncs-co-build-system @nrfconnect/ncs-nrf-cloud @nrfconnect/ncs-co-networking @nrfconnect/ncs-code-owners @nrfconnect/ncs-pluto @nrfconnect/ncs-test-leads @nrfconnect/ncs-pluto-doc @nrfconnect/ncs-co-drivers Please have a look.

select DEPRECATED
bool "[DEPRECATED] Download client"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

select goes after bool

int "Shell buffer size"
default 2048

endif #DOWNLOADER_SHELL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
endif #DOWNLOADER_SHELL
endif # DOWNLOADER_SHELL

Comment on lines 65 to 67
endif #DOWNLOADER_LOG_LEVEL_DBG

endif #DOWNLOADER
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
endif #DOWNLOADER_LOG_LEVEL_DBG
endif #DOWNLOADER
endif # DOWNLOADER_LOG_LEVEL_DBG
endif # DOWNLOADER

Comment on lines +31 to +32
depends on (DOWNLOADER_MAX_HOSTNAME_SIZE >= 64)
depends on (DOWNLOADER_MAX_FILENAME_SIZE >= 192)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi. brackets aren't needed here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll keep them here as it is used elsewhere in this file.

@eivindj-nordic eivindj-nordic force-pushed the 25700_download_client_rework branch 5 times, most recently from 96b96c0 to f04182d Compare December 13, 2024 08:18
@eivindj-nordic
Copy link
Contributor Author

ping @nrfconnect/ncs-test-leads @nrfconnect/ncs-nrf-cloud @nrfconnect/ncs-co-networking @nrfconnect/ncs-pluto-doc @nrfconnect/ncs-co-drivers

@plskeggs
Copy link
Contributor

plskeggs commented Dec 20, 2024

@eivindj-nordic Because the cloud-iot test nodes are not yet fully operational in Oslo, I ran some manual tests on this PR today.

It works fine with both CoAP and HTTPS downloads for the nrf_cloud_multi_service sample. I have a small PR to fix some warnings but that can wait.

There seems to be an issue with the nrf_cloud_rest_fota sample when using this new downloader. This same FOTA job succeeds with the main branch and the old download_client.

New downloader:
[00:01:12.154,571] <inf> nrf_cloud_fota_poll: Starting FOTA download of static.api.nrfcloud.com/v1/firmwares/modem/MODEM*3471f88e*mfw_nrf91x1_2.0.2-FOTA-TEST/mfw_nrf91x1_update_from_2.0.2_to_2.0.2-FOTA-TEST.bin
[00:01:12.174,407] <wrn> downloader: Protocol not specified for static.api.nrfcloud.com/v1/firmwares/modem/MODEM*3471f88e*mfw_nrf91x1_2.0.2-FOTA-TEST/mfw_nrf91x1_update_from_2.0.2_to_2.0.2-FOTA-TEST.bin, attempting https://
[00:01:12.428,771] <inf> downloader: Failed to resolve hostname static.api.nrfcloud.com on IPv6, err -2
[00:01:12.438,659] <inf> downloader: Host lookup failed for hostname static.api.nrfcloud.com on IPv6 (err -118), attempting IPv4
[00:01:12.450,744] <err> downloader: Host lookup failed for hostname static.api.nrfcloud.com, err -22
[00:01:12.460,693] <err> fota_download: Downloader error event -22
[00:01:12.467,620] <err> nrf_cloud_fota_poll: FOTA download error: 2
[00:01:12.474,761] <inf> nrf_cloud_fota_poll: Updating FOTA job status...


Old download_client:
00:01:30.374,694] <inf> nrf_cloud_fota_poll: Starting FOTA download of static.api.nrfcloud.com/v1/firmwares/modem/MODEM*3471f88e*mfw_nrf91x1_2.0.2-FOTA-TEST/mfw_nrf91x1_update_from_2.0.2_to_2.0.2-FOTA-TEST.bin
[00:01:30.394,317] <inf> download_client: Downloading: v1/firmwares/modem/MODEM*3471f88e*mfw_nrf91x1_2.0.2-FOTA-TEST/mfw_nrf91x1_update_from_2.0.2_to_2.0.2-FOTA-TEST.bin [0]
[00:01:30.777,709] <inf> download_client: Setting up TLS credentials, sec tag count 1
[00:01:30.786,102] <inf> download_client: Connecting to 99.83.231.82
[00:01:34.541,778] <inf> download_client: Downloaded 1700/14648 bytes (11%)
[00:01:34.549,407] <inf> dfu_target_modem_delta: Modem firmware version: 320176d5-9f40-45fc-923b-2661ec18d547
[00:01:34.562,377] <inf> dfu_target_modem_delta: Deleting firmware image, this can take several minutes
+CSCON: 0
+CEREG: 5,"352B","0D8A6017",7,,,"00001010","00000011"
[00:01:41.796,722] <inf> dfu_target_modem_delta: Modem FW delete complete
+CSCON: 1
[00:01:44.155,029] <inf> download_client: Downloaded 3400/14648 bytes (23%)
[00:01:46.139,617] <inf> download_client: Downloaded 5100/14648 bytes (34%)

@eivindj-nordic
Copy link
Contributor Author

eivindj-nordic commented Dec 20, 2024

There seems to be an issue with the nrf_cloud_rest_fota sample when using this new downloader. This same FOTA job succeeds with the main branch and the old download_client.

Thanks for checking @plskeggs. I see that IPv4 is not enabled in the sample (CONFIG_NET_IPV4=y). Hence, DNS lookup fails. I'll update this.

@eivindj-nordic eivindj-nordic force-pushed the 25700_download_client_rework branch 6 times, most recently from 6eda0cd to 4ec564d Compare December 20, 2024 12:03
The new downloader is replacing the download_client library and is
based on that.

Internal restructuring:

* Restructuring of socket functions and files.
* Parse HTTP header line for line. This reduces the size requirement
  for the download client recv buffer.
* Change TLS range override logic.
* Use range requests for nRF91 TLS only, and when specified by app.

API updates:

* Let application provide client buffer. This allows for multiple
  download clients with different buffer sizes.
* Add downloader_deinit()
* Add downloader_stop()
* Remove downloader_disconnect()
* Changed signature of downloader_init(), downloader_start()
  and downloader_get() to take a URI.
* Added downloader_get_with_host_and_path() for downloads where
  host and path are separate arguments to keep backwards compatibility.
* The transports (http, coap) are now separated out of the download
  client with its own API.

Future work:

* Take uri as input param to fota_download library and use URI in
  other relevant libaries and structures.
* Curent download client is deprecated and will be removed later.

Signed-off-by: Eivind Jølsgard <[email protected]>
@eivindj-nordic eivindj-nordic force-pushed the 25700_download_client_rework branch from 4ec564d to b4f24a7 Compare December 20, 2024 12:06
@plskeggs
Copy link
Contributor

There seems to be an issue with the nrf_cloud_rest_fota sample when using this new downloader. This same FOTA job succeeds with the main branch and the old download_client.

Thanks for checking @plskeggs. I see that IPv4 is not enabled in the sample (CONFIG_NET_IPV4=y). Hence, DNS lookup fails. I'll update this.

I tested this manually and it works now. Thanks. I'll approve assuming you make the change.

Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since rebase is needed anyway

# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
#

menuconfig DOWNLOADER
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
menuconfig DOWNLOADER
menuconfig DOWNLOADER

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.