Skip to content
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/*coap: Make APIs (more) transport agnostic #20900

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

maribu
Copy link
Member

@maribu maribu commented Oct 9, 2024

Contribution description

This changes the API of nanocoap with the goal to reduce the expose of UDP specifics in the API. The plan is to eventually support transports such as CoAP over TCP and CoAP over WebSocket directly in nanocoap while sharing most of the code, as e.g. the CoAP Option processing remains identical. Specifically, the plan is to unlock a transport with modules and introduce overhead for dispatching to specific transport only when multiple transports are actually in use.

Support for OSCORE directly in nanocoap is probably not sensible, as the serialization is very much unlike the other transports. A unified CoAP API for multiple transports including OSCORE is probably best implemented on top. But when limited to the boring set of CoAP transports, we probably can support them well with nanocoap with less overhead.

API Changes:

Breaking API Changes:

  • coap_parse() now returns ssize_t instead of int
    • This function is not really user facing, so the impact should be limited
    • This is useful for stream transports where the buffer may contain data of more than one packet. The return value contains the number of bytes actually consumed, which will match the buffer size for non-stream transports.

API Changes:

  • coap_pkt_t now contains a uint8_t *buf pointer instead of a coap_hdr_t *hdr pointer to the beginning of the buffer
    • This will also work when the buffer is used by non-UDP transports
    • A deprecated coap_udp_hdr_t *hdr has been crammed into an unnamed union with uint8_t *buf. For architectures where pointers have the same memory layout regardless of type (e.g. all of the supported ones), this will make hdr an alias for buf.
    • The alias will only be provided if no transport besides UDP is used in nanocoap. So existing apps will continue to work, new apps that want to support other transports need to move to adapt.
  • coap_hdr_t has been renamed to coap_udp_hdr_t
    • A deprecated alias was created for deprecation
  • coap_hdr*() functions have been deprecated
    • Equivalent coap_pkt*() functions have been created that work on coap_pkt_t * instead of coap_hdr_t *
    • If non-UDP transports are used, the deprecated coap_hdr*() will probably not be exposed to avoid footguns.
  • coap_build_hdr() has been renamed to coap_build_udp_hdr() and that works on an uint8_t * buffer with a given length, rather than on a coap_hdr_t * with a figers crossed length
    • a deprecated coap_build_hdr() function was added that calls to coap_build_udp_hdr() and has the same signature, so that users have time to update

Internal users have been updated, except for tests.

The deprecated functions will probably exposed only when nanocoap is used with only UDP as transport, as those will wreak havoc on non-UDP CoAP packets. That should be fine, as existing apps hardly will make use of other transports anyway.

Testing procedure

Existing apps and tests should work as before. I will do some limited testing soon and do more rigorous testing once an implementation of nanocoap using TCP as transport is ready. Otherwise there are likely still UDP specifics in the API that I have overlooked.

Issues/PRs references

See also #20792

@github-actions github-actions bot added Area: network Area: Networking Area: tests Area: tests and testing framework Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System Area: examples Area: Example Applications labels Oct 9, 2024
@maribu
Copy link
Member Author

maribu commented Oct 9, 2024

This PR is not ready for review yet, I just want to make the work more transparent.

@maribu maribu marked this pull request as draft October 9, 2024 11:45
@benpicco benpicco requested review from chrysn and benpicco October 9, 2024 13:33
@mguetschow
Copy link
Contributor

Cross-referencing @carl-tud's ongoing work with a superset of the goals of this one: #20792

@maribu maribu force-pushed the sys/net/nanocoap/transport-agnostic-api branch 4 times, most recently from 99877b3 to d832957 Compare October 14, 2024 11:12
@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 14, 2024
@riot-ci
Copy link

riot-ci commented Oct 14, 2024

Murdock results

✔️ PASSED

d82eecf examples/gcoap: upgrade users of deprecated nanocoap APIs

Success Failures Total Runtime
10249 0 10249 19m:18s

Artifacts

@maribu maribu force-pushed the sys/net/nanocoap/transport-agnostic-api branch 2 times, most recently from c13f6d0 to 34b0d34 Compare October 14, 2024 11:41
@chrysn
Copy link
Member

chrysn commented Oct 14, 2024

Could the .hdr pointer stay there unused and deprecated if none of the non-UDP gcoap modules are enabled?

Then users who just upgrade their applications would see a deprecation rather than breakage, and would just need to follow that deprecation's pointers ("use the setters and getters") before either they go to the next release or they enable any of the new modules. The Rust wrappers (affected in RIOT-OS/rust-riot-wrappers#129) could then do the same: use the old ways unless a new module is enabled, and after the next release they can go all-in with the new accessor, and thus always work with the current release and with the mai^Wmaster branch.

@chrysn
Copy link
Member

chrysn commented Oct 14, 2024

To clarify: If it's only the Rust wrappers affected, workarounds are possible (but necessary) to make things work there by detecting whether any code getter/setter is present. But if making it easier for riot-wrappers also eases other users' transitions, maybe we should do that. I can work with either outcome.

@maribu
Copy link
Member Author

maribu commented Oct 14, 2024

Could the .hdr pointer stay there unused and deprecated if none of the non-UDP gcoap modules are enabled?

I think adding an unnamed union containing .hdr and .buf if - and only if - only UDP is used would be a compromise. Having a .hdr when there is (possibly) no UDP header present is quite a footgun, as .hdr is tied towards a UDP header. We should also not have the union there when the CI is run, so that users of coap_pkt_t::hdr don't sneak back in.

@chrysn
Copy link
Member

chrysn commented Oct 14, 2024

Yes, an unnamed union .buf ∪ .hdr that only has the old item if that is legal (ie. UDP is the only transport) would be good.

CI should probably not interfere here because that'd also impede the Rust side -- but after the next release, the old item can go away anyway, and then we're enforcing the deprecation. Until then I think we'll have to rely on people following the deprecation.

@maribu maribu force-pushed the sys/net/nanocoap/transport-agnostic-api branch 2 times, most recently from 6cc1b64 to 061ca01 Compare October 15, 2024 19:45
@maribu maribu force-pushed the sys/net/nanocoap/transport-agnostic-api branch from 7720e62 to 84cbedc Compare October 18, 2024 20:05
@maribu
Copy link
Member Author

maribu commented Oct 18, 2024

Rebased on master. If I still fine issues with the API when implementing CoAP over TCP as proof of concept, I may still adapt this PR. Once confirmed that this is a sane base for adding other transports, I'll mark this PR as ready.

I think the PR will now be more stable and reviewers could take an early look now.

@maribu maribu force-pushed the sys/net/nanocoap/transport-agnostic-api branch from c111611 to 142dffe Compare November 1, 2024 21:28
@maribu
Copy link
Member Author

maribu commented Nov 1, 2024

I changed int coap_parse() to ssize_t coap_parse(). This is super useful for stream based transports, as the buffer passed to coap_parse() may contain more data than a single CoAP packet, but we need to only consume the parsed packet from the buffer when calling coap_parse() in a loop.

This is a breaking change, but not a user facing one: Apps will implement resource handlers, not write their own server loop.

I also rebased this on top of #20945, which is not yet upstream.

@maribu maribu force-pushed the sys/net/nanocoap/transport-agnostic-api branch 2 times, most recently from d331f90 to 9a17e4b Compare November 4, 2024 19:23
@maribu maribu force-pushed the sys/net/nanocoap/transport-agnostic-api branch 3 times, most recently from 73c2c8e to c51264d Compare November 11, 2024 07:37
@maribu maribu force-pushed the sys/net/nanocoap/transport-agnostic-api branch from c51264d to 7a3f62e Compare November 18, 2024 14:15
@maribu maribu removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 18, 2024
@maribu maribu force-pushed the sys/net/nanocoap/transport-agnostic-api branch from 7a3f62e to d68e00e Compare November 19, 2024 20:28
@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 26, 2024
@maribu maribu marked this pull request as ready for review November 27, 2024 09:44
@maribu maribu changed the title [WIP] sys/net/*coap: Make APIs (more) transport agnostic sys/net/*coap: Make APIs (more) transport agnostic Nov 27, 2024
@maribu maribu force-pushed the sys/net/nanocoap/transport-agnostic-api branch from d68e00e to a73f365 Compare December 13, 2024 08:54
@maribu
Copy link
Member Author

maribu commented Dec 13, 2024

Another rebase to resolve merge conflicts.

@maribu maribu added the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 13, 2024
@maribu
Copy link
Member Author

maribu commented Dec 13, 2024

We had a discussion out of band and concluded that there is a risk that !20900 changes APIs that end up being deprecated when UniCoAP lands.

We agreed to put PR on hold for now, so that we don’t end up forcing users to jump through two API deprecation hoops when this could be done with just one.

@maribu maribu force-pushed the sys/net/nanocoap/transport-agnostic-api branch from a73f365 to 3e6c331 Compare December 13, 2024 17:34

Verified

This commit was signed with the committer’s verified signature.
maribu Marian Buschsieweke
This changes the API of nanocoap with the goal to reduce the expose of
UDP specifics in the API. The plan is to eventually support transports
such as CoAP over TCP and CoAP over WebSocket directly in nanocoap
while sharing most of the code, as e.g. the CoAP Option processing
remains identical. Specifically, the plan is to unlock a transport with
modules and introduce overhead for dispatching to specific transport
only when multiple transports are actually in use.

Support for OSCORE directly in nanocoap is probably not sensible, as
the serialization is very much unlike the other transports. A unified
CoAP API for multiple transports including OSCORE is probably best
implemented on top. But when limited to the boring set of CoAP
transports, we probably can support them well with nanocoap with less
overhead.

Breaking API Changes:
=====================

- `coap_parse()` now returns `ssize_t` instead of `int`
    - This function is not really user facing, so the impact should
      be limited
    - This is useful for stream transports where the buffer may
      contain data of more than one packet. The return value contains
      the number of bytes actually consumed, which will match the
      buffer size for non-stream transports.

API Changes:
============

- `coap_pkt_t` now contains a `uint8_t *buf` pointer instead of a
  `coap_hdr_t *hdr` pointer to the beginning of the buffer
    - This will also work when the buffer is used by non-UDP
      transports
    - A deprecated `coap_udp_hdr_t *hdr` has been crammed into
      an unnamed `union` with `uint8_t *buf`. For architectures
      where pointers have the same memory layout regardless of type
      (e.g. all of the supported ones), this will make `hdr` an
      alias for `buf`.
    - The alias will only be provided if no transport besides UDP is
      used in nanocoap. So existing apps will continue to work, new
      apps that want to support other transports need to move to
      adapt.
- `coap_hdr_t` has been renamed to `coap_udp_hdr_t`
    - A deprecated alias was created for deprecation
- `coap_hdr*()` functions have been deprecated
    - Equivalent `coap_pkt*()` functions have been created that work
      on `coap_pkt_t *` instead of `coap_hdr_t *`
    - If non-UDP transports are used, the deprecated `coap_hdr*()`
      will probably not be exposed to avoid footguns.
- `coap_build_hdr()` has been renamed to `coap_build_udp_hdr()` and
  that works on an `uint8_t *` buffer with a given length, rather than
  on a `coap_hdr_t *` with a *figers crossed* length
    - a deprecated `coap_build_hdr()` function was added that calls
      to `coap_build_udp_hdr()` and has the same signature, so that
      users have time to update

Verified

This commit was signed with the committer’s verified signature.
maribu Marian Buschsieweke

Verified

This commit was signed with the committer’s verified signature.
maribu Marian Buschsieweke

Verified

This commit was signed with the committer’s verified signature.
maribu Marian Buschsieweke

Verified

This commit was signed with the committer’s verified signature.
maribu Marian Buschsieweke

Verified

This commit was signed with the committer’s verified signature.
maribu Marian Buschsieweke

Verified

This commit was signed with the committer’s verified signature.
maribu Marian Buschsieweke
@maribu maribu force-pushed the sys/net/nanocoap/transport-agnostic-api branch from 3e6c331 to d82eecf Compare December 18, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: waiting for other PR State: The PR requires another PR to be merged first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants