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

Refactor to use one request object #84

Merged
merged 19 commits into from
Feb 8, 2024
Merged

Refactor to use one request object #84

merged 19 commits into from
Feb 8, 2024

Conversation

lanwin
Copy link
Owner

@lanwin lanwin commented Feb 7, 2024

This approach is simple and allows batching of changes.

Currently work in progress.

@lanwin
Copy link
Owner Author

lanwin commented Feb 7, 2024

@atanasenko since it seems you have experience in c++ programming. What do you think about that approach?

A problem is that optional type. std::optional is not supported yet since esphome uses c++ 14. And I can not use the esphome::optional since this one is not available for my test script. That should run on my local window machine where I have the esphome source not available.

@atanasenko
Copy link
Contributor

I'm a java dev mostly, and have very limited experience with cpp, unfortunately. Most of what I did in those prs was copy-paste :)

@atanasenko
Copy link
Contributor

atanasenko commented Feb 7, 2024

But I was looking at the transport code and I think requests could be batched, but on a lower, packet level.
For example, when changes are requested, they get appended to an existing queued packet (as long as packet type is the same) instead of sending a separate packet. Then the sending loop will try to send a single packet at the front of the queue and will only remove it once it's acked. Then it could retry after a while, and timeout completely if unable to send at some point.

@lanwin
Copy link
Owner Author

lanwin commented Feb 7, 2024

I'm a java dev mostly, and have very limited experience with cpp, unfortunately. Most of what I did in those prs was copy-paste :)

Well the you are pretty fast in copy and paste :D

@lanwin
Copy link
Owner Author

lanwin commented Feb 7, 2024

Yes that is true. But my goal with the request object is also to reduce the api.

@lanwin lanwin changed the title WIP Refactor to use one request object Refactor to use one request object Feb 8, 2024
@lanwin lanwin merged commit 2b0330c into main Feb 8, 2024
3 checks passed
@lanwin lanwin deleted the refactor_request branch February 8, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants