Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

net/nanocoap: remove coap_pkt token ptr #17983

Merged
merged 2 commits into from
Oct 16, 2022
Merged

net/nanocoap: remove coap_pkt token ptr #17983

merged 2 commits into from
Oct 16, 2022

Conversation

kfessel
Copy link
Contributor

@kfessel kfessel commented Apr 22, 2022 •
edited
Loading

this might need a deprecation period first

Contribution description

remove the token ptr from nanocoap coap_pkt_t in favour of coap_get_token() introduced in #17976

Testing procedure

murdock build

Issues/PRs references

#17976

wait for #17950

@github-actions github-actions bot added Area: CI Area: Continuous Integration of RIOT components Area: network Area: Networking Area: sys Area: System labels Apr 22, 2022
@kfessel kfessel requested a review from benpicco April 22, 2022 10:23
@benpicco
Copy link
Contributor

With #17950 there shouldn't be any actual users of the pointer left - only setters, which can be removed easily.

@kfessel
Copy link
Contributor Author

kfessel commented Apr 22, 2022

lets ping @kaspar030 @miri64 @chrysn if anyone suggests to have a deprecation period for <coap_pkt_t>.token

@github-actions github-actions bot added the Area: CoAP Area: Constrained Application Protocol implementations label Apr 22, 2022
@kfessel
Copy link
Contributor Author

kfessel commented Apr 22, 2022

some of the changes in 1600184 are part of #17950

@chrysn
Copy link
Member

chrysn commented Apr 25, 2022

Personally I won't insist on having a deprecation period -- especially in this case because using the old access will reliably fail. Adding @cgundogan to the pingees because I suspect he might have PRs around that might be using the token a bit more.

@kfessel kfessel mentioned this pull request Apr 26, 2022
Merged
@kfessel kfessel force-pushed the p-remove-coap_pkt-token branch from 1600184 to 0c22bdd Compare May 3, 2022 12:22
@kfessel kfessel added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 3, 2022
@kfessel kfessel force-pushed the p-remove-coap_pkt-token branch from 0c22bdd to 6fe787e Compare May 4, 2022 10:11
@github-actions github-actions bot added Area: CI Area: Continuous Integration of RIOT components and removed Area: CI Area: Continuous Integration of RIOT components labels May 4, 2022
@kfessel kfessel force-pushed the p-remove-coap_pkt-token branch from 0c22bdd to 6fe787e Compare May 4, 2022 11:27
@kfessel kfessel marked this pull request as ready for review May 4, 2022 11:27
@github-actions github-actions bot removed the Area: CI Area: Continuous Integration of RIOT components label May 4, 2022
@kfessel kfessel requested a review from chrysn May 4, 2022 11:28
@cgundogan cgundogan mentioned this pull request May 13, 2022
Merged
@benpicco
Copy link
Contributor

benpicco commented Sep 20, 2022 •
edited
Loading

This needs a rebase, but we can merge this now soon.

@riot-ci
Copy link

riot-ci commented Oct 5, 2022 •
edited
Loading

Murdock results

✔️ PASSED

b575f0c net/nanocoap: remove coap_pkt token ptr

Success Failures Total Runtime
1983 0 1983 07m:00s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

@benpicco benpicco force-pushed the p-remove-coap_pkt-token branch from 3c7ebf8 to 0af1e1f Compare October 5, 2022 22:07
@benpicco benpicco force-pushed the p-remove-coap_pkt-token branch from 0af1e1f to b575f0c Compare October 5, 2022 22:20
@kfessel kfessel 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 Oct 7, 2022
benpicco
benpicco approved these changes Oct 15, 2022
View reviewed changes
Copy link
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.

2022.10 is branched off, we can merge this now.

@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 Oct 15, 2022
@benpicco benpicco enabled auto-merge October 15, 2022 21:51
@benpicco benpicco merged commit 72d16e1 into RIOT-OS:master Oct 16, 2022
@kaspar030 kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@benpicco benpicco benpicco 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

@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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2023.01
Development

Successfully merging this pull request may close these issues.
5 participants
@kfessel @benpicco @chrysn @riot-ci @kaspar030