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

nanocoap: Add handler for resource-based subtrees #13698

Merged
merged 2 commits into from
Apr 24, 2022

Conversation

bergzand
Copy link
Member

Contribution description

This adds a coap_handler_t function that can be used to parse new
subtrees. The subtree information is included in the context pointer of
the call and must be of type coap_resource_subtree_t. This object then
contains the pointer and length of a different coap_resource_t instance.

Testing procedure

The coap handlers in examples/suit-update should still work as before

Issues/PRs references

Follow-up with more cleanups to #13687

@bergzand bergzand 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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: OTA Area: Over-the-air updates Area: CoAP Area: Constrained Application Protocol implementations labels Mar 24, 2020
@bergzand bergzand requested review from kaspar030 and fjmolinas March 24, 2020 14:10
@benpicco benpicco requested a review from kb2ma March 26, 2020 17:35
Copy link
Member

@kb2ma kb2ma left a comment

Choose a reason for hiding this comment

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

This PR looks like a step toward a concept like gcoap_listener_t. Eventually it will make sense to add more metadata to a coap_subtree_handler_t, and to use it in place of the externs for coap_resources and coap_resources_numof.

I created #11488 to move this sort of application level functionality to nanocoap sock to avoid duplication and confusion with gcoap. The contents of the current PR are similar to #11436 from last year that was closed partly due to these concerns.

As this coap_subtree_handler_t matures and we find commonality between nanocoap sock and gcoap, we can move the common bits into nanocoap itself. Maybe we can at least move coap_handle_req() and its components to nanocoap sock for now.

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.

Only moves code from SUIT into the common domain.

kb2ma
kb2ma previously requested changes Mar 30, 2020
Copy link
Member

@kb2ma kb2ma left a comment

Choose a reason for hiding this comment

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

I would appreciate a response to my concerns before merging.

@kb2ma kb2ma dismissed their stale review June 16, 2020 10:30

no longer a stake holder

@miri64
Copy link
Member

miri64 commented Jun 25, 2020

@bergzand can you add the response requested by Ken?

@miri64 miri64 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 Jun 25, 2020
@benpicco benpicco requested a review from chrysn October 21, 2020 22:25
@stale
Copy link

stale bot commented Jun 2, 2021

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 Jun 2, 2021
@stale stale bot closed this Jul 8, 2021
@benpicco benpicco reopened this Jan 5, 2022
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Jan 5, 2022
@github-actions github-actions bot added the Area: network Area: Networking label Jan 5, 2022
@github-actions github-actions bot added the Area: sys Area: System label Jan 5, 2022
@benpicco
Copy link
Contributor

benpicco commented Jan 5, 2022

This is still a valid cleanup.
Just because there was a larger cleanup underway (that is now stalled) should not stop this from being merged.

Please rebase.

@OlegHahm OlegHahm added the State: waiting for author State: Action by the author of the PR is required label Apr 8, 2022
@OlegHahm
Copy link
Member

OlegHahm commented Apr 8, 2022

@bergzand ping

This adds a coap_handler_t function that can be used to parse new
subtrees. The subtree information is included in the context pointer of
the call and must be of type coap_resource_subtree_t. This object then
contains the pointer and length of a different coap_resource_t instance.
The SUIT CoAP code can make use of the generic subtree handler. This
commit removes the obsolete code to make use of the more generic
nanocoap subtree handlers.
@benpicco benpicco force-pushed the pr/nanocoap/subtree_handler branch from d298073 to 677871d Compare April 23, 2022 08:22
@benpicco benpicco removed the State: waiting for author State: Action by the author of the PR is required label Apr 23, 2022
@benpicco benpicco merged commit b31de65 into RIOT-OS:master Apr 24, 2022
@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
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: OTA Area: Over-the-air updates 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 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.

6 participants