-
Notifications
You must be signed in to change notification settings - Fork 878
Add support for the Eurom A/C protocol #2208
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
base: master
Are you sure you want to change the base?
Conversation
src/ir_Eurom.cpp
Outdated
if (fahrenheit) { | ||
temp_f = std::max(kEuromMinTempF, degrees); | ||
temp_f = std::min(kEuromMaxTempF, temp_f); | ||
temp_c = static_cast<uint8_t>(std::round(fahrenheitToCelsius(temp_f))); |
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.
This seems to work fine locally, but the pipeline doesn't like std::round
. What is the preferred method of achieving a rounded value for this library?
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.
Not sure why it doesn't like std::round
.
No real preference, have a look at other similar code.
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 found pretty much the same thing in IRac.cpp
: ac->setTemp(static_cast<uint8_t>(roundf(degrees)));
I might just need to #include <cmath>
, as well as <cstring>
for std::memcpy
(I just noticed the pipeline also complained about that one). I'm on macOS and apparently it's actually using a "special" Apple version of clang instead of c++/g++, so perhaps that just magically includes these libraries anyway?
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.
Most Arduino libraries/code uses roundf
instead of std::round
because of the smaller binary footprint of roundf
😉
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.
Good point. :>
// Byte 1 is used as part of the checksum only and is always 0x27 | ||
uint8_t Sum2 :8; | ||
|
||
// Byte 2 combines 2 functions and has some considerations: |
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.
Note: you can split it up into nibbles (and even single bits) in the date structure, and save yourself a lot of code doing bitwise logic operations to manipulate the data. It will also make your code more readable & simpler to follow.
E.g.
uint8_t Mode :4;
uint8_t Celsius :4;
Instead of uint8_t Mode_Celsius :8;
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.
Yeah I did consider doing that, but it didn't really seem any better because the special case of 32 C actually involves both nibbles.
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.
Did you guys still want me to change this? 😅
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.
So should mode be 3 bits, and temp 5?
Another alternative is to use a join?
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.
Perhaps the problem becomes clearer if I write out the full sequence for this particular byte:
0x01
= 16 C in cooling mode (0x04
in heating mode)0x11
= 17 C (0x14
for heating)0x21
= 18 C (0x24
for heating, you get the idea)0x31
= 19 C0x41
= 20 C0x51
= 21 C0x61
= 22 C0x71
= 23 C (this seems to be the "standard" setting, since dehumidification and ventilation modes are0x72
and0x73
respectively, which makes heating mode's "standard" setting0x74
)0x81
= 24 C0x91
= 25 C0xA1
= 26 C0xB1
= 27 C0xC1
= 28 C0xD1
= 29 C0xE1
= 30 C0xF1
= 31 C (0xF4
for heating, meaning so far it's basically always + 1 on the high nibble)0x09
= 32 C (0x0C
for heating, which both break the above pattern)
I don't really see how to represent either the mode or the temperature with just a partial byte, at least not in all cases. Splitting it into 5 and 3 bits seems like it actually might work for cooling and heating modes, but dehumidification and ventilation modes are always 0x73
or 0x74
. In those cases it no longer makes sense to have a split byte anyway, since those modes don't actually have a temperature setting. It would sort of correspond to 23 C (as in 0x71
and 0x74
) and I could just make that assumption in the code, but that doesn't really seem very clean either.
Builds are failing because a deprecated github actions/cache@v2 module is used 🤔 should probably be fixed in a separate PR (and maybe by one of the regular maintainers?) |
See #2209 |
Title says it all really. :> The entire protocol is documented within the code because that seemed like the most logical place, especially inside the
union EuromProtocol
. Do note though that I only have access to thePolar 16CH
model, there's also a12CH
variant but that doesn't support heating. I don't know if that unit uses the same protocol or not (or even how units from completely different product lines work). I did have a very old unit that finally died and it just used NEC, in any case.I'm already using this remote in "production", so all features should be working as expected.