-
Notifications
You must be signed in to change notification settings - Fork 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
sys/net/nanocoap: Introduce coap_build_pkt_t and use it #17544
Conversation
b5dbc21
to
f232e81
Compare
6bf20be
to
6e0db69
Compare
da38ef0
to
23b93b1
Compare
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.
API-wise this clearly is an improvement. I haven't used gcoap / nanocoap that much and I'm not that familiar with the roadmap, but code-wise this PR is fine.
bufpos += coap_blockwise_put_bytes(&slicer, bufpos, (uint8_t*)RIOT_BOARD, strlen(RIOT_BOARD)); | ||
bufpos += coap_blockwise_put_bytes(&slicer, bufpos, block2_mcu, sizeof(block2_mcu)-1); | ||
bufpos += coap_blockwise_put_bytes(&slicer, bufpos, (uint8_t*)RIOT_MCU, strlen(RIOT_MCU)); | ||
coap_blockwise_put_bytes(&slicer, response, block2_intro, sizeof(block2_intro)-1); |
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.
This clearly is an improvement in regard to usability :)
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.
And it can be used to avoid having to copy the payload into a separate buffer #17559
23b93b1
to
21bd79a
Compare
Now this is ready for review. |
6e89dd6
to
6104612
Compare
6104612
to
e2ab8a1
Compare
* it wants to be notified about. Create an array of resources (coap_resource_t | ||
* it wants to be notified about. Create an array of resources (gcoap_resource_t |
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.
could you move that coap_resource_t
-> gcoap_resource_t
to a seperate PR ?
(i seems to already be a separate commit)
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.
the same might be done for the link_encoder
(i like these changes as they clearup where a structure is defined)
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.
#17804 there you go
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.
Unfortunately this causes some issues with the Rust wrappers which in turn require a CI upgrade :(
So if you don't insist on splitting this, could we move forward with this as a single PR?
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.
wouldn't this PR trigger the same issue (if we restart murdock now) until rust is upgraded
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.
gcoap.h:210-308
explains how to do block transfers with gcoap:
the nanocoap functions are used i think they are missing the initialization for the respose structure after this PR is mergerd.
This section also provides a link to an example.
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.
wouldn't this PR trigger the same issue (if we restart murdock now) until rust is upgraded
no, the rust wrapper uses the presence of coap_build_pkt_t
to detect whether to use the old or the new API.
It has now been fixed to check for gcoap_resource_t
instead.
the nanocoap functions are used i think they are missing the initialization for the respose structure after this PR is mergerd.
Maybe I miss something, but those are not using any of the functions that makes use of coap_build_pkt_t
?
The new type is not used by gcoap (that's why I introduced gcoap_resource_t
) - it should be, but I was hoping this could be done as a follow-up.
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.
gcoap is using the nanocoap functions to generate the header coap_pkgs, while most of these generator functions do not make any use of buf and len
/coap_build_pkt_t
, block transfer does -> with this pr gcoap would loose nanocoaps block-transfer abilities (which is described in gcoaps documentation (gcoap.h:210-308) (also featuring an external example).
I am no sure about the state of #17168 and #16855 especial in looking at block-transfers. May be this API change is also welcome to gcoap.
While this PR tries to break just nanocoaps-API it implicitly also breaks gcoaps-API (@block-transfers).
maybe @chrysn or @miri64 have some insight into what changes are going to happen in gcoap maybe this API-change is welcome and should just be forwarded to gcoap ( which might be easier since it may not need the introduction of 'gcoap_rescource_t').
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.
#17801 implements caching for gcoap and somehow also for nanocoap (using the old buf an len but introducing a nanocoap_-prefix)
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.
while most of these generator functions do not make any use of
buf and len
/coap_build_pkt_t
, block transfer does -> with this pr gcoap would loose nanocoaps block-transfer abilities (which is described in gcoaps documentation (gcoap.h:210-308)
Please explain to me which function(s) you mean. If Gcoap would use on of the converted nanocoap functions, that would be a compilation error as Gcoap does not yet make use of coap_build_pkt_t
.
There are functions in nanocoap.c
that are exclusively used by Gcoap, but as far as I can tell those are not touched by this PR.
Blockwise GET is still working in examples/gcoap
, not sure if we also have a server example for Gcoap, but I expect no surprises there.
We had some discussion during Sprint Meeting today. Anyhow, we concluded that we need more gcoap users in-tree that test corner cases, so gcoap changes can be tested more easily. Like, this gcoap blockwise server from ken: link. |
I some how think it would be better to do the respond buffer management in |
#17958 did this, so I think this PR is now obsolete. |
This makes riot-wrappers usable across RIOT-OS/RIOT#17544 Merges: https://gitlab.com/etonomy/riot-wrappers/-/merge_requests/8
The underlying PR RIOT-OS/RIOT#17544 was abandoned.
The underlying PR RIOT-OS/RIOT#17544 was abandoned.
Contribution description
For requests NanoCOAP assembles a
coap_pkt_t
struct, however responses are built using a rawuint8_t *
andsize_t
pair.This makes it very hard to extend functionality.
For more flexibility, this introduces a new
coap_build_pkt_t
struct that is used to assemble a COAP response or request.I did not use
coap_pkt_t
since that assumes an already parsed packet.The goal is to have a separate payload pointer so that together with #17485 we do not have to copy the payload into the COAP response buffer but can just pass a pointer to it.
This is relevant with large COAP blocks (e.g 1k) via Ethernet or IEEE 802.15.4g.
But this is for a future PR, this PR only converts the
uint8_t *rsp, size_t rsp_len
functions to usecoap_pkt_t *response
instead. Some code was simplified as we no longer need to pass the packet length as return value but can always determine it from the new struct.TODO
Testing procedure
examples/suit_update
tests/riotboot_flashwrite
examples/gcoap <-> examples/nanocoap_server
Issues/PRs references