Expose OpenThread CoAP implementation#67
Conversation
| .define("OT_SLAAC", "ON") | ||
| .define("OT_ECDSA", "ON") | ||
| .define("OT_PING_SENDER", "ON") | ||
| .define("OT_COAP", "ON") |
There was a problem hiding this comment.
These have to be enabled only if the relevant feature (coap) is enabled as well.
ivmarkov
left a comment
ivmarkov
left a comment
There was a problem hiding this comment.
Thanks! (And sorry for the huge delay in reviewing!)
I like it a lot because it follows the already-established patterns in the library.
A few things we need to address before merge:
Cached .a libraries
The "cached" .a libraries should be compiled without COAP support for now (to maintain smaller size of the library).
- This means you should only enable COAP in the C OpenThread lib if the "coap" feature is enabled.
- This also means changes to
openthread-sys'sbuild.rsfile so that we enter this branch if the "coap" feature is enabled. - Note that this mechanism would come very handy for the future, if we want to also enable TCP support, or FTD.
- Note also that - since some time - the OpenThread lib compiles by default with clang and does NOT need the sysroot of your MCU, the fact that the pre-compiled binaries are not used will not be a pain for the users as long as they have "some" clang installed already
Error handling
Not the end of the world, but if you could improve a bit the error handling in the "C" callbacks, as suggested throughout the code review
Builder pattern
Also as suggested inline in the code review.
A bit more docu on the public types
... so that your PR matches the level of documentation of the existing code-base.
| slot: usize, | ||
| } | ||
|
|
||
| impl<'a> CoapResource<'a> { |
There was a problem hiding this comment.
Given that this is the main type exposed to the user, I would expect a bit more documentation - first of all on the type itself, and then what each method is supposed to do.
Given that COAP is much less popular/known compared to e.g. UDP, TCP, HTTP I think having some basic documentation is even more important.
| } | ||
| } | ||
|
|
||
| pub struct CoapObserve<'a> { |
There was a problem hiding this comment.
Ditto here - please document a bit.
| } | ||
| } | ||
|
|
||
| impl<'a> OpenThread<'a> { |
There was a problem hiding this comment.
All of this API is also completely undocumented.
| } | ||
| } | ||
|
|
||
| #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)] |
| if coap_type_raw == otCoapType_OT_COAP_TYPE_CONFIRMABLE { | ||
| let ack = unsafe { otCoapNewMessage(instance, ptr::null()) }; | ||
| if !ack.is_null() { | ||
| let init_err = unsafe { |
There was a problem hiding this comment.
Can we make the error handling a bit less verbose?
The if init_err == otError_OT_ERROR_NONE { ... } else { <free-the-damn-message-and-warn> } is laboriously repeated in this method, and other methods further below.
Perhaps you can create a simple message wrapper type that frees the message it contains in its drop fn?
Also, you might want to create a sibling fn of plat_c_request_handler that has a proper Result<whatever, OtError> so that you can use the ergonomic ? syntax for early returns on error, and the RAII pattern suggested above with the message wrapper type?
| unreachable!(); | ||
| }; | ||
|
|
||
| if result != otError_OT_ERROR_NONE || message.is_null() { |
There was a problem hiding this comment.
Here also - the whole control flow is difficult to read because you can't use '?', which is fixable by just having plat_c_response_handler delegating to something which returns Result<whatever, Error>?
| let res = unsafe { | ||
| otCoapMessageSetToken(msg, request.token.as_ptr(), request.token.len() as u8) | ||
| }; | ||
| if res != otError_OT_ERROR_NONE { |
There was a problem hiding this comment.
Here again - can't use ? because there is no RAII in the first place and the otMessageFree is repeated multiple times.
| } | ||
|
|
||
| let err = unsafe { otCoapMessageAppendObserveOption(msg, 0) }; | ||
| if err != otError_OT_ERROR_NONE { |
|
|
||
| /// Create a new OpenThread instance with support for the native OpenThread CoAP server. | ||
| #[cfg(feature = "coap")] | ||
| pub fn new_with_coap<const COAP_RES: usize, const COAP_REQ: usize, const COAP_RX_SZ: usize>( |
There was a problem hiding this comment.
I think we are now pushing the new_with_this_and_that pattern a bit to the extreme :-) In fact, I also over-used it before, with the appearance of SRP so it smells since before your COAP contribution.
Would you be open to implement a builder instead?
I..e.
impl<'a> OpenThreadBuilder<'a> {
pub fn new(ieee_eui64, rng, settings, resources) -> Self { ... }
pub fn with_udp(self, udp_resources: .&'a ..) -> Self { ... }
pub fn with_srp(self, srp_resources: &'a ...) -> Self { ... }
pub fn with_coap(self, coap_resources: &'a ...) -> Self { ... }
pub fn create(self) -> OpenThread<'a> {
...
}
}
|
Forgot to say - would be really nice to have a COAP example too, even if for the nrf52 initially only. |
|
I have been out of town for a few weeks, so I will have a busy week getting back into it at work. But I'll hopefully have time near the end of the week to address your feedback. Thanks for the review :) |
Np - but maybe please rebase your branch first on top of latest |
2 participants
This exposes the CoAP implementation from OpenThread. I have only tested it with an nRF52840 since that's the only supported device I have handy. I'm not sure what the process is to update the pre-compiled artifacts for the repo.