sys/net/nanocoap: introduce nanocoap_sock_*(), use in suit/transport/coap#17474
Merged
benpicco merged 12 commits intoRIOT-OS:masterfrom Feb 27, 2022
Merged
sys/net/nanocoap: introduce nanocoap_sock_*(), use in suit/transport/coap#17474benpicco merged 12 commits intoRIOT-OS:masterfrom
nanocoap_sock_*(), use in suit/transport/coap#17474benpicco merged 12 commits intoRIOT-OS:masterfrom
Conversation
benpicco
requested review from
MichelRottleuthner,
PeterKietzmann,
aabadie,
cgundogan,
fjmolinas,
haukepetersen,
kaspar030,
leandrolanzieri,
miri64 and
smlng
as code owners
benpicco
added
the
Type: cleanup
github-actions
bot
added
Area: CoAP
benpicco
added
the
CI: ready for build
benpicco
changed the title
benpicco
force-pushed
the
nanocoap-suit
branch
from
0c643dd to
ad791ca
Compare
JKRhb
reviewed
Jan 10, 2022
Member
JKRhb
left a comment
JKRhb
left a comment
There was a problem hiding this comment.
Unfortunately, I have not been able to test this PR locally yet, but it looks good to me :) I just noticed a very minor typo (see below).
benpicco
removed
the
CI: ready for build
benpicco
force-pushed
the
nanocoap-suit
branch
from
2991a4a to
2c8bfb0
Compare
benpicco
added
CI: ready for build
benpicco
commented
Jan 10, 2022
|
|
||
| pkt.hdr = (coap_hdr_t*)buf; | ||
| pkt.hdr = buf; | ||
| pktpos += coap_build_hdr(pkt.hdr, COAP_TYPE_CON, NULL, 0, COAP_METHOD_GET, 1); |
Contributor
Author
There was a problem hiding this comment.
Any idea why id is always 1 here?
Member
There was a problem hiding this comment.
That is indeed wrong. The message ID is used to uniquely identify a specific message (for the same sender and received endpoint) for a given timespan, after which it can be reused. If you hardcode 1 here, you might end up reusing it before the timespan is over after which it is safe to reuse.
Member
There was a problem hiding this comment.
Does it make sense to open an issue and not also solve this in this PR?
fjmolinas
added
the
Process: API change
kaspar030
requested changes
Feb 22, 2022
Contributor
kaspar030
left a comment
kaspar030
left a comment
There was a problem hiding this comment.
We're introducing a new way of using nanocoap, where a socket gets created and can be re-used.
IMO it would make a lot of sense to group this functionality in nanocoap_sock_*(), and give it a distinct type to work on.
typedef struct {
//... (probably just a sock_udp in the beginning)
} nanocoap_sock_t;
nanocoap_sock_t coap_sock;
nanocoap_sock_create(&coap_sock, ...);
nanocoap_sock_get(&coap_sock, ...);
// ...
nanocoap_sock_close(&coap_sock, ...);
I know the current API is already a bit messy, but IMO grouping in nanocoap_sock_... would at least make clear that these new functions belong together.
I'm also looking into the future a bit here, if these functions work on their own type, I could see nanocoap_sock_t maybe be a tagged union that allows more transports than plain udp. API wise, we'd only have to change the create() function.
benpicco
force-pushed
the
nanocoap-suit
branch
from
0151b42 to
204df27
Compare
Contributor
|
@kaspar030 does it seem ok now? |
kaspar030
reviewed
Feb 24, 2022
Contributor
|
LGTM, apart from the missing |
Contributor
|
@chrysn do you still have comments here? |
Contributor
|
please squash! |
kaspar030
changed the title
nanocoap_sock_*(), use in suit/transport/coap
Co-authored-by: Jan Romann <[email protected]>
benpicco
force-pushed
the
nanocoap-suit
branch
from
d3702ae to
67b98fd
Compare
benpicco
added
the
CI: ready for build
kaspar030
approved these changes
Feb 24, 2022
Contributor
|
ACK. |
Contributor
|
Tested on nrf52: Tested also over the wireless link and it works as well... just takes longer... |
Contributor
Author
yea this is to be expected due to the different ways SUIT's |
Contributor
Yeah I know its not a complaint, it's always been much longer. |
Contributor
Author
|
@chrysn have your comments been addressed? |
chrysn
approved these changes
Feb 26, 2022
Member
|
I didn't do a full review, but my concerns have been addressed (so maybe I should have clicked "dismiss review", but neither is correct in my mental model; what I meant to indicate is that I have no objections to the current state, and with Kaspar's review this can go through.) |
Contributor
Author
|
Thank you for the review! |
Contributor
|
I have no idea why, but after bisecting I have this PR as the reason why examples/suit_update |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
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.
Contribution description
This refactors the nanocoap API to not open/close a socket for each request, to allow for socket re-use and align it more with what SUIT does.
As a result, we can remove the copy of
_nanocoap_request()from SUIT.The timeout handling differs a bit: In SUIT
CONFIG_COAP_ACK_TIMEOUTis used as a timeout for the whole request (for all retransmissions combined) whereas nanocoap uses it as the initial timeout for the first request and then doubles the timeout.I kept the nanocoap behavior, but changed the timeout to milliseconds so lower initial values can be used.
As a next step, this also moves
suit_coap_get_blockwise()tonanocoap_get_blockwise()so it can be used outside SUIT.Testing procedure
examples/suit_updatestill worksmake BOARD=same54-xpro test-with-config
First, run
Issues/PRs references