Please merge in upstream IRRemote

[marcmerlin]

I almost wasted a lot of time rewriting this because this code was not available in the upstream library. It's great that you did this, but could you get it integrated upstream? I've just upstreamed the ESP32 IR support myself, even if it was a it painful since ESP32 is pretty different from arduino for interrupts (and so is ESP8266 obviously).

[marcmerlin]

Also, too late now, but is there a reason why you didn't use the github fork feature instead of seeding a new repo with code from another one? If you had used fork, it would be reasonably easy to see your git history on top of the original project, and even potentially send a merge request. Please don't think I'm here to complain or be unhappy, I'm thankful you already did this work I was about to do, but your fork may be missed by others as a result of how it was done, and then never integrated back in the original project.

[crankyoldgit]

Hey @marcmerlin (Long time no see/small world), I can't speak for @markszabo and @sebastienwarin as to why they chose this approach rather than a fork, you'll need an answer from them.

I agree, it would have been nice, but we are were we are. The drift is only getting worse. I'm one of the maintainers now, and I'll try to merge things back 'upstream' once I fully grok how this all works, but there are some other fun issues with this platform than just interrupts. e.g. timings and delays are all over the shop due to how the RTOS it runs does things. I'll take a look at how you've integrated your ESP32 code back in, but I suspect given the nature of the specifics of the ESP8266 platform, thus will be a bugger.

IIUIC, atmel etc based arduino has a single dedicated core, the ESP32 has dual core where you have pretty much a single core just for arduino stuff, and the other core handles the RTOS), that allows for fairly accurate timing/assumptions on processor usage. The 8266 has a single core, which has to do both duties. e.g. yield()s and delay()s are needed, in which the RTOS does what it needs. Sometimes those delays etc are not as requested, and throw timings out the window a lot.

[marcmerlin]

Hi David :) Yeah, from what I read of ESP8266, this port must not have been fun. I know the single core issue and how challenging it is. I'm actually impressed it works at all :) ESP32 is so much easier, it just has real hardware timers, done. Just for "fun" I'm trying to see if I can get this port of IRRemote working at the same time as neopixel. Given the interrupt situation, this maybe a futile attempt, but I'm giving it a shot just to check (it works on teensy3.1 and it should work on ESP32 too). But back to the port, yes I know the merge would not be trivial and if you look at my ESP32 merge, it wasn't that smooth because the maintainer does not like CPU specific #defines within the code, making this harder. Either way, thanks for having a look nonetheless (when you have time obviously)

[crankyoldgit]

Okay, now I'm doubly interested in how you merged it, as CPU specific defines is exactly how I'd thought you'd want to approach it without a lot of code duplication. They've also gone in the direction of minimising the library footprint due to the short storage space on classic arduinos. We (ESPers) don't have that issue as much.

Off topic, what are using for Unit Testing on this (or ESP32) platform? I haven't found anything useful yet. This project and others sorely need it.

[marcmerlin]

Didn't go so well: https://github.com/z3t0/Arduino-IRremote/pull/425 and no unittesting sadly. Note that my code isn't even complete, it only does the received that I needed, and compared to ES8266, it's a minimal patch

[crankyoldgit]

Egads. You've just lowered my priority for retro-merging our changes back upstream. They have a lot of valid concerns, but in the ESP world, as you pointed out things are very very different.

[marcmerlin]

@crankyoldgit yeah, I'm not going to tell you that it's going to be easy, and we only have so much time and all. If you think the merge is unlikely to happen soon, or at all, could maybe add a pointer in the other lib to this one. Would you fancy that better? On the plus side, despite all the magic that had to be done to make this work, I got this lib working and reading IR while writing to neopixels with FastLED in a loop, which is difficult, or not possible with some other chips due to the interrupts and timing issues. https://github.com/marcmerlin/Neopixel-IR/blob/master/Neopixel-IR.ino if you are curious Thanks much for making this work, I'll use an ESP8266 for my project instead of wasting a teensy 3.1 on it :)

[crankyoldgit]

FWIW @marcmerlin , after reading the recent threads/issues just now going on in upstream , I'm going to say there is no way in heck that I'm going to beat my head against that wall. I've seen bike-shedding & yak shaving before, life is too short for that. Nice to read, great to learn from, but I don't want any skin in that game.

Anyway, back to (one of) the original questions, why this wasn't a fork, we still need @markszabo to answer that one. Otherwise, I'm considering this closed for now.

[markszabo]

Sorry, I missed this thread. Actually there is no good reason for this not being a fork, I was simply very beginner with git back then, needed the library to work with ESP, made it work and though it would be good to publish it here. But you are absolutely right, this should have been made as a fork and not as a separate lib. Anyway if we ever reach a point where it could be merged back to upstream, I would be happy to do that, but until then I guess we need to maintain it separately.

[marcmerlin]

Thanks all for the replies. So, as timing would have it, please see: https://github.com/z3t0/Arduino-IRremote/issues/400#issuecomment-294446261 looks like someone got inspired to merge your code after I re-pointed your folk to them. Would you mind having a look? The best part is that it doesn't use interrupts for sending it seems.

[crankyoldgit]

I've already been taking a look. :-) As for interrupts .. we don't need no stinkin' interrupts for sending.

(A few hours ago, I was literally working on code to improve our software PWM and duty cycle code.)

[marcmerlin]

@crankyoldgit agreed on interrupts. I also had another issue in their github where I suggested that all this receiving code ought to work with a pin interrupt that only fires when IR is actually received and being toggled instead of a timed interrupt that fires all the time. This would still create interrupts, but a lot fewer in the common case (although sadly this would also be close to an entire rewrite of the IR receive code).

[crankyoldgit]

@marcmerlin I know. I read that. That's what prompted me updating this thread a few hours ago. ;-) Ya know, I know a library that uses pin-state-change for receiving IR signals, conveniently it's on a ESP8266 platform.

I can understand how/why they do it via a timer after reading the thread, but ... egads. Ours is so much less resource intensive over all.

[marcmerlin]

@crankyoldgit I hadn't read this code yet. Great to read that it is doing it the less resource intensive way.

[crankyoldgit]

I have to say, we do use a timer interrupt on receiving, but it's only to mark off that the signal has ended (timed out). Could be implemented without a timer/interrupt with the caveat that it could produce spurious results of it isn't polled (decoded) soon. Hence, the timer.

[marcmerlin]

@crankyoldgit @markszabo could I ask you to have a look at https://github.com/z3t0/Arduino-IRremote/issues/458 ? You can comment on the interface of the API for the upstream lib on whether it would be easy for you to integrate with it

[crankyoldgit]

@marcmerlin done ;-)

[marcmerlin]

Awesome, very detailed, thanks.

[crankyoldgit]

@marcmerlin I think I've fallen victim to one of the classic blunders. \