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

Panasonic AC - Fix Low and High fan speed #1515

Merged
merged 7 commits into from
Jul 4, 2021
Merged

Panasonic AC - Fix Low and High fan speed #1515

merged 7 commits into from
Jul 4, 2021

Conversation

ajaypala
Copy link
Contributor

@ajaypala ajaypala commented Jul 4, 2021

Added the missing low and high fan speeds for Panasonic AC. Regression from previous. Tested with my Panasonic unit.

ajaypala and others added 4 commits July 4, 2021 16:25
Add Fan low and high speeds to setFan
Add low and high fan speed.
Added/corrected test for fan low and high speed.
Update the fan speed elsewhere.
@crankyoldgit crankyoldgit requested review from NiKiZe and removed request for crankyoldgit July 4, 2021 08:05
@crankyoldgit
Copy link
Owner

Removing myself as a reviewer as I've now made significant changes myself.
I'm happy with it as it is now. i.e. LGTM

Over to @NiKiZe for an impartial code review.

@ajaypala
Copy link
Contributor Author

ajaypala commented Jul 4, 2021

LGTM.
@crankyoldgit Thanks for all your efforts on this library.

Copy link
Collaborator

@NiKiZe NiKiZe left a comment

Choose a reason for hiding this comment

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

This might give unexpected behaviour for any current consumers of fanspeeds?
Also what happens if this gets set on a unit that does not support it? Any chance for 🔥?

@ajaypala
Copy link
Contributor Author

ajaypala commented Jul 4, 2021

This might give unexpected behaviour for any current consumers of fanspeeds?
Also what happens if this gets set on a unit that does not support it? Any chance for fire?

I don't think it will affect current consumers unless they are intentionally setting the fan speed to Low or High, in which case the result would be auto fan speed.

@crankyoldgit
Copy link
Owner

This might give unexpected behaviour for any current consumers of fanspeeds?
Also what happens if this gets set on a unit that does not support it? Any chance for fire?

I don't think it will affect current consumers unless they are intentionally setting the fan speed to Low or High, in which case the result would be auto fan speed.

Looking at the rest of the code, we clearly expected those values. It looks like we lost the ability to set them in a re-factor along the way. Now you've made me look: ;-)

e.g. Prior to #934 it was:

void IRPanasonicAc::setFan(const uint8_t speed) {
  if (speed <= kPanasonicAcFanMax || speed == kPanasonicAcFanAuto)
    remote_state[16] =
        (remote_state[16] & 0x0F) | ((speed + kPanasonicAcFanOffset) << 4);
}

So I clearly screwed that up!

@crankyoldgit crankyoldgit merged commit 3c1862f into crankyoldgit:master Jul 4, 2021
crankyoldgit added a commit that referenced this pull request Jul 6, 2021
_v2.7.19 (20210706)_

**[Bug Fixes]**
- Illegal Heap write in rawbuf when the capture has overflowed. (#1516 #1517)
- PANASONIC_AC: Fix Low and High fan speeds (#1515)
- Fix MDNS in IRServer and IRMQTTServer example code (#1498 #1499)
- IRac: Fix off-by-one error in Coolix's sleep setting. (#1500)
- Fix undefined constant (#1490)

**[Features]**
- Add detailed support for Kelon ACs (#1494)
- Experimental basic support for Teknopoint A/C protocol (#1486 #1504)
- Daikin64: Add support for Heat mode (#1492)
- Basic support for `HAIER_AC176` 176 bit protocol. (#1480 #1481)

**[Misc]**
- GREE: Update inter-message gap timing (#1508 #1509)
- IRac: Change Coolix to send special messages after a normal message. (#1501 #1502)
- Fix compiler warnings causing Travis failures. (#1491)
- Update supported model info (#1477 #1485 #1488 #1489)
- Add HTML viewport meta tag to IRServer and IRMQTTServer examples (#1467 #1469)
@crankyoldgit crankyoldgit mentioned this pull request Jul 6, 2021
crankyoldgit added a commit that referenced this pull request Jul 6, 2021
* Regenerate Doxygen documentation

* v2.7.19 release
_v2.7.19 (20210706)_

**[Bug Fixes]**
- Illegal Heap write in rawbuf when the capture has overflowed. (#1516 #1517)
- PANASONIC_AC: Fix Low and High fan speeds (#1515)
- Fix MDNS in IRServer and IRMQTTServer example code (#1498 #1499)
- IRac: Fix off-by-one error in Coolix's sleep setting. (#1500)
- Fix undefined constant (#1490)

**[Features]**
- Add detailed support for Kelon ACs (#1494)
- Experimental basic support for Teknopoint A/C protocol (#1486 #1504)
- Daikin64: Add support for Heat mode (#1492)
- Basic support for `HAIER_AC176` 176 bit protocol. (#1480 #1481)

**[Misc]**
- GREE: Update inter-message gap timing (#1508 #1509)
- IRac: Change Coolix to send special messages after a normal message. (#1501 #1502)
- Fix compiler warnings causing Travis failures. (#1491)
- Update supported model info (#1477 #1485 #1488 #1489)
- Add HTML viewport meta tag to IRServer and IRMQTTServer examples (#1467 #1469)
@crankyoldgit
Copy link
Owner

FYI, the committed & merged changes have been included in the newly released version of the library. i.e. v2.7.19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants