-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add support for stateful ACs using IRremoteESP8266 #9472
Conversation
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.
Thanks for the PR. I made a suggestion to introduce a new field for StateOnly.
tasmota/xdrv_05_irremote_full.ino
Outdated
if (val = root[PSTR(D_JSON_IRHVAC_MODEL)]) { state.model = IRac::strToModel(val.getStr()); } | ||
bool stateOnly = false; // flag that means to update the state of IRremote without sending actual IR command | ||
// parse model of the ac | ||
if (val = root[PSTR(D_JSON_IRHVAC_MODEL)]) { |
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.
I'm not a big fan of changing the semantic of an existing field. I would rather have a "StateOnly":true field instead.
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.
I totally agree. Just not sure about the right processes here to add a new field. Any guidelines? Is this trivial? Should I just try to do it or do you prefer to write this part yourself? Will you accept PRs that add fields, options, etc?
Overall I understand the IR/AC part well but more cautious about the Tasmota part.
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.
Just add a new field. As it's JSON it's position isn't a problem and ignoring the field shouldn't be a problem either.
.gitignore
Outdated
@@ -7,6 +7,7 @@ | |||
.platformio | |||
.pioenvs | |||
.piolibdeps | |||
.pio/libdeps |
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.
Why do you need this one?
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.
hmmm. I think I needed this because at some point I used a dev branch of IRremote and PIO would create some temp files in .pio/libdeps .
It seems the .piolibdeps
folder (one line above in current file) has been replaced with .pio/libdeps
in newer versions of PIO.
https://community.platformio.org/t/piolibdeps-folder/8830
we can remove it from here and add in another PR or somebody on the team can add it. whatever you prefer.
I did a extra PR for the corrected .gitignore. |
hey, I have made the requested changes. Please review and send feedback. |
Thanks, looks good to me. |
great. should we do something with the documentation? what is the process? |
Yes pls do. Follow this https://tasmota.github.io/docs/Contributing/#editing-articles |
@ayavilevich Thanks for the PR! |
Description:
Tasmota uses IRremoteESP8266 for IR HVAC functionality, the relevant code is in
tasmota\xdrv_05_irremote_full.ino
. Specifically,IRAcUtils::decodeToState
andac.sendAc
functions are used. Those functions expect the caller to pass a previous AC state to them. This is required to support Air Conditioning devices that use a differential IR protocol (they send changes rather than state). Without considering the previous state, for these devices, any IRHVAC instruction where power==on will be transmitted as "toggle power" by IRremoteESP8266. This completely breaks all IRhvac functionality for such vendors as Whirlpool, Airwell and others.This PR implements a working POC/feature where a state is added to the logic.
The functionality can be implemented as an "option".
Also a POC for setting the initial state is implemented as a piggy-back on top of an existing "model" field. This is useful if one wants to sync the state with the actual state of the AC without generating an IR transmission.
The code is relatively simple. State transitions are implemented in IRremoteESP8266. Tasmota merely needs to hold the structure and pass it around.
If you wish to improve on this implementation I would suggest an additional "option" for enabling this state management (can be false by default). Also a special field can be added to the IRhvac command (https://tasmota.github.io/docs/Tasmota-IR/#sending-irhvac-commands) to signal that only a state update is required.
This was not tested functionally on an ESP32. There is nothing in the code that seems to be platform specific. I don't have an ESP32 based IR blaster, those are not common. The build passes though. If this is a big issue LMK.
Checklist:
@crankyoldgit fyi