Skip to content

nanocoap: clean up coap_iterate_option(), make it public #19713

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
Jun 8, 2023
Merged

nanocoap: clean up coap_iterate_option(), make it public #19713

merged 2 commits into from
Jun 8, 2023

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Jun 7, 2023

Contribution description

Currently we don't have a public way to iterate all options of the same type - coap_iterate_option() was intended to be public (it's not static) but was never declared in any header file.

This allows for the opportunity to clean up the API to make it more palatable for public consumption.

Testing procedure

The function is only used by coap_opt_get_string() for which we have unit tests that are still passing.

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 Jun 7, 2023
@benpicco benpicco requested review from maribu and Teufelchen1 June 7, 2023 11:06
@benpicco benpicco added 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 labels Jun 7, 2023
@riot-ci
Copy link

riot-ci commented Jun 7, 2023 •
edited
Loading

Murdock results

✔️ PASSED

030d0c8 examples/gcoap_block_server: add stm32f7508-dk to Makefile.ci

Success Failures Total Runtime
6933 0 6933 12m:33s

Artifacts

@benpicco benpicco force-pushed the coap_iterate_option branch from fc460e8 to 0c0ce81 Compare June 7, 2023 12:53
maribu
maribu approved these changes Jun 7, 2023
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.

Looks good to me. One suggestion for wording in the doc.

I think _parse_option() is unable to parse zero-length CoAP options at the end of the CoAP message (e.g. CoAP number delta < 13, CoAP option length is zero, no payload and no payload marker) :/ But that is completely unrelated to this PR.

@benpicco benpicco force-pushed the coap_iterate_option branch from db1bee2 to 923c9a3 Compare June 7, 2023 14:07
@maribu
Copy link
Member

maribu commented Jun 7, 2023

I think _parse_option() is unable to parse zero-length CoAP options at the end of the CoAP message (e.g. CoAP number delta < 13, CoAP option length is zero, no payload and no payload marker) :/ But that is completely unrelated to this PR.

No, its fine. I didn't look closely enough.

@maribu
Copy link
Member

maribu commented Jun 7, 2023

bors merge

bors bot added a commit that referenced this pull request Jun 7, 2023
@bors @benpicco
Merge #19713
a317283 
19713: nanocoap: clean up coap_iterate_option(), make it public r=maribu a=benpicco



Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
@maribu
Copy link
Member

maribu commented Jun 7, 2023

bors cancel
bors merge

@bors
Copy link
Contributor

bors bot commented Jun 7, 2023

Canceled.

bors bot added a commit that referenced this pull request Jun 7, 2023
@bors @maribu @benpicco
Merge #19705 #19713
5951c83 
19705: boards/z1: fix broken clock configuration r=maribu a=maribu

### Contribution description

The MSP430F2xx family has on RSEL bit more than the MSP430x1xxx family. The first commit updates the clock calibration accordingly.

df5c319 from #19558 broke the clock configuration of the Z1 by relying on the incorrect documentation of what clock is actually used. Closely reading the convoluted clock initialization code revealed that no XT2 crystal is present (as also indicated by some comments in `board.c`), contradicting the `#define MSP430_HAS_EXTERNAL_CRYSTAL 1` in the `board.h`.

The second commit should restore behavior (but with calibrated DCO than hard coded magic numbers).


19713: nanocoap: clean up coap_iterate_option(), make it public r=maribu a=benpicco



Co-authored-by: Marian Buschsieweke <marian.buschsieweke@posteo.net>
Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
@bors
Copy link
Contributor

bors bot commented Jun 7, 2023

Build failed (retrying...):

bors bot added a commit that referenced this pull request Jun 7, 2023
@bors @benpicco
Merge #19713
6aa050a 
19713: nanocoap: clean up coap_iterate_option(), make it public r=maribu a=benpicco



Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
@maribu
Copy link
Member

maribu commented Jun 7, 2023

bors cancel

Some Makefile.ci bumps are required :-/

@bors
Copy link
Contributor

bors bot commented Jun 7, 2023

Canceled.

@benpicco
Copy link
Contributor Author

benpicco commented Jun 7, 2023

Which is odd since I don't see any size increase locally

@maribu
Copy link
Member

maribu commented Jun 7, 2023

bors merge

@bors
Copy link
Contributor

bors bot commented Jun 7, 2023

Canceled.

@github-actions github-actions bot added the Area: examples Area: Example Applications label Jun 7, 2023
@benpicco
Copy link
Contributor Author

benpicco commented Jun 7, 2023

bors merge

@bors
Copy link
Contributor

bors bot commented Jun 8, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit ed3b894 into RIOT-OS:master Jun 8, 2023
@benpicco benpicco deleted the coap_iterate_option branch June 8, 2023 09:26
@benpicco benpicco added this to the Release 2023.07 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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

@Teufelchen1 Teufelchen1 Awaiting requested review from Teufelchen1

Assignees
No one assigned
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: examples Area: Example Applications 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 2023.07
Development

Successfully merging this pull request may close these issues.
3 participants
@benpicco @riot-ci @maribu