Initial support for York AC protocol

[danielegobbetti]

As mentioned in #1882 (comment) I'm trying to add support for the protocol used by my (pretty old) AC unit by York (AC model: MHH07P17 remote GRYLH2A).

Please consider that I have little to no experience with C++ and therefore have lots of questions, nevertheless I wanted to open the PR since basic decoding works. I am unable to figure out the checksum protocol used by this remote.

So to the questions I have:

• The AC unit has a timer for on and off power AND a sleep functionality (auto power off after some time): should I use OnTime or OnTimer in the "Native representation of the message"?
• the on/off timer base units are 10 minutes, the timer is sent as an offset (the base unit has no clock AFAIK). I noticed in the code that also the internal representation of the timer should be in units of 10 minutes, but when using "minsToString" the output is in minutes. Should I multiply by 10?
• The AC unit supports Vertical Swing but only in a on/off fashion: I am unsure of the mapping and of the usage of addSwingVToString
• It is not clear to me what the "sleep" supported feature (i.e. result.sleep in toCommon()) actually is: is it meant to be a "night/low noise mode" or rather "power the unit of after some time" mode?
• The on/offTimer do not seem to have a corresponding field in the state_t struct. Should these be mapped to some combination of clock / sleep?

I am aware of having also left some "TODOs" in the code.

Looking forward to some feedback regarding the questions above (and in general about the code).

[crankyoldgit]

First off, thanks for creating a PR. It's a big effort, I know. So congrats for making it thus far.
Don't worry, we will walk you through it.

I'll answer some of your questions before I start looking at your code.

• The AC unit has a timer for on and off power AND a sleep functionality (auto power off after some time): should I use OnTime or OnTimer in the "Native representation of the message"?

"On Timer" i.e. kOnTimerStr

IRremoteESP8266/src/locale/defaults.h

Lines 90 to 95 in 726b219

	#ifndef D_STR_TIMER
	#define D_STR_TIMER "Timer"
	#endif // D_STR_TIMER
	#ifndef D_STR_ONTIMER
	#define D_STR_ONTIMER D_STR_ON " " D_STR_TIMER // Set `D_STR_ON` first!
	#endif // D_STR_ONTIMER

IRremoteESP8266/src/IRtext.cpp

Line 66 in 726b219

IRTEXT_CONST_STRING(kOnTimerStr, D_STR_ONTIMER); ///< "On Timer"

• the on/off timer base units are 10 minutes, the timer is sent as an offset (the base unit has no clock AFAIK). I noticed in the code that also the internal representation of the timer should be in units of 10 minutes, but when using "minsToString" the output is in minutes. Should I multiply by 10?

Please use the unit of Minutes. i.e. setFunctionName(nr_of_mins) and then divide by 10 etc internally. Ditto for a getFunctionName(). i.e. Multiply by 10. etc. This way it keeps things consistent between protocols. There are several other protocols that have this sort of limitation/implementation too.

• The AC unit supports Vertical Swing but only in a on/off fashion: I am unsure of the mapping and of the usage of addSwingVToString

Don't use addSwingVToString in this case, as it is just on or off, it is a boolean, so use addBoolToString
e.g.

IRremoteESP8266/src/ir_Neoclima.cpp

Line 523 in 9228143

result += addBoolToString(getSwingV(), kSwingVStr);

[crankyoldgit]

The automatic code linter found a number of issues, these should hopefully be easy to understand and fix:

src/IRrecv.h:866:  Redundant blank line at the end of a code block should be deleted.  [whitespace/blank_line] [3]
src/ir_York.cpp:55:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_York.cpp:5[9](https://github.com/crankyoldgit/IRremoteESP8266/actions/runs/3129077653/jobs/5077712679#step:6:10):  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_York.cpp:67:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_York.cpp:67:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_York.cpp:76:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_York.cpp:83:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_York.cpp:99:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_York.cpp:99:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_York.cpp:198:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_York.cpp:270:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
src/ir_York.cpp:279:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
src/ir_York.cpp:287:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
src/ir_York.cpp:295:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
src/ir_York.cpp:303:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
src/ir_York.cpp:303:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_York.cpp:345:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
src/ir_York.cpp:345:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_York.h:29:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_York.h:31:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_York.h:32:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_York.h:33:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_York.h:36:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_York.h:38:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_York.h:39:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_York.h:40:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_York.h:42:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_York.h:44:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_York.h:45:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_York.h:46:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_York.h:48:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_York.h:50:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_York.h:56:  Lines should be <= 80 characters long  [whitespace/line_length] [2]

[danielegobbetti]

Many thanks for the thorough review and for your suggestions.

As for the OnTimer / OffTimer, additionally to the changes you suggested I also had to use getOnTimer() and getOffTimer in the Message description (it's obvious in hindsight).

I just resolved the comments where no comments where needed and added two more questions to the opening message.

[crankyoldgit]

Still got a few Lint issues to address: (See: https://github.com/crankyoldgit/IRremoteESP8266/actions/runs/3134922966/jobs/5090021582)
i.e.

src/IRsend.h:845:  "protected:" should be preceded by a blank line  [whitespace/blank_line] [3]
src/ir_York.cpp:289:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
src/ir_York.cpp:306:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]

[crankyoldgit]

Thanks for making the other changes .. getting closer now.

[crankyoldgit]

• It is not clear to me what the "sleep" supported feature (i.e. result.sleep in toCommon()) actually is: is it meant to be a "night/low noise mode" or rather "power the unit of after some time" mode?

Ha! Welcome to the world of AirConditioners and different brand/models/implementations.

To keep "Sleep" generic, it means what ever it means for that brand/model etc.
Some models have a "sleep" mode, i.e. "Run this A/C in a special sleep cycle" so it's treated like a bool. e.g. -1 means off, >=0 means on.
Some it's a turn off the A/C in X many minutes, in that case -1, or 0 is off. and anything larger than 0 is the number of minutes before it turns off.

So, it's what ever seems appropriate for the device.
TL;DR: It depends. ;-)

• The on/offTimer do not seem to have a corresponding field in the state_t struct. Should these be mapped to some combination of clock / sleep?

No, we don't support On & Off timers in the "Common A/C" (IRac class) API. They are just too different to easily equate between brands & models. (e.g. On or Off, In X many mins turn off, Operate in sleep pattern for the next X mins, Turn on sleep mode at X many minutes past midnight (i.e. Set sleep mode at 10:15pm)

If the off timer is really needed by someone, via that interface, we might map it to sleep for them, if and only if. there is no sleep "mode" setting. i.e. An actual sleep setting/mode takes precidence over a hack to make an Off or On timer available via the IRac interface.

So, only set sleep to the number of mins, if the A/C supports a setable sleep duration etc. If it is just an on or off setting (i.e. boolean), then -1 & 0 respectively.

Edit: Oh, ditto for clock too.

[danielegobbetti]

Still got a few Lint issues to address: (See: https://github.com/crankyoldgit/IRremoteESP8266/actions/runs/3134922966/jobs/5090021582) i.e.

src/IRsend.h:845:  "protected:" should be preceded by a blank line  [whitespace/blank_line] [3]

This one was introduced because you said so :-) 😇
#1889 (review)

In any case it's fixed now.

Edit: force pushed since I rebased to solve the conflicts.

[danielegobbetti]

• It is not clear to me what the "sleep" supported feature (i.e. result.sleep in toCommon()) actually is: is it meant to be a "night/low noise mode" or rather "power the unit of after some time" mode?

Ha! Welcome to the world of AirConditioners and different brand/models/implementations.

To keep "Sleep" generic, it means what ever it means for that brand/model etc. Some models have a "sleep" mode, i.e. "Run this A/C in a special sleep cycle" so it's treated like a bool. e.g. -1 means off, >=0 means on. Some it's a turn off the A/C in X many minutes, in that case -1, or 0 is off. and anything larger than 0 is the number of minutes before it turns off.

So, it's what ever seems appropriate for the device. TL;DR: It depends. ;-)
[...]
So, only set sleep to the number of mins, if the A/C supports a setable sleep duration etc. If it is just an on or off setting (i.e. boolean), then -1 & 0 respectively.

Edit: Oh, ditto for clock too.

Thanks, I did as you suggested and used the OffTimer (if set) as sleep value.

The remote control offer two buttons:
one is labeled "sleep" and sets the off timer in the future with steps of 30 minutes (I guess for user convenience)
another one is labeled "stop" and is hidden behind a plastic flap, further it requires more button presses (one has to press "stop" than "timer up" for a number of times (steps is 10 minutes in this case) and finally "enter" to send the value to the AC unit.
Nevertheless the bits in the message are the same and the remote unsets (on its screen) the "sleep" value if anything is done with "stop" and vice versa.

Long story short: I believe it is correct to use sleep the way I now do, thanks a lot for the clarification.

Just to be extra sure I got your explanation correctly: there is no way to map in the IRAac class the status of "power on X minutes in the future", is this right?

[crankyoldgit]

Just to be extra sure I got your explanation correctly: there is no way to map in the IRAac class the status of "power on X minutes in the future", is this right?

Correct.

[crankyoldgit]

Regarding the checksum(s). What have you tried.
Looking at your data, it doesn't look like a simple sum. Have you tried XORing?

[danielegobbetti]

Regarding the checksum(s). What have you tried. Looking at your data, it doesn't look like a simple sum. Have you tried XORing?

Yes I tried to make sense of it, in the second sheet there are two type of rows "MSG" and "XOR", where the latter is the xor of the 2 rows above.
Unfortunately I can hardly find a consistent correlation, I was hoping someone with more experience could find similarities with other protocols or anyway shed some light

[NiKiZe]

Just a few notes, needs to check more details, and having lack of time.

[hoky24]

Regarding the checksum(s). What have you tried. Looking at your data, it doesn't look like a simple sum. Have you tried XORing?

Yes I tried to make sense of it, in the second sheet there are two type of rows "MSG" and "XOR", where the latter is the xor of the 2 rows above. Unfortunately I can hardly find a consistent correlation, I was hoping someone with more experience could find similarities with other protocols or anyway shed some light

Coincidentally, I also have an old AC YORK, but with a GZ-12B-E1 remote. I just have no idea if it has the same IR codes. If you haven't figured out the checksum calculation yet, maybe I can help.

I tried the online calculator https://www.scadacore.com/tools/programming-calculators/online-checksum-calculator/ for a quick reference. I entered the first three codes from the https://docs.google.com/spreadsheets/d/1PsI8KSKzd8HQQeZUcCiCktq7l3xWww1AtdCdbUrWmfg/edit#gid=1789386554 table without the last three bytes.

Here is the result for the third code 0810070240080318017800000000

CRC-16-IBM (Bisync, Modbus, USB, ANSI X3.28, many others; also known as CRC-16 and CRC-16-ANSI), Big Endian (ABCD), Reversed 0xA001: D5 F0

[danielegobbetti]

Regarding the checksum(s). What have you tried. Looking at your data, it doesn't look like a simple sum. Have you tried XORing?

Yes I tried to make sense of it, in the second sheet there are two type of rows "MSG" and "XOR", where the latter is the xor of the 2 rows above. Unfortunately I can hardly find a consistent correlation, I was hoping someone with more experience could find similarities with other protocols or anyway shed some light

Coincidentally, I also have an old AC YORK, but with a GZ-12B-E1 remote. I just have no idea if it has the same IR codes. If you haven't figured out the checksum calculation yet, maybe I can help.

I tried the online calculator https://www.scadacore.com/tools/programming-calculators/online-checksum-calculator/ for a quick reference. I entered the first three codes from the https://docs.google.com/spreadsheets/d/1PsI8KSKzd8HQQeZUcCiCktq7l3xWww1AtdCdbUrWmfg/edit#gid=1789386554 table without the last three bytes.

Here is the result for the third code 0810070240080318017800000000

CRC-16-IBM (Bisync, Modbus, USB, ANSI X3.28, many others; also known as CRC-16 and CRC-16-ANSI), Big Endian (ABCD), Reversed 0xA001: D5 F0

Wow you have no idea how much time I lost on this. Thank you!

Edit: to clarify it looks like the checksum is calculated with the message truncated (-3 bytes), then recomposed like this:
truncated_message + "EC" + checksum

[danielegobbetti]

Confirmed for all the codes I have on record:

$ ./reveng  -w 16 -s $(cat ../codes_without_EC.csv  | paste -sd" ")
width=16  poly=0x8005  init=0x0000  refin=true  refout=true  xorout=0x0000  check=0xbb3d  residue=0x0000  name="CRC-16/ARC"

[danielegobbetti]

@crankyoldgit @NiKiZe

I removed the WIP: prefix as I believe it might be ready to be merged now. I added checksum calculation and a few tests.
The sleep vs scheduled power on flags could be worked on later in my opinion, the basics should be working.

Regards,
Daniele

[crankyoldgit]

@danielegobbetti Thanks for the heads up. I'll take a look now.

[crankyoldgit]

Looking good. Sorry for all the nitpicks. Nearly there.

[danielegobbetti]

Thanks again for the prompt review. I truly deeply hope that now the current PR could be accepted because I have the impression that the part of the work that needs having a real device to test against is vanishing and my inexperience with c++ is frustrating for both sides.

In other words: I am available to test if everything still works after changing parts of the code against my A/C unit but I'm short of time (and knowledge) to dedicate a substantial effort to this PR if it's not yet deemed ready to be merged. Please don't get me wrong: I really appreciated your patience and guidance toward making my code decent enough :-)

[crankyoldgit]

Looks good to me. Just the style nitpick to go.

[danielegobbetti]

Should be ok now. Many thanks again.

[crankyoldgit]

LGTM!