-
Notifications
You must be signed in to change notification settings - Fork 836
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
Pioneer send support #547
Pioneer send support #547
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First off, Congratulations on adding a new protocol to the library. It's a big effort. Thanks!!
Sorry for the heaps of comments/suggestions/fixes etc.
I'm just trying to make it as good as we can.
Do you have any rawData
lines which records what the protocol looks like? If so, I'll add a decoder routine, and some unit test code etc so we know we can be fairly sure it behaves like we want it to.
David
src/ir_Pioneer.cpp
Outdated
kNECBits * (kNecBitMarkTicks + kNecOneSpaceTicks) + | ||
kNecBitMarkTicks); | ||
|
||
#if (SEND_PIONEER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick:
#if SEND_PIONEER
src/ir_Pioneer.cpp
Outdated
kNecBitMarkTicks); | ||
|
||
#if (SEND_PIONEER) | ||
// Send a raw NEC(Renesas) formatted message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix this comment. It's not NEC. ;-)
src/ir_Pioneer.cpp
Outdated
// | ||
// Args: | ||
// data: The message to be sent. | ||
// nbits: The number of bits of the message to be sent. Typically kNECBits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the comment. Not kNECBits.
src/ir_Pioneer.cpp
Outdated
// Status: STABLE / Known working. | ||
// | ||
// Ref: | ||
// http://www.sbprojects.com/knowledge/ir/nec.php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ref is for NEC. Pls fix.
src/ir_Pioneer.cpp
Outdated
// prepare codes | ||
uint64_t NECcode1 = data; | ||
uint64_t NECcode2; | ||
NECcode1 >>= 32; // 1st code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be "nbits / 2", not 32.
src/ir_Pioneer.cpp
Outdated
|
||
// Constants | ||
// Ref: | ||
// http://www.sbprojects.com/knowledge/ir/nec.php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not nec.
src/ir_Pioneer.cpp
Outdated
// P I O O N NN E E R R | ||
// P III OOO N N EEEE EEEE R RR | ||
|
||
// NEC originally added from https://github.com/shirriff/Arduino-IRremote/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not NEC.
src/ir_Pioneer.cpp
Outdated
0, 0, 38, true, 0, // Repeats are handled later. | ||
33); | ||
|
||
// send 2nd NEC code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly you probably can replace the rest of the code/function from here on in with just:
sendNEC(NECcode2, nbits / 2, repeat);
* Simplify the sendPioneer() routine. * Add encodePioneer(). * Add decodePioneer(). * Unit tests for sendPioneer() & decodePioneer(). * Code style/lint cleanups.
No real change to functionality.
@kpalczewski I've cleaned up your code, and I verified that it produced the same output as your original PR. However, when I went to https://www.pioneerelectronics.com/PUSA/Support/Home-Entertainment-Custom-Install/IR+Codes/A+V+Receivers and looked at some of the PRONTO/HEX codes they have for the two-code messages for some devices. e.g. Spotify etc. They are not the same as what your/my code produce. I think you may have incorrectly coded up how the Pioneer protocol works. Can you please capture a |
@crankyoldgit thank you for the cleanup. VOL+ (short): Raw Timing[67]:
uint16_t rawData[67] = {8554, 4184, 592, 472, 592, 1524, 592, 1524, 592, 472, 592, 472, 598, 1520, 592, 476, 594, 1520, 596, 1520, 598, 472, 592, 472, 592, 1524, 592, 1524, 594, 472, 594, 1524, 592, 472, 594, 476, 592, 1520, 598, 472, 592, 1524, 594, 470, 594, 472, 596, 472, 592, 472, 592, 1524, 592, 472, 592, 1524, 592, 472, 594, 1524, 598, 1520, 594, 1524, 594, 1524, 594}; // NEC 659A50AF VOL- (short): Raw Timing[67]:
uint16_t rawData[67] = {8554, 4184, 592, 472, 596, 1520, 596, 1520, 596, 472, 592, 472, 592, 1524, 592, 472, 596, 1520, 596, 1520, 592, 476, 592, 472, 592, 1524, 592, 1524, 592, 472, 594, 1524, 592, 472, 596, 1520, 596, 1520, 598, 470, 594, 1520, 598, 472, 594, 470, 594, 472, 596, 472, 592, 472, 592, 472, 596, 1520, 592, 476, 594, 1520, 596, 1520, 596, 1520, 598, 1520, 598}; // NEC 659AD02F POWER (long): Raw Timing[135]:
uint16_t rawData[135] = {8550, 4188, 592, 472, 592, 1524, 592, 1524, 592, 472, 594, 470, 594, 1524, 592, 476, 592, 1524, 594, 1524, 592, 472, 592, 472, 592, 1524, 592, 1524, 592, 472, 592, 1524, 592, 476, 592, 1524, 592, 472, 592, 472, 592, 476, 592, 472, 592, 1524, 592, 472, 592, 1524, 592, 476, 588, 1524, 592, 1524, 592, 1524, 592, 1526, 592, 476, 592, 1524, 592, 472, 594, 25284, 8552, 4184, 592, 1524, 592, 1524, 592, 1524, 592, 1524, 592, 472, 592, 1524, 592, 472, 592, 1524, 592, 476, 592, 472, 592, 472, 594, 476, 588, 1528, 588, 476, 592, 1524, 594, 470, 592, 476, 588, 476, 592, 1524, 594, 1524, 592, 1526, 592, 1524, 594, 472, 592, 1524, 592, 1524, 592, 1524, 592, 472, 592, 472, 592, 476, 592, 472, 592, 1524, 592, 472, 594}; // NEC 659A857A SOURCE (long): Raw Timing[135]:
uint16_t rawData[135] = {8552, 4184, 596, 472, 592, 1524, 594, 1524, 594, 472, 592, 472, 598, 1520, 596, 472, 594, 1524, 592, 1524, 592, 472, 592, 472, 596, 1520, 598, 1520, 596, 472, 592, 1524, 592, 472, 592, 476, 592, 472, 592, 472, 592, 476, 592, 472, 592, 1524, 592, 472, 598, 1518, 598, 1520, 596, 1520, 596, 1520, 596, 1520, 596, 1520, 596, 472, 592, 1524, 592, 472, 598, 25282, 8552, 4182, 596, 1520, 598, 1518, 598, 1520, 596, 1520, 596, 472, 592, 1524, 592, 472, 598, 1520, 596, 472, 594, 472, 592, 472, 596, 472, 592, 1524, 592, 472, 592, 1524, 596, 472, 594, 1520, 596, 1520, 598, 472, 592, 472, 598, 472, 594, 1522, 594, 472, 592, 1524, 594, 472, 596, 472, 594, 1524, 592, 1524, 592, 1524, 592, 472, 594, 1524, 598, 472, 592}; // NEC 659A05FA |
@kpalczewski Thanks for that. That's excellent data. Exactly what I needed/wanted. |
* Remove inter-segment protocol message. * Add better address/command decoding to encode/decode routines. * Unit tests based on real example messages.
@kpalczewski I was correct. We had the protocol slightly wrong. There is no "middle" bit. It's just two NEC messages back to back. Anyway. I've pushed a commit that corrects that. If you test |
As I've added significant code to this, I'll have someone else do a code review as well. |
I tried to send Pioneer code: but the result wan't properly recognized.
also, the second NEC codes are all 0s. Here's the full output of the send code by esp8266: Raw Timing[135]:
uint16_t rawData[135] = {9018, 4438, 622, 510, 618, 1628, 592, 1654, 618, 510, 618, 506, 592, 1652, 594, 536, 594, 1652, 592, 1654, 594, 532, 592, 536, 618, 1628, 592, 1652, 594, 536, 594, 1654, 618, 506, 596, 1650, 598, 532, 592, 532, 594, 536, 594, 532, 592, 1654, 592, 538, 592, 1654, 618, 506, 592, 1654, 592, 1654, 596, 1650, 596, 1654, 594, 532, 592, 1654, 592, 540, 592, 40056, 9014, 4434, 596, 532, 618, 506, 596, 532, 596, 528, 592, 532, 618, 506, 596, 532, 618, 506, 592, 538, 592, 532, 594, 536, 594, 532, 592, 532, 592, 536, 620, 506, 594, 536, 620, 506, 598, 532, 592, 532, 594, 536, 594, 532, 618, 510, 594, 530, 592, 532, 592, 536, 592, 532, 618, 512, 592, 532, 618, 510, 592, 532, 592, 532, 596, 532, 592}; // NEC 659A857A when I use my remote, it's properly recognized: Raw Timing[135]:
uint16_t rawData[135] = {8556, 4180, 598, 472, 594, 1524, 592, 1520, 596, 472, 592, 472, 594, 1524, 592, 472, 592, 1524, 592, 1524, 594, 472, 594, 476, 592, 1522, 596, 1520, 592, 476, 594, 1520, 596, 472, 592, 1524, 592, 472, 592, 472, 594, 476, 588, 476, 592, 1524, 594, 472, 592, 1524, 594, 472, 594, 1524, 594, 1524, 592, 1524, 594, 1524, 594, 472, 592, 1524, 592, 472, 594, 25286, 8556, 4186, 588, 1524, 594, 1524, 594, 1524, 592, 1524, 594, 476, 588, 1524, 594, 476, 592, 1524, 594, 472, 592, 472, 594, 474, 588, 476, 592, 1524, 594, 472, 592, 1524, 594, 470, 594, 472, 592, 476, 588, 1524, 592, 1524, 594, 1524, 592, 1524, 592, 476, 594, 1520, 594, 1524, 592, 1524, 594, 476, 594, 472, 592, 472, 592, 476, 588, 1524, 592, 476, 592}; // PIONEER 659A857AF50A3DC2 |
@kpalczewski Thanks for that feedback. I'll take a look and try to see what might be causing it to misbehave. |
Trying to debug: crankyoldgit#547 (comment)
@kpalczewski I tried the Perhaps try irsend.sendPioneer(0x659A857AF50A3DC2ULL, 64, 0); instead? That should ensure the compiler sees the hex code as a 64 bit value. Maybe wipe all the |
@crankyoldgit Apparently Arduino IDE 1.8.6 on Windows 7 compiles this line differently:
Even though the space between the codes is 40056 rather than 25286 (value from the remote), the Pioneer receiver properly recognizes the codes. |
Yep. I see my mistake now. And why the unit tests pass and why it fails on the arduino. |
i.e. UL on x86/linux is 64 bit. On Arduino, it's 32bit. Force 64bit.
@kpalczewski Can you try it after that latest patch? That should fix it. Thanks again for finding the root cause of the problem. |
the pleasure is all mine. |
Excellent. Assuming no further problems this will be merged in to the master branch within 2 days. |
It's 7 days since I requested a review by someone else. No negative feedback. So merging. |
**[Bug Fixes]** - Add missing send() method to IRPanasonicAC class. (#545) - Add missing sendWhirlpoolAC() to IRMQTTServer.ino (#558) **[Features]** - Add IR receiving support to IRMQTTServer. (#543) - Pioneer support (#547) - Add support for a second LG protocol variant. (#552) - Support for short Panasonic A/C messages. (#553) - Add support for Panasonic CKP series A/Cs. (#554) - Experimental timer/clock support for Panasonic A/Cs. (#546) - Add Made With Magic (MWM) support (#557) **[Misc]** - Grammar and typo fixes (#541, #549) - Increase Panasonic A/C message tolerances. (#542) - Added command mode2_decode in tools/ (#557) - General code style cleanup (#560)
**[Bug Fixes]** - Add missing send() method to IRPanasonicAC class. (#545) - Add missing sendWhirlpoolAC() to IRMQTTServer.ino (#558) **[Features]** - Add IR receiving support to IRMQTTServer. (#543) - Pioneer support (#547) - Add support for a second LG protocol variant. (#552) - Support for short Panasonic A/C messages. (#553) - Add support for Panasonic CKP series A/Cs. (#554) - Experimental timer/clock support for Panasonic A/Cs. (#546) - Add Made With Magic (MWM) support (#557) **[Misc]** - Grammar and typo fixes (#541, #549) - Increase Panasonic A/C message tolerances. (#542) - Added command mode2_decode in tools/ (#557) - General code style cleanup (#560)
This is now live in the new v2.5.2 release of the library. |
pioneer receivers use NEC codes for IR communication.
some codes are 2 NEC codes separated by a long pauze. this PR adds support for sending those codes.
you can find Pioneer codes here:
https://www.pioneerelectronics.com/PUSA/Support/Home-Entertainment-Custom-Install/IR+Codes/A+V+Receivers