Skip to content
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

Zephyr coap client #56970

Merged
merged 3 commits into from
May 26, 2023
Merged

Conversation

jarlamsa
Copy link
Contributor

Add an asynchronous coap client.
The coap client processes requests, sends those to server and awaits for response.
Currently supports 1 request at a time.

@jarlamsa
Copy link
Contributor Author

@SeppoTakalo @juhaylinen @plskeggs please review.

Copy link
Collaborator

@pdgendt pdgendt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs more thought before being merged.

subsys/net/lib/coap/CMakeLists.txt Show resolved Hide resolved
subsys/net/lib/coap/coap_client.c Outdated Show resolved Hide resolved
tests/net/lib/coap_client/CMakeLists.txt Show resolved Hide resolved
include/zephyr/net/coap_client.h Outdated Show resolved Hide resolved
include/zephyr/net/coap_client.h Outdated Show resolved Hide resolved
include/zephyr/net/coap_client.h Outdated Show resolved Hide resolved
include/zephyr/net/coap_client.h Outdated Show resolved Hide resolved
include/zephyr/net/coap_client.h Outdated Show resolved Hide resolved
Copy link
Contributor

@plskeggs plskeggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good start for use cases where a single request/response at a time is all that is needed.

I wonder if there are use cases for low power IoT devices which would want to POST or PUT multiple items (for example, sensor data readings), and not wait for ACK between each one, but instead handle that at the end. Overlapping transfers could save battery life in that situation.

subsys/net/lib/coap/coap_client.c Outdated Show resolved Hide resolved
subsys/net/lib/coap/coap_client.c Outdated Show resolved Hide resolved
subsys/net/lib/coap/coap_client.c Outdated Show resolved Hide resolved
subsys/net/lib/coap/Kconfig Outdated Show resolved Hide resolved
subsys/net/lib/coap/coap_client.c Outdated Show resolved Hide resolved
subsys/net/lib/coap/coap_client.c Outdated Show resolved Hide resolved
subsys/net/lib/coap/coap_client.c Outdated Show resolved Hide resolved
@jarlamsa jarlamsa force-pushed the zephyr_coap_client branch 2 times, most recently from a84ceeb to e8b5cd2 Compare April 26, 2023 07:13
include/zephyr/net/coap_client.h Outdated Show resolved Hide resolved
subsys/net/lib/coap/coap_client.c Outdated Show resolved Hide resolved
include/zephyr/net/coap_client.h Outdated Show resolved Hide resolved
/**
* @brief Internal structure of CoAP client, shouldn't be modified.
*/
struct coap_client {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation has a stack per CoAP client, if I were to do multiple requests at the same time, I either need to do these sequentially or create a client for each request, which is very costly.
This becomes even more apparent if I need to observe multiple server resources (long lasting requests).

It would be great if a single client could support multiple requests.
I think this is doable if you would move some members from struct coap_client to struct coap_client_req to keep track if a single request state. And let the client keep track of al ongoing requests (for example in a linked list).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The multiple request is planned also, and doesn't need changes to the public API.

include/zephyr/net/coap_client.h Outdated Show resolved Hide resolved
include/zephyr/net/coap_client.h Show resolved Hide resolved
struct coap_client_request *coap_request;
struct coap_packet request;
k_tid_t tid;
struct k_thread thread;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having to spawn a separate thread for each client context seems like a lot of RAM overhead - couldn't the library have a single thread that would monitor all active requests (i. e. sockets)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any feedback on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, will be done in the future, as well as the multi-request version of the client.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, if there are plans to improve this in the future then I guess we can accept that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would've liked to have this functionality in this PR, as users will need to update/refactor their code in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't such a change be transparent for the applications? Yes, the application allocates the client context structure, but it's internals should not be used by the app (coap_client_init() configures the thread).

include/zephyr/net/coap_client.h Outdated Show resolved Hide resolved
subsys/net/lib/coap/coap_client.c Outdated Show resolved Hide resolved
subsys/net/lib/coap/coap_client.c Show resolved Hide resolved
subsys/net/lib/coap/coap_client.c Show resolved Hide resolved
subsys/net/lib/coap/coap_client.c Outdated Show resolved Hide resolved
subsys/net/lib/coap/coap_client.c Outdated Show resolved Hide resolved
subsys/net/lib/coap/coap_client.c Outdated Show resolved Hide resolved
@jarlamsa jarlamsa force-pushed the zephyr_coap_client branch 4 times, most recently from 4e65469 to c8426f7 Compare May 3, 2023 12:31
SeppoTakalo
SeppoTakalo previously approved these changes May 17, 2023
@martinjaeger
Copy link
Member

Nice work!

Could you please add a reference to this new API to the docs, so that the Doxygen comments are rendered and people are aware that the CoAP client exists?

@jarlamsa
Copy link
Contributor Author

Nice work!

Could you please add a reference to this new API to the docs, so that the Doxygen comments are rendered and people are aware that the CoAP client exists?

Yes, I plan to do that in a separate PR and add also similar usage example as is done with the HTTP-client.

@jarlamsa jarlamsa requested a review from rlubos May 22, 2023 07:01
include/zephyr/net/coap_client.h Outdated Show resolved Hide resolved
subsys/net/lib/coap/coap_client.c Outdated Show resolved Hide resolved
@jarlamsa
Copy link
Contributor Author

Nice work!

Could you please add a reference to this new API to the docs, so that the Doxygen comments are rendered and people are aware that the CoAP client exists?

I was planning on adding the documentation to different PR, but as there was still some changes, I pushed the documentation to this PR as well.

subsys/net/lib/coap/Kconfig Outdated Show resolved Hide resolved
@jarlamsa
Copy link
Contributor Author

Thank you @SeppoTakalo for your review comments, I have addressed those now. Please re-review.

Jarno Lämsä added 3 commits May 25, 2023 15:28
The coap client takes requests and provides responses
asynchronously to callback given in a request.
Currently supports only 1 request at a time.

Signed-off-by: Jarno Lämsä <jarno.lamsa@nordicsemi.no>
Add tests for coap client and stubs for isolating the tests.

Signed-off-by: Jarno Lämsä <jarno.lamsa@nordicsemi.no>
Sample usage and documentation of the CoAP client.

Signed-off-by: Jarno Lämsä <jarno.lamsa@nordicsemi.no>
subsys/net/lib/coap/coap_client.c Show resolved Hide resolved
@jarlamsa
Copy link
Contributor Author

@rlubos please re-review.

@jarlamsa
Copy link
Contributor Author

This now has 2 approvals with write access, could we get this merged in? @jukkar

@nashif nashif merged commit 3f8c129 into zephyrproject-rtos:main May 26, 2023
@jarlamsa jarlamsa deleted the zephyr_coap_client branch May 26, 2023 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants