update sharp to match my AC (Sharp AH-A5SAY) #1074

juliussin · 2020-04-03T22:35:52Z

First of all, this is my first pull request. Be gentle please :)
I tried to use original branch v2.7.4 to control my AC, and it failed. So I tried to dug deeper into the code. I also recorded my original remote with IRrecvDumpV2 and I found something interesting.

Your default format is 13 byte {0xAA, 0x5A, 0xCF, 0x10, 0x00, 0x01, 0x00, 0x00, 0x08, 0x80, 0x00, 0xE0, 0x01} and lets stick with it.

My first first correction is ON/OFF bit. I found that your BytePower = 5 with upper nibble as the PowerOffset is correct. But what's interesting is that with the original remote, when it turns off and I send the command on, upper nibble value is 1. When it turns on and I send the command on again (to change the temp for example), its value is 3. And when I send command off (of course from on state), its value is 2. This also get rid your BitModeNonAutoOffset which is not affected by anything else except this. (I updated this in my pull request with previous state as another parameter)

Second, your ByteManual = 10 is also my concern. Strangely, the value changes each I record the original remote. Apparently the value is in accordance with the previous command. For example, if I change from fan 3 to fan 5, then the lower nibble value is 5. When I change from mode 0(Auto) to mode 2(cool), its value is 0. When I change from temp 25 to 26 (up or down), its value is 4. That's why I removed all the ByteManual rules, since it is affected by the previous state, not current state. (I haven't added this change in my pull request, because it is hard to track the previous command. But the default 0 is fine with any command, so I stick with 0)

I'm not familiar with C or C++ programming and its style guide. So please correct me so I can contribute better in other public repos :)

NB: Tested with my AC (Sharp AH-A5SAY)

crankyoldgit

First of all, this is my first pull request. Be gentle please :)

I'll try. Thanks for going to the great effort of making a PR. It's so rare, so you're already in the good books! ;-) I'm sorry if I come across as abrupt, I apologise in advance. We'll work together on this PR. i.e. Once I understand it, I'll also push some changes to your branch/PR to clean it up/fix/do things the way I think is better etc.

Second, your ByteManual = 10 is also my concern. Strangely, the value changes each I record the original remote. Apparently the value is in accordance with the previous command. For example, if I change from fan 3 to fan 5, then the lower nibble value is 5. When I change from mode 0(Auto) to mode 2(cool), its value is 0. When I change from temp 25 to 26 (up or down), its value is 4. That's why I removed all the ByteManual rules, since it is affected by the previous state, not current state. (I haven't added this change in my pull request, because it is hard to track the previous command. But the default 0 is fine with any command, so I stick with 0)

If 0 works in all cases. i.e. the A/C still does the right thing, then I'm very cool with that. Feel free to include those changes in this PR. I don't have access to the AC & remote to test with, so I have to go on what info users pass on. i.e. This is great info to know!

I'm not familiar with C or C++ programming and its style guide. So please correct me so I can contribute better in other public repos :)

Don't worry, I (well, the automatic linter) will.
e.g. https://travis-ci.org/github/crankyoldgit/IRremoteESP8266/jobs/670788066#L540-L545

src/ir_Sharp.cpp:344:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Sharp.cpp:346:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Sharp.cpp:352:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Sharp.cpp:354:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Sharp.cpp:356:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Sharp.cpp:394:  At least two spaces is best between code and comments  [whitespace/comments] [2]

Style guides are just that guides. They are not always hard and fast, but they are there to try to make code neater/more readable for everyone, and to be done in roughly the same style.
Each project often has it's own style, there is no One True Style etc. The most correct style is "make your code fit in with whatever style is being done in the file you're editing" etc.

Here are the ones this project uses: https://github.com/crankyoldgit/IRremoteESP8266/wiki/Library-Maintainers-Guide#writing-code
You can read up on them for something relaxing and if you want to sleep, after watching the blanket Covid-19 news stories etc. :-/

Note: This PR also broke some Unit Tests which need to pass before we can merge it. We can worry about them later once we've worked out exactly what we need to do and how best to do it all.
e.g. https://travis-ci.org/github/crankyoldgit/IRremoteESP8266/jobs/670788066#L1691-L1713
https://travis-ci.org/github/crankyoldgit/IRremoteESP8266/jobs/670788066#L3960-L3963

crankyoldgit · 2020-04-03T23:59:28Z

src/ir_Sharp.h

@@ -38,8 +38,10 @@ const uint8_t kSharpAcFanHigh = 0b101;  // 5 (FAN3)
 const uint8_t kSharpAcFanMax =  0b111;  // 7 (FAN4)
 const uint8_t kSharpAcByteTemp = 4;
 const uint8_t kSharpAcBytePower = 5;
-const uint8_t kSharpAcBitPowerOffset = 4;
-const uint8_t kSharpAcBitModeNonAutoOffset = 5;  // 0b00100000
+const uint8_t kSharpAcBitPowerOffset = 4;  // 0b00xx0000


So it looks like this is similar to some other A/Cs that use a "power toggle/is changing" bit.
e.g.
0b000x0000 is the desired power state bit (i.e. power on or power off) e.g. 0 is off & 1 is on.
and
0b00x00000 is the power toggle (the power setting is being changed) bit. 1 is the power state changed, 0 the power state hasn't changed?

Does that match up with your findings?
Also, can I have some state[] lines for these cases plus some descriptions from you saying what each state means/does/is doing/what was pressed on the remote.

@crankyoldgit Thank you for helping me with this PR. I really appreciate your willingness to teach me :) I'll try to stick with the guides.

Oh I got it. As your suggestion, I think it is better with this

Suggested change

const uint8_t kSharpAcBitPowerOffset = 4; // 0b00xx0000

const uint8_t kSharpAcBitPowerOffset = 4; // 0b000x0000

const uint8_t kSharpAcBitPreviousPowerOffset = 5; // 0b00x00000

Based on my test, the data I got was:

Command ON (previously OFF) -> 0xAA 5A CF 10 CB 11 22 00 08 80 00 E0 51 Command ON (previously ON) -> 0xAA 5A CF 10 CB 31 22 00 08 80 04 E0 31 Command OFF (previously ON) -> 0xAA 5A CF 10 CB 21 22 00 08 80 00 E0 61

For the command OFF (previously OFF), I can't reproduce this scenario since I can't make changes when it's OFF. Note: The data above was obtained with the other parameters constant.

Can I get you to do a little more data collection/testing please?

Can I get you to try (and capture each):

Start with the AC off.

Press Command ON. (Prev should be OFF etc)

Press Command ON again. (prev should be ON)

Some other command e.g. change temp etc. (What does state[5] look like etc?) i.e. What is Power and Prev?

Press Command OFF.

Basically, like the data you've collected but with an extra step in the middle.

Also. ByteManual (aka. state[10]) .. I'm wondering if it contains a value that corresponds to what physical button was pressed on the remote. e.g. Fan button = 5, Mode button = 0, Temperature button = 4. Some remotes/AC protocols do this. It might explain what it's for better. You should be able to confirm this pretty easily by seeing if changes with different buttons etc.

FYI, I've pushed a change to your branch. You'll need to either merge, or do a git pull in your branch before you change things etc to re-sync. See ddd28b9

How should I present the collected / testing data?

Command ON (previously OFF) -> 0xAA 5A CF 10 CB 11 22 00 08 80 00 E0 51 Command ON (previously ON) -> 0xAA 5A CF 10 CB 31 22 00 08 80 04 E0 31 Command OFF (previously ON) -> 0xAA 5A CF 10 CB 21 22 00 08 80 00 E0 61

Would this way be acceptable?

That will work. However the state[] line from IRrecvDumpV2 is best. Less work for me to copy
& convert it into an array for the testing code. ;-)
e.g. https://github.com/juliussin/IRremoteESP8266/blob/update-sharp/test/ir_Sharp_test.cpp#L755-L759

Thanks for the extra data, I'll take a look in the morning. I'm back to being knee deep in HitachiAc1 code at moment, and its late/early morning here. ;-)

Any luck seeing if state[10] is linked to which button was pressed?

That will work. However the state[] line from IRrecvDumpV2 is best. Less work for me to copy
& convert it into an array for the testing code. ;-)
e.g. https://github.com/juliussin/IRremoteESP8266/blob/update-sharp/test/ir_Sharp_test.cpp#L755-L759

Sorry I'm not quiet understand how the test code works, so I let the data there in variable collect1 until collect 8 with all the comments :)

Can I get you to do a little more data collection/testing please?

Can I get you to try (and capture each):

1. Start with the AC off. 2. Press Command ON. (Prev should be OFF etc) 3. Press Command ON again. (prev should be ON) 4. Some other command e.g. change temp etc. (What does `state[5]` look like etc?) i.e. What is Power and Prev? 5. Press Command OFF.

Basically, like the data you've collected but with an extra step in the middle.

Also. ByteManual (aka. state[10]) .. I'm wondering if it contains a value that corresponds to what physical button was pressed on the remote. e.g. Fan button = 5, Mode button = 0, Temperature button = 4. Some remotes/AC protocols do this. It might explain what it's for better. You should be able to confirm this pretty easily by seeing if changes with different buttons etc.

From the collected data, we can see the pattern of ManualByte state[10], its value is 0x00 if the Power or Mode is changing from the previous state. 0x04 if the Temp is changing. 0x05 if the Fan is changing. It is based on my remote. But in my DIY Remote, I set it's value to 0x00 for all previous state and no error occurred.

Thanks for the extra data, I'll take a look in the morning. I'm back to being knee deep in HitachiAc1 code at moment, and its late/early morning here. ;-)

Any luck seeing if state[10] is linked to which button was pressed?

Sure, thank you for all your help :DD All of this is important knowledge and learning for me. I appreciate your time to review this PR.
No need to rush, lots of free time in the middle of this quarantine. :)

src/ir_Sharp.cpp

* Reduce redundant code in `setPower()` * Update Unit Tests to correctly recreate Known Good states. * Handle previous power in Common A/C API for SharpAc. TODO: Still have to work out `state[10]`/`ByteManual`, as with code as currently is, we can't recreate some known good messages. i.e. a Unit test is failing.

* Add class methods to set/get the previous power setting. * Display the setting in `toString() * Add unit tests based on real data and possible usage. Ref crankyoldgit#1074

crankyoldgit · 2020-04-04T13:26:57Z

how to pull this request to my branch?

git pull should do it. or git fetch then git pull

crankyoldgit · 2020-04-04T13:33:28Z

FYI, because you created a PR for this, that gave me permission to alter your update-sharp branch. The commits I've made have been to your branch/repo/copy of the library. e.g. https://github.com/juliussin/IRremoteESP8266 (The commit history: https://github.com/juliussin/IRremoteESP8266/commits/update-sharp)

juliussin · 2020-04-04T13:45:15Z

Okay, I got it. Thanks! I'll work on the data collection.

@juliussin

* Set "Button" when Power/Mode, Temp, or Fan methods are called. * add `set/getButton()` to allow the above to be changed if ever needed. * Arrange sequence of setting changes so codes match in `ReconstructKnownState` test. * Fix linter errors. * All unit tests now pass (again!) For crankyoldgit#1074 FYI @juliussin

crankyoldgit · 2020-04-05T13:00:51Z

@juliussin I've fixed the failing unit tests. I've added setButton() to allow you to set "ByteManual" to one of the 3 settings you've seen. The Common A/C API sets it to 0 for everything.

Can you test that I haven't screwed up your changes? i.e. It still works the way you need it to.
If you say it's okay, then this is all ready to merge, and I will do so on your confirmation.

juliussin · 2020-04-05T18:58:34Z

Yup, it's very well. All worked fine. The setButton is brilliant, it didn't come to my mind before :) If you don't mind, you can merge this pull request. Thank you by the way.

_v2.7.5 (20200409)_ **[Features]** - Detailed support for `HITACHI_AC1` protocol. (#1056, #1061, #1072) - update sharp to match Sharp AH-A5SAY (#1074) - Experimental support for AIRWELL protocol. (#1069, #1070) - SamsungAC: Add Breeze (Aka WindFree) control (#1062, #1071) - Support for Daikin FFN-C A/C (#1064, #1065) - Add basic support for HITACHI_AC3 protocol. (#1060, #1063) - Add support for `SYMPHONY` 11 bit protocol. (#1057, #1058) - IRMQTTServer: Improve Home-Assistant discovery by sending a 'device' with the discovery packet (#1055) **[Misc]** - Clean up support status of various protocols. - Add `decodeToState()` unit tests to all supported protocols (#1067, #1068) - Add Gree AC example code. (#1066)

crankyoldgit · 2020-04-09T06:28:42Z

FYI, This has been included in the new v2.7.5 release of the library.

juliussin added 3 commits April 4, 2020 05:02

update to match my AC (Sharp AH-A5SAY).

54eca35

update header file to match new sharp update.

6fd4100

update header file to match new sharp update (again).

be71227

crankyoldgit self-requested a review April 3, 2020 23:52

crankyoldgit self-assigned this Apr 3, 2020

crankyoldgit added the enhancement label Apr 3, 2020

crankyoldgit requested changes Apr 4, 2020

View reviewed changes

juliussin and others added 4 commits April 4, 2020 08:44

set power+previous power bit, update model.

af02f78

fix linter problem, simplify on-off, update model+remote.

e37224c

Add set/getPreviousPower() to SharpAc

8de0fa4

* Add class methods to set/get the previous power setting. * Display the setting in `toString() * Add unit tests based on real data and possible usage. Ref crankyoldgit#1074

juliussin and others added 2 commits April 4, 2020 21:23

added some collected data.

380133a

crankyoldgit approved these changes Apr 5, 2020

View reviewed changes

crankyoldgit added the Pending Confirmation Waiting for confirmation from user label Apr 5, 2020

crankyoldgit merged commit ae23f01 into crankyoldgit:master Apr 5, 2020

crankyoldgit mentioned this pull request Apr 9, 2020

v2.7.5 Release #1077

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update sharp to match my AC (Sharp AH-A5SAY) #1074

update sharp to match my AC (Sharp AH-A5SAY) #1074

juliussin commented Apr 3, 2020

crankyoldgit left a comment

crankyoldgit Apr 3, 2020

juliussin Apr 4, 2020

crankyoldgit Apr 4, 2020

crankyoldgit Apr 4, 2020

juliussin Apr 4, 2020 •

edited

Loading

crankyoldgit Apr 4, 2020

crankyoldgit Apr 4, 2020

juliussin Apr 4, 2020

crankyoldgit commented Apr 4, 2020

crankyoldgit commented Apr 4, 2020

juliussin commented Apr 4, 2020

crankyoldgit commented Apr 5, 2020

juliussin commented Apr 5, 2020

crankyoldgit commented Apr 9, 2020

	const uint8_t kSharpAcBitPowerOffset = 4; // 0b00xx0000
	const uint8_t kSharpAcBitPowerOffset = 4; // 0b000x0000
	const uint8_t kSharpAcBitPreviousPowerOffset = 5; // 0b00x00000

update sharp to match my AC (Sharp AH-A5SAY) #1074

update sharp to match my AC (Sharp AH-A5SAY) #1074

Conversation

juliussin commented Apr 3, 2020

crankyoldgit left a comment

Choose a reason for hiding this comment

crankyoldgit Apr 3, 2020

Choose a reason for hiding this comment

juliussin Apr 4, 2020

Choose a reason for hiding this comment

crankyoldgit Apr 4, 2020

Choose a reason for hiding this comment

crankyoldgit Apr 4, 2020

Choose a reason for hiding this comment

juliussin Apr 4, 2020 • edited Loading

Choose a reason for hiding this comment

crankyoldgit Apr 4, 2020

Choose a reason for hiding this comment

crankyoldgit Apr 4, 2020

Choose a reason for hiding this comment

juliussin Apr 4, 2020

Choose a reason for hiding this comment

crankyoldgit commented Apr 4, 2020

crankyoldgit commented Apr 4, 2020

juliussin commented Apr 4, 2020

crankyoldgit commented Apr 5, 2020

juliussin commented Apr 5, 2020

crankyoldgit commented Apr 9, 2020

juliussin Apr 4, 2020 •

edited

Loading