Added Old Vestel A/C support (56 Bits) with full functions. #607

EUA · 2019-01-28T01:32:55Z

Complete Vestel A/C support (56 Bits)

crankyoldgit · 2019-01-28T03:33:01Z

First off, wow. Thanks for tackling adding a full on send/decode/class for this new protocol.

I'll start a code review in a minute.
The Travis build has picked up a number of issues that we'll need to address before we can merge.
I'll try to limit the feedback to just getting this merge-worthy and do some additional cleanup (unit tests and integrate with the examples in a subsequent PR)

EUA · 2019-01-28T03:42:56Z

Fixing travis issues now...

Removed unused variable.

crankyoldgit

That will do for now. I'll take a look after we get it passing Travis.

(sorry for all the nitpicks. It's really good. I'm impressed!)

src/IRsend.h

src/ir_Vestel.cpp

src/ir_Vestel.h

src/ir_Vestel.cpp

crankyoldgit · 2019-01-28T04:28:55Z

FYI, clang-format -style=google -i src/ir_Vestel.cpp src/ir_Vestel.h may help with some of the common linter errors.

src/ir_Vestel.h

Fixed some errors.

EUA · 2019-01-28T06:08:31Z

9 am here. Will continue next day. 👍

i.e. Ran `clang-gormat -style=google` over code.

- Add a setRaw() that accepts a uint64_t. - Should correct travis build issues now.

Use IRHaierAC::timeToString() instead.

- Add some extra fan speed modes.

crankyoldgit · 2019-01-28T10:55:39Z

@EUA I've done some cleanup, fixed a few things, added support for the code examples, and added basic unit tests for it.

Doing some reading on the structure bit packing, it's "implementation dependent" which means we really shouldn't use it. e.g. Different compilers and architectures could produce problems.

Also, when writing the unit tests, I noticed there are no class methods to set any of the clock/timer options. They are optional, but the should be added at some time.

Can you supply some other values for valid codes (and what they mean) so I can add some more tests?
Also, a rawData entry would also be good for a unit test for the decoder too.

EUA · 2019-01-29T01:05:38Z

About packing, yes this is old and not a meaningful thing to talk in 2019. Sad to hear it again. I want to talk about it (as a msc computer engineer.)
Some people just love to talk about bitfield ops dangers but none of them explain about why they want to use such an "half feature set" compiler. If we don't use such a features why they are exists? I even hear some people against C++ inheritances due similar concerns (compatibility) many years ago... So why many people trying to add/implement new feature sets under C++1x since many of compilers don't gonna support them?
Also many many program/library uses inline assembly for x86 or ARM without feeling such a concerns. Since this library is already depend to Arduino and ESP and you have to use xtensa compiler due no alternative of it, I don't think if we need to worry about any compatibility issues at this point, right? If we are working on a project like "bash", yes it's better to put compatibility to the first place..
Well, GCC & LLVM supports BitField ops... Also remaining compilers probably will work ... if they are implemented proper. BitField ops are easy to implemented on a compiler. And every CPU architecture that has a "shift register" in it will support BitFields. Do you know any CPU without it?
BitField Ops is not an evil that we need to fear. For me it's old and good C art.
And the best part is you can visualize the protocol stack structure in your head at the first look. Which is priceless in coding. If there are any bugs appears later in x compiler at y architecture, I believe it's easy to fix since coder see the whole operation. It's easy to patch if you understand the structure.

Sorry. If you don't like or can't use it at this point, you can simply reject the code. I don't gonna use some other "compatible" method for implement it. I just picked the best.

For timer features, I actually don't see any possible benefit of such an implementation.
Because it's easier and better to program MCU instead of "programming MCU to program A/C unit".

Well, Vestel AC has off-timer (30 min, 1hr,2hr,3hr 5hr). Also has option to program turn on and turn off selected time but in 10 minutes interval. If you really want I could add it.

I can send IRecvDumpV2 outputs later. Can I paste here or???

EUA · 2019-01-29T01:44:50Z

Now realized that... Code flipped some how at IRrecvDumpV2...
I return with git to older state and... It still same.
I mean:

Encoding  : VESTEL_AC
Code      : F441E001FEE201 (56 bits)

becomes

Encoding  : VESTEL_AC
Code      : 01E2FE01E041F4 (56 bits)

Everything works proper, nothing changed at all but I swear yesterday (and my whole development) it's F441E001FEE201...
I check reverse engineering codes and. It was in form of: F441E001FEE201...
What happening here?? I don't have any idea about whats going on here.

EUA · 2019-01-29T02:23:48Z

I am really scared... My perception of real is altered for a while. Just like traveling parallel universes...

But I approximate and find the problem.
I developed using library v2.5.3.
But for pushing, I switched v2.5.4 from git. And it become flipped. Not realized due reverse engineering competed.

Than, this thing became a bug.

IRrecvDumpV2 is now running and waiting for IR input on Pin 14
Timestamp : 000014.488
Encoding  : VESTEL_AC
Code      : 0112FF010041F4 (56 bits)
Mesg Desc.: Power: On, Mode: 4 (HEAT), Temp: 16C, Fan: 1 (AUTO), Sleep: Off, Turbo: Off, Ion: On, Swing: Off
Library   : v2.5.4

Raw Timing[115]:
   +  3098, -  9080,    +   548, -  1538,    +   526, -   492,    +   526, -   468, 
   +   524, -   468,    +   526, -   468,    +   550, -   466,    +   526, -   466, 
   +   526, -   504,    +   540, -   466,    +   526, -  1538,    +   526, -   466, 
   +   526, -   466,    +   552, -  1540,    +   522, -   466,    +   526, -   492, 
   +   526, -   544,    +   526, -  1536,    +   526, -  1536,    +   552, -  1536, 
   +   526, -  1536,    +   552, -  1536,    +   552, -  1536,    +   526, -  1536, 
   +   526, -  1574,    +   542, -  1536,    +   526, -   492,    +   526, -   466, 
   +   526, -   494,    +   524, -   468,    +   524, -   468,    +   526, -   492, 
   +   526, -   502,    +   540, -   468,    +   524, -   494,    +   524, -   468, 
   +   526, -   468,    +   524, -   468,    +   526, -   492,    +   526, -   468, 
   +   524, -   520,    +   524, -  1538,    +   524, -   468,    +   524, -   468, 
   +   524, -   468,    +   524, -   468,    +   524, -   468,    +   524, -  1538, 
   +   524, -   506,    +   538, -   468,    +   524, -   468,    +   524, -  1538, 
   +   524, -   468,    +   550, -  1538,    +   550, -  1538,    +   524, -  1538, 
   +   534, -  1528,    +   544

uint16_t rawData[115] = {3098, 9080,  548, 1538,  526, 492,  526, 468,  524, 468,  526, 468,  550, 466,  526, 466,  526, 504,  540, 466,  526, 1538,  526, 466,  526, 466,  552, 1540,  522, 466,  526, 492,  526, 544,  526, 1536,  526, 1536,  552, 1536,  526, 1536,  552, 1536,  552, 1536,  526, 1536,  526, 1574,  542, 1536,  526, 492,  526, 466,  526, 494,  524, 468,  524, 468,  526, 492,  526, 502,  540, 468,  524, 494,  524, 468,  526, 468,  524, 468,  526, 492,  526, 468,  524, 520,  524, 1538,  524, 468,  524, 468,  524, 468,  524, 468,  524, 468,  524, 1538,  524, 506,  538, 468,  524, 468,  524, 1538,  524, 468,  550, 1538,  550, 1538,  524, 1538,  534, 1528,  544};  // VESTEL_AC
uint8_t state[7] = {0x01, 0x12, 0xFF, 0x01, 0x00, 0x41, 0xF4};

auto_analyse_raw_data.py output still show the old notation:

Found 115 timing entries.
Potential Mark Candidates:
[3098, 552]
Potential Space Candidates:
[9080, 1574, 544]

Guessing encoding type:
Looks like it uses space encoding. Yay!

Guessing key value:
kHdrMark   = 3098
kHdrSpace  = 9080
kBitMark   = 530
kOneSpace  = 1538
kZeroSpace = 478

Decoding protocol based on analysis so far:

kHdrMark+kHdrSpace+10000000010010001111111110000000000000001000001000101111
  Bits: 56
  Hex:  0x8048FF8000822F (MSB first)
        0xF4410001FF1201 (LSB first)
  Dec:  36109059220341295 (MSB first)
        68751362606699009 (LSB first)
  Bin:  0b10000000010010001111111110000000000000001000001000101111 (MSB first)
        0b11110100010000010000000000000001111111110001001000000001 (LSB first)

Total Nr. of suspected bits: 56

EUA · 2019-01-29T02:57:29Z

Auto modes exist but Fan mode is different than that. Doesn't make heat or cold.
I find that not implemented an usable function for use auto modes.
I will add a function for it asap.

about
remote_state.temp = new_temp - kVestelACMinTempH;
Well AC protocol does not take kVestelACMinTempH as a reference because there are 2 min temp available at unit. One of them 18 (for cooling), other is 16 (for heating).
I think it's better to leave it as 16. Because using MinTemp instead of 16 will work on code but logically it's a mistake since there are no a real correlation between them actually. Do they have correlation for you? If you think like that. It's OK also.

crankyoldgit · 2019-01-29T03:15:47Z

About packing, yes this is old and not a meaningful thing to talk in 2019. Sad to hear it again. I want to talk about it (as a msc computer engineer.)
Some people just love to talk about bitfield ops dangers but none of them explain about why they want to use such an "half feature set" compiler. If we don't use such a features why they are exists? I even hear some people against C++ inheritances due similar concerns (compatibility) many years ago... So why many people trying to add/implement new feature sets under C++1x since many of compilers don't gonna support them?
Also many many program/library uses inline assembly for x86 or ARM without feeling such a concerns. Since this library is already depend to Arduino and ESP and you have to use xtensa compiler due no alternative of it, I don't think if we need to worry about any compatibility issues at this point, right? If we are working on a project like "bash", yes it's better to put compatibility to the first place..
Well, GCC & LLVM supports BitField ops... Also remaining compilers probably will work ... if they are implemented proper. BitField ops are easy to implemented on a compiler. And every CPU architecture that has a "shift register" in it will support BitFields. Do you know any CPU without it?
BitField Ops is not an evil that we need to fear. For me it's old and good C art.
And the best part is you can visualize the protocol stack structure in your head at the first look. Which is priceless in coding. If there are any bugs appears later in x compiler at y architecture, I believe it's easy to fix since coder see the whole operation. It's easy to patch if you understand the structure.

Sorry. If you don't like or can't use it at this point, you can simply reject the code. I don't gonna use some other "compatible" method for implement it. I just picked the best.

I read up on a number of issues about it last night. The BitField operations are endian-dependant. i.e. Different architectures can cause it to "flip" etc. The AVR/ESP/x86 all share the same endian-ness so we are fine there. However, I did read the one of Microsoft's compiler/IDE environments does treat bitfield packing differently to GCC on the same arch. i.e. produces different bit packing. I do know some people/project that do use this library with that compiler env. so ultimately we should probably change it (not for this PR). Also, we have no control of which architecture the CI/Travis uses. As the unit tests compile outside of the AVR cross-compiler, we do need to cater for that. Currently, they are using x86 it seems so all is fine for now.

I'll probably re-code it to the correct bit-shifting/masking in a later PR.

src/ir_Vestel.h

Added AutoMode function for easy switch.

crankyoldgit · 2019-01-29T03:36:00Z

Everything works proper, nothing changed at all but I swear yesterday (and my whole development) it's F441E001FEE201...
I check reverse engineering codes and. It was in form of: F441E001FEE201...
What happening here?? I don't have any idea about whats going on here.

My guess is you used the wrong bitordering in the MSBfirst arguement of matchData() in the decode routine. or you need to reverse the bit ordering before assigning it to result.value.
Note: There is a difference when assembly an array of uint_8s vs. an entire uint64_t if you have a different bit ordering for each point.

FYI, nothing changed from 2.5.3 to 2.5.4 that would have affected this. My guess is you were looking at it character by character, than as a whole uint64_t.

This is why unit tests are good. ;-)

crankyoldgit · 2019-01-29T03:42:17Z

remote_state.temp = new_temp - kVestelACMinTempH;
Well AC protocol does not take kVestelACMinTempH as a reference because there are 2 min temp available at unit. One of them 18 (for cooling), other is 16 (for heating).
I think it's better to leave it as 16. Because using MinTemp instead of 16 will work on code but logically it's a mistake since there are no a real correlation between them actually. Do they have correlation for you? If you think like that. It's OK also.

I typically prefer to use a named constant than rather plain number, especially if it used in more than one place. 16 is the value which is the temp offset. kVestelACMinTempH is in effect that value. i.e. it is the lowest temp that can be represented (i.e. 0b0000 in the message).
It's not a hard & fast rule, but in this particular case, it is relatively the same in all the other A/C encoder/decoders.

TL;DR: I don't really care. :)

src/ir_Vestel.cpp

Fix incorrect determination of if VESTEL uses uint8[] for state. It doesn't. It uses uint64_t instead. Should fix incorrect reporting in IRrecvDumpV2. Add unit tests to ensure it is correct. * Fix a metric tonne of linter issues.

crankyoldgit · 2019-01-29T04:45:54Z

src/ir_Vestel.cpp

+std::string IRVestelAC::toString() {
+  std::string result = "";
+#endif  // ARDUINO
+  if (remote_state.power == 0x00) {


Is this correct? I don't see anything that ever sets this to 0. only 0xF or 0xC

Yes. This is main difference to identify normal command stack or time stack.

If remote_state.power byte is not set, that means we are decoding time command.

I don't add any reset for that bits since not planning to implement timing functions.
I reset it using t_not_used.
But using power=0x00 might be better idea.

My vote is to use power rather than not_used as clearly it is used.

Can you give me a code for a timer message so I can build some unit tests around it and fix toString() to reflect that.

I just add new remote state for time.. So switching is not needed anymore.

Here is the some codes. "Mesg Desc" shows the time command function :)

Timestamp : 000008.851 Encoding : VESTEL_AC Code : 2D6570B8EE201 (56 bits) Mesg Desc.: Time Command - Time : 5:45, Turn On: 14:00, Turn Off: 23:00 Library : v2.5.4 Raw Timing[115]: + 3022, - 9080, + 546, - 1536, + 526, - 466, + 526, - 492, + 526, - 468, + 526, - 492, + 524, - 468, + 524, - 494, + 524, - 504, + 540, - 492, + 524, - 1538, + 526, - 468, + 524, - 492, + 526, - 466, + 552, - 1536, + 526, - 1536, + 526, - 1570, + 542, - 492, + 524, - 1538, + 550, - 1538, + 524, - 1536, + 526, - 494, + 524, - 466, + 526, - 468, + 524, - 1574, + 540, - 1536, + 550, - 1536, + 526, - 468, + 550, - 1536, + 526, - 492, + 526, - 468, + 524, - 492, + 526, - 518, + 526, - 1536, + 552, - 1536, + 550, - 1536, + 526, - 494, + 550, - 1538, + 526, - 492, + 524, - 1538, + 526, - 504, + 540, - 466, + 526, - 1536, + 526, - 1536, + 526, - 468, + 550, - 1538, + 524, - 468, + 524, - 1538, + 550, - 1574, + 540, - 468, + 550, - 1538, + 526, - 492, + 524, - 468, + 526, - 466, + 526, - 468, + 524, - 494, + 524, - 468, + 546 uint16_t rawData[115] = {3022, 9080, 546, 1536, 526, 466, 526, 492, 526, 468, 526, 492, 524, 468, 524, 494, 524, 504, 540, 492, 524, 1538, 526, 468, 524, 492, 526, 466, 552, 1536, 526, 1536, 526, 1570, 542, 492, 524, 1538, 550, 1538, 524, 1536, 526, 494, 524, 466, 526, 468, 524, 1574, 540, 1536, 550, 1536, 526, 468, 550, 1536, 526, 492, 526, 468, 524, 492, 526, 518, 526, 1536, 552, 1536, 550, 1536, 526, 494, 550, 1538, 526, 492, 524, 1538, 526, 504, 540, 466, 526, 1536, 526, 1536, 526, 468, 550, 1538, 524, 468, 524, 1538, 550, 1574, 540, 468, 550, 1538, 526, 492, 524, 468, 526, 466, 526, 468, 524, 494, 524, 468, 546}; // VESTEL_AC 2D6570B8EE201 uint64_t data = 0x2D6570B8EE201;

Timestamp : 000053.163 Encoding : VESTEL_AC Code : 2EA50018F3201 (56 bits) Mesg Desc.: Time Command - Time : 5:46, Timer Mode Off After 03:00 Library : v2.5.4 Raw Timing[115]: + 3100, - 9076, + 550, - 1534, + 528, - 490, + 528, - 464, + 528, - 490, + 528, - 464, + 528, - 490, + 528, - 466, + 526, - 502, + 542, - 464, + 554, - 1534, + 528, - 490, + 528, - 464, + 552, - 1534, + 528, - 1534, + 528, - 464, + 528, - 526, + 542, - 1534, + 552, - 1534, + 528, - 1536, + 526, - 1534, + 528, - 466, + 528, - 464, + 528, - 492, + 526, - 1572, + 542, - 1534, + 528, - 490, + 528, - 490, + 526, - 466, + 528, - 490, + 528, - 464, + 528, - 466, + 526, - 502, + 542, - 464, + 528, - 490, + 528, - 490, + 528, - 466, + 552, - 1534, + 528, - 490, + 528, - 1534, + 528, - 526, + 542, - 490, + 554, - 1534, + 528, - 466, + 552, - 1534, + 528, - 464, + 552, - 1536, + 552, - 1534, + 528, - 1570, + 544, - 490, + 528, - 1534, + 528, - 466, + 526, - 490, + 528, - 466, + 528, - 490, + 528, - 490, + 526, - 466, + 550 uint16_t rawData[115] = {3100, 9076, 550, 1534, 528, 490, 528, 464, 528, 490, 528, 464, 528, 490, 528, 466, 526, 502, 542, 464, 554, 1534, 528, 490, 528, 464, 552, 1534, 528, 1534, 528, 464, 528, 526, 542, 1534, 552, 1534, 528, 1536, 526, 1534, 528, 466, 528, 464, 528, 492, 526, 1572, 542, 1534, 528, 490, 528, 490, 526, 466, 528, 490, 528, 464, 528, 466, 526, 502, 542, 464, 528, 490, 528, 490, 528, 466, 552, 1534, 528, 490, 528, 1534, 528, 526, 542, 490, 554, 1534, 528, 466, 552, 1534, 528, 464, 552, 1536, 552, 1534, 528, 1570, 544, 490, 528, 1534, 528, 466, 526, 490, 528, 466, 528, 490, 528, 490, 526, 466, 550}; // VESTEL_AC 2EA50018F3201 uint64_t data = 0x2EA50018F3201;

EUA · 2019-01-29T04:54:33Z

Yes verified that code reversal is fixed :) Happy now.

I implemented date timer setting but do not verify if they are work.
Since decoder stack is working and detect real remote commands, I believe you can check it.

Only current pitfall is using same VestelACState remote_state for both type of commands.
Those are completely different.
For use time stack, first we need to reset remote_state using setTimer(0).
Otherwise it corrupts the remote_state

Or we can use another remote_time_state for timing operations.
I don't think on wide because not planning to implement time stack functions...

crankyoldgit · 2019-01-29T04:59:32Z

Ah. So there is an entirely different 56bit message layout if it's timer related vs. a change mode/temp/etc message? If so, Yuck.

EUA · 2019-01-29T05:08:12Z

They call it as hug in Linux kernel anymore... :)

Yes. Only CRC bits and t_footer are in same place :)

Might be it's better to use different internal state named like remote_time_state for timing operations. Will add/change it tomorrow. Than it's done...

- Make sure IRVestelAC::send() knows which state to send. * Set the type of command to send to be the type of the last method type used. e.g. If a time(r) function was used last, send a Time message, otherwise a normal mode message is sent. - Clean up/standardise the timer human readable text. - Add missing timer get() functions. Rename set() functions to common names. - Add unit tests to cover pretty much all the missing and new class methods. - Add real capture data for unit tests.

_v2.5.5 (20190207)_ **[Bug Fixes]** - Fix decoding of Samsung A/C Extended messages. (#610) - Fix IRMQTTServer example to work with GPIO0 as IR_RX (#608) - Fix incorrect #define usage. (#597, #596) **[Features]** - Add deep decoding/construction of Daikin2 messages (#600) - Added Old Vestel A/C support (56 Bits) with full functions. (#607) **[Misc]** - Add ControlSamsungAC example code. (#599) - Add how to send a state/air-con to IRsendDemo.ino (#594)

crankyoldgit · 2019-02-07T03:58:40Z

This has been included in the new v2.5.5 release of the library.

Added Old Vestel A/C support (56 Bits) with full functions.

8867972

EUA closed this Jan 28, 2019

Added travis support.

2f0e15b

EUA reopened this Jan 28, 2019

fixed keywords file.

cc31f9b

crankyoldgit self-requested a review January 28, 2019 03:33

crankyoldgit self-assigned this Jan 28, 2019

EUA added 4 commits January 28, 2019 06:44

Formatted some code lines.

357e67d

Removed unused variable.

change function from snprintf to sprintf

56f29a3

Trying fix Travis.

1cbd82f

Added std space to sprintf

2fbb7bc

crankyoldgit reviewed Jan 28, 2019

View reviewed changes

crankyoldgit added the enhancement label Jan 28, 2019

crankyoldgit reviewed Jan 28, 2019

View reviewed changes

src/ir_Vestel.h Outdated Show resolved Hide resolved

EUA added 3 commits January 28, 2019 08:34

Added Vestel A/C decode support to IRRecvDumpV2 Example.

381d921

Fixed some errors.

Removed sprintf and replaced with timeToString fx.

6665843

Added CRC fix before GetRaw();

8e1944e

crankyoldgit added 4 commits January 28, 2019 17:33

Cleanup cpplint issues.

efb978b

i.e. Ran `clang-gormat -style=google` over code.

Update example code to handle Vestel AC.

a85cea3

- Add a setRaw() that accepts a uint64_t. - Should correct travis build issues now.

Remove duplicate code (timeToString())

ea43e2d

Use IRHaierAC::timeToString() instead.

Add basic unit tests for Vestel A/C.

93f21fa

- Add some extra fan speed modes.

crankyoldgit approved these changes Jan 28, 2019

View reviewed changes

Reverted last changes about AutoFan mode which doesn't exists.

d26b5b2