Add support for trotec ac #279

stufisher · 2017-07-19T20:18:08Z

Initial support for Trotec ac remote. No decoding yet i'm afraid.

crankyoldgit

On the whole, excellent stuff. Tackling adding an A/C unit remote is a fair chunk of hard work. Congrats!

There is a lot of comments in this review, a lot of them are really nitpicky/style related. So, please don't take them personally.
FYI, a number of them are for the Google C++ cpplint.py program we use to try to keep a consistent style. Those HAVE to be fixed/completed before our Travis build will succeed etc.

In addition:

Please add yourself to:
https://github.com/markszabo/IRremoteESP8266/blob/master/.github/Contributors.md
and
https://github.com/markszabo/IRremoteESP8266/blob/master/src/IRremoteESP8266.h

You deserve the credit. ;-)

Also, please add a line to the .travis.yaml file to confirm we don't break your code and our users know everything compiles etc:
e.g.
- arduino --verify --board $BD $PWD/examples/TurnOnTrotecAC/TurnOnTrotecAC.ino

crankyoldgit · 2017-07-20T04:29:08Z

examples/TurnOnTrotecAC/TurnOnTrotecAC.ino

@@ -0,0 +1,53 @@
+/*


No copyright message found. You should have a line: "Copyright [year] " [legal/copyright] [5]

Give yourself some credit. ;-)

crankyoldgit · 2017-07-20T04:30:07Z

src/ir_Trotec.cpp

@@ -0,0 +1,170 @@
+#include "ir_Trotec.h"


src/ir_Trotec.cpp:0: No copyright message found. You should have a line: "Copyright [year] " [legal/copyright] [5]

crankyoldgit · 2017-07-20T04:32:01Z

src/ir_Trotec.cpp

+
+void IRTrotecESP::setPower(bool state) {
+  if (state) trotec[2] |= TROTEC_ON;
+  else trotec[2] &= ~(TROTEC_ON);


src/ir_Trotec.cpp:88: Else clause should never be on same line as else (use 2 lines) [whitespace/newline] [4]

e.g.

if (state) trotec[2] |= TROTEC_ON; else trotec[2] &= ~(TROTEC_ON);

crankyoldgit · 2017-07-20T04:32:39Z

src/ir_Trotec.cpp

+
+void IRTrotecESP::setSleep(bool sleep) {
+  if (sleep) trotec[3] |= TROTEC_SLEEP_ON;
+  else trotec[3] &= ~(TROTEC_SLEEP_ON);


src/ir_Trotec.cpp:132: Else clause should never be on same line as else (use 2 lines) [whitespace/newline] [4]

Ditto the previous similar review comment.

crankyoldgit · 2017-07-20T04:33:19Z

src/ir_Trotec.cpp

+
+bool IRrecv::decodeTrotec(decode_results *results, uint16_t nbits,
+                          bool strict) {
+


src/ir_Trotec.cpp:167: Redundant blank line at the start of a code block should be deleted. [whitespace/blank_line] [2]

crankyoldgit · 2017-07-20T04:54:13Z

examples/TurnOnTrotecAC/TurnOnTrotecAC.ino

+  Serial.println("Sending...");
+
+  // Set up what we want to send. See ir_Trotec.cpp for all the options.
+  trotecir.setState(1);


What does state 1 mean?

crankyoldgit · 2017-07-20T04:57:09Z

src/ir_Trotec.cpp

+
+  for (i = 2; i < 8; i++) sum += trotec[i];
+
+  trotec[8] = sum & 0xFF;


Technically, I think this sum & 0xFF is redundant as it's a uint8_t. But better safe than sorry I guess.
e.g.
You could just use:

trotec[8] = sum;

crankyoldgit · 2017-07-20T05:06:14Z

src/ir_Trotec.cpp

+  trotec[0] = TROTEC_INTRO1;
+  trotec[1] = TROTEC_INTRO2;
+
+  setPower(0);


Nitpick:
It would be great if you used constants for these values below.
e.g.
What is 0 in this context? I think you should be using true and false given it's a Boolean.
What is speed 2 etc?

Maybe something like this would make things clearer:

#define TROTEC_DEF_TEMP 25 ... setTemp(TROTEC_DEF_TEMP);

crankyoldgit · 2017-07-20T05:06:29Z

src/ir_Trotec.cpp

+  setPower(0);
+  setTemp(25);
+  setSpeed(2);
+  setMode(0);


TROTEC_AUTO

crankyoldgit · 2017-07-20T05:07:15Z

src/ir_Trotec.cpp

+
+  setPower(0);
+  setTemp(25);
+  setSpeed(2);


TROTEC_FAN_MED

crankyoldgit · 2017-07-20T05:23:27Z

src/IRremoteESP8266.h

@@ -210,6 +214,8 @@ enum decode_type_t {
 #define SONY_20_BITS                20U
 #define SONY_MIN_BITS      SONY_12_BITS
 #define SONY_MIN_REPEAT              2U
+#define TROTEC_BITS                 72U


Maybe I'm missing it, but this doesn't seem to be used anywhere.

crankyoldgit · 2017-07-20T05:25:48Z

src/ir_Trotec.cpp

@@ -0,0 +1,170 @@
+#include "ir_Trotec.h"
+#include <algorithm>


I don't think you need this header. I don't see anything using it. e.g. max & min etc.
Try it without it.

crankyoldgit · 2017-07-20T05:27:44Z

src/ir_Trotec.cpp

+
+
+void IRTrotecESP::setTimer(uint8_t timer) {
+  if (timer < TROTEC_MIN_TIMER) timer = TROTEC_MIN_TIMER;


Redundant: timer can't be less than TROTEC_MIN_TIMER (0) as it is unsigned

stufisher · 2017-07-21T17:24:44Z

No worries, happy to keep to whatever style guide you are using, all useful points!

crankyoldgit

LGTM now.

One last thing. Please merge(rebase) this all into a single commit, so we can roll it back out if we need to. It makes for a cleaner commit history too. ;-)
e.g.
git rebase -i HEAD~11 should do the trick.

Once that's done. Let me know and I'll merge it in.

crankyoldgit · 2017-07-22T01:16:04Z

src/ir_Trotec.h

@@ -17,7 +20,8 @@
 #define TROTEC_DRY            2
 #define TROTEC_FAN            3

-#define TROTEC_ON        1 << 3


Honestly, I think your original approach was nicer.
No need to change it. Just saying.
e.g.

#define TROTEC_ON (1 << 3)

crankyoldgit · 2017-07-22T01:19:20Z

src/IRremoteESP8266.h

@@ -32,6 +32,7 @@
 *  Updated by sillyfrog for Daikin, adopted from
 * (https://github.com/mharizanov/Daikin-AC-remote-control-over-the-Internet/)
 * Fujitsu A/C code added by jonnygraham
+ * Trotec AC code by stufisher


NiKiZe · 2020-06-02T14:08:56Z

I know this is old but ...
Any chance we can get information about which model of A/C and remote this works with, and maybe also a confirmation if decode routines work or not.

stufisher · 2020-06-03T05:39:55Z

Hi @NiKiZe this was for a Trotec PAC 3200. Dont think i got round to testing the decode routines, sorry!

NiKiZe · 2020-06-03T06:25:26Z

Hi @NiKiZe this was for a Trotec PAC 3200. Dont think i got round to testing the decode routines, sorry!

Awesome, thank you for the model number

stufisher · 2020-06-03T06:26:40Z

I dont have this ac unit any more but ill see if i can get any more information about the remote

crankyoldgit requested changes Jul 20, 2017

View reviewed changes

crankyoldgit assigned crankyoldgit and stufisher Jul 20, 2017

crankyoldgit added the enhancement label Jul 20, 2017

crankyoldgit reviewed Jul 20, 2017

View reviewed changes

crankyoldgit approved these changes Jul 22, 2017

View reviewed changes

add support for Trotec AC

50b90fc

stufisher force-pushed the master branch from 26d757f to 50b90fc Compare July 23, 2017 08:35

crankyoldgit merged commit 4f5f05a into crankyoldgit:master Jul 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for trotec ac #279

Add support for trotec ac #279

stufisher commented Jul 19, 2017

crankyoldgit left a comment •

edited

Loading

crankyoldgit Jul 20, 2017

crankyoldgit Jul 20, 2017

crankyoldgit Jul 20, 2017

crankyoldgit Jul 20, 2017

crankyoldgit Jul 20, 2017

crankyoldgit Jul 20, 2017

crankyoldgit Jul 20, 2017

crankyoldgit Jul 20, 2017

crankyoldgit Jul 20, 2017

crankyoldgit Jul 20, 2017

crankyoldgit Jul 20, 2017

crankyoldgit Jul 20, 2017

crankyoldgit Jul 20, 2017

stufisher commented Jul 21, 2017

crankyoldgit left a comment

crankyoldgit Jul 22, 2017

crankyoldgit Jul 22, 2017

NiKiZe commented Jun 2, 2020

stufisher commented Jun 3, 2020

NiKiZe commented Jun 3, 2020

stufisher commented Jun 3, 2020


		bool IRrecv::decodeTrotec(decode_results *results, uint16_t nbits,
		bool strict) {


		for (i = 2; i < 8; i++) sum += trotec[i];

		trotec[8] = sum & 0xFF;



		void IRTrotecESP::setTimer(uint8_t timer) {
		if (timer < TROTEC_MIN_TIMER) timer = TROTEC_MIN_TIMER;

Add support for trotec ac #279

Add support for trotec ac #279

Conversation

stufisher commented Jul 19, 2017

crankyoldgit left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stufisher commented Jul 21, 2017

crankyoldgit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiKiZe commented Jun 2, 2020

stufisher commented Jun 3, 2020

NiKiZe commented Jun 3, 2020

stufisher commented Jun 3, 2020

crankyoldgit left a comment •

edited

Loading