Cleanup of hardware dependencies. Merge in SAM support.

[bengtmartensson]

This PR attempts to clean up the low-level hardware dependencies. This problem was made glaring by #425 and its implementation by @marcmerlin. Later, the changes for SAM boards (see #390), first by @MrBryonMiller, later fixed by @ladyada, https://github.com/adafruit/Arduino-IRremote. These changes have been "cleaned" in the same sense.

All explicit hardware dependencies are now either in boarddefs.h, or in a board-specific file. Conditional compilation in the other files are now exclusively governed by "high-level" properties. Soft carrier (which sometimes is called "bit-banging") have been implemented hardware-independent.

[marcmerlin]

First, I'm just going to say is thank you for the architecture clean up and improvements and doing a better job than me abstracting all this.

Second, since you also had a look at the receiving code, I was wondering why it was based on a timer that fires no matter what, vs a pin interrupt that would only fire each time the IR signal toggles from low to high or high to low. Is it because on some AVR chips it's just difficult to do this on any pin while on newer chips per pin interrupts are trivial?
If so, I can see that having 2 versions of the IR receive code, one that is designed to work on pin level change and counting time between each interrupt, vs the one that fires on interval and samples the pin, would probably not be very desirable since it would be 2 different code bases.

[marcmerlin]

By the way, does ESP32 in that branch need re-testing? If so let me know and I'll happily do it.

[AnalysIR]

@marcmerlin

I was wondering why it was based on a timer that fires no matter what

I suspect the original design decision was related to the relatively limited resources available on 8-bit AVRs and that reserving interrupts in addition to 1 timer would reduce/limit the wider application/adaptation of the library (on 8-bit MCUs). This would still be valid today for AVRs and will continue to be a 'conflict' as more and more lower cost 32 bit MCUs are released. You might of course design it differently, if only focused on 32 bit MCUs with lots of flash, RAM & peripherals available or as a stand-alone 8-bit application.

It would be interesting to know the trend/quantum of active users of 8-bit vs 32-bit MCUs over time. I suspect it is still largely 8-bit in Arduino-land(guess?), particularly with beginners & non-experts. @z3t0 ?

[marcmerlin]

@AnalysIR I'm confused. Are you saying it's less expensive on a limited resource chip to interrupt based on time no matter what, than to have a pin interrupt that only fires if the pin actually changes?
I thought it was exactly the other way around, which is why I was surprised about the design and I was thinking there must be another limitation that escapes me (like maybe limitations in getting interrupts on an specific pin on some AVRs)

[AnalysIR]

Are you saying it's less expensive on a limited resource chip to interrupt based on time no matter what, than to have a pin interrupt that only fires if the pin actually changes?

No. Simply when creating a library for general use along with other 3rd party libraries...all on a resource limited platform....it makes less sense to 'reserve/use-up' multiple resources. Essentially for AVRs the library uses one timer (for Rx &Tx/PWM). If it was designed with interrupts, then it would still use up 1 timer for TxPWM plus the interrupt/pin.

If it was the only functionality running on the MCU, then not a big problem. However, if you want it to work alongside other libraries & user applications, then it is a good idea for the library to use up as few resources as possible - on an 8-bit platform.

[marcmerlin]

@AnalysIR ok, maybe I just don't really understand how the code works.
When I was talking about using a pin interrupt for IR reading, I meant only that. I thought you could just track the input pin state change and how much time elapsed between those changes? (and not have a timer at all anymore). In other words, you'd only have a single on demand interrupt and free up a timer.
Isn't that a win on all platforms assuming it can indeed work that way?
Of course, if that's indeed possible, it would definitely be a big rewrite of how IR read works, so not a trivial change, and per platform pin specific interrupts may be even more of a pain to configure than just a timer.

[AnalysIR]

In other words, you'd only have a single on demand interrupt and free up a timer.

The additional timer is still required to generate the Carrier PWM for IR Tx.(generating the carrier without a dedicated Timer using bit-banging will vary by MCU/XTAL/Compiler/Optimisation and would be messy to manage/support for a general purpose library). All in all, the original design choice has stood the test of time very well for 8-bit MCUs. 32 bit platforms complicates things because of the relative abundance of resources. It is also important to note that standard IR receivers can distort mark/space timings by up to +/-200uSecs.

For custom & stand-alone applications, interrupts are clearly the better way to go on all platforms....although I remember looking at the new ESP32 datasheet and it seems to have some new dedicated IR peripheral built in, which on first read seemed to be overkill, as typical interrupts/PWM/Timers seem to serve all IR needs. I have ordered some ESP32s to investigate this new IR peripheral.

Essentially, it all boils down to making design decisions, based on the future 'goals' of the library while also taking the existing user base into consideration. I believe @z3t0 is looking into documenting the Goals and once that is in place getting answers to questions like yours should be more straightforward.

[marcmerlin]

@AnalysIR thank you for explaining, I totally missed out on the fact that this library uses a timer for sending too (which I haven't used at all). So if you're only receiving, you're losing out by using a timer over a pin interrupt, but if you're doing both, you've already lost the timer, so you might as well use it for both, makes sense.
For ESP32, you are correct, it comes with an RMT which can be used to basically make precisely timed waves of any kind (8 channels). It was originally meant for IR, but is also being used for other things like neopixels for instance. This is also because of this that I didn't try to port the current sending code to ESP32 (although I also had no use for it), because RMT is the better way to do it and would require new, separate code.

[marcmerlin]

@bengtmartensson I just tested this on ESP32, it works fine. You're just missing one line for it to compile ok and after that it's ok. Good job with the refactoring:

--- a/boarddefs.h
+++ b/boarddefs.h
@@ -607,6 +607,7 @@
 // https://github.com/ExploreEmbedded/ESP32_RMT
 #elif defined(IR_TIMER_USE_ESP32)
 
+#define TIMER_RESET
 #ifdef ISR
 #      undef ISR
 #endif

[bengtmartensson]

@marcmerlin , @AnalysIR , @z3t0
Discussing interruptdriven IR reception is, although interesting and important in its own right, here offtopic. I have thumb-down-ed OT responses. Lets keep this discussion on the topic of the PR.

[AnalysIR]

@marcmerlin , @AnalysIR , @z3t0
Discussing interruptdriven IR reception is, although interesting and important in its own right, here offtopic. I have thumb-down-ed OT responses. Lets keep this discussion on the topic of the PR.

Although in general you are of course correct. In this case, I have to disagree as I believe that the 'discussion' was both warranted and worthwhile, particularly given the recent efforts of @marcmerlin on this and related topics.

I guess as long as PRs & Issues are the only places for discussion, there will always be a certain amount of spill-over like this.

[AnalysIR]

Regarding the bit-banging of the IR signal carrier:
It would be useful to verify correct operation against a scope or LA.
This would also be relevant if more than one clock frquency is supported on the platform, in which case the calculations would need to take that into account or at least specify that only one clock speed is supported.

[MrBryonMiller]

I started looking at @bengtmartensson excellent work pulling all of this together. Thank you! In reviewing the result I was going to make comments in two areas but first I thought I could help out by testing compilation and operation on an AdaFruit Feather M0 (a SAMD processor). I realized that the SEND_PIN option I initially allowed for (and which @ladyada kept in her version) had been removed. Now the pin is hard-coded as pin 3. Unfortunately I cannot find this pin at the headers of the product so I cannot test nor use this PR unless I modify it. This brings me to my first comment.

1. I understand that it might be consistent with the original library to not allow user definition of the send pin. But I would maintain that this would be a good time to incorporate this ability. Especially as new 32 bit processors are added and soft carrier becomes more prevalent it would seem a shame to use an arbitrary assignment. Furthermore if user definition is allowed even for AVR processors it would be a very useful feature to have a run-time error stating that the chosen pin is not supported. It would be even more useful if the error could state just what pin is supported. (As a new user I found this information difficult to figure out without digging through the library's code)

2. I saw that @bengtmartensson generalized the soft carrier output by going away from the hardware dependent port assignment I used (and which @ladyada kept in her version) to using digitalWrite. I'm guessing because of this generalization a need was felt to compensate for the overhead time by using PULSE_CORRECTION_ON and PULSE_CORRECTION_ON. These are set to 5 and 6 usec respectively. For a 38kHz pulse, the half period is 13 usec. So it seems almost half the time is allocated in an open loop manner. I set out to measure what was actually needed on the 48MHz Feather M0. My software based measurements suggest that if I used the hardware specific method the loop could be exited in less than 1 usec of the correct time. This is why I felt no compensation was necessary. But if the general digitalWrite is used the loop exit could be delayed by as much as 2.6 usec. At his level I would agree that compensation should be applied, but rather than 5 or 6 usec the values should be more like 1 and 2. To me this suggests that using the hardware specific method has value in there is less that could go wrong. But if the more general digitalWrite is used there should be a hardware specific setting for the two correction values. And no matter which way prevails, as @AnalysIR suggests, the output must be verified with a 'scope (something I cannot do).

[bengtmartensson]

@MrBryonMiller :
Pin assignment: First of all, you can change TIMER_PWM_PIN (line 72 in boarddefs.h) to whatever sensible, (Yes, the name is now "semantically drifted".) I wanted to keep the API for this level of PR, therefore I did not make it user selectable. But of course, it would be silly to keep it that way in the long run, just as you say. What do you say about (at least for now) having the constructor for IRsend having a default argument?

IRsend(pin = SEND_PIN)

@z3t0 : how about renaming TIMER_PWM_PIN to SEND_PIN? It is the pin used for sending; putting how it is sent into the variable name seems wrong.

Soft carrier (which I consider a better name than "bit banging"): I used an arduino.org M0+, 48kHz. (Sorry for that; I ordered it as "Arduino Zero"). I tested with IRSenddemo (which is supposed to generate a 40kHz carrier), and analyzed the generated signal using a board like this, deploying a non-demodulation TSMP58000 and the AGirs firmware and IrScrutinizer. (I have high confidence in this configuration.) I then, with parameters like @ladyada's, measured around 32kHz. So I changed them to the current ones, which gave pretty exactly 40kHz. So, in this sense, it HAS been tested empirically, although not extensively. (No need to test with a 'scope, though.)

Having said that, I am not all convinced about the sanity of the implementation, neither the old one nor mine. It is completely non-robust against (varying) computational delays. I/We should probably try to cook up something better. But, if possible, a hardware carrier (hardware PWM) implementation should normally be preferred over a soft carrier implementation.

[z3t0]

@z3t0 : how about renaming TIMER_PWM_PIN to SEND_PIN? It is the pin used for sending; putting how it is sent into the variable name seems wrong.

Makes sense to me.

[z3t0]

But, if possible, a hardware carrier (hardware PWM) implementation should normally be preferred over a soft carrier implementation.

That is generally my experience as well. Are there any particular benefits for a soft implementation?

[MrBryonMiller]

@bengtmartensson

you can change TIMER_PWM_PIN to whatever sensible

Yes, I will edit library file so I can give this a try.

What do you say about (at least for now) having the constructor for IRsend having a default argument?
IRsend(pin = SEND_PIN)

Yes! This is the way I originally had it. @ladyada added in the invert argument but later agreed that it was a little too much.

how about renaming TIMER_PWM_PIN to SEND_PIN?

I know you weren't asking me about my opinion but I thought I'd weigh in. Only reason to keep the name TIMER_PWM_PIN is it lets you keep the description "semantically drifted". I love that description.

@z3t0 : Are there any particular benefits for a soft implementation?

I'm embarrassed to admit but the main reason was to get something working for a project I was building without having to dig through more processor specs about it timers and output multiplexers. Added benefit was it didn't tie up a second timer and it allowed complete output pin flexibility. I'm now wondering if the same timer used during receive couldn't be switched during send to run a PWM (and switched back when receive is re-enabled). Also whether with the M0's output multiplexers whether broad output pin flexibility is free even with HW PWM. I'm thinking about digging into this but results could be months away.

Regarding the use of the PULSE_CORRECTIONs terms : According to my software base measurement, corrections should not be necessary, but @bengtmartensson actual measurements suggest they are needed. I have to resolve this in my mind. So I have managed to round up a fairly modern, fairly accurate oscilloscope (as a supplement for my 1970's era Heathkit) and I'm going to be investigating pulse width and frequency accuracies with and without corrections. Hopefully results to follow in a day or so.

[MrBryonMiller]

@bengtmartensson I did not realize the default for the soft carrier was to use spin wait. I did not realize this until I saw your last commit. I figured that might have been contributing to me getting inconsistent results (in addition to the cheap receiver devices). So I tested your latest with my program that sends 50 NEC values in a row. What I found was that anywhere from 1 to 4 times out of the 50 the program would get hung up in the send routine. I could wait it out but each time it took right around 90 seconds before it would continue on. I instrumented enough to watch the value of micros() when the hangup occurred and micros was nowhere near wrapping around its 32 bit value. (as one would expect since the program only runs a few minutes, nowhere near 4,000 seconds.) I've studied your mark code in some detail and can't see anything might cause this. I thought that maybe there was something in you library that cause the receive interrupt to continue to run during send but could not find anything wrong with that either.

Have you seen anything like this? Anyone else?

[bengtmartensson]

Just a few quick comments:

• Please show us your sketch
• Please try both with and without spin wait,

something in you library that cause the receive interrupt to continue to run during send
?? Are running receive simultaneously to sending?

[MrBryonMiller]

Please show us your sketch

#include <IRremote.h>

#define LED_PIN 13

int RECV_PIN = 11;         

IRrecv My_Receiver(RECV_PIN);
IRsend My_Sender;

void setup() 
{
// start serial port
pinMode(LED_PIN, OUTPUT);
Serial.begin(9600);
int waitcnt = 0;
while(!Serial && (waitcnt++ < 50))  // wait (only so long) for serial port to connect.
     delay(100);
Serial.println("Serial Started");

for (int i=0;i<50;i++)
     {
     Serial.print(i);
     Serial.print(" ");
     unsigned long s = micros();
     sendcode(0x61A020DF);
     Serial.println(micros()-s);
     delay(250);
     }

My_Receiver.enableIRIn(); // Start the receiver
My_Receiver.blink13(0);
}

void loop() 
{
}

void sendcode(long code)
{
Serial.print("Sending ");
Serial.println(code,HEX);
My_Sender.sendNEC(code,32); 
Serial.print("delay ");
Serial.print(micros(),HEX);
delay(500);
Serial.println(" Sent");
}

Please try both with and without spin wait,

Works as expected when USE_SPIN_WAIT is defined. Hangs up randomly when USE_SPIN_WAIT is commented out.

Please note, when I say works as expected I mean sending program progresses normally. But there are still a high level of receive errors. TSOP4838 should be here tomorrow.

Are running receive simultaneously to sending?

No, but it'd be nice if we could.

[bengtmartensson]

@MrBryonMiller :
I see several problems. You are mixing different time sensitive operations; sending as well as Serial prints. Using 9600 bits/s may also be a problem. There is a lot of timing involved in Serial, involving timers... Please delete all console output and try with just

void loop() {
    irSend My_Sender.sendNEC(..., 32):
    delay(....);
}

Please also remove the irRecv instance. To be on the safe side, no not instantiate Serial at all.

Just as @AnalysIR said that he will not accept results from a cheapie IR receiver, I will not accept results with intermingled console output.

Works as expected when USE_SPIN_WAIT is defined. Hangs up randomly when USE_SPIN_WAIT is commented out.

Which at least shows that it can be valuable to have it, at least as a troubleshooting option.

[MrBryonMiller]

@bengtmartensson

Following still gets hung up every 3-10 times through loop.

#include <IRremote.h>

IRsend My_Sender;

void setup() 
{
for (int i=0;i<50;i++)
     {
     My_Sender.sendNEC(0x61A020DF,32); 
     delay(500);
     }
}

void loop() 
{
}

[bengtmartensson]

@MrBryonMiller
sorry, I am hardly able to help you at this point. Possibly you are running out of stack or something like that. What exactly does "gets hung up" mean?

Again, it runs fine for me using, I think, the same processor. I received a M0 Feather, but I have not have time to test it. After all, this is about the PR, not sorting out your problems...

How do we proceed with the PR?

[MrBryonMiller]

@bengtmartensson

I make sure I did not contaminate your library while I was making comment changes to SPIN_WAIT I downloaded your latest commit and reran my tests.

Possibly you are running out of stack or something like that.

Perhaps - but if I change the IR library to @ladyada's library there is no anomalous behavior.

What exactly does "gets hung up" mean?

It is impossible to tell with the version of the sketch you asked me to build that has no Serial.prints other than occasionally the receiver shows nothing being received for 90 seconds or so. For the version of the program that has prints, for those iteration where a hangup occurs, I can see a print is made just before calling sendNEC and about 90 seconds after the return from sendNEC. The prints after the call shows a little more than 90 million microseconds has elapsed. Please remember this 90 second problem does not occur on every send. At least 40 out of 50 seem to work fine (although many with receive decode errors).

Again, it runs fine for me using, I think, the same processor.

I also think we are using the same processor - but if you install the Feather M0 following AdaFruit's instructions you do get some different core libraries. I think I looked at the implementations of micros() and delayMicroseconds() for the two cores and I could not tell a difference, but what do I know.

How do we proceed with the PR?

I am going to rerun my test using using a TSOP34438 on my receiver end and using SPIN_WAIT from the @bengtmartensson library to see if the reception errors clear up (I should have results shortly). Depending on the results of that I would ask deferring a decision until someone can confirm the library does not conflict with something on a Feather M0. If an immediate decision does need to be made and if you wanted SAM support and you felt that support should include a Feather M0 and if it were up to me I would have to recommend @ladyada's PR over than this one. I think that would be a shame since we should all recognize that @bengtmartensson did an awful lot of cleanup and generalization work.

[MrBryonMiller]

I have retested with TSOP4838's. I used @bengtmartensson's library with one edit - I defined USE_SPIN_WAIT.

50 out of 50 were received correctly. Thank you @AnalysIR!

How do we proceed with the PR?

I would change my recommendation - If an immediate decision does need to be made and if you wanted SAM support and you felt that support should include a Feather M0 and if it were up to me I would have to recommend either this PR with change of using SPIN_WAIT being the default or @ladyada's PR. I would be willing to reconsider the change of my USE_SPIN_WAIT recommendation if someone can confirm that the Feather M0 does not get hung up.

[MrBryonMiller]

@bengtmartensson
I found what is causing the hangups when not doing SPIN_WAIT. In your loop you calculate nextPeriodEnding. On occasion (usually less than 1 time out of 10), during the last time through the loop, this value can actually be less than the current value of micros(). When that happens, your calculation of targetTime-micros() using unsigned math ends up being a very large number. Hence the very long delay. (I have no explanation why you did not see the hangups other than possibly your M0 is somewhat faster so slower than mine so you never get into the case I run into.)

I fixed this by changing your test in sleepMicros from if (us > 0U) to if ((us > 0U) && (us < 0x0000ffff))
When I made this change my 50-in-a-row test ran every time I tried it without hanging up and thanks to the higher quality sensor on my receiver, No errors!

But what I really wish you would consider is replacing your entire loop, including the USE_SPIN_WAIT distinction, with the code I suggested earlier. I have re-included it here with some added comments and updating the 'now' variable name and using SENDPIN ON and OFF to be consistent with your work.

long now = micros();
long markStop = now + (long) time;
// send out HIGHs and LOWs for the entire time of the Mark
// note the use of signed math so that wrap-arounds, when they occur, are properly handled
while ((markStop - now) > 0) { 
	SENDPIN_ON(sendPin);
	long HIGHStop = now + periodOnTime;
        // send out the HIGH for one half the period time OR until the Mark time is exceeded
	while (((HIGHStop - now) > 0) && ((markStop - now) > 0))
		now = micros();
	SENDPIN_OFF(sendPin);
        // send out LOW for one half period time OR until the Mark time is exceeded
	long LOWStop = now + periodTime-periodOnTime;
	while (((LOWStop - now) > 0) && ((markStop - now) > 0))
		now = micros();
	}

I know it has open loop timing considerations, but I feel they are small. And it has the advantage of terminating the Mark when its time is up, even if its in the middle of HIGH time - just like a HW PWM implementation would.

[bengtmartensson]

On occasion (usually less than 1 time out of 10), during the last time through the loop, this value can actually be less than the current value of micros(). When that happens, your calculation of targetTime-micros() using unsigned math ends up being a very large number. Hence the very long delay.

EXCELLENT!! Explains what happens without any guesswork, which is very satisfying, also theoretically. (Just slightly embarrassing for me...:-( ). I selected another fix though.

But what I really wish you would consider is replacing your entire loop, including the USE_SPIN_WAIT distinction, with the code I suggested earlier.

Actually, I consider my code better. It encapsulates the waits better, allowing for a clean selection between spin wait and normal wait. Also,

long now = micros();

is outright wrong.

[MrBryonMiller]

EXCELLENT!! Explains what happens without any guesswork

Yes - mystery is still where did 90 or so seconds come from. Looking inside of delayMicroseconds() I
get a glimmer of an answer due to 2^32 / 48000000 = 89. Where 2^32 is the approximate value (actually 2^32 minus a couple) that is argument to delayMicroseconds when the error occurs. And 48000000 is the clock speed of the M0. Still don't pretend to understand exactly.

Actually, I consider my code better.

I respectfully disagree - but I give up.

long now = micros(); is outright wrong.

I might not have done this in the "right" way but I needed to perform signed math so wrap-arounds worked out correctly. So I needed now to be a long. But again, I give up.

Bottom Line : I looked at @bengtmartensson latest commit - I have tested send and receive with a Feather M0. I have previously done some level of regression testing with other machines. I believe this commit would make a fine addition to this great Library.

[bengtmartensson]

Just a final remark:

I might not have done this in the "right" way but I needed to perform signed math so wrap-arounds worked out correctly. So I needed now to be a long.

AFAIK, what happens when a signed variable in C (C++) "wraps around" is undefined. There might even be hardware "traps" fired when wrapping around. C is not Java, which is guaranteed to use 2-complement. There might be compiler flags that, for a certain processor, forces a particular behavior. But, if the aim is to write portable code (that runs on all processors) -- which should be our aim -- stay out of undefined behavior.

[bengtmartensson]

Any progress on reviewing/merging this?

[z3t0]

@bengtmartensson Sorry I am currently busy with things in life and will not have a chance to merge or review any times soon.

I would suggest we go ahead and create a beta release with the changes in this merge and add that to the releases.

Thoughts?

[bengtmartensson]

Almost 3 years has passed, and no sign of a merge... I have implemented all the features: SAM support send and receive, soft carrier PWM, and much more, in the just released version 1.1.0 of my library Infrared4Arduino. (Available in the Arduino library manager using the name Infrared, in a few hours).