Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Kelon ACs #1494

Merged
merged 36 commits into from
Jul 3, 2021
Merged

Add support for Kelon ACs #1494

merged 36 commits into from
Jul 3, 2021

Conversation

depau
Copy link
Contributor

@depau depau commented Jun 5, 2021

This PR adds support for Kelon-branded (HiSense) AC units.

Tests aren't ready yet, I'm submitting this early to ensure CI tests run and for an early review if someone can give a look.,

By the way, fyi: https://blog.travis-ci.com/2021-05-07-orgshutdown

@depau depau force-pushed the ac-kelon branch 3 times, most recently from 0dc50b2 to f223564 Compare June 5, 2021 22:53
src/locale/defaults.h Outdated Show resolved Hide resolved
src/IRremoteESP8266.h Outdated Show resolved Hide resolved
src/IRremoteESP8266.h Outdated Show resolved Hide resolved
src/ir_Kelon.cpp Outdated Show resolved Hide resolved
@depau depau force-pushed the ac-kelon branch 2 times, most recently from ab6cccb to 30566a3 Compare June 5, 2021 23:59
@crankyoldgit
Copy link
Owner

FYI, you'll probably need to git rebase from the master branch here to fix some of the travis-ci issues. I've recently merged a PR which some issues.

@depau depau force-pushed the ac-kelon branch 3 times, most recently from 4e0d367 to 7838ff6 Compare June 11, 2021 22:45
@depau
Copy link
Contributor Author

depau commented Jun 11, 2021

Code + tests should be good, unit tests run on my machine. Let's see if the CI agrees.

Let me know if there's any other changes I need to make.

@crankyoldgit crankyoldgit self-requested a review June 11, 2021 22:51
@crankyoldgit
Copy link
Owner

crankyoldgit commented Jun 11, 2021

I'll start a proper code review of this soon.

@crankyoldgit crankyoldgit marked this pull request as ready for review June 11, 2021 22:52
Copy link
Owner

@crankyoldgit crankyoldgit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First off, thanks for doing this. I know it's a huge amount of work, it's appreciated.

Most of these comments are nitpicks and automated linter issues.
Sorry there are so many of them.

src/IRtext.cpp Outdated Show resolved Hide resolved
src/IRutils.cpp Outdated Show resolved Hide resolved
src/IRutils.cpp Show resolved Hide resolved
src/IRutils.cpp Show resolved Hide resolved
src/IRutils.h Show resolved Hide resolved
src/ir_Kelon.cpp Outdated Show resolved Hide resolved
src/ir_Kelon.cpp Outdated Show resolved Hide resolved
src/ir_Kelon.cpp Outdated Show resolved Hide resolved
src/ir_Kelon.cpp Outdated Show resolved Hide resolved
src/ir_Kelon.cpp Outdated Show resolved Hide resolved
@depau
Copy link
Contributor Author

depau commented Jun 13, 2021

First off, thanks for doing this. I know it's a huge amount of work, it's appreciated.

Most of these comments are nitpicks and automated linter issues.
Sorry there are so many of them.

Oh I missed the lint check :)

... line is redundant ...

I actually put it there on purpose to make it clear that I specifically intended for "fan auto" / "smart" to be handled by the default case and that I didn't just forget about it.

Anyway I'll address the requests in the next few days.

Do you have any requirements on squashing or can I just commit a "lint fixes" + whatever makes sense for everything that's not a lint issue?

@NiKiZe
Copy link
Collaborator

NiKiZe commented Jun 13, 2021

Usually squash and merge is used so go nuts ;)

@depau
Copy link
Contributor Author

depau commented Jun 14, 2021

I reverted the additions to print signed integer strings and the tests don't run since Arduino's String class is only available on Arduino.

I think I'll have to put it back.

@crankyoldgit
Copy link
Owner

crankyoldgit commented Jun 24, 2021

Still have a number of linter errors to correct/address:

src/IRac.h:296:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/IRac.h:297:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Kelon.h:5:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Kelon.h:57:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Kelon.h:69:  public: should be indented +1 space inside class IRKelonAc  [whitespace/indent] [3]
src/ir_Kelon.h:70:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Kelon.h:151:  private: should be indented +1 space inside class IRKelonAc  [whitespace/indent] [3]
src/ir_Kelon.h:167:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/IRac.cpp:1266:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/ir_Kelon.cpp:59:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Kelon.cpp:149:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Kelon.cpp:249:  Should have a space between // and comment  [whitespace/comments] [4]
src/ir_Kelon.cpp:451:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/ir_Kelon.cpp:452:  At least two spaces is best between code and comments  [whitespace/comments] [2]

https://travis-ci.org/github/crankyoldgit/IRremoteESP8266/jobs/774565115#L615-L868

@depau
Copy link
Contributor Author

depau commented Jun 24, 2021

Hi,
I'm aware of it, I just haven't had time to finish off. Anyway, before this is merged, I'd still like to write a working PoC for my final use-case and do a final round of testing with that. Hopefully I'll manage to do that within the next few weeks.

@depau
Copy link
Contributor Author

depau commented Jun 30, 2021

  • Lint issues addressed
  • Rebased on top of master (tests run on my machine)
  • I wrote my PoC and it seems to work

So if there isn't anything left to address, this is good to go for me :)

@depau depau requested a review from crankyoldgit June 30, 2021 22:13
@crankyoldgit
Copy link
Owner

Automatic linter check fails:
src/IRac.cpp:1272: Lines should be <= 80 characters long [whitespace/line_length] [2]

All the other tests look fine.

I'll try to get to the human-side of the review later today.

Copy link
Owner

@crankyoldgit crankyoldgit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly minor nitpicks and a few subtle corrections & suggestions. Other than that, looks good. Great work!

src/IRac.cpp Outdated Show resolved Hide resolved
src/IRac.cpp Outdated Show resolved Hide resolved
src/IRac.cpp Outdated Show resolved Hide resolved
src/IRac.cpp Outdated Show resolved Hide resolved
src/IRac.cpp Outdated Show resolved Hide resolved
src/IRremoteESP8266.h Show resolved Hide resolved
src/IRutils.h Outdated
Comment on lines 53 to 54
String addSignedIntToString(const int16_t value, const String label,
const bool precomma = true);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same style issue.

src/ir_Kelon.cpp Outdated Show resolved Hide resolved
src/ir_Kelon.cpp Outdated Show resolved Hide resolved
src/ir_Kelon.cpp Outdated Show resolved Hide resolved
@depau
Copy link
Contributor Author

depau commented Jul 1, 2021

Everything should be addressed.

@crankyoldgit crankyoldgit self-requested a review July 2, 2021 00:10
Copy link
Owner

@crankyoldgit crankyoldgit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor issues related to the new stuff. Other than that, Looks Good To Me!

src/ir_Kelon.cpp Outdated Show resolved Hide resolved
src/ir_Kelon.cpp Outdated Show resolved Hide resolved
@crankyoldgit
Copy link
Owner

Two minor issues related to the new stuff. Other than that, Looks Good To Me!

[optional] I suggest adding a unit test to cover those cases too.

@depau
Copy link
Contributor Author

depau commented Jul 2, 2021

Ok, these two should also be addressed now.

Copy link
Owner

@crankyoldgit crankyoldgit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again. Sorry for the long process.
LGTM!

@crankyoldgit crankyoldgit merged commit 7102d8f into crankyoldgit:master Jul 3, 2021
@depau depau deleted the ac-kelon branch July 3, 2021 11:35
crankyoldgit added a commit that referenced this pull request Jul 6, 2021
_v2.7.19 (20210706)_

**[Bug Fixes]**
- Illegal Heap write in rawbuf when the capture has overflowed. (#1516 #1517)
- PANASONIC_AC: Fix Low and High fan speeds (#1515)
- Fix MDNS in IRServer and IRMQTTServer example code (#1498 #1499)
- IRac: Fix off-by-one error in Coolix's sleep setting. (#1500)
- Fix undefined constant (#1490)

**[Features]**
- Add detailed support for Kelon ACs (#1494)
- Experimental basic support for Teknopoint A/C protocol (#1486 #1504)
- Daikin64: Add support for Heat mode (#1492)
- Basic support for `HAIER_AC176` 176 bit protocol. (#1480 #1481)

**[Misc]**
- GREE: Update inter-message gap timing (#1508 #1509)
- IRac: Change Coolix to send special messages after a normal message. (#1501 #1502)
- Fix compiler warnings causing Travis failures. (#1491)
- Update supported model info (#1477 #1485 #1488 #1489)
- Add HTML viewport meta tag to IRServer and IRMQTTServer examples (#1467 #1469)
@crankyoldgit crankyoldgit mentioned this pull request Jul 6, 2021
crankyoldgit added a commit that referenced this pull request Jul 6, 2021
* Regenerate Doxygen documentation

* v2.7.19 release
_v2.7.19 (20210706)_

**[Bug Fixes]**
- Illegal Heap write in rawbuf when the capture has overflowed. (#1516 #1517)
- PANASONIC_AC: Fix Low and High fan speeds (#1515)
- Fix MDNS in IRServer and IRMQTTServer example code (#1498 #1499)
- IRac: Fix off-by-one error in Coolix's sleep setting. (#1500)
- Fix undefined constant (#1490)

**[Features]**
- Add detailed support for Kelon ACs (#1494)
- Experimental basic support for Teknopoint A/C protocol (#1486 #1504)
- Daikin64: Add support for Heat mode (#1492)
- Basic support for `HAIER_AC176` 176 bit protocol. (#1480 #1481)

**[Misc]**
- GREE: Update inter-message gap timing (#1508 #1509)
- IRac: Change Coolix to send special messages after a normal message. (#1501 #1502)
- Fix compiler warnings causing Travis failures. (#1491)
- Update supported model info (#1477 #1485 #1488 #1489)
- Add HTML viewport meta tag to IRServer and IRMQTTServer examples (#1467 #1469)
@crankyoldgit
Copy link
Owner

FYI, the committed & merged changes have been included in the newly released version of the library. i.e. v2.7.19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants