Add basic support for YX1F8F remote#2005
Conversation
|
I should also add that this remote and AC unit has the following temperature range: 60 to 90 Fahrenheit, which is not what the current safe min/max is in the header file. I'm not sure if it is worth the effort to create a second set of "safe" minimums just for this remote, and the few extra degrees are not needed by me so I did not bother to implement it, but would be willing if the request is warranted. |
Limb
force-pushed
the
YX1F8F_Clean
branch
2 times, most recently
from
8a706a5 to
4836964
Compare
NiKiZe
left a comment
NiKiZe
left a comment
There was a problem hiding this comment.
Sorry for the delay in review, see inline review comments
| YBOFB, // (2) Green, YBOFB2, YAPOF3 | ||
| YX1FSF, // (3) Soleus Air window unit (Similar to YAW1F, but with an | ||
| // Operation mode of Energy Saver (Econo)) | ||
| YX1F8F, // Similiar to YX1FSF |
There was a problem hiding this comment.
Anything that differs?
As a reader of just the comment I wonder why it is needed, hopefully this can be improved on to help other readers understand it. Maybe: "Mostly identical to YX1FSF but different identification"
There was a problem hiding this comment.
The hard part with explaining why the model is different is because the parts of the protocol that are identified and named in the code remain the same for this model. The only differences are that this particular remote has a different set of static "unknowns" than the other models, along with the addition of the ModelA bit needing to be set like the YAW1F remote.
I don't know if it adds benefit to explain that the model is "Similiar to YX1FSF, except for different unknowns". Seems it would add more confusion than clarity?
There was a problem hiding this comment.
How about "same as YAW1F, model bits differs"
| if (_model == gree_ac_remote_model_t::YX1F8F) { | ||
| _.unknown1 = 10; // _.remote_state[3] = 0x0A; | ||
| _.unknown2 = 0; // _.remote_state[5] = 0x01; | ||
| _.unknown3 = 1; // _.remote_state[5] = 0x01; | ||
| } else { | ||
| _.unknown1 = 5; // _.remote_state[3] = 0x50; | ||
| _.unknown2 = 4; // _.remote_state[5] = 0x20; | ||
| _.unknown3 = 0; // _.remote_state[5] = 0x20; | ||
| } |
There was a problem hiding this comment.
Indentation seems of YX1F8F section should have identation of 2 spaces, not 4?
| uint8_t unknown2 :3; // value = 0b100 | ||
| uint8_t WiFi :1; | ||
| uint8_t :1; | ||
| uint8_t unknown3 :1; // value = 0b0 |
There was a problem hiding this comment.
Should unknown comments be updated to reflect the remote model, maybe with reference like See stateReset or similar?
There was a problem hiding this comment.
I agree that I think this can be a more useful comment. Perhaps all the comments for unknowns should be updated to something like "Value depends on remote model, see stateReset".
Thinking ahead I could potentially see listing the model names on the comments becoming a tedious list to maintain in a comment, so I don't think we should list the initial values for the respective models in the comment. This would only be an issue if more models were supported that required different values here though,
There was a problem hiding this comment.
Yes I'm thinking // remote model bits, see stateReset on all of them.
2 participants
This merge request adds basic support for the YX1F8F remote. This includes support for setting temperature, fan, and mode (including a new eco mode).
It does not currently include support for using the timer functionality. That requires sending 3 packets, of which the middle packet seems to contain an internal timer countdown reference that is tracked by the remote. I have mapped that packet out, but need to see if the timer value actually affects the internal timer on the AC unit or not.