net/unicoap: basic CoAP group communication support - #22484
Conversation
carl-tud
left a comment
There was a problem hiding this comment.
Couple of small things; would it be possible to cancel a multicast request using unicoap_cancel?
|
I assume this is an implementation of https://datatracker.ietf.org/doc/html/rfc7390? I was yesterday pointed to https://datatracker.ietf.org/doc/draft-ietf-core-groupcomm-bis/ which obsoletes RFC7390 with many more features. Might be worth to have a look. It is about to be published as RFC: https://queue.rfc-editor.org/#draft-ietf-core-groupcomm-bis |
7d52f88 to
993cf29
Compare
993cf29 to
d53226e
Compare
|
Basic testing of this PR was successful:
Open TODOs
Open Questions:
Footnotes
|
mguetschow
left a comment
There was a problem hiding this comment.
Thanks for coding and testing! Some comments below.
- Right now we still return the eventual request timeout as failure to the user. It's not really an error because it's simply the time at which we stop accepting responses. Should we just not inform the user of the timeout in the multicast scenario?
Agree that we should definitely not return it to the user as an error. But we probably want to inform the user about the finished wait time in case it has some resources alloced that it can free afterwards.
Also a prerequisite for the client sync APIs to continue working, I guess. See unicoap_send_request_sync and unicoap_send_request_sync_copy
|
|
||
| unicoap_event_schedule(&memo->super.exchange.timeout, _on_response_timeout, | ||
| (parameters && parameters->timeout_ms > 0) ? | ||
| parameters->timeout_ms : CONFIG_UNICOAP_TIMEOUT_CLIENT_RESPONSE_MS, | ||
| "client.resp-timeout"); | ||
| if (!multicast) { | ||
| unicoap_event_schedule(&memo->super.exchange.timeout, _on_response_timeout, | ||
| (parameters && parameters->timeout_ms > 0) ? | ||
| parameters->timeout_ms : | ||
| CONFIG_UNICOAP_TIMEOUT_CLIENT_RESPONSE_MS, | ||
| "client.resp-timeout"); | ||
| } | ||
| else if (CONFIG_UNICOAP_TIMEOUT_CLIENT_MULTICAST_RESPONSE_MS > 0) { | ||
| unicoap_event_schedule(&memo->super.exchange.timeout, _on_response_timeout, | ||
| (parameters && parameters->timeout_ms > 0) ? | ||
| parameters->timeout_ms : | ||
| CONFIG_UNICOAP_TIMEOUT_CLIENT_MULTICAST_RESPONSE_MS, | ||
| "client.resp-timeout"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
| unicoap_event_schedule(&memo->super.exchange.timeout, _on_response_timeout, | |
| (parameters && parameters->timeout_ms > 0) ? | |
| parameters->timeout_ms : CONFIG_UNICOAP_TIMEOUT_CLIENT_RESPONSE_MS, | |
| "client.resp-timeout"); | |
| if (!multicast) { | |
| unicoap_event_schedule(&memo->super.exchange.timeout, _on_response_timeout, | |
| (parameters && parameters->timeout_ms > 0) ? | |
| parameters->timeout_ms : | |
| CONFIG_UNICOAP_TIMEOUT_CLIENT_RESPONSE_MS, | |
| "client.resp-timeout"); | |
| } | |
| else if (CONFIG_UNICOAP_TIMEOUT_CLIENT_MULTICAST_RESPONSE_MS > 0) { | |
| unicoap_event_schedule(&memo->super.exchange.timeout, _on_response_timeout, | |
| (parameters && parameters->timeout_ms > 0) ? | |
| parameters->timeout_ms : | |
| CONFIG_UNICOAP_TIMEOUT_CLIENT_MULTICAST_RESPONSE_MS, | |
| "client.resp-timeout"); | |
| } | |
| } | |
| uint32_t timeout_ms = | |
| (parameters && parameters->timeout_ms > 0) ? parameters->timeout_ms : | |
| (multicast) ? CONFIG_UNICOAP_TIMEOUT_CLIENT_MULTICAST_RESPONSE_MS : | |
| CONFIG_UNICOAP_TIMEOUT_CLIENT_RESPONSE_MS; | |
| if (timeout_ms > 0) { | |
| unicoap_event_schedule(&memo->super.exchange.timeout, _on_response_timeout, | |
| timeout_ms, "client.resp-timeout"); | |
| } | |
| } | |
is timeout_ms == 0 even a sensible value?
There was a problem hiding this comment.
timeout == 0 is used to disable the timeout, see #22484 (comment).
There was a problem hiding this comment.
Oh I see. That's missing in the documentation of the config value right now. Also what does it mean to be disabled? Wouldn't the client then be unable to free up any resources it may have used for the transmission forever? Or does it not need transmissions for multicast anyways?
I could think of DTLS sessions e.g.. But anyways, how about interplay with DTLS for multicast? Probably just not supported? Is is caught somewhere right now?
There was a problem hiding this comment.
That's missing in the documentation of the config value right now
Wouldn't the client then be unable to free up any resources it may have used for the transmission forever?
The user can still free up the resource by manually calling unicoap_cancel_request. This was discussed with @carl-tud out-of-band before.
But anyways, how about interplay with DTLS for multicast? Probably just not supported? Is is caught somewhere right now?
Per RFC 7390, the DTLS- based approach for CoAP is only for unicast and does not support group security features. Draft draft-ietf-core-groupcomm-bis specifies Group OSCORE as the default.
For this PR I think it's out of scope, but I'll add a check to catch the DTLS case.
|
|
||
| if (endpoint && !unicoap_endpoint_is_equal(&memo->super.endpoint, | ||
| endpoint) && !_is_multicast(memo)) { | ||
| continue; | ||
| } | ||
|
|
||
| if (token_length == sizeof(memo->token) && memcmp(memo->token, token, token_length) == 0) { | ||
| return memo; | ||
| } |
There was a problem hiding this comment.
Not sure I understand that reasoning on the first glance. Could you elaborate?
I would expect: if multicast, just match on token. otherwise check endpoint and token. or does endpoint_is_equal does some special handling of multicast adresses?
There was a problem hiding this comment.
I would expect: if multicast, just match on token. otherwise check endpoint. or does
endpoint_is_equaldoes some special handling of multicast adresses?
Yes, that's what this implements: if the endpoint doesn't match and it's not multicast =>continue. But will move the is_multicast check to the beginning so that we check that before comparing the endpoint.
There was a problem hiding this comment.
then i would propose:
if ((multicast || (endpoint && is_equal)) && token-comparison)
in a single if-clause to avoid the continue?
There was a problem hiding this comment.
That's how it was before. I personally find these very long multi-line if-clauses harder to read and understand. But I don't feel strongly about it. Will change it back.
| if (!_is_multicast(memo)) { | ||
| unicoap_event_cancel(&memo->super.exchange.timeout); |
There was a problem hiding this comment.
maybe add a comment why we don't cancel for multicast.
Apply suggestions from code review Co-authored-by: mguetschow <mikolai.guetschow@tu-dresden.de>
Contribution description
WIP group communication support for unicoap.
Group communication is based on IP multicast, with non-confirmable messages. Most of it works already out-of-the-box using the gnrc IP multicast support. Two main changes required:
Testing procedure
Tested successfully with one client and two servers using the
unicoap_{client,server}example 1 with adafruit express boards:ifconfig 8 add ff02::fdunicoap get coap://[ff02::fd]/greeting?name=RIOTerIssues/PRs references
Small addition to #22266.
Tracking issue: #21389
Declaration of AI-Tools / LLMs usage:
AI-Tools / LLMs that were used are:
Footnotes
+ added shell for the server ↩