Skip to content

nanocoap: export coap_opt_get_uint()#13893

Merged
kb2ma merged 1 commit intoRIOT-OS:masterfrom
cgundogan:pr/nanocoap-get-opt-uint
Apr 18, 2020
Merged

nanocoap: export coap_opt_get_uint()#13893
kb2ma merged 1 commit intoRIOT-OS:masterfrom
cgundogan:pr/nanocoap-get-opt-uint

Conversation

@cgundogan
Copy link
Copy Markdown
Member

@cgundogan cgundogan commented Apr 17, 2020

Contribution description

This PR exports the coap_opt_get_uint() function (similar to the already existing public coap_opt_add_uint() function). Currently, it is a hidden function in nanocoap.c.

Testing procedure

--

Issues/PRs references

This PR is a cut-out of #13889 since it is unrelated to the actual cache implementation.

@cgundogan cgundogan added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Area: CoAP Area: Constrained Application Protocol implementations labels Apr 17, 2020
@cgundogan cgundogan requested review from benpicco and kb2ma April 17, 2020 15:41
@cgundogan cgundogan mentioned this pull request Apr 17, 2020
Merged
benpicco
benpicco approved these changes Apr 17, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Pretty straightforward, matches coap_opt_get_string()

@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 17, 2020
benpicco
benpicco reviewed Apr 17, 2020
View reviewed changes
@cgundogan cgundogan force-pushed the pr/nanocoap-get-opt-uint branch from 1614bc0 to cd954a3 Compare April 17, 2020 15:59
@cgundogan
Copy link
Copy Markdown
Member Author

cgundogan commented Apr 17, 2020

@benpicco thanks for the quick feedback. I changed the parameter from opt_num to optnum, because all other doxygen parts use the same name: optnum. (amended immediately)

@cgundogan cgundogan force-pushed the pr/nanocoap-get-opt-uint branch 2 times, most recently from 046fb9f to 048bec5 Compare April 17, 2020 16:02
kb2ma
kb2ma requested changes Apr 17, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Reasonable to expose this function, and the name fits with the API.

sys/include/net/nanocoap.h Outdated
* @param[out] value the parsed option value
*
* @return 0 if the option was found and the value was parsed correctly
* @return -1 if the option was not found in @p pkt
Copy link
Copy Markdown
Member

@kb2ma kb2ma Apr 17, 2020

Choose a reason for hiding this comment

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

Return -ENOENT if not found to be consistent with coap_opt_get_opaque() and coap_opt_get_next(). Go ahead and squash this in here and in nanocoap.c.

Copy link
Copy Markdown
Member Author

@cgundogan cgundogan Apr 17, 2020

Choose a reason for hiding this comment

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

Alright, that is a good hint. I also added -ENOSPC to the docs .. did overlook that one over in nanocoap.c::coap_opt_get_uint().

I amended the changes immediately into the commit.

@cgundogan cgundogan force-pushed the pr/nanocoap-get-opt-uint branch from 048bec5 to b38a54d Compare April 17, 2020 20:37
@cgundogan cgundogan force-pushed the pr/nanocoap-get-opt-uint branch from b38a54d to 6859de7 Compare April 17, 2020 20:39
@kb2ma kb2ma merged commit cac3509 into RIOT-OS:master Apr 18, 2020
@kb2ma kb2ma added this to the Release 2020.07 milestone Apr 18, 2020
@kb2ma
Copy link
Copy Markdown
Member

kb2ma commented Apr 18, 2020

ACK. Tested with gcoap observe.

@cgundogan cgundogan deleted the pr/nanocoap-get-opt-uint branch April 20, 2020 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@kb2ma kb2ma kb2ma approved these changes

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

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

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees

No one assigned

Labels

Area: CoAP Area: Constrained Application Protocol implementations CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2020.07

Development

Successfully merging this pull request may close these issues.

3 participants

@cgundogan @kb2ma @benpicco