Added ESP32 IR receive support (IRsend not implemented yet). #425

marcmerlin · 2017-03-20T03:32:50Z

disable a lot of defines not relevant to ESP32, set them to 1 (no-op)
change default IR pin to 35 from 11
changed serial speed to 115200 (9600 is too slow to keep up with IR input)
irSend disables code that will not compile on ESP32. It won't work,
but it won't break compilation either.

- disable a lot of defines not relevant to ESP32, set them to 1 (no-op) - change default IR pin to 35 from 11 - changed serial speed to 115200 (9600 is too slow to keep up with IR input) - irSend disables code that will not compile on ESP32. It won't work, but it won't break compilation either.

marcmerlin · 2017-03-20T03:37:17Z

Dear maintainer: If you're not overjoyed with my ifdef ESP32 for IRpin and baud rate, feel free to revert those bits.
As for the ESP32 defines, I know they'd be better if they were all in the board file, but that didn't really work out due to how different interrupts are on ESP32. Hopefully what I did is ok.
I have of course tested that the code works, IRrecv* (all 3) work fine.
As stated in the CL, irsend does not work I have not tested/used it, but receive is quite useful for many projects, including mine.

z3t0 · 2017-03-24T22:19:39Z

Sorry just saw this! My only concerned before merging is that I have no way to test it and would prefer validation from another user just to maintain a level of QC. @bengtmartensson @AnalysIR do you have access to this board?

marcmerlin · 2017-03-24T22:22:11Z

No worries, it wasn't a long wait :)
Ideally it should work with this: https://www.sparkfun.com/products/13907 and it's confirmed to work on my own board ESP32 based.

z3t0 · 2017-03-24T22:22:57Z

If not I'll just review it a bit and merge anyways, one it's released others should be able to report bugs, of any.

Also you might want to search github for other esp32 ir libraries in case they help with development

marcmerlin · 2017-03-24T22:28:04Z

There is https://github.com/ExploreEmbedded/ESP32_RMT which uses hardware specific to ESP32, but I didn't use it because I'm already using RMT to talk to neopixels and it doesn't seem to support all the IR protocols your library does.

AnalysIR · 2017-03-24T23:23:09Z

I have one on order, but it will be many weeks before it lands...
...what is the best IDE to use & which ESP32 module did you test with...

marcmerlin · 2017-03-24T23:38:22Z

@AnalysIR I have a custom board you can't really buy: https://github.com/marcmerlin/IoTuz
but as far as IR is concerned, it's simply an IR receiver plugged into an I/O pin, so that would work on any other ESP32. The main issue of course is that ESP32 does not have arduino style timers, which is what my patch deals with.
I use the arduino IDE with arduino-esp32: https://github.com/espressif/arduino-esp32
basically it works like just about any other arduino except for a (very) few hardware specific bits, namely interrupts and timers.
Example ESP32 arduino code in the IoTuz link I gave above if you'd like.

z3t0 · 2017-03-25T00:55:43Z

Okay, in that case @marcmerlin I think I'll just give the main contributors (including myself) a few days to review this PR and if there are no objections I will merge it into a release and anyone that does have an issue can report it as a bug.

@AnalysIR do you think thats a good plan? I don't like including code that hasn't been tested a lot, but it seems that the only way to get this kind of library tested is to release it?

@marcmerlin
Also I would like to let you know that currently this library is licensed under the GPLv3, however moving forwards we are going for MIT licensing. Can I have your permission to allow your code to be licensed under the MIT once that transition is done?

marcmerlin · 2017-03-25T01:02:41Z

@z3t0 indeed, your main concern is to make sure my code

isn't too dirty to your taste ;)
and that I didn't break other platforms
otherwise Linus' moto of "a bad driver beats no driver at all" is probably correct here :)
but to be honest, it's been pretty solid for me.
Now, to be entirely fair, ESP32 has only been released recently, so I can't promise to you that it'll bring you thousands of new users, but hopefully it'll be useful to other people than just me, including all the users of that IoTuz board I just referenced above.

For licensing, you may have this patch under both GPLv3 and MIT.

bengtmartensson

I think that this PR violates good practice in handling hardware dependencies. Such dependencies should be isolated to certain isolated areas, here [IRremote]boardddefs.h. There should be no #ifdef HARDWARE in the high-level code. Instead, the low-level, hardware dependent file should say

#ifdef HARDWARE
#define PROPERTY
#endif

and the highlevel code should use only #ifdef PROPERTY etc. Here, this goes for the sending ability, for example. (I think the Gnu coding guidelines states this explicitly.)

Everything "IMHO"...

Also, the change of baud rate is better separated from a PR intended to support new hardware.

Unfortunately, I do not presently have access to the hardware, and have not been able to test.

marcmerlin · 2017-03-26T21:02:13Z

@bengtmartensson I totally agree with what you're saying, I actually tried to put things in the hardware file, but sadly it didn't quite work out that way for some changes (other ones did end up in the hardware file)
The main problem is that ESP8266/ESP32 are just very different from AVR when it comes to interrupts. There is just no way around it that an easy few #define can fix, sadly :( They are however getting very popular considering how much you're getting for an arduino-like $10-ish chip (32bit, wifi, bluetooth, lots of RAM, lots of flash, and even dual core for ESP32).
If you look at an adafruit lib, the same things are going on for ESP support, sadly it's just the way it goes.
But if you find a better way of doing what I did, you could always download the arduino-esp hardware platform for arduino and compile against it, without having the hardware. If you find a better way to do what I did (and I'm not saying there is not), and it builds, send me your suggestion and I'll be happy to compile and test it.

As for the defines, there are 4 and here is an explanation for each:
first one:
+#ifndef ESP32
#include <avr/interrupt.h>
+#endif
I have to remove this include, it just doesn't exist on ESP32

second one:
+#ifdef ESP32
+void IRTimer()
+#else
ISR (TIMER_INTR_NAME)
+#endif
Those don't look anywhere the same, how would that work with a #define?

third one in irRecv.cpp:
again, the arduino code looks nothing like the ESP32 one. It's not like I can define a pin, or something, it's not remotely the same code :(

fourth one in irSend.cpp:
same issue, the code I commented out will not remotely work or even compile on ESP32.

For the rest, you'll see that I did put defines in the hardware file, when it was possible, all the way to defining code that won't compile on ESP32 to '1' instead when that code wasn't needed.

My changes to the examples (pin and serial speed), as per my first message, you are totally ok to reject/revert/remove them, they are not vital to the code working, just a suggestion at best.

z3t0 · 2017-03-27T00:44:13Z

examples/IRrecvDemo/IRrecvDemo.ino

@@ -8,16 +8,22 @@

 #include <IRremote.h>

+#ifdef ESP32


Does the ESP32 work in a way where the receive pin needs to be specified like this? For the arduino's we only have restriction on the send pins as they are tied to timers.

no, as I said earlier, on the default platform I have, pins are already assigned in other ways, but there is no hardware restriction. If I had a bare chip, I would be able to use the same pin assignment than the default code.
So if you'd rather drop this bit, as per my earlier comment, I'm totally ok doing so.

I think that removing this is a good idea, just to keep the example files consistent. Thanks for the foresight though!

z3t0 · 2017-03-27T00:45:30Z

examples/IRrecvDemo/IRrecvDemo.ino


 IRrecv irrecv(RECV_PIN);

 decode_results results;

 void setup()
 {
-  Serial.begin(9600);
+  Serial.begin(115200);


We can create a separate issue that asks the community if the Serial rate needs to be changed, otherwise it would be best to keep it as. And for sure as @bengtmartensson said if we do incorporate this change, it should be in a different PR.

Understood. 9600 was not fast enough to output timely for a test I did, but there took, as per prior comment, I'm totally ok removing this too if you'd like me to.

z3t0 · 2017-03-27T00:50:02Z

IRremote.cpp

@@ -120,7 +122,11 @@ int  MATCH_SPACE (int measured_ticks,  int desired_us)
 // As soon as first MARK arrives:
 //   Gap width is recorded; Ready is cleared; New logging starts
 //
+#ifdef ESP32


This would be better placed in boarddefs.h

Something like this

#ifdef ESP32 #define IR_USE_TIMER_ESP32 #endif // then near the bottom of the same file #if defined (IR_TIMER_USE_ESP32) ... #endif

Also, I think the actual timer definition might be better defined as modelled here

Notice how all the timer definitions are defined with the same macros, that way the higher level implementation is platform agnostic

this looks like of the same functionally speaking than the original, but if you prefer this, happy to change it.

Thanks! The only reason for this is if the macros are named in the way I specified in my example, then the code for other platforms will not be included. That way the compiler ends up only including the code needed for each platform, but the higher level implementation isnt aware of this if there is for example only ever one TIMER_INIT function.

Does that make sense?

Also, I am sorry if it appears that we are being very strict, or unwelcoming. I am sure I speak for the entire Arduino community when I say that your contributions are not only welcomed but very much encouraged! I realize it takes a lot of energy and time to contribute upstream to a project and I hope you take our criticism as nothing more than constructive and positive.

If you have any questions about any of our "demands" then feel free to extend the discussion and ask as about why we are doing something a certain way, or even suggest another way that you think would be better.

Hopefully, we don't scare you a way 😄

Haha, don't worry, I have a thick skin and been doing CR/PRs for many years. Also I know that accepting code just before it works is not a great idea by itself, there are other requirements that should be met.
Easy to change, will do.

z3t0 · 2017-03-27T00:50:46Z

boarddefs.h

@@ -39,6 +39,10 @@
 #	define BLINKLED_ON()   (PORTD |= B00000001)
 #	define BLINKLED_OFF()  (PORTD &= B11111110)

+#elif defined(ESP32)


These should be defined as writes to PORTS (or the equivalent on ESP32).

So, there is no status LED to write on, on the reference platform I have (led13 on AVR). Also, you don't write to ports like you do on AVR, digitalWrite is actually fast on ESP32, so no tricks needed.
If I were to add a status LED to an ESP32 chip, this could be made to work, but by default there is none, and my specific platform has no I/O left to send random on/off flips to.
Given that, I disabled this functionality since there is no safe I/O port to write to without affecting another peripheral.
Given this, what would like me to do? I could pick a random I/O port and send random on/off events to it. but it just doesn't seem very safe. What do you think?

So, there is no status LED to write on, on the reference platform

Oh I see my bad, I was not aware of that.

digitalWrite is actually fast on ESP32,

Thank god!!

Given this, what would like me to do? I could pick a random I/O port and send random on/off events to it. but it just doesn't seem very safe. What do you think?

Probably not a good idea to pick a random port as that would give unexpected results... I'll take a look at the code and how other platforms deal with this (if at all) and get back to you

z3t0 · 2017-03-27T00:51:42Z

boarddefs.h

@@ -147,12 +151,21 @@
 //
 #if defined(IR_USE_TIMER2)

+#ifdef ESP32 // Used in irSend, not implemented yet (FIXME)


This is good but the rest of the timer functions such as reset and count should also be implemented under here, refer to the comment before the last

This is related to IR sending, not receiving, correct?
If so, I don't have a way to test this code for now, hence my having disabled it and added a FIXME.
In other words, I know this is not the right code, but I'm leaving it to someone else to write and test, someone who is using IR sending and can test this and send you a patch after mine is in.
I do have to stub those though, otherwise the code won't build or run on the receive side.
So, I know this is the wrong code, but that's all I have for you :)

Oh I see! Sorry I saw the part about it not being implemented but didn't connect that to the rest being a stub... Sorry I'm a bit slow in the mornings!

In that case a better way to deal with this is like so:

#if defined(SOMETHING) // Make the compiler cry out since this is not yet implemented #warning "Sending has not yet been implemented for ESP32" #endif

More info

So, the irony, here, is that I was actually not to put ifdef ESP32 all around the existing code, and stick to boarddefs.h when possible (which is what you were also asking earlier on).
I can do what you suggest here, but basically it means moving board specific code away from boarddefs.h and instead add more ifdefs inside the main code.
From what you said earlier, I'm not sure this is what you wanted.

but basically it means moving board specific code away from boarddefs.h and instead add more ifdefs inside the main code.

Sorry but I may be misunderstanding you. Where would the ifdefs be placed?

Right now in change those defines to stub code that compiles and does nothing (i.e. '1').
If I need to emit a compile warning, it likely should be put inside the irsend function if it gets used/compiled in.
Or I could ifdef the whole code block in irsend
#ifdef ESP32
#warning no irsend support on ESP32
#else
the current code in the function
#fi

That's easy to do, but it adds more ifdefs directly inside the worker functions, which you didn't love (and I understand why, I don't love that either, which is why I found a way to avoid it here)

z3t0 · 2017-03-27T00:53:41Z

irRecv.cpp

@@ -117,6 +122,15 @@ IRrecv::IRrecv (int recvpin, int blinkpin)
 //
 void  IRrecv::enableIRIn ( )
 {
+// Interrupt Service Routine - Fires every 50uS
+#ifdef ESP32


I don't know enough about ESP32 to make a decision here, but there should be a neater way to do this with a compare interrupt

Sorry, you lost me here. What I understood of the AVR code, was that it fires every 50ns to run the ISR. What do you mean by "compare interrupt", or what should I be doing differently?

Oh okay sorry, I wasn't reading the code properly. It seems that you are using a compare interrupt after all. What that means is that the way timers work is that they count up to a certain number and then run an interrupt, meaning they run an interrupt on comparing the value of the timer to the value they count to.

I am not very experienced with using the Arduino methods for interrupt and am concerned that it may have a performance delay? The other method of course is to write directly to the bits ... but I am also not sure exactly how ARM bits work under Arduino, can we just write to them as usual? @bengtmartensson any ideas?

Also, this might be another issue where the function works fast on the ESP32 unlike normal Arduino and we don't have to worry about it.

Yes, it indeed running a timer/compare/reset like you said, which is why I was confused by your comment.
But I'm still confused by your comment in this context.
The code sets up an interrupt handler that fires every 50ns. I did the exact same thing on ESP32, it's just a different syntax.
Maybe there is a better way to be receiving IR, but I'm not trying to change this. I'm using the same code you already have and getting it to run as an interrupt with the same frequency on ESP32
Given that, I'm not sure there is anything to change here.

Okay so the difference here is that in your patch, Arduino functions are being used to enable the interrupt. Whereas, in the library we try our utmost to avoid Arduino functions and instead write directly to the register.

For example:

TIMER_CONFIG_NORMAL(); // Timer2 Overflow Interrupt Enable TIMER_ENABLE_INTR; TIMER_RESET;

These three macros are used to write to the registers of the microcontroller and to enable and configure the timers. This is done because the level of abstraction provided by the arduino functions are convenient but often slower than a direct write.

So for example, those macros above expand into for one of the Timer2 implementations:

#define TIMER_RESET #define TIMER_ENABLE_INTR (TIMSK2 = _BV(OCIE2A)) #define TIMER_CONFIG_KHZ(val) ({ \ const uint8_t pwmval = SYSCLOCK / 2000 / (val); \ TCCR2A = _BV(WGM20); \ TCCR2B = _BV(WGM22) | _BV(CS20); \ OCR2A = pwmval; \ OCR2B = pwmval / 3; \ })

The overall result is a higher performance library than if we were to directly use:

timer = timerBegin(1, 80, 1); timerAttachInterrupt(timer, &IRTimer, 1); // every 50ns, autoreload = true timerAlarmWrite(timer, 50, true); timerAlarmEnable(timer);

I am not sure about one thing though, as you mentioned earlier, the ESP32 does a better job with its high level functions? For example, you said digitalWrite is much faster on the ESP32. Perhaps do a bit of googling or searching on the Arduino forums to determine whether there is a real performance hit in using the Arduino functions to schedule timers on ESP32. I know for a fact that there is on the Arduino boards, which is why we prefer to write to the registers directly instead.

Does that make more sense? Sorry for the wall of text!

Also ESP32 is an order of magnitude, or two, faster than many AVR chips :)

Also ESP32 is an order of magnitude, or two, faster than many AVR chips :)

True! My only concern is that we should not take that speed for granted, especially not for optimizations that are easily made during compilation

Ah, I see your point now, but I don't think it's relevant in the ESP32 case:

you only setup the timer once. It does everything on its own after that.
ESP32 is definitely not AVR. You actually have a supervisor/HAL underneath and you're not talking to the hardware directly either way. However the one time I tried to use a way to bypass digitalwrite, I was told by the arduino-esp maintainer that what I did was actually slower and that I should stick to the digitalwrite API.
Long story short, using the actual API timer libraries on ESP32 is the correct thing to do, the arduino layer on ESP32 was definitely written to be fast, and trying to talk directly to the hardware is actually discouraged.
So everything you said makes sense, and is the way to go on AVR, but not the case for ESP32.

Ah that would make a lot of sense. Just for the sake of verbosity could you perhaps link that discussion with the maintainer or another source on this topic as I am not very well versed in it and would like to read up on it before making a decision? It's just that we would be changing a guideline that has been followed for a while on this library, which I have no concern over changing! But to do so I'd like to document the reasons in the process.

Mmmh, that was over gitter (i.e. IM), so I don't really have a good link.
But basically, this is the only supported way I know to fire interrupts on ESP32, and was told by the arduino-esp maintainer to use this.
I can add a comment stating so in my new pull request.

I've added a comment to address your concern/questions.

marcmerlin · 2017-03-27T14:59:23Z

Thanks for the detailed review and comments. Please see my followup questions before I can give you a new version.

marcmerlin · 2017-03-28T19:24:44Z

@z3t0 just to be clear, I did have a few questions above I was waiting for an answer on, before I can do that new version for you :)

z3t0 · 2017-03-28T22:42:54Z

@marcmerlin done!

marcmerlin · 2017-03-28T23:19:10Z

@z3t0 replied to your comments, PTAL

z3t0 · 2017-03-28T23:46:10Z

@marcmerlin done!

marcmerlin · 2017-03-29T00:05:42Z

Ok, is there still anything outstanding? If not, I'll work on an updated version of the PR with the comments I can act on with new code.
If I missed something, or you'd still like to confirm details before I go ahead, please let me know, I prefer to wait and spin one PR instead of 2 or more :)

z3t0 · 2017-03-29T00:54:54Z

No I think we're good! You can just push your new changes and it'll automatically be added to this pr

…ples. - fixed indenting on existing code in a few places for consistency - introduced IR_TIMER_USE_ESP32 for ifdefs within the code as per request - added comments explaining what's missing for irsend support on ESP32 - IRrecvDemo.ino gets a warning before and after interrupt is enabled in case it causes a crash TESTED=IoTuz ESP32 board and original 328p arduino to make sure current code did not break.

marcmerlin · 2017-04-01T05:00:14Z

IRremote.cpp

@@ -18,14 +18,17 @@
 // Whynter A/C ARC-110WD added by Francesco Meschia
 //******************************************************************************

-#include <avr/interrupt.h>
-


Note that I had to move this down for IR_TIMER_USE_ESP32 to be defined (you asked me to use this instead of ESP32, which I was able to test without moving the include line, but now I have to move it down)

marcmerlin · 2017-04-01T05:05:37Z

Ok, I believe I took care of all your comments, including reverting changes to the example code.
I've tested it on both 328p and ESP32 and it works fine on both for me.
Please take another look :)

z3t0

We still have some issues with ESP32 functions vs using generic macros with ESP32 specific implementations

z3t0 · 2017-04-01T05:07:51Z

boarddefs.h

@@ -39,9 +39,14 @@
 #	define BLINKLED_ON()   (PORTD |= B00000001)
 #	define BLINKLED_OFF()  (PORTD &= B11111110)

+// No system LED on ESP32, disable blinking
+#elif defined(ESP32)
+#	define BLINKLED        255


This works, @bengtmartensson what do you think?

z3t0 · 2017-04-01T05:16:01Z

boarddefs.h

+// way to do this on ESP32 is using the RMT built in driver like in this incomplete library below
+// https://github.com/ExploreEmbedded/ESP32_RMT
+#elif defined(IR_TIMER_USE_ESP32)
+#define TIMER_RESET	    1


I think it would be a better idea to use these macros instead of specifying the code elsewhere.

For example:

// In boarddefs.h #elif defined(IR_TIMER_USE_ESP32) #define TIMER_RESET() ({ timer = timerBegin(1, 80, 1);\ }) #define TIMER_INTR () ({ timerAttachInterrupt(timer, &IRTimer, 1); })

Note: this code doesn't actually compile since timer isnt defined but it explains the concept of using macro functions which can be replaced by the processor so the actual code compiled has no concept of "boards" because the compiler takes care of that

Mmh, I supposed I can do that, but that makes the code hard to read and debug to be honest (ESP32 actually has crash tracebacks with line numbers, but by the time you're using complex macros like this, it all stops working in a useful fashion).
If you insist, I can go ahead, but again the architectures are sufficiently different that

it kind of feels like pushing a round peg in a square hole

I'm not sure every last ESP32 define can be removed from the main code, or it's going to be ugly macros.
Basically I think macros were reasonable as long as you stuck with AVR, but by the time you're adding different platforms, I think a macro only solution is going to get out of hand, and honestly I haven't seen any other arduino library that goes through those extremes once they're supporting non AVR platforms.
I do realize it kind of is kicking the sandcastle and you're not very happy with the idea, so I'll let you consider and decide.

I haven't seen any other arduino library that goes through those extremes once they're supporting non AVR platforms.
I do realize it kind of is kicking the sandcastle and you're not very happy with the idea, so I'll let you consider and decide.

My experience there is quite limited as I haven't worked with any non AVR boards hmm.

I'm just trying to make suggestions from what I do understand about micro controllers 😄, no judgement. But it seems that my understanding doesn't really extend to non-AVR platforms as you have mentioned.

I'm perfectly fine with breaking the sandcastle though! I'll give this another run through and merge 🎉

z3t0 · 2017-04-01T05:18:43Z

IRremote.cpp

@@ -120,7 +123,11 @@ int  MATCH_SPACE (int measured_ticks,  int desired_us)
 // As soon as first MARK arrives:
 //   Gap width is recorded; Ready is cleared; New logging starts
 //
+#ifdef IR_TIMER_USE_ESP32


I'm not too keen on having a separate function just for the ESP32 and would again prefer macros dealing with this its much cleaner by ensuring that the only file that deals with the hardware differences is boarddefs.h

Though I am open to doing it this way as well, @bengtmartensson thoughts?

#ifdef IR_TIMER_USE_ESP32
void IRTimer()
#else
ISR (TIMER_INTR_NAME)
#endif
If there is a fancy way to write this with a #define, let me know. Otherwise, what do you mean by "separate function' ? I'm using the same function code, I just have to name it something else since the signature is different.

This essentially refers to the other comment about macros, il go reply on that.

z3t0 · 2017-04-01T05:20:50Z

boarddefs.h

+#elif defined(IR_TIMER_USE_ESP32)
+#define TIMER_RESET	    1
+#define TIMER_ENABLE_PWM    1
+#define TIMER_DISABLE_PWM   Serial.println("IRsend not implemented for ESP32 yet");


Try this just for consistency within the library code

Sure, done.

Just ping me once you've committed this change

z3t0 · 2017-04-01T05:21:11Z

examples/IRrecvDemo/IRrecvDemo.ino

  irrecv.enableIRIn(); // Start the receiver
+  Serial.println("Enabled IRin");


this is a good idea

z3t0 · 2017-04-01T06:18:48Z

Could you also add timer info to the README?

marcmerlin · 2017-04-01T15:33:55Z

Sorry, I missed the part where you asked me to increase y and not z.
Fixed.

PaulStoffregen · 2017-04-04T19:36:00Z

I don't understand why the ESP code can't use the macros like everyone else.

If this is done for more boards, the code will become quite a mess.

PaulStoffregen · 2017-04-04T19:37:38Z

Especially thing like this:

#ifdef IR_TIMER_USE_ESP32
void IRTimer()
#else
ISR (TIMER_INTR_NAME)
#endif

Really, how hard is it to just define TIMER_INTR_NAME to be IRTimer and define ISR the same as Teensy and other non-AVR boards do.

This pull request makes a mess just for one chip. Not good.

marcmerlin · 2017-04-04T21:58:08Z

@PaulStoffregen you obviously have more experience with this than I do (I have your teensy chips and 3rd party drivers, thank you).
I honestly didn't see how to do all of them with macros cleanly without multiline macros, but I'm not a macro expert.
At the same time, I do remember once doing a lot of macro magic where entire functions were defined as multine macros, which indeed could allow removing all ifdefs back to the single board file, but when I last did that, the code was undebugable. When you got tracebacks (and you get crash tracebacks on ESP32), you didn't get usable line numbers anymore. And never mind tracebacks, the code was just not readable to humans after that.
So, how about this: can you send a pull request with how you'd have done it, assuming it is possible to remove all the ifdefs without replacing entire code blocks with a single multi line macro (unless @z3t0 actually is ok with that tradeoff, that's his call to make afterall), and I'll be happy to review it as well as test it on real hardware.
That said, this macro trick:
#define ISR(f) void f(void)
is well done, I just didn't see it, but even that, it's ok for the compiler, but not easy to parse for a human how now will start thinking "oh, you scan use ISR(foo) on ESP32, great, I'll start doing that" when in fact, they can't.
I'm happy to send a PR to address this one, but I'm not sure how it will address the other multi-line ones.
If you think it's doable to do them all with the restriction above, do you mind sending a PR so that I can see and learn from it? (again, you won't have to test the code, I'll do that part).

z3t0 · 2017-04-05T16:27:39Z

@PaulStoffregen These were my thoughts as well... I think I'll revert the PR for the moment and allow the discussion to continue, but ideally yes we should avoid breaking the standards.

@marcmerlin sorry for all the fuss!

marcmerlin · 2017-04-05T16:43:58Z

Before you revert, I'm happy to send a followup CL, as long as you give me clear actionable direction. Otherwise honestly, I'm just going to give up on this.
In line ifdefs make it clear what's happening to each platform, yes, it's a bit spaghetti, and yes it's better if you can hide some trivial changes behind macros instead, but ultimately this is what many multi platform projects do.
Then, you have the purist method with 0 ifdefs no matter what, and everything hidden behind macros, no matter how complex those definitions end up being, how unreadable the end result is (if you care about the actual code run for a given platform, because if you're just reading for concepts it is more readable, you're just not seeing the real per platform code being run by the time you have dual macro substitutions being run as per the example discussed just above), and again it makes debugging with tracebacks, mostly not possible.
On that last bit, I realize if you're on a platform that doesn't support crash cores and tracebacks, you're losing nothing and you don't care :), but if you do have them, it's a 30-40 year jump back in time from before C and gdb were invented (complex multi-line macro definitions make it mostly impossible to see where a crash was since line numbers don't mean anything that useful anymore).

In this case, I've tried to use a hybrid approach, but things that were easy in the boards file to avoid sprinkling the entire code with ifdefs, but stay clear of double macro substitutions on a single line or macros that expand to multi-line code.
Paul, who I want to be clear, has much more experience with this than I do, made a cleaner port than me, but I also think it's a bit easy for him to point the finger here since his platform was closer to AVR than ESP32 IMO.

So back to this patch, I see 3 options:

you give me very clear and "will obviously work" alternatives for each part of the patch you still object to, even in hindsight, and I'll send you a followup patch.
you show me how what I wrote above is wrong, and how there is a clean nice way to write this with a patch you write, and I'll test and validate the code for you. Honestly for bigger changes this is going to be much quicker than endless back and forth between you and me. In the linux world, it's not uncommon for maintainers to take concept code from someone and integrate it in a better way that the original submitter wouldn't have necessarily been able to. I will take 0 offense if you rewrite my patch a better way, and will happily learn from the result.
you revert the code without further easily actionable guidance, and I'll just keep and use my fork and point everyone to it (although I will not maintain it in the future, so it'll stay at today's functionality)

#3 is silly, but at the same time, there is only so much time I can spend on this, and chasing a goal that I'm not even sure is achievable reasonably, or goes against what I think is reasonable readable code, isn't that appealing to me either.
Hope this makes sense.

z3t0 · 2017-04-05T16:47:14Z

Agreed #3 is silly, I'm quite busy at the moment, so I'll hold on reverting until I am able to give you actionable feedback

marcmerlin · 2017-04-05T16:48:44Z

And to be clear, "silly" applied to "me keeping an unmerged out of date fork", not "you reverting" :)

marcmerlin · 2017-04-05T16:55:18Z

If that helps my point that multiple plaforms for certain kinds of operations, ends up being hard, look at show() in
https://github.com/adafruit/Adafruit_NeoPixel/blob/master/Adafruit_NeoPixel.cpp#L117
it's not lovely but at the same time you can easily read what's happening for each platform.

bengtmartensson · 2017-04-06T08:21:47Z

I think that the present version is not "acceptable", as I have argued in my previous message, That also goes for the Adafruit example. The core problem is that the "high-level" files mix in concepts on the wrong abstraction level, like the type of the board.

I fooled around a bit, and came up with this. It is also found in my fork. Since I do not presently have the hardware, have not tested it though.

marcmerlin · 2017-04-06T16:15:40Z

@bengtmartensson thanks for giving this a shot, definitely helps.

I see you have a new esp32.cpp but you also have the same timer code in irRecv.cpp. I assume you meant to remove it from irRecv.cpp?
1.5) no ifndef => sure, why not.
#ifdef BLINKLED => good idea
#ifdef SENDING_SUPPORTED => also good idea also I'm not sure it will work as is since the method is verified to be there I think and things won't compile without it (I'd have to double check). This can be tweaked accordingly though.

If you are interested in seeing if your code compiles with the right defines, you can either force a define ESP32 at the top of the .h file to enable those code paths and verify that it builds although it might not quite due to the interrupt definitions.
or you can also install https://github.com/espressif/arduino-esp32 which will allow your IDE to build the code and make sure the compile goes through.
or if you're pretty sure it should just compile and work I'm happy to get your branch and try to build it (although right now comment 1) likely ought to be resolved first)

bengtmartensson · 2017-04-06T16:32:46Z

You are right, it should be removed from irRecv.cpp
1.5 ??? Note how #ifdef USE_DEFAULT_ENABLE_IR_IN should work.
The library should compile, however, for example IRsendDemo will not (with ESP32 defined). That is a GOOD THING, IMHO. The user does not have to flash his HW to get the error.

I wrote the patch for discussion, not for direct deployment. (Yes, I am a theoretician...)

marcmerlin · 2017-04-06T17:06:28Z

Understood
Can you send a pull request, not to get the code included as is, but to allow for seeing the diff in github, and give @z3t0 a chance to comment inline.
Once he's happy with it, I can take your branch over, test it, fix it up if needed and resubmit for inclusion (or we can get yours merged if it just happens to work already)

bengtmartensson · 2017-04-07T10:04:37Z

I have updated the fork; the update is available as patch here.

to allow for seeing the diff in github, and give @z3t0 a chance to comment inline.

I think it is reviewable as-is, but on his request, I will do so.

Add to 2. above: Cf. OO programming: Calling fly() on an object of type Penguin is supposed to give a compile-time error, NOT throw a run-time exception, or even worse, produce an error message saying that penguins cannot fly :-).

marcmerlin · 2017-04-07T14:04:36Z

Thanks @bengtmartensson . Your patch was reviewable before, just not as convenient to comment inline, in, as when it's a real github patch where the github UI allows one to type inline inside the patch.
So now, there are 2 incremental patches to review, one overrides the other one somewhat, correct?
master...bengtmartensson:master
Is easier to review since ti merges both patches, and if you click the pull request button, that allows use to comment directly on the patch in the right place.
Are you ok clicking the pull request button to allow that?

bengtmartensson · 2017-04-12T19:13:08Z

As you desired, I have created a PR, #437. Just for the fun of it, I threw in SAM support too, see #390.

z3t0 · 2017-04-12T22:09:14Z

Great thank you so much. Il take a look once I get a chance. In the meantime. If @ladyada and @marcmerlin and others could test.

marcelkottmann · 2017-04-27T12:43:13Z

@marcmerlin @z3t0 Are there any plans on implementing IRsend for ESP32? I am only asking because of the title of the PR (...not implemented yet).

bengtmartensson · 2017-04-27T12:48:09Z

Just defining USE_SOFT_CARRIER and removing the undef of SENDING_SUPPORTED for ESP32 and it "should" work. Please try. (USE_SOFT_CARRIER "should" work on anything fast enough.)

marcmerlin · 2017-04-27T13:57:11Z

@PePe79 I won't be adding this myself simply because I don't have hardware to test it, or any use for that feature.
What @bengtmartensson said is worth trying, but keep in mind that ESP32 can interrupt the arduino task at any time to run background stuff and therefore it might not be ultra reliable.
The proper, reliable way to do it is to use the RMT support on the chip which will output a perfect wave without tying up the CPU or relying on any timing, but that would be brand new code that doesn't exist.
For now the suggestion from @bengtmartensson may just work well enough.

marcelkottmann · 2017-05-04T20:00:59Z

@marcmerlin @bengtmartensson I had time to test your suggestions.

It took a while to understand that your suggested changes are based upon your current fork/PR: #437

In the end I accomplished to send some commands to my LG-television with my ESP32 board....woohoo ;-)

In order to do so, some minor other changes were necessary to compile successfully: marcelkottmann@9b71ca8

marcmerlin · 2017-05-04T20:16:01Z

@bengtmartensson just making sure you got this, would be good to merge into your temporary fork

bengtmartensson · 2017-05-05T07:02:20Z

I think the best way to proceed is to merge #437 ASAP, then to merge extensions like the one of @PePe79. Keeping my PR changing makes the process harder. (There is already too much in it.)

Added ESP32 IR receive support (IRsend not implemented yet).

Rename ESP32 timer name so that it doesn't conflict with other timers.

1b56da6

marcmerlin mentioned this pull request Mar 24, 2017

Using this code renders I2C unstable on ESP32/arduino? MartyMacGyver/ESP32-Digital-RGB-LED-Drivers#4

Closed

bengtmartensson suggested changes Mar 26, 2017

View reviewed changes

z3t0 reviewed Mar 27, 2017

View reviewed changes

z3t0 added New labels Mar 27, 2017

marcmerlin commented Apr 1, 2017

View reviewed changes

marcmerlin added 2 commits March 31, 2017 22:01

move comment about 'or else' chips that use TIMER2.

6b8f2bd

Merge branch 'master' into master

ffdb608

z3t0 requested changes Apr 1, 2017

View reviewed changes

Fixed rev to 2.3.3 and added info on timer used.

419c948

z3t0 merged commit e0c2649 into Arduino-IRremote:master Apr 1, 2017

z3t0 mentioned this pull request Apr 5, 2017

Revert "Added ESP32 IR receive support (IRsend not implemented yet)." #433

Closed

marcmerlin mentioned this pull request Apr 9, 2017

Please merge in upstream IRRemote crankyoldgit/IRremoteESP8266#148

Closed

bengtmartensson mentioned this pull request Apr 12, 2017

Cleanup of hardware dependencies. Merge in SAM support. #437

Closed

Theaninova pushed a commit to Theaninova/Arduino-IRremote that referenced this pull request Dec 2, 2023

Merge pull request Arduino-IRremote#425 from marcmerlin/master

b838f3e

Added ESP32 IR receive support (IRsend not implemented yet).

		irrecv.enableIRIn(); // Start the receiver
		Serial.println("Enabled IRin");

Added ESP32 IR receive support (IRsend not implemented yet). #425

Added ESP32 IR receive support (IRsend not implemented yet). #425

Conversation

marcmerlin commented Mar 20, 2017

marcmerlin commented Mar 20, 2017 • edited Loading

z3t0 commented Mar 24, 2017 • edited Loading

marcmerlin commented Mar 24, 2017

z3t0 commented Mar 24, 2017

marcmerlin commented Mar 24, 2017

AnalysIR commented Mar 24, 2017

marcmerlin commented Mar 24, 2017 • edited Loading

z3t0 commented Mar 25, 2017

marcmerlin commented Mar 25, 2017

bengtmartensson left a comment

Choose a reason for hiding this comment

marcmerlin commented Mar 26, 2017 • edited Loading

Choose a reason for hiding this comment

marcmerlin Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcmerlin Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcmerlin commented Mar 27, 2017 • edited Loading

marcmerlin commented Mar 28, 2017

z3t0 commented Mar 28, 2017

marcmerlin commented Mar 28, 2017

z3t0 commented Mar 28, 2017

marcmerlin commented Mar 29, 2017

z3t0 commented Mar 29, 2017

Choose a reason for hiding this comment

marcmerlin commented Apr 1, 2017

z3t0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

z3t0 commented Apr 1, 2017

marcmerlin commented Apr 1, 2017

PaulStoffregen commented Apr 4, 2017

PaulStoffregen commented Apr 4, 2017

marcmerlin commented Apr 4, 2017 • edited Loading

z3t0 commented Apr 5, 2017

marcmerlin commented Apr 5, 2017 • edited Loading

z3t0 commented Apr 5, 2017

marcmerlin commented Apr 5, 2017

marcmerlin commented Apr 5, 2017

bengtmartensson commented Apr 6, 2017

marcmerlin commented Mar 20, 2017 •

edited

Loading

z3t0 commented Mar 24, 2017 •

edited

Loading

marcmerlin commented Mar 24, 2017 •

edited

Loading

marcmerlin commented Mar 26, 2017 •

edited

Loading

marcmerlin Mar 27, 2017 •

edited

Loading

marcmerlin Mar 27, 2017 •

edited

Loading

marcmerlin commented Mar 27, 2017 •

edited

Loading

marcmerlin commented Apr 4, 2017 •

edited

Loading

marcmerlin commented Apr 5, 2017 •

edited

Loading

marcmerlin commented Apr 6, 2017 •

edited

Loading

marcmerlin commented Apr 6, 2017 •

edited

Loading

bengtmartensson commented Apr 27, 2017 •

edited

Loading