Skip to content

nanocoap: use void * for payload bytes #17990

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 10, 2022
Merged

nanocoap: use void * for payload bytes #17990

merged 2 commits into from
May 10, 2022

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Apr 22, 2022 •
edited
Loading

Contribution description

Casting payload to (uint8_t *) or (void *) is really annoying and unnecessary.

Testing procedure

No change in code output or on the caller side, just more convenient interface.

Issues/PRs references

@benpicco benpicco added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Apr 22, 2022
@github-actions github-actions bot added Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System labels Apr 22, 2022
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 22, 2022
@benpicco benpicco requested review from maribu and fjmolinas April 22, 2022 17:00
kfessel
kfessel reviewed Apr 22, 2022
View reviewed changes
@benpicco benpicco added the Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation label Apr 22, 2022
@benpicco
Copy link
Contributor Author

Urgh, so while this is a non-breaking change for C code, it breaks the Rust wrappers - how can this be resolved @chrysn

@benpicco benpicco requested a review from chrysn April 23, 2022 08:25
@fjmolinas fjmolinas added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Apr 25, 2022
@github-actions github-actions bot added the Area: examples Area: Example Applications label Apr 30, 2022
@benpicco benpicco added the State: waiting for other PR State: The PR requires another PR to be merged first label May 2, 2022
chrysn added a commit to chrysn-pull-requests/RIOT that referenced this pull request May 3, 2022
@chrysn
{examples,tests}/rust: Rust updates
ec498cd 
Updating to riot-sys 0.7.7 enables support for more recent C2Rust
versions, and to riot-wrappers 0.7.22 unblocks [17990].

[17990]: RIOT-OS#17990
@chrysn chrysn mentioned this pull request May 3, 2022
Merged
@benpicco benpicco removed the State: waiting for other PR State: The PR requires another PR to be merged first label May 3, 2022
@github-actions github-actions bot removed the Area: examples Area: Example Applications label May 3, 2022
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 3, 2022
maribu
maribu approved these changes May 10, 2022
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. This is a very sensible API change that should go unnoticed for the C callers and the Rust wrappers have apparently been updated.

@maribu maribu enabled auto-merge May 10, 2022 08:48
@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 May 10, 2022
@maribu maribu merged commit 3dad674 into RIOT-OS:master May 10, 2022
@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 May 10, 2022
@maribu
Copy link
Member

maribu commented May 10, 2022

Looks like the automerge was faster than giving the CI another spin :) Anyway, the next CI run with also test this.

@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 May 10, 2022
@benpicco benpicco deleted the nanocoap-void branch May 10, 2022 08:58
benpicco pushed a commit to benpicco/RIOT that referenced this pull request May 16, 2022
@chrysn @benpicco
{examples,tests}/rust: Rust updates
2059a94 
Updating to riot-sys 0.7.7 enables support for more recent C2Rust
versions, and to riot-wrappers 0.7.22 unblocks [17990].

[17990]: RIOT-OS#17990

(cherry picked from commit ec498cd)
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kfessel kfessel kfessel left review comments

@maribu maribu maribu approved these changes

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

@cgundogan cgundogan Awaiting requested review from cgundogan

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@chrysn chrysn Awaiting requested review from chrysn

Assignees
No one assigned
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2022.07
Development

Successfully merging this pull request may close these issues.
5 participants
@benpicco @maribu @kfessel @chrysn @fjmolinas