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.

Sign up for GitHub

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 basic support for YX1F8F remote #2005

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add basic support for YX1F8F remote #2005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c317316
Initial YX1F8F support
Limb May 16, 2023
348e097
Add modelA
Limb Jun 14, 2023
5cb2326
Code lint cleanup
Limb Jun 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/IRsend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ enum gree_ac_remote_model_t {
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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"

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about "same as YAW1F, model bits differs"

};

/// HAIER_AC176 A/C model numbers
Expand Down
18 changes: 14 additions & 4 deletions src/ir_Gree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,16 @@ void IRGreeAC::stateReset(void) {
std::memset(_.remote_state, 0, sizeof _.remote_state);
_.Temp = 9; // _.remote_state[1] = 0x09;
_.Light = true; // _.remote_state[2] = 0x20;
_.unknown1 = 5; // _.remote_state[3] = 0x50;
_.unknown2 = 4; // _.remote_state[5] = 0x20;

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;
}
Comment on lines +126 to +134
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation seems of YX1F8F section should have identation of 2 spaces, not 4?

}

/// Fix up the internal state so it is correct.
Expand Down Expand Up @@ -188,7 +196,8 @@ void IRGreeAC::setModel(const gree_ac_remote_model_t model) {
switch (model) {
case gree_ac_remote_model_t::YAW1F:
case gree_ac_remote_model_t::YBOFB:
case gree_ac_remote_model_t::YX1FSF: _model = model; break;
case gree_ac_remote_model_t::YX1FSF:
case gree_ac_remote_model_t::YX1F8F: _model = model; break;
default: _model = gree_ac_remote_model_t::YAW1F;
}
}
Expand All @@ -209,7 +218,8 @@ void IRGreeAC::off(void) { setPower(false); }
void IRGreeAC::setPower(const bool on) {
_.Power = on;
// May not be needed. See #814
_.ModelA = (on && _model == gree_ac_remote_model_t::YAW1F);
_.ModelA = (on && (_model == gree_ac_remote_model_t::YAW1F
|| _model == gree_ac_remote_model_t::YX1F8F));
}

/// Get the value of the current power setting.
Expand Down
3 changes: 2 additions & 1 deletion src/ir_Gree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
// Brand: Vailland, Model: YACIFB remote
// Brand: Vailland, Model: VAI5-035WNI A/C
// Brand: Soleus Air, Model: window A/C (YX1FSF)
// Brand: Frigidaire, Model: YX1F8F remote

#ifndef IR_GREE_H_
#define IR_GREE_H_
Expand Down Expand Up @@ -76,7 +77,7 @@ union GreeProtocol{
uint8_t IFeel :1;
uint8_t unknown2 :3; // value = 0b100
uint8_t WiFi :1;
uint8_t :1;
uint8_t unknown3 :1; // value = 0b0
Comment on lines 78 to +80
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should unknown comments be updated to reflect the remote model, maybe with reference like See stateReset or similar?

Copy link
Author

Choose a reason for hiding this comment

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

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,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I'm thinking // remote model bits, see stateReset on all of them.

// Byte 6
uint8_t :8;
// Byte 7
Expand Down