Skip to content

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

Merged
kaspar030 merged 4 commits intoRIOT-OS:masterfrom
kb2ma:gcoap/use_opt_finish
Mar 12, 2019
Merged

net/gcoap: Replace use of gcoap_finish() with coap_opt_finish()#10892
kaspar030 merged 4 commits intoRIOT-OS:masterfrom
kb2ma:gcoap/use_opt_finish

Conversation

@kb2ma
Copy link
Copy Markdown
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
Copy Markdown
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