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.

Sign up for GitHub

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

Jump to bottom

Add TECO AC #622

Merged
merged 12 commits into from
Feb 27, 2019
Merged

Add TECO AC #622

merged 12 commits into from
Feb 27, 2019

Conversation

hcoohb
Copy link
Contributor

@hcoohb hcoohb commented Feb 24, 2019

Hi,

I have added the protocol to send and receive Teco A/C messages.
That would be great if that can be merged to master.
First PR here, so let me know if some things need to be changed.

Regards,
Fabien

@crankyoldgit crankyoldgit self-requested a review February 24, 2019 04:15
@crankyoldgit crankyoldgit self-assigned this Feb 24, 2019
@crankyoldgit
Copy link
Owner

@hcoohb Wow. Thanks for this. I've got very little changes etc to add to this.
It looks pretty good so far.

I'd like to add some unit tests for this protocol to ensure it does what we think it does.
For that, can you please provide some "raw data" for a few messages and a full description of what you think should be setting in each message etc.
e.g. "Off", "On, Cool 25C Fan High, Swing toggle", ... etc.

We also need to update the examples too to reflect the new additions. I'm happy to do that later etc.

crankyoldgit
crankyoldgit reviewed Feb 24, 2019
View reviewed changes
src/ir_Teco.cpp Outdated Show resolved Hide resolved
src/ir_Teco.cpp Outdated Show resolved Hide resolved
src/ir_Teco.cpp Show resolved Hide resolved
src/ir_Teco.cpp Outdated Show resolved Hide resolved
crankyoldgit and others added 3 commits February 24, 2019 18:55
@crankyoldgit
Style cleanup and some Unit tests for Teco protocol.
94b17e0 
* Add some unit tests to cover basic functionality of Teco protocol.
* Improve message decoding slightly (Correct expected min size)
* C++ style lint/cleanup/improvements etc.
* Make changes to example code for full Teco support.

Ref #622
@hcoohb
Correct display of decode. Change state of reset (swing:off)
9f8b34b
@hcoohb
Add real message from Raw Data
ccdcc80
@hcoohb
Copy link
Contributor Author

hcoohb commented Feb 24, 2019

@crankyoldgit Thank you so much for the very quick and in-depth review!
I was giving it a go to try to write the unit test, but you were much faster than me...
I corrected a few things following your comments and I have added a "real massage from raw data". Is one enough or do we need several?

Thanks again!

@crankyoldgit
Fine tune Teco before merge.
cd0d608 
* Add a message reconstruction test to verify class can rebuild message.
* Add some comments.
* Minor code cleanup.

Ref #622
@crankyoldgit
Update Contrib info for Teco.
f74bd2b 
Also bump IRMQTTServer version number.
@crankyoldgit
Copy link
Owner

I have added a "real massage from raw data". Is one enough or do we need several?

Yep. One is fine. Feel free to add some more raw ones if you want. I like to have something that confirms we can decode a real message. Hence, one is okay. More is almost always better.

About the only thing I'd add is some more tests that it decodes/creates message codes correctly via the class methods.

e.g. Confirm via the display on your remote == some code which also has a .toString() representation that has the same info as your remote.
That can be done via ac.setRaw(code) rather than the entire raw data method.

Before I merge this, can you confirm IRrecvDumpV2 decodes your remote correctly (inc. the text description etc) as a TECO message, and that the sendTeco() method controls your A/C as expected?

hcoohb
hcoohb commented Feb 26, 2019
View reviewed changes
src/ir_Teco.h Show resolved Hide resolved
@hcoohb
Copy link
Contributor Author

hcoohb commented Feb 26, 2019

I just have tested the receiving from remote and sending to AC with the newest changes:
It still works beautifully. Everything works well.
And I confirm that IRrecvDumpV2 recognize the remote IR as a Teco message and display the right information about the signal received.

@crankyoldgit
Copy link
Owner

Thanks for the confirmation. I'll merge as soon as the travis check passes after I resolved a merge conflict.

@crankyoldgit crankyoldgit merged commit 73a0b7e into crankyoldgit:master Feb 27, 2019
crankyoldgit added a commit that referenced this pull request Mar 16, 2019
@crankyoldgit
v2.5.6 Release
1b91bf5 
_v2.5.6 (20190316)_

**[Bug Fixes]**
- Fix Coolix A/C Class to handle special states better. (#633, #624)

**[Features]**
- Fix case style for recent A/C protocols. (#631)
- Update `IRsend::send()` to include all simple protocols. (#629, #628)
- Experimental basic support for 112 bit TCL AC messages (#627, #619)
- Add support for TECO AC (#622)
- Experimental support for Samsung 36 bit protocol (#625, #621)

**[Misc]**
- Set Coolix to default to 1 repeat. (#637, #636, #624, #439)
- Set Daikin2 modulation to 36.7kHz. (#635)
- Refactor IRVestelAC class to use portable code. (#617)
- Adjust Daikin2 timings and tolerance. (#616, #582)
@crankyoldgit crankyoldgit mentioned this pull request Mar 16, 2019
Merged
crankyoldgit added a commit that referenced this pull request Mar 16, 2019
@crankyoldgit
v2.5.6 Release (#639)
b706945 
_v2.5.6 (20190316)_

**[Bug Fixes]**
- Fix Coolix A/C Class to handle special states better. (#633, #624)

**[Features]**
- Fix case style for recent A/C protocols. (#631)
- Update `IRsend::send()` to include all simple protocols. (#629, #628)
- Experimental basic support for 112 bit TCL AC messages (#627, #619)
- Add support for TECO AC (#622)
- Experimental support for Samsung 36 bit protocol (#625, #621)

**[Misc]**
- Set Coolix to default to 1 repeat. (#637, #636, #624, #439)
- Set Daikin2 modulation to 36.7kHz. (#635)
- Refactor IRVestelAC class to use portable code. (#617)
- Adjust Daikin2 timings and tolerance. (#616, #582)
@crankyoldgit
Copy link
Owner

FYI, v2.5.6 has just been release which includes the changes/improvements mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crankyoldgit crankyoldgit crankyoldgit approved these changes

Assignees

@crankyoldgit crankyoldgit

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hcoohb @crankyoldgit