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

net/gcoap: Replace use of gcoap_finish() with coap_opt_finish() #10892

Merged
merged 4 commits into from
Mar 12, 2019

Conversation

kb2ma
Copy link
Member

@kb2ma kb2ma commented Jan 28, 2019

Contribution description

#9309 and API Options introduced the struct-based Options API for nanocoap and gcoap. As part of that work, this PR transitions gcoap to use coap_opt_finish() to mark completion of writing message metadata -- header, token, and options.

As we have developed and integrated Options, it became clear that gcoap_finish(), the original mechanism for closing a message, was not flexible enough. Once the payload is written, it becomes difficult and inefficient to attempt to insert Options in front of it. This PR fixes that rookie mistake. :-/ However, to be clear, the procedure to build a message with gcoap_finish() still works.

This PR includes gcoap's internal use of coap_opt_finish(), unit tests, and the module documentation. To limit the scope for a reviewer, we plan to update the gcoap and cord examples in a follow-up PR.

Testing procedure

The items below will exercise everything that has changed.

  1. Run gcoap unit tests
  2. Use the gcoap example as a server, and send requests to it for /.well-known/core and some unknown path
  3. Review module documentation

Issues/PRs references

Part of #9309 struct-based Options API

@kb2ma kb2ma added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: CoAP Area: Constrained Application Protocol implementations labels Jan 28, 2019
@kb2ma kb2ma requested review from kaspar030 and smlng January 28, 2019 19:22
@kb2ma kb2ma mentioned this pull request Feb 15, 2019
6 tasks
@kb2ma kb2ma added this to the Release 2019.04 milestone Feb 24, 2019
@kaspar030 kaspar030 added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Mar 12, 2019
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

Tested gcoap example & unittests on native. Code gets a little smaller (unless DEVELHELP is enabled...)

@kaspar030 kaspar030 merged commit 297efdd into RIOT-OS:master Mar 12, 2019
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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines 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.

2 participants