Coolix improvements #944

ribeirodanielf · 2019-10-03T21:50:18Z

Hi David,

After your comments, seems I've found the less intrusive way to improve and fix some issues with the code. I'm testing it on my real setup and so far is working fine.
My apologies but I don't know yet how to execute the automated tests.
Could you please check the code?
Regards,

Daniel Ribeiro

…he max temperature instead

crankyoldgit · 2019-10-03T23:42:33Z

My apologies but I don't know yet how to execute the automated tests.

See https://github.com/crankyoldgit/IRremoteESP8266/wiki/Library-Maintainers-Guide#unit-tests for how to run them yourself locally.

Could you please check the code?

Will do.

crankyoldgit

Much easier to read this time! Thanks!

src/ir_Coolix.cpp

test/ir_Coolix_test.cpp

…state

ribeirodanielf · 2019-10-04T02:51:27Z

My apologies but I don't know yet how to execute the automated tests.

See https://github.com/crankyoldgit/IRremoteESP8266/wiki/Library-Maintainers-Guide#unit-tests for how to run them yourself locally.

Could you please check the code?

Will do.

Hi David, I've changed most of the points you raised and fixed the tests but seems something else is wrong and I cannot figure out.
Travis is giving an error but I can see no red lines for 2672.2 build job.

Could you please check and advise.
Thanks

crankyoldgit · 2019-10-04T04:22:39Z

There was a bunch of linter issues when I looked last time. Did you address those? Search for "exited with" in the Travis log.

crankyoldgit · 2019-10-04T09:15:21Z

Now I'm near a keyboard again, here are the current issues causing the Travis checks to fail:

https://travis-ci.org/crankyoldgit/IRremoteESP8266/jobs/593345709#L441-L450

src/ir_Coolix.h:78:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:79:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:80:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:81:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:82:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:83:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:84:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:85:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:86:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:141:  Missing spaces around =  [whitespace/operators] [4]

&
https://travis-ci.org/crankyoldgit/IRremoteESP8266/jobs/593345709#L486-L487

src/ir_Coolix.cpp:131:  Missing space before {  [whitespace/braces] [5]
src/ir_Coolix.cpp:133:  Missing space before {  [whitespace/braces] [5]

They are just minor linter issues.

ribeirodanielf · 2019-10-04T13:37:39Z

Now I'm near a keyboard again, here are the current issues causing the Travis checks to fail:

https://travis-ci.org/crankyoldgit/IRremoteESP8266/jobs/593345709#L441-L450

src/ir_Coolix.h:78:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:79:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:80:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:81:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:82:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:83:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:84:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:85:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:86:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Coolix.h:141:  Missing spaces around =  [whitespace/operators] [4]

&
https://travis-ci.org/crankyoldgit/IRremoteESP8266/jobs/593345709#L486-L487

src/ir_Coolix.cpp:131:  Missing space before {  [whitespace/braces] [5]
src/ir_Coolix.cpp:133:  Missing space before {  [whitespace/braces] [5]

They are just minor linter issues.

My bad! I was so focused to solve the Test issues that I forgot to check linter :(
Just submitted the minor fixes.
Thanks

crankyoldgit

Code wise, I'm fine with this. Just a few questions to clarify/confirm what you are proposing is correct.
Once confirmed, I'm happy to merge etc.

crankyoldgit · 2019-10-05T08:06:28Z

src/IRac.cpp

@@ -1905,6 +1910,7 @@ namespace IRAcUtils {
 #if DECODE_COOLIX
      case decode_type_t::COOLIX: {
        IRCoolixAC ac(0);
+        ac.on();


Why is this statement required? setRaw() should (in theory) generate the correct internal state for setPower() from result->value.
As I understand it (I don't have access to a "Coolix" compatible A/C system), every IR message that is NOT an "off" command is considered an "on" message. Is that a correct assumption?

src/ir_Coolix.cpp

crankyoldgit · 2019-10-05T08:11:31Z

src/ir_Coolix.cpp

@@ -170,11 +218,10 @@ uint8_t IRCoolixAC::getTemp() {
  uint8_t i;
  for (i = 0; i < kCoolixTempRange; i++)
    if (kCoolixTempMap[i] == code) return kCoolixTempMin + i;
-  return kCoolixUnknown;  // Not a temp we expected.
+  return kCoolixTempMax;  // Not a temp we expected.


Might this hide when we are getting a value we don't (yet) understand, and perhaps should?

crankyoldgit · 2019-10-05T08:15:09Z

src/ir_Coolix.cpp

@@ -278,43 +344,41 @@ void IRCoolixAC::setMode(const uint8_t mode) {
  switch (actualmode) {
    case kCoolixAuto:
    case kCoolixDry:
-      if (this->getFan() == kCoolixFanAuto)
-        //  No kCoolixFanAuto in Dry/Auto mode.


Is this comment/assumption incorrect? IIRC when I added this, the person said there was no "auto" fan speed when in dry/auto operation modes.
Can you confirm that isn't the case? If you can have fan set to auto in dry/auto operation mode, then this is obviously fine as is.

src/ir_Coolix.cpp

src/ir_Coolix.h

crankyoldgit · 2019-10-16T08:58:31Z

@ribeirodanielf Could you please answer some of my code review questions?

_v2.7.0 (20191030)_ **[Bug Fixes]** - auto_analyse: Fix > 64 bit send code generation. (#976) - auto_analyse: Fix missing arguments in generated code for send64+ (#972) - IRsendProntoDemo: Fix compile issue on ESP32 platform. (#938) - IRMQTTServer: Fix compile error when `MQTT_ENABLE` is false. (#933) **[Features]** - Add Hitachi 424 bit A/C support. (#975, #980, #981) - Experimental detailed support for `DAIKIN152` (#971) - Mitsubishi 112bit A/C support (#947, #968) - gc_decode: Adding Support for Decoding codes in raw code format (#963) - Refactor to use common routines/macros to handle bit manipulation. (#934) - Use centralised common strings. Saves ~1.5k of program space. (#946) - Add Internationalisation (i18n) / Locale support. (#946, #955, #966) - `de-CH`: Swiss German. (#949, #954) - `de-DE`: German. (#946, #950, #952) - `en-AU`: English/Australian (Default locale) (#946) - `en-IE`: English/Irish (#946) - `en-UK`: English/United Kingdom (#946) - `en-US`: English/United States (#946) - `es-ES`: Spanish. (#953) - `fr-FR`: French. (#962) - Port CI pipeline to PlatformIO (#936) **[Misc]** - Add DAIKIN128 & DAIKIN152 to `decodeToState()` (#982) - auto_analyse: Produce better code when leader is detected. (#977) - Coolix A/C improvements (#944) - A/C setRaw/getRaw/stateReset() cleanup. (#967) - Add documentation on how to use & support the i18n aspects of the library. - Make travis checks faster. (#957) - Translate README.md to french (#959) - Fixed Coolix kCoolixDefaultState (#941) - Improve generation of list of pio projects. (#940)

_v2.7.0 (20191030)_ **[Bug Fixes]** - auto_analyse: Fix > 64 bit send code generation. (#976) - auto_analyse: Fix missing arguments in generated code for send64+ (#972) - IRsendProntoDemo: Fix compile issue on ESP32 platform. (#938) - IRMQTTServer: Fix compile error when `MQTT_ENABLE` is false. (#933) **[Features]** - Add Hitachi 424 bit A/C support. (#975, #980, #981) - Experimental detailed support for `DAIKIN152` (#971) - Mitsubishi 112bit A/C support (#947, #968) - gc_decode: Adding Support for Decoding codes in raw code format (#963) - Refactor to use common routines/macros to handle bit manipulation. (#934) - Use centralised common strings. Saves ~1.5k of program space. (#946) - Add Internationalisation (i18n) / Locale support. (#946, #955, #966) - `de-CH`: Swiss German. (#949, #954) - `de-DE`: German. (#946, #950, #952) - `en-AU`: English/Australia (Default locale) (#946) - `en-IE`: English/Ireland (#946) - `en-UK`: English/United Kingdom (#946) - `en-US`: English/United States (#946) - `es-ES`: Spanish. (#953) - `fr-FR`: French. (#962) - Port CI pipeline to PlatformIO (#936) **[Misc]** - Add DAIKIN128 & DAIKIN152 to `decodeToState()` (#982) - auto_analyse: Produce better code when leader is detected. (#977) - Coolix A/C improvements (#944) - A/C setRaw/getRaw/stateReset() cleanup. (#967) - Add documentation on how to use & support the i18n aspects of the library. - Make travis checks faster. (#957) - Translate README.md to french (#959) - Fixed Coolix kCoolixDefaultState (#941) - Improve generation of list of pio projects. (#940)

crankyoldgit · 2019-10-30T06:42:13Z

FYI, the changes mention above have been included in the v2.7.0 release of the library.

CRCinAU · 2019-11-01T13:00:08Z

Just to be annoying, this seems to have broken the Off function at a bare minimum.

I'm trying to debug to see what the story is - but I currently can't turn off my coolix based unit :)

CRCinAU · 2019-11-01T13:14:29Z

Ah - if I understand this right, I can no longer use ac.setpower(false) - but I have to use ac.off()

Is there any updated examples of the Coolix functionality now that I can check the rest of my code against?

crankyoldgit · 2019-11-02T08:09:31Z

Ah - if I understand this right, I can no longer use ac.setpower(false) - but I have to use ac.off()

In ir_Coolix.cpp:

void IRCoolixAC::off(void) { this->setPower(false); }

So all ac.off() is doing for Coolix is setPower(false) i.e. the should be interchangeable and identical.

Is there any updated examples of the Coolix functionality now that I can check the rest of my code against?

Not sure what you mean, but I think we can address this in your new issue you've logged.

Daniel Ribeiro added 4 commits October 3, 2019 16:20

fix some issues and add flags for transient states

128aa7d

while sending Fan command temperature goes to 0xf, force it to show t…

c2e88c3

…he max temperature instead

adjustment for automated tests

8d4eb5c

missing some initialization and removing Raw test for transiente state

1c4c7fe

crankyoldgit self-requested a review October 3, 2019 23:40

crankyoldgit self-assigned this Oct 3, 2019

crankyoldgit added the enhancement label Oct 3, 2019

crankyoldgit added the Hacktoberfest Hacktoberfest participation label Oct 3, 2019

crankyoldgit requested changes Oct 4, 2019

View reviewed changes

src/ir_Coolix.cpp Show resolved Hide resolved

src/ir_Coolix.cpp Show resolved Hide resolved

test/ir_Coolix_test.cpp Outdated Show resolved Hide resolved

Daniel Ribeiro added 6 commits October 3, 2019 21:08

Off is a transient state, it requires Send to return back to regular …

a48806b

…state

fixing code review comments

a319149

more adjustments based on the test results

b106510

must be on to run commands

d857d08

changed the common send message

eee693f

additional point

fc8bb31

minor adjustment to comply with linter

6815104

ribeirodanielf requested a review from crankyoldgit October 4, 2019 13:58

make sure no more commands will be send after turn off coolix ac

2007645

crankyoldgit approved these changes Oct 5, 2019

View reviewed changes

crankyoldgit and others added 3 commits October 21, 2019 16:42

Merge branch 'master' into coolix_imp

3c14000

Cleanup after merge.

92b825b

Ammend code to address review comments.

618f3b2

crankyoldgit merged commit 6d3269a into crankyoldgit:master Oct 22, 2019

crankyoldgit mentioned this pull request Oct 30, 2019

v2.7.0 Release #983

Merged

crankyoldgit mentioned this pull request Nov 2, 2019

Coolix: Power off on initial state turns on a/c #985

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coolix improvements #944

Coolix improvements #944

ribeirodanielf commented Oct 3, 2019

crankyoldgit commented Oct 3, 2019

crankyoldgit left a comment

ribeirodanielf commented Oct 4, 2019

crankyoldgit commented Oct 4, 2019

crankyoldgit commented Oct 4, 2019

ribeirodanielf commented Oct 4, 2019

crankyoldgit left a comment

crankyoldgit Oct 5, 2019

crankyoldgit Oct 5, 2019

crankyoldgit Oct 5, 2019

crankyoldgit commented Oct 16, 2019

crankyoldgit commented Oct 30, 2019

CRCinAU commented Nov 1, 2019

CRCinAU commented Nov 1, 2019

crankyoldgit commented Nov 2, 2019

Coolix improvements #944

Coolix improvements #944

Conversation

ribeirodanielf commented Oct 3, 2019

crankyoldgit commented Oct 3, 2019

crankyoldgit left a comment

Choose a reason for hiding this comment

ribeirodanielf commented Oct 4, 2019

crankyoldgit commented Oct 4, 2019

crankyoldgit commented Oct 4, 2019

ribeirodanielf commented Oct 4, 2019

crankyoldgit left a comment

Choose a reason for hiding this comment

crankyoldgit Oct 5, 2019

Choose a reason for hiding this comment

crankyoldgit Oct 5, 2019

Choose a reason for hiding this comment

crankyoldgit Oct 5, 2019

Choose a reason for hiding this comment

crankyoldgit commented Oct 16, 2019

crankyoldgit commented Oct 30, 2019

CRCinAU commented Nov 1, 2019

CRCinAU commented Nov 1, 2019

crankyoldgit commented Nov 2, 2019