subsys: net: download_client rework #17481
Open
+5,306
−405
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
doc-required
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 898ce25474b7446472debd4a630b8488f9d5c2b2 more detailssdk-nrf:
Github labels
List of changed files detected by CI (110)Outputs:ToolchainVersion: f51bdba1d9 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
SeppoTakalo
reviewed
Sep 26, 2024
include/net/download_client.h
Outdated
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; |
There was a problem hiding this comment.
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.
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
force-pushed
the
25700_download_client_rework
branch
from
3af4cbb
to
3e9412e
Compare
github-actions
bot
removed
the
changelog-entry-required
eivindj-nordic
force-pushed
the
25700_download_client_rework
branch
6 times, most recently
from
6573c1d
to
2666f2d
Compare
eivindj-nordic
force-pushed
the
25700_download_client_rework
branch
9 times, most recently
from
272b2de
to
c058985
Compare
eivindj-nordic
force-pushed
the
25700_download_client_rework
branch
from
c058985
to
49e7000
Compare
eivindj-nordic
added
CI-run-twister
and removed
CI-run-zephyr-twister
CI-run-twister
labels
|
Marking ready for review to trigger CI. |
eivindj-nordic
requested review from
umapraseeda and
divipillai
and removed request for
a team
eivindj-nordic
force-pushed
the
25700_download_client_rework
branch
5 times, most recently
from
b39dec5
to
a25fb0d
Compare
peknis
requested changes
Nov 13, 2024
| :local: | ||
| :depth: 2 | ||
|
|
||
| The download client library can be used to download files from a server. |
There was a problem hiding this comment.
Suggested change
| The download client library can be used to download files from a server. | |
| You can use the Download client library to download files from a server. |
| Overview | ||
| ******** | ||
|
|
||
| The download is carried out in a separate thread, and the application receives events such as :c:enumerator:`DOWNLOADER_EVT_FRAGMENT` that contain the data fragments as the download progresses. |
There was a problem hiding this comment.
Suggested change
| The download is carried out in a separate thread, and the application receives events such as :c:enumerator:`DOWNLOADER_EVT_FRAGMENT` that contain the data fragments as the download progresses. | |
| The download is carried out in a separate thread and the application receives events such as :c:enumerator:`DOWNLOADER_EVT_FRAGMENT` that contain the data fragments as the download progresses. |
| Protocols | ||
| ========= | ||
|
|
||
| The library supports HTTP, HTTPS (TLS 1.2), CoAP, and CoAPS (DTLS 1.2) over IPv4 and IPv6. If other protocols are required they can be added by the application. See :c:file:`downloader_transport.h` for details. |
There was a problem hiding this comment.
Suggested change
| The library supports HTTP, HTTPS (TLS 1.2), CoAP, and CoAPS (DTLS 1.2) over IPv4 and IPv6. If other protocols are required they can be added by the application. See :c:file:`downloader_transport.h` for details. | |
| The library supports HTTP, HTTPS (TLS 1.2), CoAP, and CoAPS (DTLS 1.2) over IPv4 and IPv6. | |
| If other protocols are required they can be added by the application. | |
| See :c:file:`downloader_transport.h` for details. |
| ========= | ||
|
|
||
| The library supports HTTP, HTTPS (TLS 1.2), CoAP, and CoAPS (DTLS 1.2) over IPv4 and IPv6. If other protocols are required they can be added by the application. See :c:file:`downloader_transport.h` for details. | ||
| The protocol used for the download is specified by the start of the ``uri``. Use ``http://`` for HTTP, ``https://`` for HTTPS, ``coap://`` for COAP and ``coaps://`` for COAPS. |
There was a problem hiding this comment.
Suggested change
| The protocol used for the download is specified by the start of the ``uri``. Use ``http://`` for HTTP, ``https://`` for HTTPS, ``coap://`` for COAP and ``coaps://`` for COAPS. | |
| The protocol used for the download is specified in the beginning of the URI. | |
| Use ``http://`` for HTTP, ``https://`` for HTTPS, ``coap://`` for COAP and ``coaps://`` for COAPS. |
|
|
||
| The library supports HTTP, HTTPS (TLS 1.2), CoAP, and CoAPS (DTLS 1.2) over IPv4 and IPv6. If other protocols are required they can be added by the application. See :c:file:`downloader_transport.h` for details. | ||
| The protocol used for the download is specified by the start of the ``uri``. Use ``http://`` for HTTP, ``https://`` for HTTPS, ``coap://`` for COAP and ``coaps://`` for COAPS. | ||
| If no protocol is specified, the download client will default to HTTP or HTTPS, dependent on the server security configuration. |
There was a problem hiding this comment.
Suggested change
| If no protocol is specified, the download client will default to HTTP or HTTPS, dependent on the server security configuration. | |
| If no protocol is specified, the download client defaults to HTTP or HTTPS, depending on the server security configuration. |
| printk("Download client deinit failed, err %d\n", err); | ||
| } | ||
|
|
||
| This will free up the resources used by the download client. |
There was a problem hiding this comment.
Suggested change
| This will free up the resources used by the download client. | |
| This will free up the resources used by the library. |
|
|
||
| This will free up the resources used by the download client. | ||
|
|
||
| The following snippet shouws how to download a file using HTTPS: |
There was a problem hiding this comment.
Suggested change
| The following snippet shouws how to download a file using HTTPS: | |
| The following snippet shows how to download a file using HTTPS: |
include/net/downloader.h
Outdated
| * @c downloader_host_cfg.downloader_host_cfg is set, the downloader will stay | ||
| * connected to the server. If a new download is initiated from a different server, the | ||
| * current connection is closed and the downloader will connect to the new server. The | ||
| * connection can be closed by deinitilzing the downloader, which will also free the |
There was a problem hiding this comment.
Suggested change
| * connection can be closed by deinitilzing the downloader, which will also free the | |
| * connection can be closed by deinitializing the downloader, which will also free the |
include/net/downloader.h
Outdated
| * connection can be closed by deinitilzing the downloader, which will also free the | ||
| * downloaders resources. | ||
| * | ||
| * If the @c downloader_host_cfg.keep_connection flag is not set the downloader |
There was a problem hiding this comment.
Suggested change
| * If the @c downloader_host_cfg.keep_connection flag is not set the downloader | |
| * If the @c downloader_host_cfg.keep_connection flag is not set, the downloader |
include/net/downloader.h
Outdated
| * This initiates an asynchronous connect-download-disconnect sequence to the target | ||
| * host. | ||
| * | ||
| * Downloads are handled one at a time. If previous download is not finished |
There was a problem hiding this comment.
Suggested change
| * Downloads are handled one at a time. If previous download is not finished | |
| * Downloads are handled one at a time. If previous download is not finished, |
eivindj-nordic
force-pushed
the
25700_download_client_rework
branch
2 times, most recently
from
f31e3f6
to
bfb7c2a
Compare
|
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. |
eivindj-nordic
force-pushed
the
25700_download_client_rework
branch
12 times, most recently
from
649d1d7
to
73302f2
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]>
eivindj-nordic
force-pushed
the
25700_download_client_rework
branch
from
73302f2
to
898ce25
Compare
|
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 Publish GitHub Action. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Work in progress. No need for thorough review yet.
TODOs