-
Notifications
You must be signed in to change notification settings - Fork 6.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
net: lib: coap_client: observe-related fixes #72948
net: lib: coap_client: observe-related fixes #72948
Conversation
@glarsennordic ATTN |
abcc6ad
to
dde8ad7
Compare
subsys/net/lib/coap/coap_client.c
Outdated
@@ -779,6 +781,16 @@ static int handle_response(struct coap_client *client, const struct coap_packet | |||
} | |||
} | |||
|
|||
/* MID-based deduplication */ | |||
uint16_t response_id = coap_header_get_id(response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually try to introduce variables at the start of the block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That policy isn't even followed by the function I added the variable to :D but sure, fixed
LGTM! But being honest, I don't really have any horses in this race, other than the |
An earlier pull request implementing observe support was merged too hastily. It had a few issues: 1. The predicate for whether a request should be marked not ongoing was wrong (it checked ret != 0 instead of ret < 0) 2. Without observes in mind, MID-based deduplication is not a required feature. Deduplication was handled implicitly - the exchange would get dropped after the first response anyway, so duplicate responses would not get matched to anything. But with observes, there are several responses in an exchange. This commit adds this. 3. Using coap_request_is_observe(&internal_req->request) in the response handler requires the whole request to stay in scope for the lifetime of the observation, which I observed was not always the case. Adding an is_observe bool to the internal struct improved stability significantly. With these fixes, GETs with observe option works very well. Signed-off-by: Benjamin Lindqvist <benjamin@eub.se>
dde8ad7
to
e983684
Compare
An earlier pull request implementing observe support was merged too hastily. It had a few issues:
With these fixes, GETs with observe option works very well.