sys/net/nanocoap: Introduce coap_build_pkt_t and use it by benpicco · Pull Request #17544 · RIOT-OS/RIOT

benpicco · 2022-01-20T16:55:08Z

Contribution description

For requests NanoCOAP assembles a coap_pkt_t struct, however responses are built using a raw uint8_t * and size_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 use coap_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

convert GCOAP
update documentation

Testing procedure

examples/suit_update

suit_coap: trigger received
suit_coap: downloading "coap://[2001:db8::1]/fw/same54-xpro/suit_update-riot.suit_signed.latest.bin"
suit_coap: got manifest with size 495
suit: verifying manifest signature
suit: validated manifest version
)riotboot_hdr_validate: riotboot_hdr magic number invalid
Manifest seq_no: 1646321196, highest available: 1646321023
suit: validated sequence number
)Formatted component name: 
Comparing manifest offset 4000 with other slot offset
Comparing manifest offset 82000 with other slot offset
validating vendor ID
Comparing 547d0d74-6d3a-5a92-9662-4881afd9407b to 547d0d74-6d3a-5a92-9662-4881afd9407b from manifest
validating vendor ID: OK
validating class id
Comparing 50244518-6a7c-5ce7-932b-88b318336c82 to 50244518-6a7c-5ce7-932b-88b318336c82 from manifest
validating class id: OK
Comparing manifest offset 4000 with other slot offset
Comparing manifest offset 82000 with other slot offset
SUIT policy check OK.
Formatted component name: 
riotboot_flashwrite: initializing update to target slot 1
Fetching firmware |█████████████████████████| 100%
Finalizing payload store
Verifying image digest
Starting digest verification against image
Install correct payload
Verifying image digest
Starting digest verification against image
Install correct payload
Image magic_number: 0x544f4952
Image Version: 0x6220de2c
Image start address: 0x00082400
Header chksum: 0x449701f7

suit_coap: rebooting...
----> ethos: hello received
Failed to send flush request: Operation not permitted
gnrc_uhcpc: Using 4 as border interface and 0 as wireless interface.
gnrc_uhcpc: only one interface found, skipping setup.
main(): This is RIOT! (Version: 2022.04-devel-616-g1a182-nanocoap-coap_rsp_pkt_t)
RIOT SUIT update example application
Running from slot 1
Image magic_number: 0x544f4952
Image Version: 0x6220de2c
Image start address: 0x00082400
Header chksum: 0x449701f7

suit_coap: started.
Starting the shell
>

tests/riotboot_flashwrite

/home/benpicco/dev/RIOT/dist/tools/ethos/ethos riot0 /dev/ttyACM0 
----> ethos: sending hello.
----> ethos: activating serial pass through.
----> ethos: hello reply received
----> ethos: hello reply received


riotboot-hdr

> 
> 
> 
> riotboot-hdr
Image magic_number: 0x544f4952
Image Version: 0x6220dbfb


Image start address: 0x00004400
Header chksum: 0x7bcb1fbe

> current-slot

> 
> current-slot
Running from slot 0
> ifconfig
ifconfig
Iface  4  HWaddr: 66:9B:87:4C:41:52 
          L2-PDU:1500  MTU:1500  HL:64  Source address length: 6
          Link type: wired
          inet6 addr: fe80::649b:87ff:fe4c:4152  scope: link  VAL
pinging node...
PING fe80::649b:87ff:fe4c:4152%riot0(fe80::649b:87ff:fe4c:4152%riot0) 56 data bytes

--- fe80::649b:87ff:fe4c:4152%riot0 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 37.085/37.085/37.085/0.000 ms
pinging node succeeded.
compiling /home/benpicco/dev/RIOT/dist/tools/riotboot_gen_hdr/bin/genhdr...
make: Nothing to be done for 'all'.
creating /home/benpicco/dev/RIOT/tests/riotboot_flashwrite/bin/same54-xpro/tests_riotboot_flashwrite-slot0.1646320636.riot.bin...
creating /home/benpicco/dev/RIOT/tests/riotboot_flashwrite/bin/same54-xpro/tests_riotboot_flashwrite-slot1.1646320636.riot.bin...
          inet6 addr: fe80::2  scope: link  VAL
          inet6 group: ff02::1
          inet6 group: ff02::1:ff4c:4152
          inet6 group: ff02::1:ff00:2
          
> _flashwrite_handler(): received data: offset=0 len=64 blockwise=1 more=1
_flashwrite_handler(): init len=64
riotboot_flashwrite: initializing update to target slot 1
_flashwrite_handler(): received data: offset=64 len=64 blockwise=1 more=1
_flashwrite_handler(): received data: offset=128 len=64 blockwise=1 more=1
[…]
_flashwrite_handler(): received data: offset=69952 len=64 blockwise=1 more=1
_flashwrite_handler(): received data: offset=70016 len=24 blockwise=1 more=0
_flashwrite_handler(): finish
WARNING:coap.blockwise-requester:Block1 option completely ignored by server, assuming it knows what it is doing.
reboot
reboot
----> ethos: hello received
Failed to send flush request: Operation not permitted
NETOPT_RX_END_IRQ not implemented by driver
gnrc_uhcpc: Using 4 as border interface and 0 as wireless interface.
gnrc_uhcpc: only one interface found, skipping setup.
main(): This is RIOT! (Version: 2022.04-devel-616-gc798e-nanocoap-coap_rsp_pkt_t)
riotboot_flashwrite test application
Running from slot 1
Image magic_number: 0x544f4952
Image Version: 0x6220dbfc
Image start address: 0x00082400
Header chksum: 0x3bd7ffc6

Starting the shell


> current-slot

> 
> current-slot
Running from slot 1
> ifconfig
ifconfig
Iface  4  HWaddr: 66:9B:87:4C:41:52 
          L2-PDU:1500  MTU:1500  HL:64  Source address length: 6
          Link type: wired
          inet6 addr: fe80::649b:87ff:fe4c:4152  scope: link  VAL
pinging node...
PING fe80::649b:87ff:fe4c:4152%riot0(fe80::649b:87ff:fe4c:4152%riot0) 56 data bytes

--- fe80::649b:87ff:fe4c:4152%riot0 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 38.856/38.856/38.856/0.000 ms
pinging node succeeded.
compiling /home/benpicco/dev/RIOT/dist/tools/riotboot_gen_hdr/bin/genhdr...
make: Nothing to be done for 'all'.
creating /home/benpicco/dev/RIOT/tests/riotboot_flashwrite/bin/same54-xpro/tests_riotboot_flashwrite-slot0.1646320637.riot.bin...
creating /home/benpicco/dev/RIOT/tests/riotboot_flashwrite/bin/same54-xpro/tests_riotboot_flashwrite-slot1.1646320637.riot.bin...
          inet6 addr: fe80::2  scope: link  VAL
          inet6 group: ff02::1
          inet6 group: ff02::1:ff4c:4152
          inet6 group: ff02::1:ff00:2
          
> _flashwrite_handler(): received data: offset=0 len=64 blockwise=1 more=1
_flashwrite_handler(): init len=64
riotboot_flashwrite: initializing update to target slot 0
_flashwrite_handler(): received data: offset=64 len=64 blockwise=1 more=1
_flashwrite_handler(): received data: offset=128 len=64 blockwise=1 more=1
[…]
_flashwrite_handler(): received data: offset=69952 len=64 blockwise=1 more=1
_flashwrite_handler(): received data: offset=70016 len=24 blockwise=1 more=0
_flashwrite_handler(): finish
WARNING:coap.blockwise-requester:Block1 option completely ignored by server, assuming it knows what it is doing.
reboot
reboot
----> ethos: hello received
Failed to send flush request: Operation not permitted
NETOPT_RX_END_IRQ not implemented by driver
gnrc_uhcpc: Using 4 as border interface and 0 as wireless interface.
gnrc_uhcpc: only one interface found, skipping setup.
main(): This is RIOT! (Version: 2022.04-devel-616-gc798e-nanocoap-coap_rsp_pkt_t)
riotboot_flashwrite test application
Running from slot 0
Image magic_number: 0x544f4952
Image Version: 0x6220dbfd
Image start address: 0x00004400
Header chksum: 0x7bd31fc0

Starting the shell


> current-slot

> 
> current-slot
Running from slot 0
> ifconfig
ifconfig
Iface  4  HWaddr: 66:9B:87:4C:41:52 
          L2-PDU:1500  MTU:1500  HL:64  Source address length: 6
          Link type: wired
          inet6 addr: fe80::649b:87ff:fe4c:4152  scope: link  VAL
pinging node...
PING fe80::649b:87ff:fe4c:4152%riot0(fe80::649b:87ff:fe4c:4152%riot0) 56 data bytes

--- fe80::649b:87ff:fe4c:4152%riot0 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 39.409/39.409/39.409/0.000 ms
pinging node succeeded.
riotboot-invalidate 0
reboot
          inet6 addr: fe80::2  scope: link  VAL
          inet6 group: ff02::1
          inet6 group: ff02::1:ff4c:4152
          inet6 group: ff02::1:ff00:2
          
> riotboot-invalidate 0
> reboot
----> ethos: hello received
Failed to send flush request: Operation not permitted
NETOPT_RX_END_IRQ not implemented by driver
gnrc_uhcpc: Using 4 as border interface and 0 as wireless interface.
gnrc_uhcpc: only one interface found, skipping setup.
main(): This is RIOT! (Version: 2022.04-devel-616-gc798e-nanocoap-coap_rsp_pkt_t)
riotboot_flashwrite test application
Running from slot 1
Image magic_number: 0x544f4952
Image Version: 0x6220dbfc
Image start address: 0x00082400
Header chksum: 0x3bd7ffc6

Starting the shell


> current-slot

> 
> current-slot
Running from slot 1
TEST PASSED

examples/gcoap <-> examples/nanocoap_server

> coap get fe80::9483:27ff:feaf:5d66 5683 /riot/ver
coap get fe80::9483:27ff:feaf:5d66 5683 /riot/ver
gcoap_cli: sending msg ID 20789, 15 bytes
--- blockwise start ---
gcoap: response Success, code 2.05, 16 bytes
This is RIOT (Ve
gcoap: response Success, code 2.05, 16 bytes
rsion: 2022.04-d
gcoap: response Success, code 2.05, 16 bytes
evel-74-g23b93-n
gcoap: response Success, code 2.05, 16 bytes
anocoap-coap_rsp
gcoap: response Success, code 2.05, 16 bytes
_pkt_t) running 
gcoap: response Success, code 2.05, 16 bytes
on a native boar
gcoap: response Success, code 2.05, 16 bytes
d with a native 
gcoap: response Success, code 2.05, 4 bytes
MCU.
--- blockwise complete ---

Issues/PRs references

maribu

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.

maribu · 2022-01-31T13:46:53Z

examples/nanocoap_server/coap_handler.c

-    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);


This clearly is an improvement in regard to usability :)

And it can be used to avoid having to copy the payload into a separate buffer #17559

benpicco · 2022-02-21T08:14:30Z

I'd rather have #17474 in first to avoid a merge conflict there

benpicco · 2022-03-03T16:53:36Z

Now this is ready for review.

kfessel · 2022-03-14T12:44:15Z

sys/include/net/gcoap.h

 *
 * gcoap allows an application to specify a collection of request resource paths
- * 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


could you move that coap_resource_t -> gcoap_resource_t to a seperate PR ?
(i seems to already be a separate commit)

the same might be done for the link_encoder

(i like these changes as they clearup where a structure is defined)

#17804 there you go

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?

wouldn't this PR trigger the same issue (if we restart murdock now) until rust is upgraded

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.

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.

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').

#17801 implements caching for gcoap and somehow also for nanocoap (using the old buf an len but introducing a nanocoap_-prefix)

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.

kaspar030 · 2022-03-22T10:15:30Z

We had some discussion during Sprint Meeting today.
This PR's direction is nice, but as is it would effectively split nanocoap and gcoap, and go against the plain laid out here.
The main reason to do so was because changing this in nanocoap and give gcoap its own type needs only minimal changes to gcoap...

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.

kfessel · 2022-03-30T10:46:16Z

I some how think it would be better to do the respond buffer management in coap_pkt_t - many of the respond writer functions already need and modify coap_pkt_t and this buffer information isn't much extra bytes.

benpicco · 2022-05-18T22:39:23Z

I some how think it would be better to do the respond buffer management in coap_pkt_t - many of the respond writer functions already need and modify coap_pkt_t and this buffer information isn't much extra bytes.

#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.

benpicco requested a review from bergzand January 20, 2022 16:55

benpicco added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Jan 20, 2022

github-actions bot added Area: CoAP Area: Constrained Application Protocol implementations Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework labels Jan 20, 2022

benpicco requested review from haukepetersen, kaspar030 and kb2ma January 20, 2022 17:23

benpicco force-pushed the nanocoap-coap_rsp_pkt_t branch from b5dbc21 to f232e81 Compare January 21, 2022 21:15

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 21, 2022

benpicco force-pushed the nanocoap-coap_rsp_pkt_t branch 2 times, most recently from 6bf20be to 6e0db69 Compare January 24, 2022 14:35

benpicco marked this pull request as ready for review January 24, 2022 15:08

benpicco requested review from MichelRottleuthner, PeterKietzmann, aabadie, cgundogan, fjmolinas, leandrolanzieri, miri64 and smlng as code owners January 24, 2022 15:08

benpicco requested review from chrysn and maribu January 24, 2022 15:08

benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 24, 2022

benpicco mentioned this pull request Jan 24, 2022

sys/net/nanocoap: allow for zero-copy payload response #17559

Closed

benpicco force-pushed the nanocoap-coap_rsp_pkt_t branch from da38ef0 to 23b93b1 Compare January 31, 2022 12:34

benpicco changed the title ~~sys/net/nanocoap: Introduce coap_rsp_pkt_t and use it~~ sys/net/nanocoap: Introduce coap_build_pkt_t and use it Jan 31, 2022

maribu reviewed Jan 31, 2022

View reviewed changes

sys/net/nanocoap: Introduce coap_build_pkt_t and use it

4b71cd2

benpicco force-pushed the nanocoap-coap_rsp_pkt_t branch from 23b93b1 to 21bd79a Compare March 3, 2022 15:34

github-actions bot added the Area: OTA Area: Over-the-air updates label Mar 3, 2022

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 3, 2022

benpicco force-pushed the nanocoap-coap_rsp_pkt_t branch 2 times, most recently from 6e89dd6 to 6104612 Compare March 8, 2022 11:03

chrysn mentioned this pull request Mar 8, 2022

{examples,tests}/rust: Rust updates (-sys/-wrappers version, stable) #17761

Merged

benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 8, 2022

benpicco added 2 commits March 8, 2022 22:21

net/gcoap: introduce and use gcoap_resource_t

dc7df25

sys/net/nanocoap: use coap_build_pkt_t in coap_blockwise_put_bytes()

e2ab8a1

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 8, 2022

benpicco force-pushed the nanocoap-coap_rsp_pkt_t branch from 6104612 to e2ab8a1 Compare March 8, 2022 21:21

benpicco added this to the Release 2022.04 milestone Mar 10, 2022

benpicco requested a review from kfessel March 11, 2022 11:59

kfessel reviewed Mar 14, 2022

View reviewed changes

benpicco mentioned this pull request Mar 14, 2022

net/gcoap: introduce and use gcoap_resource_t #17804

Closed

benpicco mentioned this pull request Mar 22, 2022

examples/gcoap_block_server: add gcoap block server example #17843

Merged

benpicco mentioned this pull request Apr 2, 2022

sys/net/nanocoap: refactor to remove work-buffer requirement #17509

Merged

benpicco mentioned this pull request Apr 16, 2022

nanocoap_sock: add nanocoap_sock_block_request() #17958

Merged

benpicco removed this from the Release 2022.04 milestone May 2, 2022

benpicco closed this May 18, 2022

chrysn added a commit to RIOT-OS/rust-riot-wrappers that referenced this pull request Jun 8, 2022

marker: Remove gcoap_resource_t

4f05786

The underlying PR RIOT-OS/RIOT#17544 was abandoned.

chrysn added a commit to RIOT-OS/rust-riot-wrappers that referenced this pull request Jun 9, 2022

marker: Remove gcoap_resource_t

babe080

The underlying PR RIOT-OS/RIOT#17544 was abandoned.

benpicco mentioned this pull request Jul 23, 2024

Overhauled CoAP Stack and API #20792

Open

25 tasks

Conversation

benpicco commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

TODO

Testing procedure

Issues/PRs references

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benpicco commented Feb 21, 2022

Uh oh!

benpicco commented Mar 3, 2022

Uh oh!

kfessel Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benpicco Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Mar 22, 2022

Uh oh!

kfessel commented Mar 30, 2022

Uh oh!

benpicco commented May 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benpicco commented Jan 20, 2022 •

edited

Loading

kfessel Mar 14, 2022 •

edited

Loading

kfessel Mar 17, 2022 •

edited

Loading

benpicco Mar 21, 2022 •

edited

Loading