Maint: convert IR library to Arduino-IRremote #4732
Conversation
WalkthroughThe changes update the infrared (IR) remote control functionality to use the newer Arduino-IRremote library (version 4.4.2) instead of IRremoteESP8266. The IR handling code is refactored to use the new IrReceiver API, with added debug output and compatibility helpers. Infrared-related header includes are removed from the main header file. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wled00/ir.cpp (1)
712-723: Consider making the protocol type conversion more complete.The
decodeTypeToStr()function only handles a subset of protocols. Consider adding more protocol types or using a more generic approach.You could enhance this function to handle all protocols supported by Arduino-IRremote:
const char* decodeTypeToStr(uint8_t type) { switch (type) { case LG : return "LG"; case NEC: return "NEC"; case SONY: return "SONY"; case SAMSUNG: return "SAMSUNG"; case MAGIQUEST: return "MAGIQUEST"; - // Add other protocol cases as needed... + case RC5: return "RC5"; + case RC6: return "RC6"; + case PANASONIC: return "PANASONIC"; + case JVC: return "JVC"; + case SHARP: return "SHARP"; default: return "UNKNOWN"; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
platformio.ini(2 hunks)wled00/ir.cpp(2 hunks)wled00/wled.h(0 hunks)
💤 Files with no reviewable changes (1)
- wled00/wled.h
🔇 Additional comments (7)
platformio.ini (1)
142-142:✅ Verification successful
Verify the Arduino-IRremote library version for security and compatibility.
The migration from IRremoteESP8266 to Arduino-IRremote looks good. However, please verify that version 4.4.2 is still secure and compatible with your requirements.
Also applies to: 230-230
🌐 Web query:
What is the latest stable version of Arduino-IRremote library and are there any known security issues with version 4.4.2?💡 Result:
The latest stable version of the Arduino-IRremote library is v4.4.2, released on May 10, 2025. (github.com)
As of now, there are no known security issues associated with version 4.4.2. The changelog for this release includes improvements such as support for SAMD51 timer3 when timer5 is unavailable, fixes for overflow handling, enhanced repeat detection for DistanceWidthProtocol, and optimizations like storing PulseDistanceWidthProtocolConstants in PROGMEM to save RAM. (github.com)
For more detailed information, you can refer to the official changelog. (github.com)
Citations:
- 1: https://github.com/Arduino-IRremote/Arduino-IRremote/blob/master/changelog.md?utm_source=openai
- 2: https://github.com/Arduino-IRremote/Arduino-IRremote/blob/master/changelog.md?utm_source=openai
- 3: https://github.com/Arduino-IRremote/Arduino-IRremote/blob/master/changelog.md?utm_source=openai
Arduino-IRremote v4.4.2 confirmed as latest stable release with no known security issues.
No changes required for platformio.ini (lines 142 & 230).wled00/ir.cpp (6)
4-4: LGTM!The header inclusion is correct for the Arduino-IRremote library.
708-709: LGTM!The deinitialization is properly simplified for the Arduino-IRremote library.
726-732: LGTM!The
reverse8()function correctly reverses the bit order using standard bit manipulation techniques.
735-746: Well-implemented compatibility layer.The
esp8266Value()function correctly maintains backward compatibility with existing IR codes by transforming the raw data format from Arduino-IRremote to match the old IRremoteESP8266 format. This is a clever solution to avoid modifying all entries in ir_codes.h.
752-771:✅ Verification successful
Excellent adaptation to the new Arduino-IRremote API.
The migration properly uses the new IrReceiver singleton API and adds comprehensive debugging output. The use of
esp8266Value()ensures backward compatibility with existing IR codes.
🏁 Script executed:
#!/bin/bash # Description: Verify that serialCanTX is defined elsewhere in the codebase # Search for serialCanTX definition rg "serialCanTX" --type cpp --type h -B 2 -A 2 # Also check if it's a global variable or macro ast-grep --pattern 'serialCanTX'Length of output: 3430
LGTM: New IRremote API integrated correctly
I’ve confirmed that
serialCanTXis declared as a global in wled00/wled.h and initialized in wled00/wled.cpp. The updatedhandleIR():
- Uses
IrReceiver.decode()and accessesIrReceiver.decodedIRDatacorrectly- Provides detailed debug output with protocol, raw data, address, command, and bit count
- Maintains backward compatibility via
esp8266Value()- Calls
IrReceiver.resume()after processingNo further changes needed—ready to merge.
701-704:❓ Verification inconclusive
Verify the second parameter in IrReceiver.begin().
The migration to use the IrReceiver singleton looks correct. However, please confirm that the
falseparameter is intended (typically this disables LED feedback).
🏁 Script executed:
#!/bin/bash # Description: Search for documentation or usage of IrReceiver.begin() to understand the second parameter # Search for IrReceiver.begin usage patterns in the codebase rg -A 3 "IrReceiver\.begin\(" # Look for any comments or documentation about the begin method ast-grep --pattern 'IrReceiver.begin($_, $_)'Length of output: 178
Confirm LED feedback parameter in IrReceiver.begin()
The migration to the
IrReceiversingleton is correct. Inwled00/ir.cpparound lines 701–704:
IrReceiver.begin(irPin, false);The second argument (
false) disables the onboard LED feedback. Please verify against the Arduino-IRremote library documentation that disabling LED feedback is intentional here.
| */ | ||
|
|
||
| IRrecv* irrecv; | ||
| decode_results results; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused variable.
The global decode_results results variable appears to be unused after the migration to Arduino-IRremote, as the new library uses IrReceiver.decodedIRData instead.
-decode_results results;
unsigned long irCheckedTime = 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| decode_results results; | |
| unsigned long irCheckedTime = 0; |
🤖 Prompt for AI Agents
In wled00/ir.cpp at line 11, remove the declaration of the unused global
variable `decode_results results` since the code now uses
`IrReceiver.decodedIRData` from the Arduino-IRremote library and no longer
requires this variable.
|
Thank you! |
wled00/ir.cpp
Outdated
| Serial.printf_P(PSTR(" Protocol Received: %s\n"), decodeTypeToStr(results.protocol)); | ||
| Serial.printf_P(PSTR(" Raw Data: %d\n"), results.decodedRawData); | ||
| Serial.printf_P(PSTR(" Address: 0x%X\n"), results.address); | ||
| Serial.printf_P(PSTR(" Command: 0x%lX\n"), (unsigned long)results.command); | ||
| Serial.printf_P(PSTR(" Num Bits: %d\n"), results.numberOfBits); | ||
| Serial.printf_P(PSTR(" Code: 0x%06lX\n"), (unsigned long)value); | ||
|
|
||
| Serial.println(); | ||
| Serial.println(F("=========================")); |
There was a problem hiding this comment.
Please follow the normal WLED debug logging rather than raw Serial.print*
There was a problem hiding this comment.
This code is supposed to replace "IR recv" string printed out to serial with actual IR code received.
This may be needed to create custom JSON IR override. However there is no need for such detailed output as only IR code is needed.
There is one caveat we've learned on devices with CDC USB: if nobody is reading the output ESP may enter extensive waits when UART transmit buffer is full. So this piece of code may need a revision.
There was a problem hiding this comment.
This can be slimmed down, it was in there to make it easy to debug what was going on with the IR input while testing hardware stacks.
There was a problem hiding this comment.
Please follow the normal WLED debug logging rather than raw Serial.print*
@netmindz - You're looking for DEBUG_PRINTF_P(), correct?
I don't see anything in the contributing docs about logging syntax, so just went with what was already in place here.
There was a problem hiding this comment.
@Graimalkin you made it debug-like output.
Stick with only IR code and Serial.print() is ok. For anything else DEBUG_PRINT...() is the way to go.
There was a problem hiding this comment.
Sorry, that will have been my bad guidance. I didn't realise we ever used raw Serial in AC, for MM we have USER_PRINT for non-debug messages
There was a problem hiding this comment.
Made changes to the logging here.
- serial now only outputs Protocol received and value, and only if WLED_DEBUG isn't defined
- if WLED_DEBUG is defined, the full IR results are returned to make it easier to debug what's going on here.
Let me know if you want this cleaned up more, but I think there's value in saving it for folks who want to debug remotes that don't do the reversing bit trick. ( Since results.value is a mix of Address / Command broken out of Raw Data ).
Debugging:
Non-Debug serial out:
|
checked for code size: looks like it saves about 3k on a C3 build in current state. |
|
While the library we were using had started life as being "based off" the library you are swapping to here, there is mention on GitHub about almost total rewrite for version 2.0 There is also the question about which remotes are supposed by which library. The change brings support for the remote you are trying to use if I'm understanding correctly, but which remotes might we loose support for by swapping? |
So right now, platformio.ini is by default enabling these IR remote types: All of these are supported by the new Arduino-IRremote library. Of these, it looks like in the IRremoteESP8266 (old) repo, NEC based remotes ( like my white 44 key one ), Sony, and Samsung have their results.value info come in in LSB (least significant bit) order but their address and command have been hit by reverseBits which does what it sounds like - reverses the bits. This puts these in MSB order. So when I reconstruct results.value from decodedRawData, it needs to be reversed if we want the values to match what is already in ir_codes.h. Arduino-IRremote on converting from MSB first to LSB first -- https://github.com/Arduino-IRremote/Arduino-IRremote/tree/master#how-to-convert-old-msb-first-32-bit-ir-data-codes-to-new-lsb-first-32-bit-ir-data-codes I did make a change to the code to only convert those remotes that I found in IRremoteESP8266 that were being reversed ( and that arduino-IRremote mentioned needing it ). You can see those changes here before the debug logging changes: Anyways, I went ahead and ordered a few more IR remotes that are supported per docs and mentioned in ir_codes.h. These should be here in a couple of weeks, so I'm going to mark this a DRAFT for now, and re-open when the remotes come in and I can test them out. |
|
Just so you are informed: Many users (including me) use various remotes using JSON IR remote option. If the new library reverses bits all of these remotes will stop working until JSON files are updated (and accompanying JSON files in WLED-Docs) or this reversal is taken care of. EDIT: I'm very fond of these. |
I believe the Apple TV remote is NEC based, so would need the reversebit to work. I think it'll work with these changes as is. If you get a chance, can you check it out? Most NEC remotes should work as-is. There are some weird 16 bit address extended NEC remotes that might potentially see issues - but I'm not sure without testing with one. That little remote you linked looks like another NEC remote. I grabbed these ones for testing with (In addition to the 44 key one I have):
|
It will be more than 2 weeks before I'm back home. Where all my equipment is. |
It's all good. It's about 2 weeks until I get those remotes in also. 👍 |
|
|
||
| #ifndef WLED_DEBUG | ||
| if (results.numberOfBits != 0 && serialCanTX) { // only print results if anything is received ( != 0 ) | ||
| Serial.printf_P(PSTR(" Protocol Received: %s, Value: 0x%0lX\n"), decodeTypeToStr(results.protocol), (unsigned long)value); |
There was a problem hiding this comment.
I don't think the encoding type is of any use to the average user and may add more questions than it answers as this is auto-detected and nothing that is selectable. Or what's the reasoning to add it?
There was a problem hiding this comment.
I left the encoding type for users to be able to do an easy sanity check that the IR signal they received is from the remote device they're using. If you're trying to add a Samsung remote, you'd expect to see SAMSUNG as the encoding device. Also, since platformio.ini only enables a very small set of brand remotes, for folks who are trying to add a LG based remote, they would get UNKNOWN with their code - which could prompt them to dig deeper into what's going on. Remote brands that aren't covered wouldn't be decoded properly and would most likely be treated as a HASH based decode.
There was a problem hiding this comment.
I have to agree with @DedeHai . "Regular" user, who may be instructed to monitor serial port to see IR codes, doesn't need to know which brand his/her/its remote belongs to, he/she/it only needs to know IR code itself (to construct JSON file, for example).
However, if you insist on providing this information to the enduser, please move all of the strings into flash.
|
So I got my collection of remotes in, and I'm taking a look at this some more today. I'm currently testing with:
All of these are NEC based remotes, and they all seem to be working correctly. I can change colors, dim, turn off, and even repeat press for dimming up / down. I also grabbed one of the TV remotes around here (Samsung) which according to platformio.ini is being decoded, to see if the codes stayed the same between MAIN / this branch. These remotes aren't in ir_codes.h, or supported in the drop down. BUT they should be getting decoded. They're going to use a variant of the NEC standard, so 🤷. Here's some testing from it: That's about it for the remotes that I have available for testing with. Let me know if there are next steps, or if I should just abort. |
There was a problem hiding this comment.
While I am very comfortable using IRremoteESP8266, I do not exclude switching to a library that is more robust, has smaller footprint and/or uses less resources.
When I looked at Arduino-IRremote I noticed this pragraph. If it is true, it might be a dealbreaker or may require additional safeguards implemented.
As for testing, unfortunately life took a turn and I do not have time to do any proper testing. Sorry. IMO it may be best to ask for help with testing on Discord or forum. @scottrbailey was involved with IR in the past so he hight be able to help.
|
|
||
| #ifndef WLED_DEBUG | ||
| if (results.numberOfBits != 0 && serialCanTX) { // only print results if anything is received ( != 0 ) | ||
| Serial.printf_P(PSTR(" Protocol Received: %s, Value: 0x%0lX\n"), decodeTypeToStr(results.protocol), (unsigned long)value); |
There was a problem hiding this comment.
I have to agree with @DedeHai . "Regular" user, who may be instructed to monitor serial port to see IR codes, doesn't need to know which brand his/her/its remote belongs to, he/she/it only needs to know IR code itself (to construct JSON file, for example).
However, if you insist on providing this information to the enduser, please move all of the strings into flash.
|
If you can, please test on all platforms, especially ESP8266, C3 and S2 as those have a single core. |
TL;DR: WLED will be fine. That warning is intended for people trying to do IR and LEDs using bit-banging on slower hardware and 2017-era libraries. -- So as it happens, I was looking through our current IR library earlier because I was curious as to whether or not it used the RMT hardware on ESP32. Sending and receiving IR data is what the RMT device was intended for, after all! It turns out that no, IRremoteESP8266 is already just using a generic GPIO interrupt, and timestamping when the line changed state. Inefficient perhaps compared to using the dedicated hardware; but works well enough and isn't particularly platform specific. (This is good for me, in the context of replacing the RMT driver, as there's no way to share the RMT interrupt between two different drivers ... ) I just took a quick look through the Arduino-IRremote code, it seems like they've taken a different approach: essentially sampling the receiver with a 20kHz (!!) timer interrupt. This also works, but it'll be really wasteful of CPU - you pay the cost for all those interrupts even when the IR line is idle. That said: I don't anticipate that it will cause any (new) issues with NeoPixelBus LED output. Any time we're using real hardware (I2S, RMT, UART, whatever) we're insulated from small CPU timing gaps; and even the bit-banging driver allows interrupts through after every pixel is written. (I should know; I PR'd that feature to NeoPixelBus myself to fix some glitching/crashes on an 8266 ;) |
|
not using hardware pin-interrupts is a very bad design... |
|
Ran a quick test, the polling eats up 10% of CPU time i.e. I see a 10-15% drop in FPS. Not acceptable. |
In fairness to the devs, (ab)using a timer is almost certainly the most portable solution. It's the equivalent of bit-banging -- highest CPU costs, most vulnerable to timing issues, but works absolutely everywhere with every protocol on any pin that can do GPIO. Having spent time digging through FastLED and NeoPixelBus - both libraries that aim to reach for good performance everywhere, with a large number of custom drivers included - I can appreciate why a library maintainer (or team) might opt to say "no, we are not interested in being a library of optimized drivers for every chip ever -- we just do the one simple thing all the time; if you don't want that, go elsewhere". Unfortunately, the long term impact of such a choice is that we, the developer community, never get to settle on the "one battle-tested library that everyone uses"; we end up with a panopoly of competing libraries, forcing people to choose between performance vs protocol support vs hardware support vs active maintenance vs ease-of-use .. sigh. |
|
That may have sounded wrong. Not questioning the devs of that library: if it's to be universal and for arduino purposes, using timer polling is the easiest option, no doubt. For our purposes however, it's a bad design so it can't be used. |
Agreed - that's a significant regression; I don't want to merge that for all users. I would like to see some path forward for the ultimate goal here, though - my family has good memories of Great Wolf Lodge, we're hoping to visit sometime soon. ;) As I see it, that leaves:
I did a quick search and I couldn't find one - basically every library that does decoding also includes one or more hardware drivers to read into their own idiosyncratic decoding format. (Also the hardware drivers are legitimately nontrivial -- you need precise enough timestamping, which isn't standard on most platforms, so it has to be set up by the driver.) IMO it's probably easier to port the decoding from one library to another than it is to replace the drivers for one of the existing libraries. YMMV, though. https://github.com/IRMP-org/IRMP might be worth looking at; it supports both timer-based and pin-irq-based sampling. Although curiously the README consistently links over to IRremoteESP8266 (?!?). |
|
Ok, going to go ahead and close this then as not a good change due to the CPU increase. Thanks for all of the feedback folks! |
5 participants
IRremoteESP8266 appears abandoned ( last updated July 2023 ).
It's based on Arduino-IRremote, which IS being maintained, and is not quite a drop in swap.
So, I swapped them. 😛
I did have to make a choice here to either re-order all of ir_codes.h or swap the byte order for results.decodedRawData so that it could mimic results.value. I went with swapping the byte order via esp8266Value(), because changing all of ir_codes.h sounds unpleasant to review, and like it's just asking for issues.
Also added in some debugging output for the serial monitor so it's easier to see what's going on.
License changes from GNU GPL -> MIT, so that seems fine here.
This is part of a bigger change that allows WLED to work with MagiQuest wands from Great Wolf Lodge. The wands aren't working with the old IRremoteESP8266 library, but they do with this one! I'm just breaking the changes up for ease of processing. (Graimalkin#2)
Testing:
Tested with an ESP32 dev board, TSOP4838 IR sensor, and generic LED light strip. Also grabbed one of the white 44 key IR remotes from Amazon -- https://www.amazon.com/dp/B0982BVCSD
Colors changed, brightness goes up and down, can swap to presets.
I'd recommend trying on a few more hardware stacks if you have it.
Summary by CodeRabbit
Refactor
Chores