Skip to content

gcoap: wrap nanocoap access#17168

Closed
miri64 wants to merge 5 commits intoRIOT-OS:masterfrom
miri64:gcoap/enh/nanocoap-wrappers
Closed

gcoap: wrap nanocoap access#17168
miri64 wants to merge 5 commits intoRIOT-OS:masterfrom
miri64:gcoap/enh/nanocoap-wrappers

Conversation

@miri64
Copy link
Copy Markdown
Member

@miri64 miri64 commented Nov 9, 2021

Contribution description

This PR aims to address the first three tasks in #16855.

This is still WIP, but before I start changing types and documentation, I wanted to have some input.

I followed the following strategies for the wrappers:

  • Most of the time the wrapping is straightforward (just call the respective nanocoap function in an inline function)
  • due to API inconsistencies in nanocoap sometimes casting is necessary; I aimed to keep the types consistent in the wrappers
  • sometimes there are \0-terminated and length variants for adding strings (with the \0-terminated variants calling strlen() most of the time, so I decided to use the length variants only.
  • some nanocoap functions use coap_pkt_t, some use coap_hdr_t, some use just raw buffers as the object acted upon. I decided for the wrapper to always use coap_pkt_t as the object acted upon to simplify things towards the front-end. Only exception is gcoap_pkt_build_hdr(), as coap_pkt_init() requires the headers in buf already to be initialized, but it also sets pkt->hdr to the buffer pointer. So requiring pkt->hdr to be already initialized in gcoap_pkt_build_hdr() would lead to pkt->hdr to be initialized twice (once by the caller of gcoap_pkt_build_hdr() and once to in coap_pkt_init(). As such, I decided to have just an opaque buffer be the object acted upon by gcoap_pkt_build_hdr() (while writing this I reallized that as such the name gcoap_build_hdr() might be better suited, but I leave this up to discussion).
  • some nanocoap functions are always used with other functions (e.g. setting the payload always adds the payload marker first) in gcoap and its applications), so the wrapper always does this as well.

Future TODOs:

  • introduce gcoap-specific type wrappers for nanocoap types
  • unify type naming in gcoap types to always be prefixed with gcoap
  • use the gcoap-specific types in newly introduced functions
  • use the newly introduced functions throughout gcoap and the applications.

Testing procedure

examples/gcoap should still compile. Currently, the new wrappers are not used yet, so nothing to test.

Issues/PRs references

Addresse parts of #16855

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Nov 9, 2021
@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Nov 9, 2021
@miri64 miri64 requested review from chrysn and kaspar030 November 9, 2021 16:02
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all pretty straightforward. Using void * for buffers would make for a nicer to use API.

Would it make sense to

typedef gcoap_pkt_t coap_pkt_t

? Tbh I can't quite yet wrap my head around why those wrappers are needed in the first place, but I suppose they will be extended later?

* @name Buffer manipulation
* @{
*/
static inline uint8_t *gcoap_pkt_get_buf(coap_pkt_t *pdu)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static inline uint8_t *gcoap_pkt_get_buf(coap_pkt_t *pdu)
static inline void *gcoap_pkt_get_buf(coap_pkt_t *pdu)

should be more flexible

return (uint8_t *)pdu->hdr;
}

static inline void gcoap_pkt_set_buf(coap_pkt_t *pdu, uint8_t *buf)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Suggested change
static inline void gcoap_pkt_set_buf(coap_pkt_t *pdu, uint8_t *buf)
static inline void gcoap_pkt_set_buf(coap_pkt_t *pdu, void *buf)

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Dec 4, 2021

Would it make sense to

typedef gcoap_pkt_t coap_pkt_t

Yes, that will be done as soon as I find some time to continue on this.

Tbh I can't quite yet wrap my head around why those wrappers are needed in the first place, but I suppose they will be extended later?

See #16855. Also, if you look at libOSCORE's API you will find, that having these wrappers will be very useful to make the access transparent to the user ;-).

@miri64 miri64 mentioned this pull request Dec 23, 2021
2 tasks
@stale
Copy link
Copy Markdown

stale bot commented Jul 10, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jul 10, 2022
@miri64 miri64 removed the State: stale State: The issue / PR has no activity for >185 days label Jul 11, 2022
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jan 27, 2026

Not needed anymore, thanks to unicoap.

@miri64 miri64 closed this Jan 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: network Area: Networking Area: sys Area: System Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants