-
Notifications
You must be signed in to change notification settings - Fork 2.1k
net/unicoap: Unified and Modular CoAP stack: Messaging and Minimal Server (pt 2) #21582
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
base: master
Are you sure you want to change the base?
Conversation
da014c7 to
4d00949
Compare
|
Note that this change drastically lowers the complexity of our path matching functions (UNICOAP_PATH to /a/string/path and UNCOAP_PATH to |
|
Also, note that this change can potentially decrease ROM size given an application that defines a lot of deeply nested paths. Each path component is stored as a separate string, hence multiple paths sharing a bunch of components do not necessarily replicate the same information in memory (either through compiler optimisation or thru manually reusing the same string). |
|
@mguetschow Do you want to have a look before I rebase this onto the current master to fix DTLS (and thereby rewrite history (at least this PR's...)) |
Co-authored-by: Mikolai Gütschow <[email protected]>
305f40f to
5c3c66f
Compare
|
@carl-tud i need to fix this: unicoap holds stale dTLS session until they are explicitly closed by the peer |
This is the way! Really nice change! |
@mguetschow Is there really a bug to fix here? Just tried Doesn't waiting for the client to close the session make sense given that the client may want to further observe the resource or continue to send requests to that particular host? |
|
@carl-tud can you work around resources and handle all requests in one function? I.e., does a listener’s matcher need to return a resource? Figure out if that can be NULL. |
mguetschow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just gone through your pre-Christmas changes (Github UI shows me 22 commits).
Mostly nits as usual, some static tests failing - and a 👍 for the path spec!
|
|
||
| # This is the full version of debug logging, including trace logs. When this is enabled, | ||
| # the previously mentioned API misuse, missing modules, and fix-its will also be diagnosed. | ||
| # So when you enable debug logging, you do not need to set CONFIG_UNICOAP_ASSIST. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be @ref CONFIG_UNICOAP_ASSIST, I guess
| import aiocoap.resource as resource | ||
| from aiocoap.transports.tinydtls import DTLSClientConnection | ||
|
|
||
| logging.basicConfig(level=logging.DEBUG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want to move these down to fix static tests.
| error: | ||
| SERVER_DEBUG("failed to send response, trying to send 5.05 unreliably\n"); | ||
|
|
||
| /* try to send 5.05, */ | ||
| unicoap_response_init_empty(packet->message, UNICOAP_STATUS_INTERNAL_SERVER_ERROR); | ||
| unicoap_messaging_send(packet, _messaging_flags_resource(resource->flags) & | ||
| ~UNICOAP_MESSAGING_FLAG_RELIABLE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should i not send a 500 instead (thereby leaving the option to send a 500 to the app)?
Were both supposed to be 500? If so, I don't understand your proposal.
| LOG_PREFIX = Path(__file__).name | ||
|
|
||
| tell = lambda message: print(f"{LOG_PREFIX}: {message}") | ||
| complain = lambda message: print(f"{LOG_PREFIX}: error: {message}", file=sys.stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E731 (static tests) complain about your lambda :P
| */ | ||
| typedef struct { | ||
| /** | ||
| * @brief Pointer to contiguously stored character pointers to null-terminated strings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
..., terminated by NULL.
|
|
||
| _TEST_ASSERT_FALSE(_MATCH_STRING(&r, "/")); | ||
| _TEST_ASSERT_FALSE(_MATCH_STRING(&r, "a")); | ||
| _TEST_ASSERT_TRUE(_MATCH_STRING(&r, "a")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, interesting, so we consider a the same as /a?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Background: Technically, UNICOAP_PATH doesn't necessarily carry the relative-to-root semantics anymore, I.e. there's nothing like a leading slash, or something like UNICOAP_PATH(ROOT, "a", "b"). Theoretically, we could employ UNICOAP_PATH for relative (albeit static) paths because it doesn't say "I'm a path starting at the root" anymore, though this is more of a subjective beauty argument.
I don't know what restricting a path object to be relative to the root would be useful for (e.g., by introducing a private path object property).
The only thing the path object is used for, and in the foreseeable future will be, is the resource definition. And in that context, the path is interpreted to be relative to the root. Consequently, I think it's fine to allow both "a" and "/a" to match UNICOAP_PATH("a"). Furthermore, this a resource matching function, so that context /interpretation is baked into the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see and agree, I don't see a reason for adding that boolean either. Maybe this fact could be somehow reflected in the UNICOAP_PATH doc, though.
| /* | ||
| char test_buffer[20] = {}; | ||
| ssize_t res = unicoap_path_serialize(&r.path, test_buffer, sizeof(test_buffer)); | ||
| TEST_ASSERT_EQUAL_INT(res, static_strlen("/a123/a567")); | ||
| printf("'%.*s'\n", (int)res, test_buffer); | ||
| printf("'"); | ||
| unicoap_path_print(&r.path); | ||
| printf("'\n"); | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can probably be removed?
| const unicoap_pathspec_t my_root = UNICOAP_PATH_ROOT; | ||
| TEST_ASSERT_EQUAL_INT(my_root._components, NULL); | ||
|
|
||
| const unicoap_pathspec_t my_root2 = UNICOAP_PATH(NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason we allow both ways of representing the root path? Otherwise I'd drop this one and maybe have a static error when doing this? Could potentially simplify some code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Static asserts won't work for that in a macro, but I'll come up with something hacky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason we allow both ways of representing the root path? Otherwise I'd drop this one and maybe have a static error when doing this? Could potentially simplify some code?
You're right to point this out, the runtime checks should be asserts and that case generally be disallowed.
|
|
||
| static void test_path_object_is_root(void) { | ||
| const unicoap_pathspec_t my_path1 = UNICOAP_PATH("a"); | ||
| _TEST_ASSERT_FALSE(unicoap_path_is_root(&my_path1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could have used unicoap_path_is_root in several places instead of explicitly checking for the null pointers, but, on the other hand, it's maybe good to be explicit.
| _TEST_ASSERT_FALSE(path_is_equal(UNICOAP_PATH(NULL), UNICOAP_PATH("a"))); | ||
| } | ||
|
|
||
| static void test_path_object_subtree_is_equal(void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test does not check for subtree equality as the name implies, or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant was, "Given a path object that has a subtree beneath the root, does the equality check work?"
Sounds sensible to me, indeed. Doesn't tinydtls have a (large) timeout on those anyways? |
This PR is the second in a series to introduce
unicoap, a unified and modular CoAP implementation for RIOT. An overview of all PRs related tounicoapis presented in #21389, including reasons whyunicoapis needed and a performance analysis.What does this PR include?
unicoapon a thread of your choice, e.g., the main thread.The new API is more flexible. CoAP endpoints are abstracted into a
unicoap_endpoint_tstructure and transport-specific settings are controlled by flags. For example, this is a simple resource that responds with "Hello, World!".More in the documentation (CI build sometimes not available, e.g., due to failing tests).