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.

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

gcoap: move tl_type to coap_request_ctx_t #18313

Merged
merged 1 commit into from Aug 17, 2022
Merged

gcoap: move tl_type to coap_request_ctx_t #18313

merged 1 commit into from Aug 17, 2022

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Jul 17, 2022 •
edited

Contribution description

We don't have to carry this information inside every coap_pkt_t, it's enough to provide it via the request context information.

Testing procedure

examples/gcoap_dtls still works

> coap get fe80::7837:fcff:fe7d:1aaf 5684 /.well-known/core
gcoap_cli: sending msg ID 7277, 23 bytes
gcoap: response Success, code 2.05, 46 bytes
</cli/stats>;ct=0;rt="count";obs,</riot/board>

Issues/PRs references

@github-actions github-actions bot added Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System labels Jul 17, 2022
@benpicco benpicco requested review from chrysn and fabian18 July 17, 2022 15:11
@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 Jul 17, 2022
@benpicco benpicco force-pushed the coap_request_ctx_get_tl_type branch from 1d6de72 to 7f1ae72 Compare July 17, 2022 16:02
@benpicco benpicco force-pushed the coap_request_ctx_get_tl_type branch from 7f1ae72 to d7bb422 Compare July 17, 2022 16:05
@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 Jul 17, 2022
@benpicco benpicco added the Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation label Jul 27, 2022
@benpicco benpicco requested a review from maribu August 9, 2022 16:34
maribu
maribu approved these changes Aug 16, 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. Trusting your reasoning and testing.

I agree that the entanglement between nanocoap and gcoap should be sorted out, but this doesn't increase this entanglement. I am hoping to see a cleanup in which gcoap would become basically a convenience layer on top of nanocoap, so that nanocoap would need to be aware of gcoap anymore.

@benpicco benpicco merged commit 0713e0d into RIOT-OS:master Aug 17, 2022
@benpicco benpicco deleted the coap_request_ctx_get_tl_type branch August 17, 2022 14:51
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 left review comments

@maribu maribu maribu approved these changes

@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

@fabian18 fabian18 Awaiting requested review from fabian18

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 Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2022.10
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@benpicco @miri64 @maribu