net: coap: Pull various fixes to CoAP client #2205
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Oct 31, 2024
SeppoTakalo
force-pushed
the
coap_client_nrf_fix
branch
2 times, most recently
from
135bbfb to
3def4ab
Compare
|
I'm puzzled why wasn't the compliance issue reported in the upstream PR?
About the Commit tags failure, were the commits modified somehow? Note, we should not do that, if there are conflicts, pull dependencies. |
|
@carlescufi IMO this qualifies for an exception from the conflict-free cherry-pick rule, fixing dependencies would require to pull this tree-wide change, which does not apply cleanly on it's own: |
juhaylinen
approved these changes
Oct 31, 2024
SeppoTakalo
force-pushed
the
coap_client_nrf_fix
branch
from
3def4ab to
08f0a94
Compare
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ś <[email protected]> (cherry picked from commit 70c9bf1)
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 <[email protected]> (cherry picked from commit bfed8d0)
…llocated 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 <[email protected]> (cherry picked from commit 13cad59)
Passing MACRO arguments that need expansion require _CONCAT. Signed-off-by: Pieter De Gendt <[email protected]> (cherry picked from commit 476c12f)
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 <[email protected]> (cherry picked from commit 736344b)
…rable 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 <[email protected]> (cherry picked from commit 98289e5)
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 <[email protected]> (cherry picked from commit f8a7035)
Protect global list of clients with mutex. Signed-off-by: Seppo Takalo <[email protected]> (cherry picked from commit 1ea569d)
…ailed 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 <[email protected]> (cherry picked from commit 46b7c84)
…etrying 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 <[email protected]> (cherry picked from commit 4c6dd4c)
…ng loop Forward recv() errors to handle_poll(), so there is only one place to handle error codes. Signed-off-by: Seppo Takalo <[email protected]> (cherry picked from commit 6481b0e)
… 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 <[email protected]> (cherry picked from commit 623a1ff)
…stead 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 <[email protected]> (cherry picked from commit a14f083)
Fix handling of received CoAP reset. Signed-off-by: Seppo Takalo <[email protected]> (cherry picked from commit 1890dbd)
Add helper API to construct CoAP Reset message. Signed-off-by: Seppo Takalo <[email protected]> (cherry picked from commit e96e95b)
Response tokens are already compared in get_request_with_token(). Signed-off-by: Seppo Takalo <[email protected]> (cherry picked from commit 1dc2487)
When receiving unknown response, respond with CoAP Reset. Signed-off-by: Seppo Takalo <[email protected]> (cherry picked from commit 350d20e)
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 <[email protected]> (cherry picked from commit 9c9dc9f)
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 <[email protected]> (cherry picked from commit 1e5a537)
SeppoTakalo
force-pushed
the
coap_client_nrf_fix
branch
from
08f0a94 to
5c922fc
Compare
|
Updated the PR. Also fixed some merging issues as I previously had some commits in wrong order. Should apply more cleanly now. |
|
a bit unfortunate that our CI runs those compliance checks that upsream did not. Now it complains about styling on commits that are already merged in. |
I guess there's no way around then other than taking this as is. |
rlubos
approved these changes
Nov 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull fixes from upstream to coap_client used by nRF Cloud libraries.
Internal issue reported: NCSDK-29890