esphome-daikin-s21
esphome-daikin-s21 copied to clipboard
joshbenner
Add serial command state machine to prevent blocking esphome main loop
- setup() initializes a pool of commands/queries to be periodically run
- loop() transmits each command/query and waits for the response, yielding control if it's not ready
- Changes to settings must be staged and applied asynchronously
- All of this requires much more saved state in the DaikinS21 object
Other changes:
- Add ETX -> ENQ checksum character escaping
- Lower minimum setpoint (for my basement)
- Replace some std::vector use with const strings or sized local buffers (embedded habits die hard)
- Add support for reading out quiet fan mode from RG query
This is a request for comments pull request.
I noticed when doing a lot of communication the component will block for longer than esphome likes. Do it too much and basic functionality stops working. So here's a communications state machine that allows loop() to yield and resume the queries if there's nothing to do. Unfortunately, that means quite a few changes so Github's diff isn't that helpful.
I can post a bunch of comments explaining what's going on if you like the basic idea
I am midway through abstracting the serial state machine from the command processing to make it less messy and easier to follow. Will update the PR when that's done.
Hey @asund, do you think this is ready to be reviewed and merged?
Sure. My biggest concern with this one is dumping a bunch of drastic changes on the project and forcing others to read and understand them. Please let me know if I've overstepped by changing anything too much or making unnecessary changes that could be their own pull requests with discussion (lowering the minimum temperature, etc).
Questions:
- How do people like the enum+bitset use instead of a bunch of bools? Can easily switch to bools.
- Are we ok with a single NAK removing a query from the pool? I can add a three strikes rule or something else if it's considered risky.
- Is the iterator use with the query pool cumbersome? Should I just use an index?
At a glance, this all looks compelling. I lack the time at this point to give this a good review, and I'm currently not setup to test even relatively simple PRs. Your interest and time investment suggest to me that you should fork and grow this beyond what it currently is, because I don't have the time to put into taking care of it right now. I'll happily add a forwarding link to the README if you (or someone else?) take over.
Thanks, I have a bit of free time now and then and can probably pitch in in a larger capacity. I am a C++ developer and don't know the ESPhome side very well. @benwa you seem pretty active, would you be interested in helping out with a fork? Is there a way to have a project with a couple admins?
I'm by no means a developer (a sysadmin by trade), but I'm happy to assist however I can.
GitHub does allow for different levels of permissions.
At a glance, this all looks compelling. I lack the time at this point to give this a good review, and I'm currently not setup to test even relatively simple PRs. Your interest and time investment suggest to me that you should fork and grow this beyond what it currently is, because I don't have the time to put into taking care of it right now. I'll happily add a forwarding link to the README if you (or someone else?) take over.
@asund has put in a whole lot of effort on their fork. Do you think you can add the notice to point to https://github.com/asund/esphome-daikin-s21?
I do need to re-add support for the basic commands my unit doesn't support before I can say it's not a regression for some users, but I'm hoping to do that soon.