From 9dfba7659e1c5233a038da11cc74dd7cb4a23ee7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hubert=20Mi=C5=9B?= Date: Mon, 23 Sep 2024 21:49:06 +0200 Subject: [PATCH 01/19] [nrf fromtree] net: coap: add no-response option definition and values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Constrained application can require to avoid waiting for response from a server. CoAP No-response option defined in RFC 7967 allows to do that by suppressing selected response types. Signed-off-by: Hubert Miś (cherry picked from commit 70c9bf1da7049432933d27a1617c59de5cbef423) --- include/zephyr/net/coap.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/include/zephyr/net/coap.h b/include/zephyr/net/coap.h index 11d2c3711f2..bf0d86cfaf7 100644 --- a/include/zephyr/net/coap.h +++ b/include/zephyr/net/coap.h @@ -64,6 +64,7 @@ enum coap_option_num { COAP_OPTION_PROXY_SCHEME = 39, /**< Proxy-Scheme */ COAP_OPTION_SIZE1 = 60, /**< Size1 */ COAP_OPTION_ECHO = 252, /**< Echo (RFC 9175) */ + COAP_OPTION_NO_RESPONSE = 258, /**< No-Response (RFC 7967) */ COAP_OPTION_REQUEST_TAG = 292 /**< Request-Tag (RFC 9175) */ }; @@ -222,6 +223,22 @@ enum coap_content_format { COAP_CONTENT_FORMAT_APP_CBOR = 60 /**< application/cbor */ }; +/** + * @brief Set of No-Response option values for CoAP. + * + * To be used when encoding or decoding a No-Response option defined + * in RFC 7967. + */ +enum coap_no_response { + COAP_NO_RESPONSE_SUPPRESS_2_XX = 0x02, + COAP_NO_RESPONSE_SUPPRESS_4_XX = 0x08, + COAP_NO_RESPONSE_SUPPRESS_5_XX = 0x10, + + COAP_NO_RESPONSE_SUPPRESS_ALL = COAP_NO_RESPONSE_SUPPRESS_2_XX | + COAP_NO_RESPONSE_SUPPRESS_4_XX | + COAP_NO_RESPONSE_SUPPRESS_5_XX, +}; + /** @cond INTERNAL_HIDDEN */ /* block option helper */ From 718954688dde72e78f4a5783d43bf3b8dacbb019 Mon Sep 17 00:00:00 2001 From: Francois Gervais Date: Tue, 24 Sep 2024 21:26:44 -0400 Subject: [PATCH 02/19] [nrf fromtree] net: lib: coap_client: wait for all acknowledgements This commit makes sure we continue to wait for extra confirmations even after the request is done so we can handle duplicate confirmations if any. Detailed description: rfc7252#section-4.5 specifies that: "The recipient SHOULD acknowledge each duplicate copy of a Confirmable message". So if, for example, the client sends to a multicast destination address, the server will get multiple requests and will confirm all of them. Without this commit, the client will set the request to done after receiving the first answer. From here the request object will be marked as free and the duplicate acknowledgements will stay buffered in the network stack. Once the client tries to send a new request, it will unbuffer those duplicate acknowledgements but now the request object is unallocated so the client won't be able to handle those acknowledgements as duplicates. It will instead treat it as an unexpected ACK. To work around this issue, rfc7252#section-4.8.2 states that: "EXCHANGE_LIFETIME is the time from starting to send a Confirmable message to the time when an acknowledgement is no longer expected, i.e., message-layer information about the message exchange can be purged." Keeping the request object allocated for EXCHANGE_LIFETIME ensures that duplicate acknowledgements can be handled accordingly. This commit adds a basic implementation of what is stated in the RFC. EXCHANGE_LIFETIME has been arbitrarily set to 3 * ACK_TIMEOUT which seems more reasonable than the 247 seconds stated in the RFC. Signed-off-by: Francois Gervais (cherry picked from commit bfed8d0966993c4218f8a85fd41e724831ba2657) --- subsys/net/lib/coap/coap_client.c | 53 +++++++++++++++++++++++++++---- 1 file changed, 47 insertions(+), 6 deletions(-) diff --git a/subsys/net/lib/coap/coap_client.c b/subsys/net/lib/coap/coap_client.c index 7c49a90981b..e841a1998dc 100644 --- a/subsys/net/lib/coap/coap_client.c +++ b/subsys/net/lib/coap/coap_client.c @@ -16,6 +16,7 @@ LOG_MODULE_DECLARE(net_coap, CONFIG_COAP_LOG_LEVEL); #define COAP_VERSION 1 #define COAP_SEPARATE_TIMEOUT 6000 #define COAP_PERIODIC_TIMEOUT 500 +#define COAP_EXCHANGE_LIFETIME_FACTOR 3 #define BLOCK1_OPTION_SIZE 4 #define PAYLOAD_MARKER_SIZE 1 @@ -86,6 +87,21 @@ static int coap_client_schedule_poll(struct coap_client *client, int sock, return 0; } +static bool exchange_lifetime_exceeded(struct coap_client_internal_request *internal_req) +{ + int64_t time_since_t0, exchange_lifetime; + + if (coap_header_get_type(&internal_req->request) == COAP_TYPE_NON_CON) { + return true; + } + + time_since_t0 = k_uptime_get() - internal_req->pending.t0; + exchange_lifetime = + (internal_req->pending.params.ack_timeout * COAP_EXCHANGE_LIFETIME_FACTOR); + + return time_since_t0 > exchange_lifetime; +} + static bool has_ongoing_request(struct coap_client *client) { for (int i = 0; i < CONFIG_COAP_CLIENT_MAX_REQUESTS; i++) { @@ -97,10 +113,23 @@ static bool has_ongoing_request(struct coap_client *client) return false; } +static bool has_ongoing_exchange(struct coap_client *client) +{ + for (int i = 0; i < CONFIG_COAP_CLIENT_MAX_REQUESTS; i++) { + if (client->requests[i].request_ongoing == true || + !exchange_lifetime_exceeded(&client->requests[i])) { + return true; + } + } + + return false; +} + static struct coap_client_internal_request *get_free_request(struct coap_client *client) { for (int i = 0; i < CONFIG_COAP_CLIENT_MAX_REQUESTS; i++) { - if (client->requests[i].request_ongoing == false) { + if (client->requests[i].request_ongoing == false && + exchange_lifetime_exceeded(&client->requests[i])) { return &client->requests[i]; } } @@ -110,13 +139,24 @@ static struct coap_client_internal_request *get_free_request(struct coap_client static bool has_ongoing_requests(void) { - bool has_requests = false; + for (int i = 0; i < num_clients; i++) { + if (has_ongoing_request(clients[i])) { + return true; + } + } + return false; +} + +static bool has_ongoing_exchanges(void) +{ for (int i = 0; i < num_clients; i++) { - has_requests |= has_ongoing_request(clients[i]); + if (has_ongoing_exchange(clients[i])) { + return true; + } } - return has_requests; + return false; } static enum coap_block_size coap_client_default_block_size(void) @@ -618,7 +658,8 @@ static struct coap_client_internal_request *get_request_with_token( response_tkl = coap_header_get_token(resp, response_token); for (int i = 0; i < CONFIG_COAP_CLIENT_MAX_REQUESTS; i++) { - if (client->requests[i].request_ongoing) { + if (client->requests[i].request_ongoing || + !exchange_lifetime_exceeded(&client->requests[i])) { if (client->requests[i].request_tkl != response_tkl) { continue; } @@ -921,7 +962,7 @@ static void coap_client_recv(void *coap_cl, void *a, void *b) } /* There are more messages coming */ - if (has_ongoing_requests()) { + if (has_ongoing_exchanges()) { continue; } else { idle: From 9af5bcef783c4fccf91ccb5400c0ccc593e047c3 Mon Sep 17 00:00:00 2001 From: Francois Gervais Date: Tue, 24 Sep 2024 18:51:56 -0400 Subject: [PATCH 03/19] [nrf fromtree] tests: coap_client: wait enough for requests to be unallocated As confirmable requests will stay allocated for (3 * ACK_TIMEOUT), we need to adjust the timings so all requests are unallocated by the end of the test. Signed-off-by: Francois Gervais (cherry picked from commit 13cad59542d15e57fd4a70dfc9354b7251a8b0f4) --- tests/net/lib/coap_client/CMakeLists.txt | 2 +- tests/net/lib/coap_client/src/main.c | 18 ++++-------------- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/tests/net/lib/coap_client/CMakeLists.txt b/tests/net/lib/coap_client/CMakeLists.txt index d99bb3145f9..e12214fc6a9 100644 --- a/tests/net/lib/coap_client/CMakeLists.txt +++ b/tests/net/lib/coap_client/CMakeLists.txt @@ -26,7 +26,7 @@ add_compile_definitions(CONFIG_COAP_CLIENT_MESSAGE_HEADER_SIZE=48) add_compile_definitions(CONFIG_COAP_CLIENT_STACK_SIZE=1024) add_compile_definitions(CONFIG_COAP_CLIENT_THREAD_PRIORITY=10) add_compile_definitions(CONFIG_COAP_LOG_LEVEL=4) -add_compile_definitions(CONFIG_COAP_INIT_ACK_TIMEOUT_MS=200) +add_compile_definitions(CONFIG_COAP_INIT_ACK_TIMEOUT_MS=10) add_compile_definitions(CONFIG_COAP_CLIENT_MAX_REQUESTS=2) add_compile_definitions(CONFIG_COAP_CLIENT_MAX_INSTANCES=2) add_compile_definitions(CONFIG_COAP_MAX_RETRANSMIT=4) diff --git a/tests/net/lib/coap_client/src/main.c b/tests/net/lib/coap_client/src/main.c index 665aab30892..b487bdf7d5a 100644 --- a/tests/net/lib/coap_client/src/main.c +++ b/tests/net/lib/coap_client/src/main.c @@ -377,7 +377,6 @@ ZTEST(coap_client, test_get_request) zassert_true(ret >= 0, "Sending request failed, %d", ret); set_socket_events(ZSOCK_POLLIN); - k_sleep(K_MSEC(5)); k_sleep(K_MSEC(100)); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); } @@ -406,7 +405,7 @@ ZTEST(coap_client, test_resend_request) LOG_INF("Send request"); ret = coap_client_req(&client, 0, &address, &client_request, NULL); zassert_true(ret >= 0, "Sending request failed, %d", ret); - k_sleep(K_MSEC(300)); + k_sleep(K_MSEC(15)); set_socket_events(ZSOCK_POLLIN); k_sleep(K_MSEC(100)); @@ -440,7 +439,6 @@ ZTEST(coap_client, test_echo_option) zassert_true(ret >= 0, "Sending request failed, %d", ret); set_socket_events(ZSOCK_POLLIN); - k_sleep(K_MSEC(5)); k_sleep(K_MSEC(100)); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); } @@ -471,7 +469,6 @@ ZTEST(coap_client, test_echo_option_next_req) zassert_true(ret >= 0, "Sending request failed, %d", ret); set_socket_events(ZSOCK_POLLIN); - k_sleep(K_MSEC(5)); k_sleep(K_MSEC(100)); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); @@ -486,7 +483,6 @@ ZTEST(coap_client, test_echo_option_next_req) zassert_true(ret >= 0, "Sending request failed, %d", ret); set_socket_events(ZSOCK_POLLIN); - k_sleep(K_MSEC(5)); k_sleep(K_MSEC(100)); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); } @@ -540,7 +536,6 @@ ZTEST(coap_client, test_send_large_data) zassert_true(ret >= 0, "Sending request failed, %d", ret); set_socket_events(ZSOCK_POLLIN); - k_sleep(K_MSEC(5)); k_sleep(K_MSEC(100)); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); } @@ -574,8 +569,8 @@ ZTEST(coap_client, test_no_response) ret = coap_client_req(&client, 0, &address, &client_request, ¶ms); zassert_true(ret >= 0, "Sending request failed, %d", ret); - k_sleep(K_MSEC(300)); + k_sleep(K_MSEC(700)); zassert_equal(last_response_code, -ETIMEDOUT, "Unexpected response"); } @@ -605,9 +600,7 @@ ZTEST(coap_client, test_separate_response) zassert_true(ret >= 0, "Sending request failed, %d", ret); set_socket_events(ZSOCK_POLLIN); - k_sleep(K_MSEC(5)); k_sleep(K_MSEC(100)); - zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); } @@ -638,11 +631,9 @@ ZTEST(coap_client, test_multiple_requests) ret = coap_client_req(&client, 0, &address, &client_request, NULL); zassert_true(ret >= 0, "Sending request failed, %d", ret); - k_sleep(K_MSEC(5)); k_sleep(K_MSEC(100)); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); - k_sleep(K_MSEC(5)); k_sleep(K_MSEC(100)); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); } @@ -676,9 +667,8 @@ ZTEST(coap_client, test_unmatching_tokens) zassert_true(ret >= 0, "Sending request failed, %d", ret); set_socket_events(ZSOCK_POLLIN); - k_sleep(K_MSEC(1)); - k_sleep(K_MSEC(1)); + k_sleep(K_MSEC(2)); clear_socket_events(); - k_sleep(K_MSEC(500)); + k_sleep(K_MSEC(700)); zassert_equal(last_response_code, -ETIMEDOUT, "Unexpected response"); } From 79cad384d2352be4d6d69d919d3427c452e28e7b Mon Sep 17 00:00:00 2001 From: Pieter De Gendt Date: Mon, 30 Sep 2024 13:55:00 +0200 Subject: [PATCH 04/19] [nrf fromtree] include: zephyr: net: coap_service: Use _CONCAT expansion Passing MACRO arguments that need expansion require _CONCAT. Signed-off-by: Pieter De Gendt (cherry picked from commit 476c12fddec44c372d7369aa8ee7c255b31157c1) --- include/zephyr/net/coap_service.h | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/include/zephyr/net/coap_service.h b/include/zephyr/net/coap_service.h index 718d1fb1cfc..f5bf3f8405b 100644 --- a/include/zephyr/net/coap_service.h +++ b/include/zephyr/net/coap_service.h @@ -59,7 +59,7 @@ struct coap_service { }; #define __z_coap_service_define(_name, _host, _port, _flags, _res_begin, _res_end) \ - static struct coap_service_data coap_service_data_##_name = { \ + static struct coap_service_data _CONCAT(coap_service_data_, _name) = { \ .sock_fd = -1, \ }; \ const STRUCT_SECTION_ITERABLE(coap_service, _name) = { \ @@ -69,7 +69,7 @@ struct coap_service { .flags = _flags, \ .res_begin = (_res_begin), \ .res_end = (_res_end), \ - .data = &coap_service_data_##_name, \ + .data = &_CONCAT(coap_service_data_, _name), \ } /** @endcond */ @@ -111,8 +111,8 @@ struct coap_service { * @param _service Name of the associated service. */ #define COAP_RESOURCE_DEFINE(_name, _service, ...) \ - STRUCT_SECTION_ITERABLE_ALTERNATE(coap_resource_##_service, coap_resource, _name) \ - = __VA_ARGS__ + STRUCT_SECTION_ITERABLE_ALTERNATE(_CONCAT(coap_resource_, _service), coap_resource, \ + _name) = __VA_ARGS__ /** * @brief Define a CoAP service with static resources. @@ -132,11 +132,11 @@ struct coap_service { * @param _flags Configuration flags @see @ref COAP_SERVICE_FLAGS. */ #define COAP_SERVICE_DEFINE(_name, _host, _port, _flags) \ - extern struct coap_resource _CONCAT(_coap_resource_##_name, _list_start)[]; \ - extern struct coap_resource _CONCAT(_coap_resource_##_name, _list_end)[]; \ + extern struct coap_resource _CONCAT(_CONCAT(_coap_resource_, _name), _list_start)[]; \ + extern struct coap_resource _CONCAT(_CONCAT(_coap_resource_, _name), _list_end)[]; \ __z_coap_service_define(_name, _host, _port, _flags, \ - &_CONCAT(_coap_resource_##_name, _list_start)[0], \ - &_CONCAT(_coap_resource_##_name, _list_end)[0]) + &_CONCAT(_CONCAT(_coap_resource_, _name), _list_start)[0], \ + &_CONCAT(_CONCAT(_coap_resource_, _name), _list_end)[0]) /** * @brief Count the number of CoAP services. @@ -177,7 +177,7 @@ struct coap_service { * @param _it Name of iterator (of type @ref coap_resource) */ #define COAP_RESOURCE_FOREACH(_service, _it) \ - STRUCT_SECTION_FOREACH_ALTERNATE(coap_resource_##_service, coap_resource, _it) + STRUCT_SECTION_FOREACH_ALTERNATE(_CONCAT(coap_resource_, _service), coap_resource, _it) /** * @brief Iterate over all static resources associated with @p _service . From 36024b8d6459f97c1dee5481f4427e7c52a637cc Mon Sep 17 00:00:00 2001 From: Francois Gervais Date: Sat, 28 Sep 2024 08:43:57 -0400 Subject: [PATCH 05/19] [nrf fromtree] tests: coap_client: optimize/reduce sleep time The goal of this commit is to reduce the overall test time by optimizing the time spent sleeping. However while doing so, a few glitches became apparent and those have been fixed as well. 1. The way the ZSOCK_POLLIN event is managed has been modified In most tests, the event would stick for the whole test and this would sometimes results in the client receiving more data than intended which in turn would results in various unexpected warnings and errors. The management of the ZSOCK_POLLIN event has now been mostly moved to the send/receive overrides which better represent how things would happen outside of the test scenario. For example, when sending data, if this send would normally result in receiving some response, the send function sets the ZSOCK_POLLIN event. Then when receiving, we clear the event as the data has been received. There are a few exceptions to this for cases where we need to simulate some specific abnormal behavior. 2. The `messages_needing_response` queue now includes a valid bit The test manages a queue of messages id to be able to respond to the correct id in the receive hooks. It was built in a way that the id 0 would be treated as invalid although it is a valid id. In practice, it would not be an issue in most cases however, it would result in an unexpected behavior if the `test_multiple_requests` test would be run first. To fix this issue in the simplest way, the queue has been changed to uint32_t which gives us 16 extra bits for the management of the queue, 1 of which is used to tell if the entry is valid or not. Signed-off-by: Francois Gervais (cherry picked from commit 736344b31093beb158b65839ce21f09573e20604) --- tests/net/lib/coap_client/src/main.c | 311 +++++++++++++------------- tests/net/lib/coap_client/src/stubs.c | 9 +- 2 files changed, 156 insertions(+), 164 deletions(-) diff --git a/tests/net/lib/coap_client/src/main.c b/tests/net/lib/coap_client/src/main.c index b487bdf7d5a..25c2c58f2fd 100644 --- a/tests/net/lib/coap_client/src/main.c +++ b/tests/net/lib/coap_client/src/main.c @@ -16,87 +16,123 @@ LOG_MODULE_REGISTER(coap_client_test); DEFINE_FFF_GLOBALS; #define FFF_FAKES_LIST(FAKE) +#define LONG_ACK_TIMEOUT_MS 200 +#define MORE_THAN_EXCHANGE_LIFETIME_MS 4 * CONFIG_COAP_INIT_ACK_TIMEOUT_MS +#define MORE_THAN_LONG_EXCHANGE_LIFETIME_MS 4 * LONG_ACK_TIMEOUT_MS +#define MORE_THAN_ACK_TIMEOUT_MS \ + (CONFIG_COAP_INIT_ACK_TIMEOUT_MS + CONFIG_COAP_INIT_ACK_TIMEOUT_MS / 2) + +#define VALID_MESSAGE_ID BIT(31) + static int16_t last_response_code; static const char *test_path = "test"; -static uint16_t messages_needing_response[2]; +static uint32_t messages_needing_response[2]; static struct coap_client client; static char *short_payload = "testing"; static char *long_payload = LOREM_IPSUM_SHORT; +static uint16_t get_next_pending_message_id(void) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(messages_needing_response); i++) { + if (messages_needing_response[i] & VALID_MESSAGE_ID) { + messages_needing_response[i] &= ~VALID_MESSAGE_ID; + return messages_needing_response[i]; + } + } + + return UINT16_MAX; +} + +static void set_next_pending_message_id(uint16_t id) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(messages_needing_response); i++) { + if (!(messages_needing_response[i] & VALID_MESSAGE_ID)) { + messages_needing_response[i] = id; + messages_needing_response[i] |= VALID_MESSAGE_ID; + return; + } + } +} + static ssize_t z_impl_zsock_recvfrom_custom_fake(int sock, void *buf, size_t max_len, int flags, - struct sockaddr *src_addr, socklen_t *addrlen) + struct sockaddr *src_addr, socklen_t *addrlen) { uint16_t last_message_id = 0; LOG_INF("Recvfrom"); - uint8_t ack_data[] = { - 0x68, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 - }; + uint8_t ack_data[] = {0x68, 0x40, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}; - if (messages_needing_response[0] != 0) { - last_message_id = messages_needing_response[0]; - messages_needing_response[0] = 0; - } else { - last_message_id = messages_needing_response[1]; - messages_needing_response[1] = 0; - } + last_message_id = get_next_pending_message_id(); - ack_data[2] = (uint8_t) (last_message_id >> 8); - ack_data[3] = (uint8_t) last_message_id; + ack_data[2] = (uint8_t)(last_message_id >> 8); + ack_data[3] = (uint8_t)last_message_id; memcpy(buf, ack_data, sizeof(ack_data)); + clear_socket_events(); + return sizeof(ack_data); } -static ssize_t z_impl_zsock_sendto_custom_fake(int sock, void *buf, size_t len, - int flags, const struct sockaddr *dest_addr, - socklen_t addrlen) +static ssize_t z_impl_zsock_sendto_custom_fake(int sock, void *buf, size_t len, int flags, + const struct sockaddr *dest_addr, socklen_t addrlen) { uint16_t last_message_id = 0; + uint8_t type; + + last_message_id |= ((uint8_t *)buf)[2] << 8; + last_message_id |= ((uint8_t *)buf)[3]; - last_message_id |= ((uint8_t *) buf)[2] << 8; - last_message_id |= ((uint8_t *) buf)[3]; + type = (((uint8_t *)buf)[0] & 0x30) >> 4; - if (messages_needing_response[0] == 0) { - messages_needing_response[0] = last_message_id; - } else { - messages_needing_response[1] = last_message_id; + set_next_pending_message_id(last_message_id); + LOG_INF("Latest message ID: %d", last_message_id); + + if (type == 0) { + set_socket_events(ZSOCK_POLLIN); } - last_response_code = ((uint8_t *) buf)[1]; + return 1; +} +static ssize_t z_impl_zsock_sendto_custom_fake_no_reply(int sock, void *buf, size_t len, int flags, + const struct sockaddr *dest_addr, + socklen_t addrlen) +{ + uint16_t last_message_id = 0; + + last_message_id |= ((uint8_t *)buf)[2] << 8; + last_message_id |= ((uint8_t *)buf)[3]; + + set_next_pending_message_id(last_message_id); LOG_INF("Latest message ID: %d", last_message_id); return 1; } -static ssize_t z_impl_zsock_sendto_custom_fake_echo(int sock, void *buf, size_t len, - int flags, const struct sockaddr *dest_addr, - socklen_t addrlen) +static ssize_t z_impl_zsock_sendto_custom_fake_echo(int sock, void *buf, size_t len, int flags, + const struct sockaddr *dest_addr, + socklen_t addrlen) { uint16_t last_message_id = 0; struct coap_packet response = {0}; struct coap_option option = {0}; - last_message_id |= ((uint8_t *) buf)[2] << 8; - last_message_id |= ((uint8_t *) buf)[3]; - - if (messages_needing_response[0] == 0) { - messages_needing_response[0] = last_message_id; - } else { - messages_needing_response[1] = last_message_id; - } - - last_response_code = ((uint8_t *) buf)[1]; + last_message_id |= ((uint8_t *)buf)[2] << 8; + last_message_id |= ((uint8_t *)buf)[3]; + set_next_pending_message_id(last_message_id); LOG_INF("Latest message ID: %d", last_message_id); int ret = coap_packet_parse(&response, buf, len, NULL, 0); - if (ret < 0) { LOG_ERR("Invalid data received"); } @@ -108,32 +144,27 @@ static ssize_t z_impl_zsock_sendto_custom_fake_echo(int sock, void *buf, size_t z_impl_zsock_sendto_fake.custom_fake = z_impl_zsock_sendto_custom_fake; + set_socket_events(ZSOCK_POLLIN); + return 1; } static ssize_t z_impl_zsock_sendto_custom_fake_echo_next_req(int sock, void *buf, size_t len, - int flags, const struct sockaddr *dest_addr, - socklen_t addrlen) + int flags, + const struct sockaddr *dest_addr, + socklen_t addrlen) { uint16_t last_message_id = 0; struct coap_packet response = {0}; struct coap_option option = {0}; - last_message_id |= ((uint8_t *) buf)[2] << 8; - last_message_id |= ((uint8_t *) buf)[3]; - - if (messages_needing_response[0] == 0) { - messages_needing_response[0] = last_message_id; - } else { - messages_needing_response[1] = last_message_id; - } - - last_response_code = ((uint8_t *) buf)[1]; + last_message_id |= ((uint8_t *)buf)[2] << 8; + last_message_id |= ((uint8_t *)buf)[3]; + set_next_pending_message_id(last_message_id); LOG_INF("Latest message ID: %d", last_message_id); int ret = coap_packet_parse(&response, buf, len, NULL, 0); - if (ret < 0) { LOG_ERR("Invalid data received"); } @@ -153,6 +184,8 @@ static ssize_t z_impl_zsock_sendto_custom_fake_echo_next_req(int sock, void *buf z_impl_zsock_sendto_fake.custom_fake = z_impl_zsock_sendto_custom_fake; + set_socket_events(ZSOCK_POLLIN); + return 1; } @@ -162,50 +195,17 @@ static ssize_t z_impl_zsock_recvfrom_custom_fake_response(int sock, void *buf, s { uint16_t last_message_id = 0; - static uint8_t ack_data[] = { - 0x48, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 - }; + static uint8_t ack_data[] = {0x48, 0x40, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}; - if (messages_needing_response[0] != 0) { - last_message_id = messages_needing_response[0]; - messages_needing_response[0] = 0; - } else { - last_message_id = messages_needing_response[1]; - messages_needing_response[1] = 0; - } + last_message_id = get_next_pending_message_id(); - ack_data[2] = (uint8_t) (last_message_id >> 8); - ack_data[3] = (uint8_t) last_message_id; + ack_data[2] = (uint8_t)(last_message_id >> 8); + ack_data[3] = (uint8_t)last_message_id; memcpy(buf, ack_data, sizeof(ack_data)); - return sizeof(ack_data); -} - -static ssize_t z_impl_zsock_recvfrom_custom_fake_delayed_response(int sock, void *buf, - size_t max_len, int flags, - struct sockaddr *src_addr, - socklen_t *addrlen) -{ - uint16_t last_message_id = 0; - - static uint8_t ack_data[] = { - 0x68, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 - }; - - if (messages_needing_response[0] != 0) { - last_message_id = messages_needing_response[0]; - messages_needing_response[0] = 0; - } else { - last_message_id = messages_needing_response[1]; - messages_needing_response[1] = 0; - } - - ack_data[2] = (uint8_t) (last_message_id >> 8); - ack_data[3] = (uint8_t) last_message_id; - - memcpy(buf, ack_data, sizeof(ack_data)); - k_sleep(K_MSEC(10)); + clear_socket_events(); return sizeof(ack_data); } @@ -216,20 +216,13 @@ static ssize_t z_impl_zsock_recvfrom_custom_fake_empty_ack(int sock, void *buf, { uint16_t last_message_id = 0; - static uint8_t ack_data[] = { - 0x68, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 - }; + static uint8_t ack_data[] = {0x68, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}; - if (messages_needing_response[0] != 0) { - last_message_id = messages_needing_response[0]; - messages_needing_response[0] = 0; - } else { - last_message_id = messages_needing_response[1]; - messages_needing_response[1] = 0; - } + last_message_id = get_next_pending_message_id(); - ack_data[2] = (uint8_t) (last_message_id >> 8); - ack_data[3] = (uint8_t) last_message_id; + ack_data[2] = (uint8_t)(last_message_id >> 8); + ack_data[3] = (uint8_t)last_message_id; memcpy(buf, ack_data, sizeof(ack_data)); @@ -244,23 +237,18 @@ static ssize_t z_impl_zsock_recvfrom_custom_fake_unmatching(int sock, void *buf, { uint16_t last_message_id = 0; - static uint8_t ack_data[] = { - 0x68, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01 - }; + static uint8_t ack_data[] = {0x68, 0x40, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x01}; - if (messages_needing_response[0] != 0) { - last_message_id = messages_needing_response[0]; - messages_needing_response[0] = 0; - } else { - last_message_id = messages_needing_response[1]; - messages_needing_response[1] = 0; - } + last_message_id = get_next_pending_message_id(); - ack_data[2] = (uint8_t) (last_message_id >> 8); - ack_data[3] = (uint8_t) last_message_id; + ack_data[2] = (uint8_t)(last_message_id >> 8); + ack_data[3] = (uint8_t)last_message_id; memcpy(buf, ack_data, sizeof(ack_data)); + clear_socket_events(); + return sizeof(ack_data); } @@ -275,22 +263,18 @@ static ssize_t z_impl_zsock_recvfrom_custom_fake_echo(int sock, void *buf, size_ 0x00, 0x00, 0x00, 0x00, 0xda, 0xef, 'e', 'c', 'h', 'o', '_', 'v', 'a', 'l', 'u', 'e'}; - if (messages_needing_response[0] != 0) { - last_message_id = messages_needing_response[0]; - messages_needing_response[0] = 0; - } else { - last_message_id = messages_needing_response[1]; - messages_needing_response[1] = 0; - } + last_message_id = get_next_pending_message_id(); - ack_data[2] = (uint8_t) (last_message_id >> 8); - ack_data[3] = (uint8_t) last_message_id; + ack_data[2] = (uint8_t)(last_message_id >> 8); + ack_data[3] = (uint8_t)last_message_id; memcpy(buf, ack_data, sizeof(ack_data)); z_impl_zsock_recvfrom_fake.custom_fake = z_impl_zsock_recvfrom_custom_fake_response; z_impl_zsock_sendto_fake.custom_fake = z_impl_zsock_sendto_custom_fake_echo; + clear_socket_events(); + return sizeof(ack_data); } @@ -305,22 +289,18 @@ static ssize_t z_impl_zsock_recvfrom_custom_fake_echo_next_req(int sock, void *b 0x00, 0x00, 0x00, 0x00, 0xda, 0xef, 'e', 'c', 'h', 'o', '_', 'v', 'a', 'l', 'u', 'e'}; - if (messages_needing_response[0] != 0) { - last_message_id = messages_needing_response[0]; - messages_needing_response[0] = 0; - } else { - last_message_id = messages_needing_response[1]; - messages_needing_response[1] = 0; - } + last_message_id = get_next_pending_message_id(); - ack_data[2] = (uint8_t) (last_message_id >> 8); - ack_data[3] = (uint8_t) last_message_id; + ack_data[2] = (uint8_t)(last_message_id >> 8); + ack_data[3] = (uint8_t)last_message_id; memcpy(buf, ack_data, sizeof(ack_data)); z_impl_zsock_recvfrom_fake.custom_fake = z_impl_zsock_recvfrom_custom_fake_response; z_impl_zsock_sendto_fake.custom_fake = z_impl_zsock_sendto_custom_fake_echo_next_req; + clear_socket_events(); + return sizeof(ack_data); } @@ -333,6 +313,8 @@ static void *suite_setup(void) static void test_setup(void *data) { + int i; + /* Register resets */ DO_FOREACH_FAKE(RESET_FAKE); /* reset common FFF internal structures */ @@ -340,8 +322,12 @@ static void test_setup(void *data) z_impl_zsock_recvfrom_fake.custom_fake = z_impl_zsock_recvfrom_custom_fake; z_impl_zsock_sendto_fake.custom_fake = z_impl_zsock_sendto_custom_fake; - messages_needing_response[0] = 0; - messages_needing_response[1] = 0; + + for (i = 0; i < ARRAY_SIZE(messages_needing_response); i++) { + messages_needing_response[i] = 0; + } + + last_response_code = 0; } void coap_callback(int16_t code, size_t offset, const uint8_t *payload, size_t len, bool last_block, @@ -375,10 +361,10 @@ ZTEST(coap_client, test_get_request) LOG_INF("Send request"); ret = coap_client_req(&client, 0, &address, &client_request, NULL); zassert_true(ret >= 0, "Sending request failed, %d", ret); - set_socket_events(ZSOCK_POLLIN); - k_sleep(K_MSEC(100)); + k_sleep(K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS)); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); + LOG_INF("Test done"); } ZTEST(coap_client, test_resend_request) @@ -398,19 +384,20 @@ ZTEST(coap_client, test_resend_request) client_request.payload = short_payload; client_request.len = strlen(short_payload); - z_impl_zsock_recvfrom_fake.custom_fake = z_impl_zsock_recvfrom_custom_fake_delayed_response; + z_impl_zsock_sendto_fake.custom_fake = z_impl_zsock_sendto_custom_fake_no_reply; k_sleep(K_MSEC(1)); LOG_INF("Send request"); ret = coap_client_req(&client, 0, &address, &client_request, NULL); zassert_true(ret >= 0, "Sending request failed, %d", ret); - k_sleep(K_MSEC(15)); + k_sleep(K_MSEC(MORE_THAN_ACK_TIMEOUT_MS)); set_socket_events(ZSOCK_POLLIN); - k_sleep(K_MSEC(100)); + k_sleep(K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS)); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); zassert_equal(z_impl_zsock_sendto_fake.call_count, 2); + LOG_INF("Test done"); } ZTEST(coap_client, test_echo_option) @@ -437,10 +424,10 @@ ZTEST(coap_client, test_echo_option) LOG_INF("Send request"); ret = coap_client_req(&client, 0, &address, &client_request, NULL); zassert_true(ret >= 0, "Sending request failed, %d", ret); - set_socket_events(ZSOCK_POLLIN); - k_sleep(K_MSEC(100)); + k_sleep(K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS)); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); + LOG_INF("Test done"); } ZTEST(coap_client, test_echo_option_next_req) @@ -467,9 +454,8 @@ ZTEST(coap_client, test_echo_option_next_req) LOG_INF("Send request"); ret = coap_client_req(&client, 0, &address, &client_request, NULL); zassert_true(ret >= 0, "Sending request failed, %d", ret); - set_socket_events(ZSOCK_POLLIN); - k_sleep(K_MSEC(100)); + k_sleep(K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS)); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); char *payload = "echo testing"; @@ -481,9 +467,8 @@ ZTEST(coap_client, test_echo_option_next_req) LOG_INF("Send next request"); ret = coap_client_req(&client, 0, &address, &client_request, NULL); zassert_true(ret >= 0, "Sending request failed, %d", ret); - set_socket_events(ZSOCK_POLLIN); - k_sleep(K_MSEC(100)); + k_sleep(K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS)); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); } @@ -534,9 +519,8 @@ ZTEST(coap_client, test_send_large_data) LOG_INF("Send request"); ret = coap_client_req(&client, 0, &address, &client_request, NULL); zassert_true(ret >= 0, "Sending request failed, %d", ret); - set_socket_events(ZSOCK_POLLIN); - k_sleep(K_MSEC(100)); + k_sleep(K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS)); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); } @@ -554,7 +538,7 @@ ZTEST(coap_client, test_no_response) .len = 0 }; struct coap_transmission_parameters params = { - .ack_timeout = 200, + .ack_timeout = LONG_ACK_TIMEOUT_MS, .coap_backoff_percent = 200, .max_retransmission = 0 }; @@ -562,15 +546,16 @@ ZTEST(coap_client, test_no_response) client_request.payload = short_payload; client_request.len = strlen(short_payload); + z_impl_zsock_sendto_fake.custom_fake = z_impl_zsock_sendto_custom_fake_no_reply; + k_sleep(K_MSEC(1)); LOG_INF("Send request"); - clear_socket_events(); ret = coap_client_req(&client, 0, &address, &client_request, ¶ms); zassert_true(ret >= 0, "Sending request failed, %d", ret); - k_sleep(K_MSEC(700)); + k_sleep(K_MSEC(MORE_THAN_LONG_EXCHANGE_LIFETIME_MS)); zassert_equal(last_response_code, -ETIMEDOUT, "Unexpected response"); } @@ -598,15 +583,16 @@ ZTEST(coap_client, test_separate_response) LOG_INF("Send request"); ret = coap_client_req(&client, 0, &address, &client_request, NULL); zassert_true(ret >= 0, "Sending request failed, %d", ret); - set_socket_events(ZSOCK_POLLIN); - k_sleep(K_MSEC(100)); + k_sleep(K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS)); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); } ZTEST(coap_client, test_multiple_requests) { int ret = 0; + int retry = MORE_THAN_EXCHANGE_LIFETIME_MS; + struct sockaddr address = {0}; struct coap_client_request client_request = { .method = COAP_METHOD_GET, @@ -621,8 +607,9 @@ ZTEST(coap_client, test_multiple_requests) client_request.payload = short_payload; client_request.len = strlen(short_payload); + z_impl_zsock_sendto_fake.custom_fake = z_impl_zsock_sendto_custom_fake_no_reply; + k_sleep(K_MSEC(1)); - set_socket_events(ZSOCK_POLLIN); LOG_INF("Send request"); ret = coap_client_req(&client, 0, &address, &client_request, NULL); @@ -631,10 +618,15 @@ ZTEST(coap_client, test_multiple_requests) ret = coap_client_req(&client, 0, &address, &client_request, NULL); zassert_true(ret >= 0, "Sending request failed, %d", ret); - k_sleep(K_MSEC(100)); + set_socket_events(ZSOCK_POLLIN); + while (last_response_code == 0 && retry > 0) { + retry--; + k_sleep(K_MSEC(1)); + } zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); - k_sleep(K_MSEC(100)); + set_socket_events(ZSOCK_POLLIN); + k_sleep(K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS)); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); } @@ -652,7 +644,7 @@ ZTEST(coap_client, test_unmatching_tokens) .len = 0 }; struct coap_transmission_parameters params = { - .ack_timeout = 200, + .ack_timeout = LONG_ACK_TIMEOUT_MS, .coap_backoff_percent = 200, .max_retransmission = 0 }; @@ -662,13 +654,12 @@ ZTEST(coap_client, test_unmatching_tokens) z_impl_zsock_recvfrom_fake.custom_fake = z_impl_zsock_recvfrom_custom_fake_unmatching; + k_sleep(K_MSEC(1)); + LOG_INF("Send request"); ret = coap_client_req(&client, 0, &address, &client_request, ¶ms); zassert_true(ret >= 0, "Sending request failed, %d", ret); - set_socket_events(ZSOCK_POLLIN); - k_sleep(K_MSEC(2)); - clear_socket_events(); - k_sleep(K_MSEC(700)); + k_sleep(K_MSEC(MORE_THAN_LONG_EXCHANGE_LIFETIME_MS)); zassert_equal(last_response_code, -ETIMEDOUT, "Unexpected response"); } diff --git a/tests/net/lib/coap_client/src/stubs.c b/tests/net/lib/coap_client/src/stubs.c index 13effd8f91b..471cc8642bc 100644 --- a/tests/net/lib/coap_client/src/stubs.c +++ b/tests/net/lib/coap_client/src/stubs.c @@ -13,7 +13,7 @@ DEFINE_FAKE_VALUE_FUNC(uint32_t, z_impl_sys_rand32_get); DEFINE_FAKE_VOID_FUNC(z_impl_sys_rand_get, void *, size_t); DEFINE_FAKE_VALUE_FUNC(ssize_t, z_impl_zsock_recvfrom, int, void *, size_t, int, struct sockaddr *, socklen_t *); -DEFINE_FAKE_VALUE_FUNC(ssize_t, z_impl_zsock_sendto, int, void*, size_t, int, +DEFINE_FAKE_VALUE_FUNC(ssize_t, z_impl_zsock_sendto, int, void *, size_t, int, const struct sockaddr *, socklen_t); struct zsock_pollfd { @@ -42,11 +42,12 @@ int z_impl_zsock_socket(int family, int type, int proto) int z_impl_zsock_poll(struct zsock_pollfd *fds, int nfds, int poll_timeout) { LOG_INF("Polling, events %d", my_events); - k_sleep(K_MSEC(10)); + k_sleep(K_MSEC(1)); fds->revents = my_events; + if (my_events) { return 1; - } else { - return 0; } + + return 0; } From c3b8ff20b76ab3e2e6c7a92b69f18321070cdb77 Mon Sep 17 00:00:00 2001 From: Adrian Friedli Date: Thu, 26 Sep 2024 15:12:00 +0200 Subject: [PATCH 06/19] [nrf fromtree] net: lib: coap: make ACK random factor runtime configurable Extend the `coap_transmission_parameters` struct with the field `ack_random_percent`. This was the last remaining CoAP transmission parameter that was not configurable at runtime. Signed-off-by: Adrian Friedli (cherry picked from commit 98289e594ded291a934b32c8b0672d095e7ffe7d) --- include/zephyr/net/coap.h | 9 ++++++++- subsys/net/lib/coap/coap.c | 8 ++++++-- tests/net/lib/coap/src/main.c | 6 ++++++ 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/include/zephyr/net/coap.h b/include/zephyr/net/coap.h index bf0d86cfaf7..7a0c7ee7f91 100644 --- a/include/zephyr/net/coap.h +++ b/include/zephyr/net/coap.h @@ -355,8 +355,15 @@ typedef int (*coap_reply_t)(const struct coap_packet *response, * @brief CoAP transmission parameters. */ struct coap_transmission_parameters { - /** Initial ACK timeout. Value is used as a base value to retry pending CoAP packets. */ + /** Initial ACK timeout. Value is used as a base value to retry pending CoAP packets. */ uint32_t ack_timeout; +#if defined(CONFIG_COAP_RANDOMIZE_ACK_TIMEOUT) || defined(__DOXYGEN__) + /** + * Set CoAP ack random factor. A value of 150 means a factor of 1.5. A value of 0 defaults + * to @kconfig{CONFIG_COAP_ACK_RANDOM_PERCENT}. The value must be >= 100. + */ + uint16_t ack_random_percent; +#endif /* defined(CONFIG_COAP_RANDOMIZE_ACK_TIMEOUT) */ /** Set CoAP retry backoff factor. A value of 200 means a factor of 2.0. */ uint16_t coap_backoff_percent; /** Maximum number of retransmissions. */ diff --git a/subsys/net/lib/coap/coap.c b/subsys/net/lib/coap/coap.c index c557f3c424b..25ad262c923 100644 --- a/subsys/net/lib/coap/coap.c +++ b/subsys/net/lib/coap/coap.c @@ -59,6 +59,9 @@ static uint16_t message_id; static struct coap_transmission_parameters coap_transmission_params = { .max_retransmission = CONFIG_COAP_MAX_RETRANSMIT, .ack_timeout = CONFIG_COAP_INIT_ACK_TIMEOUT_MS, +#if defined(CONFIG_COAP_RANDOMIZE_ACK_TIMEOUT) + .ack_random_percent = CONFIG_COAP_ACK_RANDOM_PERCENT, +#endif /* defined(CONFIG_COAP_RANDOMIZE_ACK_TIMEOUT) */ .coap_backoff_percent = CONFIG_COAP_BACKOFF_PERCENT }; @@ -1706,8 +1709,9 @@ struct coap_pending *coap_pending_next_to_expire( static uint32_t init_ack_timeout(const struct coap_transmission_parameters *params) { #if defined(CONFIG_COAP_RANDOMIZE_ACK_TIMEOUT) - const uint32_t max_ack = params->ack_timeout * - CONFIG_COAP_ACK_RANDOM_PERCENT / 100; + const uint16_t random_percent = params->ack_random_percent ? params->ack_random_percent + : CONFIG_COAP_ACK_RANDOM_PERCENT; + const uint32_t max_ack = params->ack_timeout * random_percent / 100U; const uint32_t min_ack = params->ack_timeout; /* Randomly generated initial ACK timeout diff --git a/tests/net/lib/coap/src/main.c b/tests/net/lib/coap/src/main.c index 18012cc7873..10c7cef1d2c 100644 --- a/tests/net/lib/coap/src/main.c +++ b/tests/net/lib/coap/src/main.c @@ -1750,12 +1750,15 @@ ZTEST(coap, test_transmission_parameters) params = coap_get_transmission_parameters(); zassert_equal(params.ack_timeout, CONFIG_COAP_INIT_ACK_TIMEOUT_MS, "Wrong ACK timeout"); + zassert_equal(params.ack_random_percent, CONFIG_COAP_ACK_RANDOM_PERCENT, + "Wrong ACK random percent"); zassert_equal(params.coap_backoff_percent, CONFIG_COAP_BACKOFF_PERCENT, "Wrong backoff percent"); zassert_equal(params.max_retransmission, CONFIG_COAP_MAX_RETRANSMIT, "Wrong max retransmission value"); params.ack_timeout = 1000; + params.ack_random_percent = 110; params.coap_backoff_percent = 150; params.max_retransmission = 2; @@ -1772,6 +1775,7 @@ ZTEST(coap, test_transmission_parameters) zassert_not_null(pending, "No free pending"); params.ack_timeout = 3000; + params.ack_random_percent = 130; params.coap_backoff_percent = 250; params.max_retransmission = 3; @@ -1780,6 +1784,7 @@ ZTEST(coap, test_transmission_parameters) zassert_equal(r, 0, "Could not initialize packet"); zassert_equal(pending->params.ack_timeout, 3000, "Wrong ACK timeout"); + zassert_equal(pending->params.ack_random_percent, 130, "Wrong ACK random percent"); zassert_equal(pending->params.coap_backoff_percent, 250, "Wrong backoff percent"); zassert_equal(pending->params.max_retransmission, 3, "Wrong max retransmission value"); @@ -1788,6 +1793,7 @@ ZTEST(coap, test_transmission_parameters) zassert_equal(r, 0, "Could not initialize packet"); zassert_equal(pending->params.ack_timeout, 1000, "Wrong ACK timeout"); + zassert_equal(pending->params.ack_random_percent, 110, "Wrong ACK random percent"); zassert_equal(pending->params.coap_backoff_percent, 150, "Wrong backoff percent"); zassert_equal(pending->params.max_retransmission, 2, "Wrong max retransmission value"); } From f590884986788cd8b9a454500939b1dfbe5e1804 Mon Sep 17 00:00:00 2001 From: Benjamin Lindqvist Date: Wed, 23 Oct 2024 14:26:25 +0200 Subject: [PATCH 07/19] [nrf fromtree] net: coap_client: signal socket error to user Before this patch, any unexpected socket error during poll (caused by LTE disconnects for instance) would lead to a infinite loop because the error state was never cleared, handled or even signaled to the user. Signed-off-by: Benjamin Lindqvist (cherry picked from commit f8a7035c0aa289a391fbc848998b4d03dc5e530e) --- include/zephyr/net/coap_client.h | 1 + subsys/net/lib/coap/coap_client.c | 30 +++++++++++++++++++++++++++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/include/zephyr/net/coap_client.h b/include/zephyr/net/coap_client.h index a4f35374c93..00b97af60af 100644 --- a/include/zephyr/net/coap_client.h +++ b/include/zephyr/net/coap_client.h @@ -115,6 +115,7 @@ struct coap_client { struct coap_client_internal_request requests[CONFIG_COAP_CLIENT_MAX_REQUESTS]; struct coap_option echo_option; bool send_echo; + int socket_error; }; /** @endcond */ diff --git a/subsys/net/lib/coap/coap_client.c b/subsys/net/lib/coap/coap_client.c index e841a1998dc..24b1767fea5 100644 --- a/subsys/net/lib/coap/coap_client.c +++ b/subsys/net/lib/coap/coap_client.c @@ -550,19 +550,24 @@ static int handle_poll(void) } else { for (int i = 0; i < nfds; i++) { + if (fds[i].revents & ZSOCK_POLLERR) { LOG_ERR("Error in poll for socket %d", fds[i].fd); + clients[i]->socket_error = -EIO; } if (fds[i].revents & ZSOCK_POLLHUP) { LOG_ERR("Error in poll: POLLHUP for socket %d", fds[i].fd); + clients[i]->socket_error = -ENOTCONN; } if (fds[i].revents & ZSOCK_POLLNVAL) { LOG_ERR("Error in poll: POLLNVAL - fd %d not open", fds[i].fd); + clients[i]->socket_error = -EINVAL; } if (fds[i].revents & ZSOCK_POLLIN) { clients[i]->response_ready = true; } + } return 0; @@ -918,7 +923,24 @@ void coap_client_cancel_requests(struct coap_client *client) k_sleep(K_MSEC(COAP_PERIODIC_TIMEOUT)); } -static void coap_client_recv(void *coap_cl, void *a, void *b) +static void signal_socket_error(struct coap_client *cli) +{ + for (int i = 0; i < CONFIG_COAP_CLIENT_MAX_REQUESTS; i++) { + struct coap_client_internal_request *req = &cli->requests[i]; + + if (!req->request_ongoing) { + continue; + } + + req->request_ongoing = false; + if (req->coap_request.cb) { + req->coap_request.cb(cli->socket_error, 0, NULL, 0, + true, req->coap_request.user_data); + } + } +} + +void coap_client_recv(void *coap_cl, void *a, void *b) { int ret; @@ -959,6 +981,12 @@ static void coap_client_recv(void *coap_cl, void *a, void *b) clients[i]->response_ready = false; k_mutex_unlock(&clients[i]->lock); } + + if (clients[i]->socket_error) { + signal_socket_error(clients[i]); + clients[i]->socket_error = 0; + } + } /* There are more messages coming */ From 4f8da1b1599cdc43323faeba5f8813b1ddcb1e45 Mon Sep 17 00:00:00 2001 From: Seppo Takalo Date: Wed, 23 Oct 2024 16:41:17 +0300 Subject: [PATCH 08/19] [nrf fromtree] net: lib: coap_client: Protect initialization with mutex Protect global list of clients with mutex. Signed-off-by: Seppo Takalo (cherry picked from commit 1ea569d7764a3b12fe9cd5841ee6c9b73bad7a5b) --- subsys/net/lib/coap/coap_client.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/subsys/net/lib/coap/coap_client.c b/subsys/net/lib/coap/coap_client.c index 24b1767fea5..bfe6aa323cf 100644 --- a/subsys/net/lib/coap/coap_client.c +++ b/subsys/net/lib/coap/coap_client.c @@ -20,6 +20,7 @@ LOG_MODULE_DECLARE(net_coap, CONFIG_COAP_LOG_LEVEL); #define BLOCK1_OPTION_SIZE 4 #define PAYLOAD_MARKER_SIZE 1 +static K_MUTEX_DEFINE(coap_client_mutex); static struct coap_client *clients[CONFIG_COAP_CLIENT_MAX_INSTANCES]; static int num_clients; static K_SEM_DEFINE(coap_client_recv_sem, 0, 1); @@ -1006,7 +1007,9 @@ int coap_client_init(struct coap_client *client, const char *info) return -EINVAL; } + k_mutex_lock(&coap_client_mutex, K_FOREVER); if (num_clients >= CONFIG_COAP_CLIENT_MAX_INSTANCES) { + k_mutex_unlock(&coap_client_mutex); return -ENOSPC; } @@ -1015,6 +1018,7 @@ int coap_client_init(struct coap_client *client, const char *info) clients[num_clients] = client; num_clients++; + k_mutex_unlock(&coap_client_mutex); return 0; } From a2825b4d4c068b9895862df196604fd315d4109a Mon Sep 17 00:00:00 2001 From: Seppo Takalo Date: Wed, 23 Oct 2024 16:42:41 +0300 Subject: [PATCH 09/19] [nrf fromtree] net: lib: coap_client: Release internal request when failed to send When transmission of first request fails, reset the internal request buffer as there is no ongoing CoAP transaction. Application can deal with the failure. Signed-off-by: Seppo Takalo (cherry picked from commit 46b7c8451291cb88ee72d22b367ff1b70062aca5) --- subsys/net/lib/coap/coap_client.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/subsys/net/lib/coap/coap_client.c b/subsys/net/lib/coap/coap_client.c index bfe6aa323cf..d7cfcdd680e 100644 --- a/subsys/net/lib/coap/coap_client.c +++ b/subsys/net/lib/coap/coap_client.c @@ -69,6 +69,7 @@ static void reset_internal_request(struct coap_client_internal_request *request) request->offset = 0; request->last_id = 0; request->last_response_id = -1; + request->request_ongoing = false; reset_block_contexts(request); } @@ -426,6 +427,7 @@ int coap_client_req(struct coap_client *client, int sock, const struct sockaddr &client->address, client->socklen); if (ret < 0) { LOG_ERR("Transmission failed: %d", errno); + reset_internal_request(internal_req); } else { /* Do not return the number of bytes sent */ ret = 0; From 7773455f1dfe9c1766dee3814eb1364e3adff3e3 Mon Sep 17 00:00:00 2001 From: Seppo Takalo Date: Thu, 24 Oct 2024 13:25:37 +0300 Subject: [PATCH 10/19] [nrf fromtree] net: lib: coap_client: check poll() condition before retrying CoAP msg Refactor the CoAP retry handling into the handle_poll() function, so that we only try to send retries if the socket reports POLLOUT. Also move the receiving into same loop, so when poll() reports POLLIN we recv() the message and handle it before proceeding to other sockets. Also fix tests to handle POLLOUT flag and add support for testing multiple clients. Signed-off-by: Seppo Takalo (cherry picked from commit 4c6dd4c7b7d0d3dc75a6d4800beabb176ffec85b) --- include/zephyr/net/coap_client.h | 2 - subsys/net/lib/coap/coap_client.c | 204 +++++++++-------------- tests/net/lib/coap_client/CMakeLists.txt | 1 + tests/net/lib/coap_client/src/main.c | 94 +++++++++-- tests/net/lib/coap_client/src/stubs.c | 17 +- tests/net/lib/coap_client/src/stubs.h | 23 ++- 6 files changed, 194 insertions(+), 147 deletions(-) diff --git a/include/zephyr/net/coap_client.h b/include/zephyr/net/coap_client.h index 00b97af60af..90a691de4e9 100644 --- a/include/zephyr/net/coap_client.h +++ b/include/zephyr/net/coap_client.h @@ -108,14 +108,12 @@ struct coap_client { int fd; struct sockaddr address; socklen_t socklen; - bool response_ready; struct k_mutex lock; uint8_t send_buf[MAX_COAP_MSG_LEN]; uint8_t recv_buf[MAX_COAP_MSG_LEN]; struct coap_client_internal_request requests[CONFIG_COAP_CLIENT_MAX_REQUESTS]; struct coap_option echo_option; bool send_echo; - int socket_error; }; /** @endcond */ diff --git a/subsys/net/lib/coap/coap_client.c b/subsys/net/lib/coap/coap_client.c index d7cfcdd680e..896161b1a75 100644 --- a/subsys/net/lib/coap/coap_client.c +++ b/subsys/net/lib/coap/coap_client.c @@ -26,6 +26,13 @@ static int num_clients; static K_SEM_DEFINE(coap_client_recv_sem, 0, 1); static atomic_t coap_client_recv_active; +static bool timeout_expired(struct coap_client_internal_request *internal_req); +static void cancel_requests_with(struct coap_client *client, int error); +static int recv_response(struct coap_client *client, struct coap_packet *response, bool *truncated); +static int handle_response(struct coap_client *client, const struct coap_packet *response, + bool response_truncated); + + static int send_request(int sock, const void *buf, size_t len, int flags, const struct sockaddr *dest_addr, socklen_t addrlen) { @@ -127,27 +134,26 @@ static bool has_ongoing_exchange(struct coap_client *client) return false; } -static struct coap_client_internal_request *get_free_request(struct coap_client *client) +static bool has_timeout_expired(struct coap_client *client) { for (int i = 0; i < CONFIG_COAP_CLIENT_MAX_REQUESTS; i++) { - if (client->requests[i].request_ongoing == false && - exchange_lifetime_exceeded(&client->requests[i])) { - return &client->requests[i]; + if (timeout_expired(&client->requests[i])) { + return true; } } - - return NULL; + return false; } -static bool has_ongoing_requests(void) +static struct coap_client_internal_request *get_free_request(struct coap_client *client) { - for (int i = 0; i < num_clients; i++) { - if (has_ongoing_request(clients[i])) { - return true; + for (int i = 0; i < CONFIG_COAP_CLIENT_MAX_REQUESTS; i++) { + if (client->requests[i].request_ongoing == false && + exchange_lifetime_exceeded(&client->requests[i])) { + return &client->requests[i]; } } - return false; + return NULL; } static bool has_ongoing_exchanges(void) @@ -498,86 +504,91 @@ static int resend_request(struct coap_client *client, return ret; } -static int coap_client_resend_handler(void) +static void coap_client_resend_handler(struct coap_client *client) { int ret = 0; - for (int i = 0; i < num_clients; i++) { - k_mutex_lock(&clients[i]->lock, K_FOREVER); + k_mutex_lock(&client->lock, K_FOREVER); - for (int j = 0; j < CONFIG_COAP_CLIENT_MAX_REQUESTS; j++) { - if (timeout_expired(&clients[i]->requests[j])) { - ret = resend_request(clients[i], &clients[i]->requests[j]); + for (int i = 0; i < CONFIG_COAP_CLIENT_MAX_REQUESTS; i++) { + if (timeout_expired(&client->requests[i])) { + ret = resend_request(client, &client->requests[i]); + if (ret < 0) { + report_callback_error(&client->requests[i], ret); + reset_internal_request(&client->requests[i]); } } - - k_mutex_unlock(&clients[i]->lock); } - return ret; + k_mutex_unlock(&client->lock); } static int handle_poll(void) { int ret = 0; - while (1) { - struct zsock_pollfd fds[CONFIG_COAP_CLIENT_MAX_INSTANCES] = {0}; - int nfds = 0; - - /* Use periodic timeouts */ - for (int i = 0; i < num_clients; i++) { - fds[i].fd = clients[i]->fd; - fds[i].events = ZSOCK_POLLIN; - fds[i].revents = 0; - nfds++; - } + struct zsock_pollfd fds[CONFIG_COAP_CLIENT_MAX_INSTANCES] = {0}; + int nfds = 0; - ret = zsock_poll(fds, nfds, COAP_PERIODIC_TIMEOUT); + /* Use periodic timeouts */ + for (int i = 0; i < num_clients; i++) { + fds[i].fd = clients[i]->fd; + fds[i].events = (has_ongoing_exchange(clients[i]) ? ZSOCK_POLLIN : 0) | + (has_timeout_expired(clients[i]) ? ZSOCK_POLLOUT : 0); + fds[i].revents = 0; + nfds++; + } - if (ret < 0) { - LOG_ERR("Error in poll:%d", errno); - errno = 0; - return ret; - } else if (ret == 0) { - /* Resend all the expired pending messages */ - ret = coap_client_resend_handler(); + ret = zsock_poll(fds, nfds, COAP_PERIODIC_TIMEOUT); - if (ret < 0) { - LOG_ERR("Error resending request: %d", ret); - } + if (ret < 0) { + ret = -errno; + LOG_ERR("Error in poll:%d", ret); + return ret; + } else if (ret == 0) { + return 0; + } - if (!has_ongoing_requests()) { - return ret; - } + for (int i = 0; i < nfds; i++) { + if (fds[i].revents & ZSOCK_POLLOUT) { + coap_client_resend_handler(clients[i]); + } + if (fds[i].revents & ZSOCK_POLLIN) { + struct coap_packet response; + bool response_truncated = false; - } else { - for (int i = 0; i < nfds; i++) { + k_mutex_lock(&clients[i]->lock, K_FOREVER); - if (fds[i].revents & ZSOCK_POLLERR) { - LOG_ERR("Error in poll for socket %d", fds[i].fd); - clients[i]->socket_error = -EIO; - } - if (fds[i].revents & ZSOCK_POLLHUP) { - LOG_ERR("Error in poll: POLLHUP for socket %d", fds[i].fd); - clients[i]->socket_error = -ENOTCONN; - } - if (fds[i].revents & ZSOCK_POLLNVAL) { - LOG_ERR("Error in poll: POLLNVAL - fd %d not open", - fds[i].fd); - clients[i]->socket_error = -EINVAL; - } - if (fds[i].revents & ZSOCK_POLLIN) { - clients[i]->response_ready = true; - } + ret = recv_response(clients[i], &response, &response_truncated); + if (ret < 0) { + LOG_ERR("Error receiving response"); + cancel_requests_with(clients[i], -EIO); + k_mutex_unlock(&clients[i]->lock); + continue; + } + ret = handle_response(clients[i], &response, response_truncated); + if (ret < 0) { + LOG_ERR("Error handling response"); } - return 0; + k_mutex_unlock(&clients[i]->lock); + } + if (fds[i].revents & ZSOCK_POLLERR) { + LOG_ERR("Error in poll for socket %d", fds[i].fd); + cancel_requests_with(clients[i], -EIO); + } + if (fds[i].revents & ZSOCK_POLLHUP) { + LOG_ERR("Error in poll: POLLHUP for socket %d", fds[i].fd); + cancel_requests_with(clients[i], -EIO); + } + if (fds[i].revents & ZSOCK_POLLNVAL) { + LOG_ERR("Error in poll: POLLNVAL - fd %d not open", fds[i].fd); + cancel_requests_with(clients[i], -EIO); } } - return ret; + return 0; } static bool token_compare(struct coap_client_internal_request *internal_req, @@ -895,14 +906,13 @@ static int handle_response(struct coap_client *client, const struct coap_packet } } fail: - client->response_ready = false; if (ret < 0 || !internal_req->is_observe) { internal_req->request_ongoing = false; } return ret; } -void coap_client_cancel_requests(struct coap_client *client) +static void cancel_requests_with(struct coap_client *client, int error) { k_mutex_lock(&client->lock, K_FOREVER); @@ -914,33 +924,20 @@ void coap_client_cancel_requests(struct coap_client *client) * do not reenter it. In that case, the user knows their * request was cancelled anyway. */ - report_callback_error(&client->requests[i], -ECANCELED); - client->requests[i].request_ongoing = false; - client->requests[i].is_observe = false; + report_callback_error(&client->requests[i], error); + reset_internal_request(&client->requests[i]); } } atomic_clear(&coap_client_recv_active); k_mutex_unlock(&client->lock); - /* Wait until after zsock_poll() can time out and return. */ - k_sleep(K_MSEC(COAP_PERIODIC_TIMEOUT)); } -static void signal_socket_error(struct coap_client *cli) +void coap_client_cancel_requests(struct coap_client *client) { - for (int i = 0; i < CONFIG_COAP_CLIENT_MAX_REQUESTS; i++) { - struct coap_client_internal_request *req = &cli->requests[i]; - - if (!req->request_ongoing) { - continue; - } - - req->request_ongoing = false; - if (req->coap_request.cb) { - req->coap_request.cb(cli->socket_error, 0, NULL, 0, - true, req->coap_request.user_data); - } - } + cancel_requests_with(client, -ECANCELED); + /* Wait until after zsock_poll() can time out and return. */ + k_sleep(K_MSEC(COAP_PERIODIC_TIMEOUT)); } void coap_client_recv(void *coap_cl, void *a, void *b) @@ -957,41 +954,6 @@ void coap_client_recv(void *coap_cl, void *a, void *b) goto idle; } - for (int i = 0; i < num_clients; i++) { - if (clients[i]->response_ready) { - struct coap_packet response; - bool response_truncated = false; - - k_mutex_lock(&clients[i]->lock, K_FOREVER); - - ret = recv_response(clients[i], &response, &response_truncated); - if (ret < 0) { - LOG_ERR("Error receiving response"); - clients[i]->response_ready = false; - k_mutex_unlock(&clients[i]->lock); - if (ret == -EOPNOTSUPP) { - LOG_ERR("Socket misconfigured."); - goto idle; - } - continue; - } - - ret = handle_response(clients[i], &response, response_truncated); - if (ret < 0) { - LOG_ERR("Error handling response"); - } - - clients[i]->response_ready = false; - k_mutex_unlock(&clients[i]->lock); - } - - if (clients[i]->socket_error) { - signal_socket_error(clients[i]); - clients[i]->socket_error = 0; - } - - } - /* There are more messages coming */ if (has_ongoing_exchanges()) { continue; diff --git a/tests/net/lib/coap_client/CMakeLists.txt b/tests/net/lib/coap_client/CMakeLists.txt index e12214fc6a9..d7f0c0cce5e 100644 --- a/tests/net/lib/coap_client/CMakeLists.txt +++ b/tests/net/lib/coap_client/CMakeLists.txt @@ -31,3 +31,4 @@ add_compile_definitions(CONFIG_COAP_CLIENT_MAX_REQUESTS=2) add_compile_definitions(CONFIG_COAP_CLIENT_MAX_INSTANCES=2) add_compile_definitions(CONFIG_COAP_MAX_RETRANSMIT=4) add_compile_definitions(CONFIG_COAP_BACKOFF_PERCENT=200) +add_compile_definitions(CONFIG_COAP_LOG_LEVEL=4) diff --git a/tests/net/lib/coap_client/src/main.c b/tests/net/lib/coap_client/src/main.c index 25c2c58f2fd..20f1e4d8381 100644 --- a/tests/net/lib/coap_client/src/main.c +++ b/tests/net/lib/coap_client/src/main.c @@ -77,7 +77,7 @@ static ssize_t z_impl_zsock_recvfrom_custom_fake(int sock, void *buf, size_t max memcpy(buf, ack_data, sizeof(ack_data)); - clear_socket_events(); + clear_socket_events(ZSOCK_POLLIN); return sizeof(ack_data); } @@ -205,7 +205,7 @@ static ssize_t z_impl_zsock_recvfrom_custom_fake_response(int sock, void *buf, s memcpy(buf, ack_data, sizeof(ack_data)); - clear_socket_events(); + clear_socket_events(ZSOCK_POLLIN); return sizeof(ack_data); } @@ -247,7 +247,7 @@ static ssize_t z_impl_zsock_recvfrom_custom_fake_unmatching(int sock, void *buf, memcpy(buf, ack_data, sizeof(ack_data)); - clear_socket_events(); + clear_socket_events(ZSOCK_POLLIN); return sizeof(ack_data); } @@ -273,7 +273,7 @@ static ssize_t z_impl_zsock_recvfrom_custom_fake_echo(int sock, void *buf, size_ z_impl_zsock_recvfrom_fake.custom_fake = z_impl_zsock_recvfrom_custom_fake_response; z_impl_zsock_sendto_fake.custom_fake = z_impl_zsock_sendto_custom_fake_echo; - clear_socket_events(); + clear_socket_events(ZSOCK_POLLIN); return sizeof(ack_data); } @@ -299,7 +299,7 @@ static ssize_t z_impl_zsock_recvfrom_custom_fake_echo_next_req(int sock, void *b z_impl_zsock_recvfrom_fake.custom_fake = z_impl_zsock_recvfrom_custom_fake_response; z_impl_zsock_sendto_fake.custom_fake = z_impl_zsock_sendto_custom_fake_echo_next_req; - clear_socket_events(); + clear_socket_events(ZSOCK_POLLIN); return sizeof(ack_data); } @@ -335,6 +335,9 @@ void coap_callback(int16_t code, size_t offset, const uint8_t *payload, size_t l { LOG_INF("CoAP response callback, %d", code); last_response_code = code; + if (user_data) { + k_sem_give((struct k_sem *) user_data); + } } ZTEST_SUITE(coap_client, NULL, suite_setup, test_setup, NULL, NULL); @@ -392,7 +395,7 @@ ZTEST(coap_client, test_resend_request) ret = coap_client_req(&client, 0, &address, &client_request, NULL); zassert_true(ret >= 0, "Sending request failed, %d", ret); k_sleep(K_MSEC(MORE_THAN_ACK_TIMEOUT_MS)); - set_socket_events(ZSOCK_POLLIN); + set_socket_events(ZSOCK_POLLIN | ZSOCK_POLLOUT); k_sleep(K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS)); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); @@ -547,6 +550,7 @@ ZTEST(coap_client, test_no_response) client_request.len = strlen(short_payload); z_impl_zsock_sendto_fake.custom_fake = z_impl_zsock_sendto_custom_fake_no_reply; + set_socket_events(ZSOCK_POLLOUT); k_sleep(K_MSEC(1)); @@ -592,30 +596,35 @@ ZTEST(coap_client, test_multiple_requests) { int ret = 0; int retry = MORE_THAN_EXCHANGE_LIFETIME_MS; + struct k_sem sem1, sem2; struct sockaddr address = {0}; - struct coap_client_request client_request = { + struct coap_client_request req1 = { .method = COAP_METHOD_GET, .confirmable = true, .path = test_path, .fmt = COAP_CONTENT_FORMAT_TEXT_PLAIN, .cb = coap_callback, - .payload = NULL, - .len = 0 + .payload = short_payload, + .len = strlen(short_payload), + .user_data = &sem1 }; + struct coap_client_request req2 = req1; - client_request.payload = short_payload; - client_request.len = strlen(short_payload); + req2.user_data = &sem2; + + k_sem_init(&sem1, 0, 1); + k_sem_init(&sem2, 0, 1); z_impl_zsock_sendto_fake.custom_fake = z_impl_zsock_sendto_custom_fake_no_reply; k_sleep(K_MSEC(1)); LOG_INF("Send request"); - ret = coap_client_req(&client, 0, &address, &client_request, NULL); + ret = coap_client_req(&client, 0, &address, &req1, NULL); zassert_true(ret >= 0, "Sending request failed, %d", ret); - ret = coap_client_req(&client, 0, &address, &client_request, NULL); + ret = coap_client_req(&client, 0, &address, &req2, NULL); zassert_true(ret >= 0, "Sending request failed, %d", ret); set_socket_events(ZSOCK_POLLIN); @@ -625,8 +634,10 @@ ZTEST(coap_client, test_multiple_requests) } zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); + last_response_code = 0; set_socket_events(ZSOCK_POLLIN); - k_sleep(K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS)); + zassert_ok(k_sem_take(&sem1, K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS))); + zassert_ok(k_sem_take(&sem2, K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS))); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); } @@ -653,6 +664,7 @@ ZTEST(coap_client, test_unmatching_tokens) client_request.len = strlen(short_payload); z_impl_zsock_recvfrom_fake.custom_fake = z_impl_zsock_recvfrom_custom_fake_unmatching; + set_socket_events(ZSOCK_POLLIN | ZSOCK_POLLOUT); k_sleep(K_MSEC(1)); @@ -663,3 +675,57 @@ ZTEST(coap_client, test_unmatching_tokens) k_sleep(K_MSEC(MORE_THAN_LONG_EXCHANGE_LIFETIME_MS)); zassert_equal(last_response_code, -ETIMEDOUT, "Unexpected response"); } + +ZTEST(coap_client, test_multiple_clients) +{ + int ret; + int retry = MORE_THAN_EXCHANGE_LIFETIME_MS; + static struct coap_client client2 = { + .fd = 2, + }; + struct k_sem sem1, sem2; + struct sockaddr address = {0}; + struct coap_client_request req1 = { + .method = COAP_METHOD_GET, + .confirmable = true, + .path = test_path, + .fmt = COAP_CONTENT_FORMAT_TEXT_PLAIN, + .cb = coap_callback, + .payload = short_payload, + .len = strlen(short_payload), + .user_data = &sem1 + }; + struct coap_client_request req2 = req1; + + req2.user_data = &sem2; + req2.payload = long_payload; + req2.len = strlen(long_payload); + + zassert_ok(k_sem_init(&sem1, 0, 1)); + zassert_ok(k_sem_init(&sem2, 0, 1)); + + zassert_ok(coap_client_init(&client2, NULL)); + + k_sleep(K_MSEC(1)); + + LOG_INF("Sending requests"); + ret = coap_client_req(&client, 1, &address, &req1, NULL); + zassert_true(ret >= 0, "Sending request failed, %d", ret); + + ret = coap_client_req(&client2, 2, &address, &req2, NULL); + zassert_true(ret >= 0, "Sending request failed, %d", ret); + + while (last_response_code == 0 && retry > 0) { + retry--; + k_sleep(K_MSEC(1)); + } + set_socket_events(ZSOCK_POLLIN); + + k_sleep(K_SECONDS(1)); + + /* ensure we got both responses */ + zassert_ok(k_sem_take(&sem1, K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS))); + zassert_ok(k_sem_take(&sem2, K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS))); + zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); + +} diff --git a/tests/net/lib/coap_client/src/stubs.c b/tests/net/lib/coap_client/src/stubs.c index 471cc8642bc..63307e27fb7 100644 --- a/tests/net/lib/coap_client/src/stubs.c +++ b/tests/net/lib/coap_client/src/stubs.c @@ -29,9 +29,9 @@ void set_socket_events(short events) my_events |= events; } -void clear_socket_events(void) +void clear_socket_events(short events) { - my_events = 0; + my_events &= ~events; } int z_impl_zsock_socket(int family, int type, int proto) @@ -41,13 +41,14 @@ int z_impl_zsock_socket(int family, int type, int proto) int z_impl_zsock_poll(struct zsock_pollfd *fds, int nfds, int poll_timeout) { - LOG_INF("Polling, events %d", my_events); + int events = 0; k_sleep(K_MSEC(1)); - fds->revents = my_events; - - if (my_events) { - return 1; + for (int i = 0; i < nfds; i++) { + fds[i].revents = my_events & (fds[i].events | ZSOCK_POLLERR | ZSOCK_POLLHUP); + if (fds[i].revents) { + events++; + } } - return 0; + return events; } diff --git a/tests/net/lib/coap_client/src/stubs.h b/tests/net/lib/coap_client/src/stubs.h index eb340d914eb..9a9f929ce76 100644 --- a/tests/net/lib/coap_client/src/stubs.h +++ b/tests/net/lib/coap_client/src/stubs.h @@ -15,11 +15,30 @@ #include -#define ZSOCK_POLLIN 1 + +/* Copy from zephyr/include/zephyr/net/socket.h */ +/** + * @name Options for poll() + * @{ + */ +/* ZSOCK_POLL* values are compatible with Linux */ +/** zsock_poll: Poll for readability */ +#define ZSOCK_POLLIN 1 +/** zsock_poll: Poll for exceptional condition */ +#define ZSOCK_POLLPRI 2 +/** zsock_poll: Poll for writability */ #define ZSOCK_POLLOUT 4 +/** zsock_poll: Poll results in error condition (output value only) */ +#define ZSOCK_POLLERR 8 +/** zsock_poll: Poll detected closed connection (output value only) */ +#define ZSOCK_POLLHUP 0x10 +/** zsock_poll: Invalid socket (output value only) */ +#define ZSOCK_POLLNVAL 0x20 +/** @} */ + void set_socket_events(short events); -void clear_socket_events(void); +void clear_socket_events(short events); DECLARE_FAKE_VALUE_FUNC(uint32_t, z_impl_sys_rand32_get); DECLARE_FAKE_VOID_FUNC(z_impl_sys_rand_get, void *, size_t); From c7852c05a4ee54e807e7be06f8d927b1eef311fc Mon Sep 17 00:00:00 2001 From: Seppo Takalo Date: Fri, 25 Oct 2024 15:47:44 +0300 Subject: [PATCH 11/19] [nrf fromtree] net: lib: coap_client: Forward recv() errors to handling loop Forward recv() errors to handle_poll(), so there is only one place to handle error codes. Signed-off-by: Seppo Takalo (cherry picked from commit 6481b0ec6ca5e668c12184181642bd7a150957ff) --- subsys/net/lib/coap/coap_client.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/subsys/net/lib/coap/coap_client.c b/subsys/net/lib/coap/coap_client.c index 896161b1a75..f10adc39d98 100644 --- a/subsys/net/lib/coap/coap_client.c +++ b/subsys/net/lib/coap/coap_client.c @@ -557,16 +557,17 @@ static int handle_poll(void) struct coap_packet response; bool response_truncated = false; - k_mutex_lock(&clients[i]->lock, K_FOREVER); - ret = recv_response(clients[i], &response, &response_truncated); if (ret < 0) { + if (ret == -EAGAIN) { + continue; + } LOG_ERR("Error receiving response"); cancel_requests_with(clients[i], -EIO); - k_mutex_unlock(&clients[i]->lock); continue; } + k_mutex_lock(&clients[i]->lock, K_FOREVER); ret = handle_response(clients[i], &response, response_truncated); if (ret < 0) { LOG_ERR("Error handling response"); @@ -622,14 +623,11 @@ static int recv_response(struct coap_client *client, struct coap_packet *respons flags, &client->address, &client->socklen); if (total_len < 0) { - LOG_ERR("Error reading response: %d", errno); - if (errno == EOPNOTSUPP) { - return -errno; - } - return -EINVAL; + ret = -errno; + return ret; } else if (total_len == 0) { - LOG_ERR("Zero length recv"); - return -EINVAL; + /* Ignore, UDP can be zero length, but it is not CoAP anymore */ + return 0; } available_len = MIN(total_len, sizeof(client->recv_buf)); @@ -640,7 +638,6 @@ static int recv_response(struct coap_client *client, struct coap_packet *respons ret = coap_packet_parse(response, client->recv_buf, available_len, NULL, 0); if (ret < 0) { LOG_ERR("Invalid data received"); - return ret; } return ret; From 0172538be77aa47d86d748c89a6ee635aa562ca2 Mon Sep 17 00:00:00 2001 From: Seppo Takalo Date: Fri, 25 Oct 2024 15:51:26 +0300 Subject: [PATCH 12/19] [nrf fromtree] net: lib: coap_client: Don't decrease retry counter on send() failure If send() fails, we have not technically send the CoAP retry yet, so restore the same pending structure, so our timeouts and retry counters stay the same. This will trigger a retry next time the poll() return POLLOUT, so we know that we can send. Signed-off-by: Seppo Takalo (cherry picked from commit 623a1ffd5221c571a307ed95a28e93a8bebb3918) --- subsys/net/lib/coap/coap_client.c | 34 +++++++++++++++++++------------ 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/subsys/net/lib/coap/coap_client.c b/subsys/net/lib/coap/coap_client.c index f10adc39d98..79c4c883f12 100644 --- a/subsys/net/lib/coap/coap_client.c +++ b/subsys/net/lib/coap/coap_client.c @@ -471,10 +471,13 @@ static int resend_request(struct coap_client *client, { int ret = 0; + /* Copy the pending structure if we need to restore it */ + struct coap_pending tmp = internal_req->pending; + if (internal_req->request_ongoing && internal_req->pending.timeout != 0 && coap_pending_cycle(&internal_req->pending)) { - LOG_ERR("Timeout in poll, retrying send"); + LOG_ERR("Timeout, retrying send"); /* Reset send block context as it was updated in previous init from packet */ if (internal_req->send_blk_ctx.total_size > 0) { @@ -483,22 +486,27 @@ static int resend_request(struct coap_client *client, ret = coap_client_init_request(client, &internal_req->coap_request, internal_req, true); if (ret < 0) { - LOG_ERR("Error re-creating CoAP request"); + LOG_ERR("Error re-creating CoAP request %d", ret); + return ret; + } + + ret = send_request(client->fd, internal_req->request.data, + internal_req->request.offset, 0, &client->address, + client->socklen); + if (ret > 0) { + ret = 0; + } else if (ret == -1 && errno == EAGAIN) { + /* Restore the pending structure, retry later */ + internal_req->pending = tmp; + /* Not a fatal socket error, will trigger a retry */ + ret = 0; } else { - ret = send_request(client->fd, internal_req->request.data, - internal_req->request.offset, 0, &client->address, - client->socklen); - if (ret > 0) { - ret = 0; - } else { - LOG_ERR("Failed to resend request, %d", ret); - } + ret = -errno; + LOG_ERR("Failed to resend request, %d", ret); } } else { - LOG_ERR("Timeout in poll, no more retries left"); + LOG_ERR("Timeout, no more retries left"); ret = -ETIMEDOUT; - report_callback_error(internal_req, ret); - internal_req->request_ongoing = false; } return ret; From c3c879c9deda2b9cf1c6fc87cbb7519e526f51e3 Mon Sep 17 00:00:00 2001 From: Seppo Takalo Date: Fri, 25 Oct 2024 15:58:39 +0300 Subject: [PATCH 13/19] [nrf fromtree] net: lib: coap_client: Use reset_internal_request() instead of flagging It is error prone to flag separate booleans, so try to use reset_internal_request() every time we release the internal request structure. Also refactor the reset_internal_request() so that we reset the timeout value so it does not trigger again. Signed-off-by: Seppo Takalo (cherry picked from commit a14f08303065c5c6e528ae84ee264084a8d5f5b4) --- subsys/net/lib/coap/coap_client.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/subsys/net/lib/coap/coap_client.c b/subsys/net/lib/coap/coap_client.c index 79c4c883f12..550c790f69b 100644 --- a/subsys/net/lib/coap/coap_client.c +++ b/subsys/net/lib/coap/coap_client.c @@ -60,24 +60,16 @@ static int receive(int sock, void *buf, size_t max_len, int flags, return err; } -static void reset_block_contexts(struct coap_client_internal_request *request) -{ - request->recv_blk_ctx.block_size = 0; - request->recv_blk_ctx.total_size = 0; - request->recv_blk_ctx.current = 0; - - request->send_blk_ctx.block_size = 0; - request->send_blk_ctx.total_size = 0; - request->send_blk_ctx.current = 0; -} - static void reset_internal_request(struct coap_client_internal_request *request) { request->offset = 0; request->last_id = 0; request->last_response_id = -1; request->request_ongoing = false; - reset_block_contexts(request); + request->is_observe = false; + request->pending.timeout = 0; + request->recv_blk_ctx = (struct coap_block_context){ 0 }; + request->send_blk_ctx = (struct coap_block_context){ 0 }; } static int coap_client_schedule_poll(struct coap_client *client, int sock, @@ -912,7 +904,7 @@ static int handle_response(struct coap_client *client, const struct coap_packet } fail: if (ret < 0 || !internal_req->is_observe) { - internal_req->request_ongoing = false; + reset_internal_request(internal_req); } return ret; } From f3ac92a0c74734e56f3b8edac9b4f54e636f25c9 Mon Sep 17 00:00:00 2001 From: Seppo Takalo Date: Thu, 24 Oct 2024 17:38:14 +0300 Subject: [PATCH 14/19] [nrf fromtree] net: lib: coap_client: Fix reset handling Fix handling of received CoAP reset. Signed-off-by: Seppo Takalo (cherry picked from commit 1890dbd63732c0075f0fde39c46cd78ee118ae65) --- include/zephyr/net/coap_client.h | 2 +- subsys/net/lib/coap/coap_client.c | 54 ++++++++++++++++++++++++------- 2 files changed, 43 insertions(+), 13 deletions(-) diff --git a/include/zephyr/net/coap_client.h b/include/zephyr/net/coap_client.h index 90a691de4e9..1d68661da3b 100644 --- a/include/zephyr/net/coap_client.h +++ b/include/zephyr/net/coap_client.h @@ -88,7 +88,7 @@ struct coap_client_option { struct coap_client_internal_request { uint8_t request_token[COAP_TOKEN_MAX_LEN]; uint32_t offset; - uint32_t last_id; + uint16_t last_id; uint8_t request_tkl; bool request_ongoing; atomic_t in_callback; diff --git a/subsys/net/lib/coap/coap_client.c b/subsys/net/lib/coap/coap_client.c index 550c790f69b..ca592291ddf 100644 --- a/subsys/net/lib/coap/coap_client.c +++ b/subsys/net/lib/coap/coap_client.c @@ -31,6 +31,8 @@ static void cancel_requests_with(struct coap_client *client, int error); static int recv_response(struct coap_client *client, struct coap_packet *response, bool *truncated); static int handle_response(struct coap_client *client, const struct coap_packet *response, bool response_truncated); +static struct coap_client_internal_request *get_request_with_mid( + struct coap_client *client, const struct coap_packet *resp); static int send_request(int sock, const void *buf, size_t len, int flags, @@ -689,6 +691,23 @@ static struct coap_client_internal_request *get_request_with_token( return NULL; } +static struct coap_client_internal_request *get_request_with_mid( + struct coap_client *client, const struct coap_packet *resp) +{ + uint16_t mid = coap_header_get_id(resp); + + for (int i = 0; i < CONFIG_COAP_CLIENT_MAX_REQUESTS; i++) { + if (client->requests[i].request_ongoing) { + if (client->requests[i].last_id == mid) { + return &client->requests[i]; + } + } + } + + return NULL; +} + + static bool find_echo_option(const struct coap_packet *response, struct coap_option *option) { return coap_find_options(response, COAP_OPTION_ECHO, option, 1); @@ -698,7 +717,6 @@ static int handle_response(struct coap_client *client, const struct coap_packet bool response_truncated) { int ret = 0; - int response_type; int block_option; int block_num; bool blockwise_transfer = false; @@ -711,33 +729,45 @@ static int handle_response(struct coap_client *client, const struct coap_packet * NCON request results only as a separate CON or NCON message as there is no ACK * With RESET, just drop gloves and call the callback. */ - response_type = coap_header_get_type(response); - - internal_req = get_request_with_token(client, response); - /* Reset and Ack need to match the message ID with request */ - if ((response_type == COAP_TYPE_ACK || response_type == COAP_TYPE_RESET) && - internal_req == NULL) { - LOG_ERR("Unexpected ACK or Reset"); - return -EFAULT; - } else if (response_type == COAP_TYPE_RESET) { - coap_pending_clear(&internal_req->pending); - } /* CON, NON_CON and piggybacked ACK need to match the token with original request */ uint16_t payload_len; + uint8_t response_type = coap_header_get_type(response); uint8_t response_code = coap_header_get_code(response); uint16_t response_id = coap_header_get_id(response); const uint8_t *payload = coap_packet_get_payload(response, &payload_len); + if (response_type == COAP_TYPE_RESET) { + internal_req = get_request_with_mid(client, response); + if (!internal_req) { + LOG_WRN("No matching request for RESET"); + return 0; + } + report_callback_error(internal_req, -ECONNRESET); + reset_internal_request(internal_req); + return 0; + } + /* Separate response coming */ if (payload_len == 0 && response_type == COAP_TYPE_ACK && response_code == COAP_CODE_EMPTY) { + internal_req = get_request_with_mid(client, response); + if (!internal_req) { + LOG_WRN("No matching request for ACK"); + return 0; + } internal_req->pending.t0 = k_uptime_get(); internal_req->pending.timeout = internal_req->pending.t0 + COAP_SEPARATE_TIMEOUT; internal_req->pending.retries = 0; return 1; } + internal_req = get_request_with_token(client, response); + if (!internal_req) { + LOG_WRN("No matching request for response"); + return 0; + } + if (internal_req == NULL || !token_compare(internal_req, response)) { LOG_WRN("Not matching tokens"); return 1; From a1dd4f6644bcd2c9384977979a97de7397db4c51 Mon Sep 17 00:00:00 2001 From: Seppo Takalo Date: Fri, 25 Oct 2024 13:30:37 +0300 Subject: [PATCH 15/19] [nrf fromtree] net: coap: Add API to send reset message Add helper API to construct CoAP Reset message. Signed-off-by: Seppo Takalo (cherry picked from commit e96e95b6f63046dad3e77af8fb46cf995381a1d6) --- include/zephyr/net/coap.h | 15 +++++++++++++++ subsys/net/lib/coap/coap.c | 13 +++++++++++++ 2 files changed, 28 insertions(+) diff --git a/include/zephyr/net/coap.h b/include/zephyr/net/coap.h index 7a0c7ee7f91..09df1e98f8a 100644 --- a/include/zephyr/net/coap.h +++ b/include/zephyr/net/coap.h @@ -552,6 +552,21 @@ int coap_packet_init(struct coap_packet *cpkt, uint8_t *data, uint16_t max_len, int coap_ack_init(struct coap_packet *cpkt, const struct coap_packet *req, uint8_t *data, uint16_t max_len, uint8_t code); +/** + * @brief Create a new CoAP Reset message for given request. + * + * This function works like @ref coap_packet_init, filling CoAP header type, + * and CoAP header message id fields. + * + * @param cpkt New packet to be initialized using the storage from @a data. + * @param req CoAP request packet that is being acknowledged + * @param data Data that will contain a CoAP packet information + * @param max_len Maximum allowable length of data + * + * @return 0 in case of success or negative in case of error. + */ +int coap_rst_init(struct coap_packet *cpkt, const struct coap_packet *req, + uint8_t *data, uint16_t max_len); /** * @brief Returns a randomly generated array of 8 bytes, that can be * used as a message's token. diff --git a/subsys/net/lib/coap/coap.c b/subsys/net/lib/coap/coap.c index 25ad262c923..70f2610adf0 100644 --- a/subsys/net/lib/coap/coap.c +++ b/subsys/net/lib/coap/coap.c @@ -232,6 +232,19 @@ int coap_ack_init(struct coap_packet *cpkt, const struct coap_packet *req, token, code, id); } +int coap_rst_init(struct coap_packet *cpkt, const struct coap_packet *req, + uint8_t *data, uint16_t max_len) +{ + uint16_t id; + uint8_t ver; + + ver = coap_header_get_version(req); + id = coap_header_get_id(req); + + return coap_packet_init(cpkt, data, max_len, ver, COAP_TYPE_RESET, 0, + NULL, 0, id); +} + static void option_header_set_delta(uint8_t *opt, uint8_t delta) { *opt = (delta & 0xF) << 4; From 78c7c1dce295a53a8017988bfc7cf2ff258d8ca1 Mon Sep 17 00:00:00 2001 From: Seppo Takalo Date: Fri, 25 Oct 2024 13:32:43 +0300 Subject: [PATCH 16/19] [nrf fromtree] net: lib: coap_client: Remove duplicate token comparison Response tokens are already compared in get_request_with_token(). Signed-off-by: Seppo Takalo (cherry picked from commit 1dc24872ce833bcfd702963b1eff98e84d64fb43) --- subsys/net/lib/coap/coap_client.c | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/subsys/net/lib/coap/coap_client.c b/subsys/net/lib/coap/coap_client.c index ca592291ddf..31b065be951 100644 --- a/subsys/net/lib/coap/coap_client.c +++ b/subsys/net/lib/coap/coap_client.c @@ -594,21 +594,6 @@ static int handle_poll(void) return 0; } -static bool token_compare(struct coap_client_internal_request *internal_req, - const struct coap_packet *resp) -{ - uint8_t response_token[COAP_TOKEN_MAX_LEN]; - uint8_t response_tkl; - - response_tkl = coap_header_get_token(resp, response_token); - - if (internal_req->request_tkl != response_tkl) { - return false; - } - - return memcmp(&internal_req->request_token, &response_token, response_tkl) == 0; -} - static int recv_response(struct coap_client *client, struct coap_packet *response, bool *truncated) { int total_len; @@ -768,11 +753,6 @@ static int handle_response(struct coap_client *client, const struct coap_packet return 0; } - if (internal_req == NULL || !token_compare(internal_req, response)) { - LOG_WRN("Not matching tokens"); - return 1; - } - /* MID-based deduplication */ if (response_id == internal_req->last_response_id) { LOG_WRN("Duplicate MID, dropping"); From 16a34d07d842c6d10776b3ab91f75033b4d994fa Mon Sep 17 00:00:00 2001 From: Seppo Takalo Date: Fri, 25 Oct 2024 13:58:15 +0300 Subject: [PATCH 17/19] [nrf fromtree] net: lib: coap_client: Send RST for unknown queries When receiving unknown response, respond with CoAP Reset. Signed-off-by: Seppo Takalo (cherry picked from commit 350d20e027a9a13e741c8c72a4f642f748a27f99) --- subsys/net/lib/coap/coap_client.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/subsys/net/lib/coap/coap_client.c b/subsys/net/lib/coap/coap_client.c index 31b065be951..cfbf8c42c05 100644 --- a/subsys/net/lib/coap/coap_client.c +++ b/subsys/net/lib/coap/coap_client.c @@ -651,6 +651,26 @@ static int send_ack(struct coap_client *client, const struct coap_packet *req, return 0; } +static int send_rst(struct coap_client *client, const struct coap_packet *req) +{ + int ret; + struct coap_packet rst; + + ret = coap_rst_init(&rst, req, client->send_buf, MAX_COAP_MSG_LEN); + if (ret < 0) { + LOG_ERR("Failed to initialize CoAP RST-message"); + return ret; + } + + ret = send_request(client->fd, rst.data, rst.offset, 0, &client->address, client->socklen); + if (ret < 0) { + LOG_ERR("Error sending a CoAP RST-message"); + return ret; + } + + return 0; +} + static struct coap_client_internal_request *get_request_with_token( struct coap_client *client, const struct coap_packet *resp) { @@ -750,6 +770,7 @@ static int handle_response(struct coap_client *client, const struct coap_packet internal_req = get_request_with_token(client, response); if (!internal_req) { LOG_WRN("No matching request for response"); + (void) send_rst(client, response); /* Ignore errors, unrelated to our queries */ return 0; } From f43a05c9dc72281f245c3ed114eb683616d86b4b Mon Sep 17 00:00:00 2001 From: Seppo Takalo Date: Fri, 25 Oct 2024 16:27:20 +0300 Subject: [PATCH 18/19] [nrf fromtree] tests: coap_client: Add tests for poll() errors If we receive poll() error during a request, it must be forwarded to application. If instead receive poll() error later, it should not be forwarded as it might be result of application closing the socket. Signed-off-by: Seppo Takalo (cherry picked from commit 9c9dc9f760e77d462fb5649fc63f9acc22d9ac03) --- tests/net/lib/coap_client/src/main.c | 61 ++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/tests/net/lib/coap_client/src/main.c b/tests/net/lib/coap_client/src/main.c index 20f1e4d8381..f06a0ce646c 100644 --- a/tests/net/lib/coap_client/src/main.c +++ b/tests/net/lib/coap_client/src/main.c @@ -322,6 +322,7 @@ static void test_setup(void *data) z_impl_zsock_recvfrom_fake.custom_fake = z_impl_zsock_recvfrom_custom_fake; z_impl_zsock_sendto_fake.custom_fake = z_impl_zsock_sendto_custom_fake; + clear_socket_events(ZSOCK_POLLIN | ZSOCK_POLLOUT | ZSOCK_POLLERR); for (i = 0; i < ARRAY_SIZE(messages_needing_response); i++) { messages_needing_response[i] = 0; @@ -727,5 +728,65 @@ ZTEST(coap_client, test_multiple_clients) zassert_ok(k_sem_take(&sem1, K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS))); zassert_ok(k_sem_take(&sem2, K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS))); zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); +} + + +ZTEST(coap_client, test_poll_err) +{ + int ret = 0; + struct sockaddr address = {0}; + struct coap_client_request client_request = { + .method = COAP_METHOD_GET, + .confirmable = true, + .path = test_path, + .fmt = COAP_CONTENT_FORMAT_TEXT_PLAIN, + .cb = coap_callback, + .payload = short_payload, + .len = strlen(short_payload), + }; + + z_impl_zsock_sendto_fake.custom_fake = z_impl_zsock_sendto_custom_fake_no_reply; + set_socket_events(ZSOCK_POLLERR); + + k_sleep(K_MSEC(1)); + + LOG_INF("Send request"); + ret = coap_client_req(&client, 0, &address, &client_request, NULL); + zassert_true(ret >= 0, "Sending request failed, %d", ret); + + k_sleep(K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS)); + zassert_equal(last_response_code, -EIO, "Unexpected response"); +} + +ZTEST(coap_client, test_poll_err_after_response) +{ + int ret = 0; + struct k_sem sem1; + struct sockaddr address = {0}; + struct coap_client_request client_request = { + .method = COAP_METHOD_GET, + .confirmable = true, + .path = test_path, + .fmt = COAP_CONTENT_FORMAT_TEXT_PLAIN, + .cb = coap_callback, + .payload = short_payload, + .len = strlen(short_payload), + .user_data = &sem1 + }; + + zassert_ok(k_sem_init(&sem1, 0, 1)); + z_impl_zsock_sendto_fake.custom_fake = z_impl_zsock_sendto_custom_fake_no_reply; + set_socket_events(ZSOCK_POLLIN); + + k_sleep(K_MSEC(1)); + + LOG_INF("Send request"); + ret = coap_client_req(&client, 0, &address, &client_request, NULL); + zassert_true(ret >= 0, "Sending request failed, %d", ret); + + zassert_ok(k_sem_take(&sem1, K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS))); + zassert_equal(last_response_code, COAP_RESPONSE_CODE_OK, "Unexpected response"); + set_socket_events(ZSOCK_POLLERR); + zassert_not_ok(k_sem_take(&sem1, K_MSEC(MORE_THAN_EXCHANGE_LIFETIME_MS))); } From 5c922fc2513e792d3dfe12593c3dd0f58e57c33b Mon Sep 17 00:00:00 2001 From: Seppo Takalo Date: Mon, 28 Oct 2024 13:29:14 +0200 Subject: [PATCH 19/19] [nrf fromtree] net: lib: coap_client: Remove unnecessary atomic variable In receiving thread, continuing the loops is based on has_ongoing_exchanges() so it does not need atomic coap_client_recv_active variable. When idling, it wakes from semaphore. But there was potential deadlock when coap_client_schedule_poll() would not signal the semaphore, if atomic variable was already showing that it runs. Removing the atomic variable removes this deadlock. Signed-off-by: Seppo Takalo (cherry picked from commit 1e5a537adee75805a9f3f52801c845876bf6c265) --- subsys/net/lib/coap/coap_client.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/subsys/net/lib/coap/coap_client.c b/subsys/net/lib/coap/coap_client.c index cfbf8c42c05..ebf99bade9f 100644 --- a/subsys/net/lib/coap/coap_client.c +++ b/subsys/net/lib/coap/coap_client.c @@ -24,7 +24,6 @@ static K_MUTEX_DEFINE(coap_client_mutex); static struct coap_client *clients[CONFIG_COAP_CLIENT_MAX_INSTANCES]; static int num_clients; static K_SEM_DEFINE(coap_client_recv_sem, 0, 1); -static atomic_t coap_client_recv_active; static bool timeout_expired(struct coap_client_internal_request *internal_req); static void cancel_requests_with(struct coap_client *client, int error); @@ -82,10 +81,7 @@ static int coap_client_schedule_poll(struct coap_client *client, int sock, memcpy(&internal_req->coap_request, req, sizeof(struct coap_client_request)); internal_req->request_ongoing = true; - if (!coap_client_recv_active) { - k_sem_give(&coap_client_recv_sem); - } - atomic_set(&coap_client_recv_active, 1); + k_sem_give(&coap_client_recv_sem); return 0; } @@ -956,7 +952,6 @@ static void cancel_requests_with(struct coap_client *client, int error) reset_internal_request(&client->requests[i]); } } - atomic_clear(&coap_client_recv_active); k_mutex_unlock(&client->lock); } @@ -974,7 +969,6 @@ void coap_client_recv(void *coap_cl, void *a, void *b) k_sem_take(&coap_client_recv_sem, K_FOREVER); while (true) { - atomic_set(&coap_client_recv_active, 1); ret = handle_poll(); if (ret < 0) { /* Error in polling */ @@ -987,7 +981,6 @@ void coap_client_recv(void *coap_cl, void *a, void *b) continue; } else { idle: - atomic_set(&coap_client_recv_active, 0); k_sem_take(&coap_client_recv_sem, K_FOREVER); } }