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

nanocoap: Move application functionality to nanocoap sock #11488

Closed
wants to merge 3 commits into from

Conversation

kb2ma
Copy link
Member

@kb2ma kb2ma commented May 4, 2019

Contribution description

#11098 added the ability for a single (nano|g)coap resource to respond to a subtree of request URIs. Open PR #11436 additionally will provide a mechanism to lookup into a nested set of resources to handle requests within that subtree. This mechanism includes a new function and a subtree handler struct in the base nanocoap library. However, gcoap already has the ability to specify multiple sets of handlers via its listener mechanism.

The location of the additional capability for 11436 in the base nanocoap library means gcoap must evaluate whether or how to incorporate the new nanocoap subtree handlers. In this case, the new mechanism is not required. This conflict illustrates how "nanocoap" really is a combination of two tools:

  • a base CoAP library, useful for any application level tool like gcoap
  • an application level tool itself

nanocoap as an application already includes the nanocoap sock module for high level functions to send and receive a message. So, this PR extends the split of these two roles by moving more application level functionality to the nanocoap sock module. Specifically, this includes these methods and structs:

  • coap_build_reply()
  • coap_block2_build_reply()
  • coap_handle_req()
  • coap_tree_handler()
  • coap_reply_simple()
  • coap_well_known_core_default_handler()
  • coap_resources[]
  • coap_resources_numof

With this change, nanocoap sock independently can add the subtree handler functionality in 11436. This relocation allows nanocoap to innovate without causing a conflict with gcoap. At the same time, if this new mechanism develops into a generally worthwhile feature, it can be migrated to the base nanocoap library in the future.

Testing procedure

No new functionality, so ensure nothing is broken:

  • nanocoap unit tests
  • nanocoap_cli test
  • nanocoap_server example

Issues/PRs references

See description.

@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 May 4, 2019
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

As I understand this PR, RIOT's CoAP would be divided in nanocoap-message, nanocoap-socket and gcoap, where both the latter use the former to manipulate their messages. When the gcoap user needs to interact with nanocoap-message, they should not be presented with functions inapplicable to them. (Indeed that'd have saved me some time).

Looking at the built documentation, I don't think this is complete though; see inline comments, and:

  • "Server path matching" is a prominent part in the nanocoap-message overview. As there is only one coap_resource_t method, it might be more suitable to describe there (after all, this is now only a tool method).

What I still can't fully judge is whether the separation is precise. Would anything go wrong if someone used, for example, coap-build_reply in a gcoap handler?

@@ -18,7 +18,7 @@
* provides high and low level interfaces to CoAP options, including Block.
Copy link
Member

Choose a reason for hiding this comment

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

The whole section could now use a name that indicates it's about the messages

@@ -18,7 +18,7 @@
* provides high and low level interfaces to CoAP options, including Block.
*
* nanocoap includes the core structs to store message information. It also
* also provides support for sending and receiving messages, such as
* also provides functions to support sending and receiving messages, such as
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* also provides functions to support sending and receiving messages, such as
* also provides helpers for use before sending of after receiving messages, such as

Copy link
Member Author

Choose a reason for hiding this comment

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

Added this suggestion to #12231 since it's an improvement to generic nanocoap.

@kb2ma
Copy link
Member Author

kb2ma commented Sep 15, 2019

Would anything go wrong if someone used, for example, coap-build_reply in a gcoap handler?

For this specific example, yes you can do this, and you must use the Buffer API for the rest of this response. Also, you must manually add any Observe option to the response. See gcoap_resp_init().

I respond in more detail in a comment on 12201. IMO for a user the choice of gcoap vs. nanocoap sock is challenging enough. Layering on documentation to selectively use the Buffer API in gcoap or the Packet API in nanocoap requires qualifications that would be difficult to explain clearly and simply.

@stale
Copy link

stale bot commented Mar 18, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 18, 2020
@kb2ma kb2ma added the State: don't stale State: Tell state-bot to ignore this issue label Mar 27, 2020
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Mar 27, 2020
@benpicco benpicco requested a review from bergzand March 29, 2020 20:44
@@ -9,13 +9,14 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to split this into nanocoap/client.h and nanocoap/server.h?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with this, but I don't mind leaving this to a follow up PR. @kb2ma What do you think?

Copy link
Member Author

@kb2ma kb2ma Mar 31, 2020

Choose a reason for hiding this comment

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

would it make sense to split this into nanocoap/client.h and nanocoap/server.h?

I like the 'name substitution' aspect. In other words, get rid of "sock" from the name, which is descriptive but overlaps with the name of the messaging stack networking API. The use of 'client' and 'server' imply endpoint messaging without really conflicting with gcoap. It's possible that nanocoap.h might split someday for other reasons, but I think we can manage the distinction from the client/server headers. The module documentation title can change from 'Nanocoap Sock' to 'Nanocoap Client/Server' or something like that.

In terms of implementation files, remember that the client/server split can be murky. A client might send a NON request but receive a CON response and have to manage retries. Neither gcoap or nanocoap supports that now, but it's out there.

[edit -- Client/Server example is wrong -- a client already manages retries for a CON request. OTOH, a server might want to send an immediate ACK to a request, and then send a separate response confirmably.]

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with this, but I don't mind leaving this to a follow up PR. @kb2ma What do you think?

That works for me. Let's make the migration out of nanocoap.(h|c) itself as one step, and then the group that works more closely with it can split it up however works best.

@kb2ma
Copy link
Member Author

kb2ma commented Apr 1, 2020

Rebased for your reviewing pleasure. Also moved coap_tree_handler(), introduced in #13687, to nanocoap sock.

@kb2ma kb2ma closed this Jun 16, 2020
@miri64 miri64 added State: archived State: The PR has been archived for possible future re-adaptation and removed State: don't stale State: Tell state-bot to ignore this issue labels Jun 16, 2020
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 State: archived State: The PR has been archived for possible future re-adaptation 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.

None yet

5 participants