Please merge in upstream IRRemote #148

Closed
marcmerlin opened this Issue Apr 8, 2017 · 19 comments

Comments

Projects
None yet
3 participants
@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

This comment has been minimized.

Show comment
Hide comment
@marcmerlin

marcmerlin Apr 8, 2017

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.

marcmerlin commented Apr 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@crankyoldgit

crankyoldgit Apr 9, 2017

Collaborator

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.

Collaborator

crankyoldgit commented Apr 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@marcmerlin

marcmerlin Apr 9, 2017

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)

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

This comment has been minimized.

Show comment
Hide comment
@crankyoldgit

crankyoldgit Apr 9, 2017

Collaborator

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.

Collaborator

crankyoldgit commented Apr 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@marcmerlin

marcmerlin Apr 9, 2017

Didn't go so well:
z3t0/Arduino-IRremote#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

Didn't go so well:
z3t0/Arduino-IRremote#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

This comment has been minimized.

Show comment
Hide comment
@crankyoldgit

crankyoldgit Apr 9, 2017

Collaborator

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.

Collaborator

crankyoldgit commented Apr 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@marcmerlin

marcmerlin Apr 10, 2017

@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 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 :)

@marcmerlin marcmerlin referenced this issue in z3t0/Arduino-IRremote Apr 10, 2017

Open

ESP8266F support #400

@crankyoldgit

This comment has been minimized.

Show comment
Hide comment
@crankyoldgit

crankyoldgit Apr 17, 2017

Collaborator

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.

Collaborator

crankyoldgit commented Apr 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@markszabo

markszabo Apr 17, 2017

Owner

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.

Owner

markszabo commented Apr 17, 2017

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.

@markszabo markszabo closed this Apr 17, 2017

@marcmerlin

This comment has been minimized.

Show comment
Hide comment
@marcmerlin

marcmerlin Apr 17, 2017

Thanks all for the replies.
So, as timing would have it, please see:
z3t0/Arduino-IRremote#400 (comment)
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.

Thanks all for the replies.
So, as timing would have it, please see:
z3t0/Arduino-IRremote#400 (comment)
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

This comment has been minimized.

Show comment
Hide comment
@crankyoldgit

crankyoldgit Apr 17, 2017

Collaborator

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.)

Collaborator

crankyoldgit commented Apr 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@marcmerlin

marcmerlin Apr 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@crankyoldgit

crankyoldgit Apr 17, 2017

Collaborator

@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.

Collaborator

crankyoldgit commented Apr 17, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@marcmerlin

marcmerlin Apr 17, 2017

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

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

@crankyoldgit

This comment has been minimized.

Show comment
Hide comment
@crankyoldgit

crankyoldgit Apr 17, 2017

Collaborator

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.

Collaborator

crankyoldgit commented Apr 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@marcmerlin

marcmerlin Apr 23, 2017

@crankyoldgit @markszabo could I ask you to have a look at z3t0/Arduino-IRremote#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 @markszabo could I ask you to have a look at z3t0/Arduino-IRremote#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

This comment has been minimized.

Show comment
Hide comment
@crankyoldgit

crankyoldgit Apr 24, 2017

Collaborator

@marcmerlin done ;-)

Collaborator

crankyoldgit commented Apr 24, 2017

@marcmerlin done ;-)

@marcmerlin

This comment has been minimized.

Show comment
Hide comment
@marcmerlin

marcmerlin Apr 24, 2017

Awesome, very detailed, thanks.

Awesome, very detailed, thanks.

@crankyoldgit

This comment has been minimized.

Show comment
Hide comment
@crankyoldgit

crankyoldgit Apr 25, 2017

Collaborator

@marcmerlin I think I've fallen victim to one of the classic blunders. </vizzini>

Collaborator

crankyoldgit commented Apr 25, 2017

@marcmerlin I think I've fallen victim to one of the classic blunders. </vizzini>

Repository owner locked and limited conversation to collaborators Jun 1, 2018

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