net/unicoap: Unified and Modular CoAP stack: Messaging and Minimal Server (pt 2) #21582
Conversation
da014c7 to
4d00949
Compare
|
@LasseRosenow RIOT/sys/include/net/unicoap/transport.h Lines 344 to 355 in 4d00949 |
| @@ -0,0 +1,2 @@ | |||
| CONFIG_UNICOAP_DEBUG_LOGGING=y | |||
There was a problem hiding this comment.
maybe turning this off would also shrink the Makefile.ci.
There was a problem hiding this comment.
yes this a good idea.
There was a problem hiding this comment.
Is there a way I can let the CI generate the Makefile.ci for me? I'm asking because this is taking me ages locally @crasbe
There was a problem hiding this comment.
Yes and no, you can let the CI do a "full build" with "no fast fail" and extract the failed boards from the list. However due to our limited CI resources currently, that takes about 6 hours.
Unfortunately my Thinkpad is still at work (I'm on vacation) and our skyleaf server is offline, so I can't really generate it for you.
There was a problem hiding this comment.
Okay, thanks, good to know. I don’t really need it atm. I guess now I have an excuse to buy more expensive hardware urgh
|
|
||
| If the built-in | ||
| behavior does not fit your scenario, you can customize the matching algorithm per listener. | ||
| To do this, set the @ref unicoap_listener_t.request_matcher property. By default, if this property |
There was a problem hiding this comment.
| To do this, set the @ref unicoap_listener_t.request_matcher property. By default, if this property | |
| To do this, set the @ref unicoap_listener_t::request_matcher property. By default, if this property |
|
|
||
| ### Responding | ||
|
|
||
| There are multiple techniques for responding. |
There was a problem hiding this comment.
Ah, here is the explanation I was asking for in the guide / example code. Maybe just refer to here then.
|
|
||
| 2. **Call send**: Initialize a response message with the status code, and | ||
| optionally, also payload and options. You can repurpose the memory used by the `message` | ||
| parameter for the response, but you must not write into the payload buffer of mutate options. |
There was a problem hiding this comment.
| parameter for the response, but you must not write into the payload buffer of mutate options. | |
| parameter for the response, but you must not write into the payload buffer or mutate options. |
| You may also directly return the result of @ref unicoap_send_response. | ||
| Please be aware any processing performed inside this handler is executed in the server's | ||
| processing loop and will thus block it. Given an expensive operation, we encourage you to switch | ||
| to the third technique. Nevertheless, this should be the preferred response technique for all |
There was a problem hiding this comment.
| to the third technique. Nevertheless, this should be the preferred response technique for all | |
| to the third technique. Nevertheless, this should be the preferred response technique for most |
?
|
|
||
|
|
||
| @remark | ||
| `unicoap` runs a CoAP server for you in the background. Each driver asynchronously forwards packets |
There was a problem hiding this comment.
...if it is not disabled via the config.
There was a problem hiding this comment.
A first look-through. Mikolai already commented about the indentation of the preprocessor commands, but I only saw that once I went through it 😅
There are probably some functions that slipped through, but I'll do a second round when most of the comments are resolved. It's becoming a bit crowded.
| * @returns Null-terminated transport description, such as `"UDP"` or `"DTLS"` | ||
| * @returns `"?"` if protocol number is unknown |
There was a problem hiding this comment.
| * @returns Null-terminated transport description, such as `"UDP"` or `"DTLS"` | |
| * @returns `"?"` if protocol number is unknown | |
| * @retval Null-terminated transport description, such as `"UDP"` or `"DTLS"` | |
| * @retval `"?"` if protocol number is unknown |
| * @return Zero if resource is found and matcher determined request matches resource definition | ||
| * @return Non-zero CoAP status code appropriate for the mismatch |
There was a problem hiding this comment.
| * @return Zero if resource is found and matcher determined request matches resource definition | |
| * @return Non-zero CoAP status code appropriate for the mismatch | |
| * @retval Zero if resource is found and matcher determined request matches resource definition | |
| * @retval Non-zero CoAP status code appropriate for the mismatch |
| @@ -1,4 +1,4 @@ | |||
| @defgroup net_unicoap_drivers_dtls CoAP over DTLS Driver | |||
| g@defgroup net_unicoap_drivers_dtls CoAP over DTLS Driver | |||
There was a problem hiding this comment.
| g@defgroup net_unicoap_drivers_dtls CoAP over DTLS Driver | |
| @defgroup net_unicoap_drivers_dtls CoAP over DTLS Driver |
| uint8_t* pdu; | ||
|
|
||
| /** | ||
| * @brief Size of @ref unicoap_messaging_state_udp_dtls_t.pdu |
There was a problem hiding this comment.
| * @brief Size of @ref unicoap_messaging_state_udp_dtls_t.pdu | |
| * @brief Size of @ref unicoap_messaging_state_udp_dtls_t::pdu |
or rather?
| * @brief Size of @ref unicoap_messaging_state_udp_dtls_t.pdu | |
| * @brief Size of @ref transmission_t::pdu |
| uint8_t remaining_retransmissions : 5; | ||
|
|
||
| bool is_used : 1; | ||
| } transmission_t; |
There was a problem hiding this comment.
| } transmission_t; | |
| } _transmission_t; |
to signal this is a private type.
| /** @brief Message ID unicoap is going to use for the next outbound request or response */ | ||
| atomic_uint next_message_id; | ||
|
|
||
| # if CONFIG_UNICOAP_CARBON_COPIES_MAX > 0 |
There was a problem hiding this comment.
| # if CONFIG_UNICOAP_CARBON_COPIES_MAX > 0 | |
| #if CONFIG_UNICOAP_CARBON_COPIES_MAX > 0 |
and in all other places
|
|
||
| typedef struct { | ||
| /** @brief Message ID unicoap is going to use for the next outbound request or response */ | ||
| atomic_uint next_message_id; |
There was a problem hiding this comment.
maybe document from which thread contexts this is supposed to be accessed that warrants it being an atomic_uint.
| UNICOAP_PREPROCESSING_ERROR_UNSUPPORTED = -0x1002, | ||
|
|
||
| /** | ||
| * @brief The given message is unknown and cannot be processed |
There was a problem hiding this comment.
| * @brief The given message is unknown and cannot be processed | |
| * @brief The given message is unexpected and cannot be processed |
| UNICOAP_PREPROCESSING_ERROR_RESPONSE_UNEXPECTED = -0x1004, | ||
|
|
||
| /** | ||
| * @brief Unknown notification, no known client exchange (observation) |
There was a problem hiding this comment.
| * @brief Unknown notification, no known client exchange (observation) | |
| * @brief Unexpected notification, no known client exchange (observation) |
| * @brief The given message is unknown and cannot be processed | ||
| * | ||
| * There's no known client exchange running. |
| * | ||
| * @param[in] endpoint Remote endpoint to release buffers for | ||
| */ | ||
| void unicoap_exchange_forget_endpoint(const unicoap_endpoint_t* endpoint); |
There was a problem hiding this comment.
How does this differ from void unicoap_exchange_forget_failed_endpoint(const unicoap_endpoint_t* endpoint); above?
There was a problem hiding this comment.
My mistake, same fucntions
| int unicoap_messaging_process_rfc7252(const uint8_t* pdu, size_t size, bool truncated, | ||
| unicoap_packet_t* packet); |
There was a problem hiding this comment.
Is this missing a "Mark: extension point driver"?
| { | ||
| (void)event; | ||
| sock_dtls_session_t session; | ||
| if (dsm_get_num_available_slots() < CONFIG_UNICOAP_DTLS_MINIMUM_AVAILABLE_SESSIONS) { |
There was a problem hiding this comment.
I still find the naming of the constant confusing.
| sizeof(unicoap_receiver_buffer), | ||
| CONFIG_UNICOAP_DTLS_HANDSHAKE_TIMEOUT_MS); | ||
| _UNICOAP_DEBUG_HEX(unicoap_receiver_buffer, res); | ||
| DEBUG("^ this really looks like a PDU...\n"); |
There was a problem hiding this comment.
how could you tell? :P (consider removing)
There was a problem hiding this comment.
True, this is from when DTLS stopped working during some hack n ack
| dsm_state_t prev_state = dsm_store(sock, &session, SESSION_STATE_ESTABLISHED, false); | ||
|
|
||
| /* If session is already stored and the state was SESSION_STATE_HANDSHAKE | ||
| before, the handshake has been initiated internally by a gcoap client request |
There was a problem hiding this comment.
| before, the handshake has been initiated internally by a gcoap client request | |
| before, the handshake has been initiated internally by a unicoap client request |
is this still true though?
There was a problem hiding this comment.
we shall not relapse
|
|
||
| unicoap_endpoint_t endpoint = { .proto = UNICOAP_PROTO_DTLS }; | ||
| sock_dtls_session_get_udp_ep(&session, unicoap_endpoint_get_dtls(&endpoint)); | ||
| unicoap_exchange_forget_failed_endpoint(&endpoint); |
There was a problem hiding this comment.
Why failed? Couldn't it just be a finished connection?
| unicoap_packet_t packet = { .remote = &remote, .dtls_session = &session }; | ||
|
|
||
| #if IS_ACTIVE(CONFIG_UNICOAP_GET_LOCAL_ENDPOINTS) | ||
| unicoap_endpoint_t local = { .proto = UNICOAP_PROTO_UDP }; |
There was a problem hiding this comment.
why UDP? if on purpose, please add an explaining comment.
| { | ||
| /* generate initial notification value */ | ||
| return unicoap_options_set_observe( | ||
| options, (ztimer_now(ZTIMER_MSEC) >> UNICOAP_OBS_TICK_EXPONENT) & 0xFFFFFF); |
There was a problem hiding this comment.
| options, (ztimer_now(ZTIMER_MSEC) >> UNICOAP_OBS_TICK_EXPONENT) & 0xFFFFFF); | |
| options, (ztimer_now(UNICOAP_CLOCK) >> UNICOAP_OBS_TICK_EXPONENT) & 0xFFFFFF); |
? Also, why is this part of state.c?
| unicoap_link_encoder_ctx_t ctx; | ||
| ctx.content_format = UNICOAP_FORMAT_LINK; | ||
| /* indicate initial link for the list */ | ||
| ctx.uninitialized = true; |
There was a problem hiding this comment.
| unicoap_link_encoder_ctx_t ctx; | |
| ctx.content_format = UNICOAP_FORMAT_LINK; | |
| /* indicate initial link for the list */ | |
| ctx.uninitialized = true; | |
| unicoap_link_encoder_ctx_t ctx = { | |
| .content_format = UNICOAP_FORMAT_LINK, | |
| /* indicate initial link for the list */ | |
| .uninitialized = true, | |
| } |
why not?
| continue; | ||
| } | ||
| ssize_t res; | ||
| if (out) { |
There was a problem hiding this comment.
buffer is not documented to be allowed as NULL.
| /* The Uri-Path options are longer than | ||
| * CONFIG_UNIOCOAP_PATH_SIZE_MAX, and thus do not match anything | ||
| * that could be found by this handler. */ | ||
| SERVER_DEBUG("could not copy Uri-Path\n"); | ||
| return UNICOAP_STATUS_PATH_NOT_FOUND; |
There was a problem hiding this comment.
Could we instead fall-back to the less efficient options parsing approach?
There was a problem hiding this comment.
but that would scale horribly if you have multiple resources wouldn’t it
why would you prefer the less efficient version instead
There was a problem hiding this comment.
Sure, but only if the path is too long to fit in the buffer.
| (*resource_ptr)->flags & UNICOAP_RESOURCE_FLAG_MATCH_SUBTREE ? "/**" : "", | ||
| unicoap_string_from_method(unicoap_request_get_method(packet->message))); | ||
| break; | ||
| case UNICOAP_STATUS_PATH_NOT_FOUND: |
There was a problem hiding this comment.
this cannot happen, since ret is only set to res in case of UNICOAP_STATUS_METHOD_NOT_ALLOWED.
| ``` | ||
|
|
||
| where `fe80::c0:ff:ee` is the IPv6 address of the RIOT interface behind the tap interface. | ||
| If you run the `examples/default` shell application and type `ifconfig` you should see a list of |
There was a problem hiding this comment.
| If you run the `examples/default` shell application and type `ifconfig` you should see a list of | |
| If you run the `examples/basic/default` shell application and type `ifconfig` you should see a list of |
also, that one didn't show me the IP address, I used examples/networking/gnrc/gnrc_networking for this purpose.
|
One test datapoint: |
|
@mguetschow You forgot the |
Okay, maybe this is stupid. |
But I tried to get the root resource which does not require query parameters: (TL;DR: something in the resource matching is off) |
| ``` | ||
| This will compile and run the application. | ||
|
|
||
| Then, create a tap interface (to which RIOT will connect): |
There was a problem hiding this comment.
Don't you need to create the tap interface first?
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?
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 not available yet).