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

Technibel support #1259

Merged
merged 5 commits into from
Sep 6, 2020
Merged

Technibel support #1259

merged 5 commits into from
Sep 6, 2020

Conversation

Quentinbri
Copy link
Contributor

Add Technibel A/C protocol
Modifications tested IRL on Technibel IRO 13 PLUS

@Quentinbri
Add Technibel support
4bc96de
@Quentinbri
Merge branch 'master' into Technibel_Support
ae117ed 
# Conflicts:
#	src/IRac.cpp
#	src/IRrecv.cpp
#	src/IRrecv.h
#	src/IRremoteESP8266.h
#	src/IRsend.h
#	src/IRtext.cpp
@Quentinbri
Fix issues raised with C++ linter
268de3f
@crankyoldgit crankyoldgit self-requested a review September 3, 2020 23:58
@crankyoldgit crankyoldgit self-assigned this Sep 3, 2020
@crankyoldgit
Copy link
Owner

@Quentinbri First off, this is amazing. Congrats on doing this. It's a huge effort, I know.
I'll start a proper review of it soon, but I glanced over it on my phone and noticed some minor issues, and alas, one large one.

The "large" issue is I'm pretty sure you've chosen the wrong bit-order for the message. If you choose MSBF order (instead of the LSBF order you've picked), you shouldn't need to do all the bit reversing for the numeric values. e.g. Temperature.
While it will be a lot of work to undo that decision, it will reduce the complexity and the size of the code, and also increase the speed of it.
If you don't feel up to doing that, I can allow it as is, but I'll change it straight away. i.e. The decoded values etc will all be changed/reversed. Your choice. As you've got access to the device, I'd prefer it if you did, that way you can more easily verify the process as you go.

Anyway, again, this is a TOP effort. Thanks very very much for doing it and contributing.

crankyoldgit
crankyoldgit requested changes Sep 4, 2020
View reviewed changes
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.

You're also missing a section in:
IRac::decodeToState() in IRac.cpp. (I can add this later)
an IRac::technibel() function in IRac.cpp (I can add this later)
and a corresponding call to technibel() in bool IRac::sendAc(const stdAc::state_t desired, const stdAc::state_t *prev) function. (I can add this later)

Sorry for all the nitpicks etc, everything should be fairly minor, except for the LSB->MSB swap. that will be a doosey :-(

Again, top effort. This is amazing work. Please keep it up!

examples/TurnOnTechnibelAC/TurnOnTechnibelAC.ino Outdated Show resolved Hide resolved
src/IRac.cpp Outdated Show resolved Hide resolved
src/IRac.cpp Outdated Show resolved Hide resolved
src/IRrecv.h Outdated Show resolved Hide resolved
src/IRsend.h Outdated Show resolved Hide resolved
src/ir_Technibel.cpp Outdated Show resolved Hide resolved
src/ir_Technibel.cpp Outdated Show resolved Hide resolved
src/ir_Technibel.h Show resolved Hide resolved
test/ir_Technibel_test.cpp Outdated Show resolved Hide resolved
test/ir_Technibel_test.cpp Outdated Show resolved Hide resolved
@Quentinbri
Copy link
Contributor Author

Thank you so much for your comments @crankyoldgit.
I will handle all these issues by myself.

@Quentinbri
Change bit-order of the message from LSBF to MSBF
0b3a5e3 
Change comments into doxygen ones
Update a section of IRac.cpp
Remove TurnOnTechnibelAC example
Various minor improvements
crankyoldgit
crankyoldgit approved these changes Sep 6, 2020
View reviewed changes
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.

Looks good to me. Thanks for all the improvements and the code itself!

I'll merge this and make some minor improvements/changes/tests.

src/ir_Technibel.cpp
sendGeneric(kTechnibelAcHdrMark, kTechnibelAcHdrSpace,
kTechnibelAcBitMark, kTechnibelAcOneSpace,
kTechnibelAcBitMark, kTechnibelAcZeroSpace,
kTechnibelAcBitMark, kTechnibelAcGap,
data, nbits, kTechnibelAcFreq, false, // LSB First.
data, nbits, kTechnibelAcFreq, true, // LSB First.
Copy link
Owner

Choose a reason for hiding this comment

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

Comment is now incorrect. LSB -> MSB

@crankyoldgit crankyoldgit merged commit 8bf0aef into crankyoldgit:master Sep 6, 2020
crankyoldgit added a commit that referenced this pull request Sep 6, 2020
@crankyoldgit
Technibel: Cleanup and code fixes/improvements.
1ddd42f 
* Add Fahrenheit support to `IRac` for Technibel.
* Add checksum verification to `decodeTechnibelAc()`.
* Add unit tests for `IRac::technibel()`
* Fix old horrible typo.
* Add more Technibel Unit tests.
* Fix poor Technibel Unit test.
* [bug] Fix problem with `IRTechnibelAc` initial state not being initalised properly.
* [bug] Fix problem where "Auto" being returned for "Cool" mode. (Auto mode doesn't exist!)
* Lots of minor code style cleanups.
* Refactor some class methods to make them simpler/smaller etc.
* Fix/Add some Doxygen comments.

FYI @Quentinbri
Ref #1259
@crankyoldgit crankyoldgit mentioned this pull request Sep 6, 2020
Merged
crankyoldgit added a commit that referenced this pull request Sep 6, 2020
@crankyoldgit
Technibel: Cleanup and code fixes/improvements. (#1266)
d332973 
* Add Fahrenheit support to `IRac` for Technibel.
* Add checksum verification to `decodeTechnibelAc()`.
* Add unit tests for `IRac::technibel()`
* Fix old horrible typo.
* Add more Technibel Unit tests.
* Fix poor Technibel Unit test.
* [bug] Fix problem with `IRTechnibelAc` initial state not being initalised properly.
* [bug] Fix problem where "Auto" being returned for "Cool" mode. (Auto mode doesn't exist!)
* Lots of minor code style cleanups.
* Refactor some class methods to make them simpler/smaller etc.
* Fix/Add some Doxygen comments.

Ref #1259
crankyoldgit added a commit that referenced this pull request Oct 2, 2020
@crankyoldgit
v2.7.11 release
786e9cd 
_v2.7.11 (20200902)_

**[Features]**
- Transcold: Add detailed support. (#1256 #1278)
- Airwell/Whirlpool: Add handling of previous state to `.toCommon()` (#1275 #1276)
- IRMQTTServer: Change how MQTT packet/buffer size is set. (#1271)
- Fujitsu: Add support for timers. (#1255 #1261 #1262)
- Neoclima: Add Economy & Fahrenheit support (#1260 #1265)
- Technibel: Cleanup and code fixes/improvements. (#1259 #1266)
- Technibel: Add detailed A/C support (#1259)
- Transcold: Add basic support. (#1256 #1258)

**[Misc]**
- refactor ir_Delonghi (#1285)
- Whirlpool: Change default mode in `convertMode()` (#1283 #1284)
- SamsungAC: Unit tests to help debug poor signal (#1277 #1280)
- Add question & note about VS1838b use to issue template. (#1281)
- rewrite ir_Corona (#1274)
- tools/mkkeywords: Fix minor parsing issue. (#1272)
- Add Zhongxian Li to Contributers.md (#1270)
- rewrite Carrier (#1269)
- rewrite ir_Argo by using bit field (#1264)
- rewrite ir_Amcor by using bit field (#1263)
- Update Fujitsu supported model info.
- Clarify the scope of the LittleFS breaking change.
crankyoldgit added a commit that referenced this pull request Oct 2, 2020
@crankyoldgit
v2.7.11 release
a8cc14b 
_v2.7.11 (20201002)_

**[Features]**
- Transcold: Add detailed support. (#1256 #1278)
- Airwell/Whirlpool: Add handling of previous state to `.toCommon()` (#1275 #1276)
- IRMQTTServer: Change how MQTT packet/buffer size is set. (#1271)
- Fujitsu: Add support for timers. (#1255 #1261 #1262)
- Neoclima: Add Economy & Fahrenheit support (#1260 #1265)
- Technibel: Cleanup and code fixes/improvements. (#1259 #1266)
- Technibel: Add detailed A/C support (#1259)
- Transcold: Add basic support. (#1256 #1258)

**[Misc]**
- refactor ir_Delonghi (#1285)
- Whirlpool: Change default mode in `convertMode()` (#1283 #1284)
- SamsungAC: Unit tests to help debug poor signal (#1277 #1280)
- Add question & note about VS1838b use to issue template. (#1281)
- rewrite ir_Corona (#1274)
- tools/mkkeywords: Fix minor parsing issue. (#1272)
- Add Zhongxian Li to Contributers.md (#1270)
- rewrite Carrier (#1269)
- rewrite ir_Argo by using bit field (#1264)
- rewrite ir_Amcor by using bit field (#1263)
- Update Fujitsu supported model info.
- Clarify the scope of the LittleFS breaking change.
@crankyoldgit crankyoldgit mentioned this pull request Oct 2, 2020
Merged
siriuslzx pushed a commit that referenced this pull request Oct 4, 2020
@crankyoldgit
v2.7.11 release (#1286)
922a104 
* Regenerate Doxygen documentation

* v2.7.11 release
_v2.7.11 (20201002)_

**[Features]**
- Transcold: Add detailed support. (#1256 #1278)
- Airwell/Whirlpool: Add handling of previous state to `.toCommon()` (#1275 #1276)
- IRMQTTServer: Change how MQTT packet/buffer size is set. (#1271)
- Fujitsu: Add support for timers. (#1255 #1261 #1262)
- Neoclima: Add Economy & Fahrenheit support (#1260 #1265)
- Technibel: Cleanup and code fixes/improvements. (#1259 #1266)
- Technibel: Add detailed A/C support (#1259)
- Transcold: Add basic support. (#1256 #1258)

**[Misc]**
- refactor ir_Delonghi (#1285)
- Whirlpool: Change default mode in `convertMode()` (#1283 #1284)
- SamsungAC: Unit tests to help debug poor signal (#1277 #1280)
- Add question & note about VS1838b use to issue template. (#1281)
- rewrite ir_Corona (#1274)
- tools/mkkeywords: Fix minor parsing issue. (#1272)
- Add Zhongxian Li to Contributers.md (#1270)
- rewrite Carrier (#1269)
- rewrite ir_Argo by using bit field (#1264)
- rewrite ir_Amcor by using bit field (#1263)
- Update Fujitsu supported model info.
- Clarify the scope of the LittleFS breaking change.
@crankyoldgit
Copy link
Owner

FYI, the changes mentioned above have now been included in the new v2.7.11 release of the library.

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
@Quentinbri @crankyoldgit