-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
base: main
Are you sure you want to change the base?
subsys: net: download_client rework #17481
Conversation
eivindj-nordic
commented
Sep 25, 2024
•
edited
Loading
edited
- 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.
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: b4f24a78b92e61b9931858cb4e5543e685169544 more detailssdk-nrf:
Github labels
List of changed files detected by CI (121)
Outputs:ToolchainVersion: b77d8c1312 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
include/net/download_client.h
Outdated
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
3af4cbb
to
3e9412e
Compare
6573c1d
to
2666f2d
Compare
272b2de
to
c058985
Compare
c058985
to
49e7000
Compare
Marking ready for review to trigger CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍
1e839d8
to
4710ce7
Compare
@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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
select goes after bool
subsys/net/lib/downloader/Kconfig
Outdated
int "Shell buffer size" | ||
default 2048 | ||
|
||
endif #DOWNLOADER_SHELL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endif #DOWNLOADER_SHELL | |
endif # DOWNLOADER_SHELL |
subsys/net/lib/downloader/Kconfig
Outdated
endif #DOWNLOADER_LOG_LEVEL_DBG | ||
|
||
endif #DOWNLOADER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endif #DOWNLOADER_LOG_LEVEL_DBG | |
endif #DOWNLOADER | |
endif # DOWNLOADER_LOG_LEVEL_DBG | |
endif # DOWNLOADER |
depends on (DOWNLOADER_MAX_HOSTNAME_SIZE >= 64) | ||
depends on (DOWNLOADER_MAX_FILENAME_SIZE >= 192) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
96b96c0
to
f04182d
Compare
ping @nrfconnect/ncs-test-leads @nrfconnect/ncs-nrf-cloud @nrfconnect/ncs-co-networking @nrfconnect/ncs-pluto-doc @nrfconnect/ncs-co-drivers |
@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.
|
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. |
6eda0cd
to
4ec564d
Compare
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]>
4ec564d
to
b4f24a7
Compare
I tested this manually and it works now. Thanks. I'll approve assuming you make the change. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
menuconfig DOWNLOADER | |
menuconfig DOWNLOADER |