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

sys/net/nanocoap: improve coap_build_reply() #21155

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

maribu
Copy link
Member

@maribu maribu commented Jan 22, 2025

Contribution description

  • The responsibility for handling matching CoAP No-Response Options has been split:
    • coap_build_reply() only needs to report this and return -ECANCLED
    • coap_handle_req() does generate the empty ACK is needed.
      ==> As a result, writing CoAP request handlers correctly becomes a lost easier. Correct error handling to be present is now sufficient for correct handling of No-Response options.
      ==> This change is backward compatible with existing code.
  • The API doc has been cleaned up and straightened

Testing procedure

  • CoAP reply handlers that behaved correctly before should still behave correctly now
  • CoAP reply handlers that did not behave correctly before in presence of No-Response options should now behave correctly, if the had correct error handling

(CoAP reply handlers that did not get error handling correctly will stay broken.)

Issues/PRs references

None

@maribu maribu added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: doc Area: Documentation Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 22, 2025
@maribu maribu requested review from benpicco and mguetschow January 22, 2025 21:01
@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System and removed Area: doc Area: Documentation labels Jan 22, 2025
@riot-ci
Copy link

riot-ci commented Jan 22, 2025

Murdock results

✔️ PASSED

d1da109 sys/net/nanocoap: improve coap_build_reply

Success Failures Total Runtime
10271 0 10271 10m:23s

Artifacts

Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Thanks, nice start! Some suggestions below.

sys/include/net/nanocoap.h Outdated Show resolved Hide resolved
sys/include/net/nanocoap.h Outdated Show resolved Hide resolved
sys/include/net/nanocoap.h Outdated Show resolved Hide resolved
sys/include/net/nanocoap.h Outdated Show resolved Hide resolved
sys/include/net/nanocoap.h Show resolved Hide resolved
sys/include/net/nanocoap.h Outdated Show resolved Hide resolved
sys/include/net/nanocoap.h Show resolved Hide resolved
@maribu maribu force-pushed the sys/net/nanocoap/coap_build_reply branch 3 times, most recently from e2464a0 to 58ca598 Compare January 23, 2025 10:45
Using a constant is easier than explaining where the magic 1 came from
in size estimations.
@maribu maribu force-pushed the sys/net/nanocoap/coap_build_reply branch 2 times, most recently from 7141f4f to 38397bc Compare January 23, 2025 17:43
@maribu maribu force-pushed the sys/net/nanocoap/coap_build_reply branch from 38397bc to 526310d Compare January 24, 2025 15:24
@github-actions github-actions bot added the Area: CoAP Area: Constrained Application Protocol implementations label Jan 24, 2025
@maribu maribu changed the title sys/net/nanocoap: improve doc for coap_build_reply() sys/net/nanocoap: improve coap_build_reply() Jan 24, 2025
@maribu maribu added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Jan 24, 2025
- The responsibility for handling matching CoAP No-Response Options
  has been split:
    - `coap_build_reply()` only needs to report this and return
      `-ECANCLED`
    - `coap_handle_req()` does generate the empty ACK is needed.
  ==> As a result, writing CoAP request handlers correctly becomes a
      lost easier. Correct error handling to be present is now
      sufficient for correct handling of No-Response options.
  ==> This change is backward compatible with existing code.
- The API doc has been cleaned up and straightened

Co-authored-by: mguetschow <mikolai.guetschow@tu-dresden.de>
@maribu maribu force-pushed the sys/net/nanocoap/coap_build_reply branch from 526310d to d1da109 Compare January 24, 2025 15:32
@maribu
Copy link
Member Author

maribu commented Jan 24, 2025

I changed the API for handling with the No-Response case, so that less foot-shooting should occur.

The API change is not breaking:

  • Everything that did work correctly before will keep working correctly
  • Some handlers that were not working correctly (e.g. shot in their foot in presence of No-Response Options in the request) will now work
  • Response handlers that did not check the return value of coap_build_reply() and pass it on were broken before and stay broken

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: sys Area: System 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 Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. 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.

3 participants