nrfcloud coap: use downloader library #19908
Open
+144
−492
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
the
changelog-entry-required
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 41264acb5915f862fe18edb02e3adde1adf2571f more detailssdk-nrf:
Github labels
List of changed files detected by CI (19)Outputs:ToolchainVersion: 342151af73 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
|
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
force-pushed
the
downloader-nrfcloud-coap
branch
2 times, most recently
from
ff29a72 to
48a0dae
Compare
|
note: |
maxd-nordic
force-pushed
the
downloader-nrfcloud-coap
branch
2 times, most recently
from
575a09a to
c6ee5da
Compare
maxd-nordic
changed the title
maxd-nordic
force-pushed
the
downloader-nrfcloud-coap
branch
from
c6ee5da to
2b2c4d8
Compare
maxd-nordic
force-pushed
the
downloader-nrfcloud-coap
branch
from
2b2c4d8 to
f9b7ac7
Compare
plskeggs
approved these changes
Jan 17, 2025
| } | ||
|
|
||
| /* we are already connected using the given socket */ | ||
| client->sock = socket; |
There was a problem hiding this comment.
maybe lock around these two lines
jtguggedal
reviewed
Jan 20, 2025
| * Callback to do client authentication. | ||
| * This is called after connecting. | ||
| */ | ||
| int (*auth_cb)(int sock); |
There was a problem hiding this comment.
The callback returns an int, but it does not seem to be checked by the code.
I suggest this is either made void or that it's documented what the auth_cb should return and that the return is handled by the code.
| return err; | ||
| } | ||
| LOG_DBG("dl->file: %s", dl->file); | ||
| strncpy(file, dl->file, sizeof(file)); |
There was a problem hiding this comment.
Suggested change
| strncpy(file, dl->file, sizeof(file)); | |
| strncpy(file, dl->file, sizeof(file) - 1); | |
| file[FILENAME_SIZE - 1] = '\0\; |
To ensure null-termination. It might be safe in this case to not do this, but then I think it warrants a comment just to make it obvious that it has been considered as safe
| @@ -448,6 +465,11 @@ static int dl_coap_connect(struct downloader *dl) | |||
|
|
|||
| coap->new_data_req = true; | |||
|
|
|||
| /* run auth callback if set */ | |||
| if (coap->auth_cb != NULL) { | |||
| coap->auth_cb(coap->sock.fd); | |||
There was a problem hiding this comment.
Suggested change
| coap->auth_cb(coap->sock.fd); | |
| err = coap->auth_cb(coap->sock.fd); |
Feels like this should be returned or at least checked + logged
maxd-nordic
force-pushed
the
downloader-nrfcloud-coap
branch
from
f9b7ac7 to
44bc70a
Compare
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]>
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]>
maxd-nordic
force-pushed
the
downloader-nrfcloud-coap
branch
from
44bc70a to
41264ac
Compare
jtguggedal
approved these changes
Jan 20, 2025
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.
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.