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

Gree: Fix reporting vertical swing #2125

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

gentoo-root
Copy link
Contributor

toCommonSwingV() is only called when SwingAuto is false, but it converts kGreeSwingLastPos to kAuto. It doesn't make sense, because:

  1. kGreeSwingLastPos means that swinging is stopped (i.e. the shutter remains in its last position), which corresponds to kOff.
  2. kAuto shouldn't be returned from this function at all, because it's handled separately in toCommon() when SwingAuto is true.
  3. As can be seen in setSwingVertical(), when automatic is false, the valid set of positions includes kGreeSwingLastPos, but not kGreeSwingAuto.

Fix the logic by amending toCommonSwingV() according to the considerations above. It fixes parsing of received IR packets when the user disables vertical swinging from the remote (tested with YAP1FB).

For consistency and robustness, educate setSwingVertical() and convertSwingV() about the supported kGreeSwingLastPos mode.

Add a unit test for the described bug.

toCommonSwingV() is only called when SwingAuto is false, but it converts
kGreeSwingLastPos to kAuto. It doesn't make sense, because:

1. kGreeSwingLastPos means that swinging is stopped (i.e. the shutter
   remains in its last position), which corresponds to kOff.
2. kAuto shouldn't be returned from this function at all, because it's
   handled separately in toCommon() when SwingAuto is true.
3. As can be seen in setSwingVertical(), when automatic is false, the
   valid set of positions includes kGreeSwingLastPos, but not
   kGreeSwingAuto.

Fix the logic by amending toCommonSwingV() according to the
considerations above. It fixes parsing of received IR packets when the
user disables vertical swinging from the remote (tested with YAP1FB).

For consistency and robustness, educate setSwingVertical() and
convertSwingV() about the supported kGreeSwingLastPos mode.

Add a unit test for the described bug.
@NiKiZe
Copy link
Collaborator

NiKiZe commented Sep 5, 2024

I'm curious if there could be any case where the existing behavior is the wanted one. (Don't want to break anything, at least not without documentation)

@gentoo-root
Copy link
Contributor Author

if there could be any case where the existing behavior is the wanted one

Well, given the considerations that I gave in the commit message, it doesn't look like it was intended. Too many things don't add up. Plus I added a unit test that sets kOff and checks if the reported state becomes kOff (it wasn't the case before my patch) — I don't think any other result should be expected after setting kOff :)

@NiKiZe
Copy link
Collaborator

NiKiZe commented Sep 5, 2024

Yes your test case is perfect. And at least also shows that there isn't any existing cases that we expect to behave differently.

To be honest I don't follow the explanation fully. (I don't disagree)
Reading again I do fully agree.

@NiKiZe NiKiZe merged commit 8a049f5 into crankyoldgit:master Sep 5, 2024
42 checks passed
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