Bug in Raw IR Decoder for Arduino Code?

[dcutler1958]

I've been looking at the Raw IR Decoder For Arduino code (https://github.com/adafruit/Raw-IR-deco ... decode.pde) and I think I see a bug.

At lines 49 and 67 in the code, counters highpulse and lowpulse are incremented by one to keep track of the number of RESOLUTION (20 microsecond) delays waiting for a transition from high to low and vice-versa. Then, these counts are compared in their respective loops to see if the number of delays exceeds MAXPULSE (65 milliseconds or 65000 microseconds) to determine if a timeout occurred.

Instead of being incremented by one, shouldn't highpulse and lowpulse be incremented by RESOLUTION (20)? Otherwise, isn't the timeout delay RESOLUTION * 65 milliseconds rather than the expected 65 milliseconds?

Am I missing something here?

Thanks,

David Cutler

[adafruit_support_rick]

printpulses(void) takes care of scaling the values by multiplying by RESOLUTION:

Code: Select all

    Serial.print(pulses[i][0] * RESOLUTION, DEC);
    Serial.print(" usec, ");
    Serial.print(pulses[i][1] * RESOLUTION, DEC);
    Serial.println(" usec");


[adafruit_support_rick]

D'oh! I misread your question. Sorry.

I think you're right. MAXPULSE should probably be divided by RESOLUTION in the compare:

Code: Select all

     if ((highpulse >= (MAXPULSE/RESOLUTION)) && (currentpulse != 0)) {


and

Code: Select all

     if ((lowpulse >= (MAXPULSE/RESOLUTION))  && (currentpulse != 0)) {


[dcutler1958]

That's the quickest fix but personally, I prefer incrementing the counter by the number of milliseconds delayed. This also eliminates the need for multiplying the delay counts later when dumping out the data to the serial monitor.

I made some other changes to the code, for example, factoring out constant expressions in the while loop tests, making the value of currentpulse point to the last valid entry in the table, pretty printing the pulse table, eliminating the second delay counter, ... By the time I was done, I'd pretty much rewritten it.

Having done that, it all makes sense to me now.

[adafruit_support_rick]

That's the quickest fix but personally, I prefer incrementing the counter by the number of milliseconds delayed. This also eliminates the need for multiplying the delay counts later when dumping out the data to the serial monitor.

Interesting. You got me thinking about this. My intuition in a situation like this has always been to avoid doing increments by values other than 1 when I'm in a timing loop. A lot of the micros I've used have instructions for incrementing by 1, but incrementing by 20 would involve extra instructions and/or cycles for perfoming the add. I assumed that the AVR was similar, and that's why I suggested the constant divide.

So I just coded it up a few different ways and checked the disassembly. On a 16-bit increment, the compiler uses the ADIW (add immediate word) instruction for both incrementing by 1 and incrementing by 20. When I changed highpulse and lowpulse to uint8_t, the compiler used the SUBI (subtract immediate) instruction instead (that is, it subtracted -1 to increment by 1, and subtracted -20 to increment by 20).

Conclusions:
- For the AVR, at least, there's no time penalty for incrementing by RESOLUTION. I like your way better, since it's clearer, and cleaner.
- Never assume.

(FWIW, ADIW takes 2 cycles, and SUBI takes 1 cycle)

[dcutler1958]

My day-job is compiler writing; I haven't begun to look into the code generated by the IDE. I do that enough during the day.

I've attached my rewrite of the RawCListener code below if you are curious. I also set the pin used for the RawCListener to pin 4 to be consistent with the NECremote code (see below) so I didn't have to re-wire it when flipping back and forth between the two code modules. Several of these changes should be applied to the sample code to help keep the frustration level of new users to a minimum.

My next thought is to subtract a "fudge factor" to the call to delayMicrosceonds() to compensate for the time elapsed in actual code execution. Something like the equivalent of:
delays += RESOLUTION;
delayMicroseconds (RESOLUTION - 1);
and see how it works.

I should also change the timeout check from
if (delays >= MAX_DELAY) {
...
to
if (delays < MAX_DELAY)
continue;
...
in order to eliminate a likely branch to a branch. Hopefully there is peep-hole optimization to deal with situations like this.

The reason for starting all this work in RawCListener is that I ran into problems using the listener and NECremote code provided for the IR detector and remote control (https://github.com/adafruit/NEC-remote-control-library).

The code at line 70 of NECremote.cpp to verify that the 4-bytes of data read is valid is
if ((data[0] == (~data[1] & 0xFF)) && (data[2] == (~data[3] & 0xFF))) {
return data[2];
}
and does not work with my remote. The first two bytes sent by my remote are always 0x00 and 0xBF (even before I modified the code.) The third byte sent identifies the key depressed and the last byte is the 1's complement value of that key identifier.

After fixing the first part of the check, I found the one's complement check "(data[2] == (~data[3] & 0xFF))" fails when key 0 is depressed. I've printed out the values for data[2] and (~data[3] & 0xFF)) and they match in the output but the comparison invariably fails. I suspect the code generated for the comparison is somehow incorrect (maybe I just have a bad controller.) I'm using the 1.0 version of the Arduino IDE. Right now, just to get past the problem, I have an array map indexed by the key value that returns the expected 4th byte value.

Currently, my rewrite of NECRemote.cpp works well when no timeout is specified. However the code seems to get lost quickly when a timeout is specified. I suspect I've either forgotten to set/reset something when a timeout occurs or that the changes have altered the elapsed execution times and the timing is too far out to be useful. I you are curious, I'll send that out too.

BTW, you may notice in the code that I prefer declaring an enum to name a literal rather than using a #define. The first reason for doing so is to ensure the literal expression is a static expression at compile-time. The second reason is mainly a habit I've acquired working on large code systems because most modern debuggers can display the value of an enum but none can display the resulting expression value of a #define (except as the raw text of the define body.) The third reason is that enum literals are declared using the C/C++ scoping rules and #defines are not.

-----------------------------------------------------------------------------------------

// Raw IR decoder sketch!
//
// This sketch/program uses the Arduno and a PNA4602 to decode IR received.
// This can be used to make a IR receiver (by looking for a particular code)
// or transmitter (by pulsing an IR LED at ~38KHz for the durations detected.
//
// Code is public domain, check out http://www.ladyada.net and adafruit.com
// for more tutorials!

// We need to use the 'raw' pin reading methods because timing is
// very important here and the digitalRead() procedure is slower!
// Digital pin #4 is the same as Pin D2 see
// http://arduino.cc/en/Hacking/PinMapping168 for 'raw' pin mapping.
#define IRpin_PIN PIND
enum { IRpin = 4 } ;

// Mask to access IR input quickly.
enum { IRpinmask = _BV (IRpin) };

// The maximum pulse we'll listen for. 65 milliseconds is a long time.
enum { MAX_DELAY = 65000 } ;

// What our timing resolution should be, larger is better as it's
// more 'precise' but too large and you wont get accurate timing.
enum { RESOLUTION = 20 } ;

// Maximum number of pulses that can be recorded.
// We will store up to 100 pulse pairs (this is -a lot-).
enum { MAX_PULSES = 200 } ;

uint16_t pulses [MAX_PULSES];

// Index for pulses we're storing.
int16_t currentpulse = -1;

void setup (void) {
Serial.begin (115200);
Serial.println ("Ready to decode IR!");
}

void loop (void) {
uint16_t delays = 0;

// Wait for a transition from HIGH to LOW.
while ((IRpin_PIN & IRpinmask) != 0) { // Using "digitalRead (IRpin)" is too slow.
// Pin is still HIGH.

// Delay.
delays += RESOLUTION;
delayMicroseconds (RESOLUTION);

// If the pulse is too long, we 'timed out' - either NOTHING was received
// or the code is finished. Print what we've grabbed so far, and then reset.
if (delays >= MAX_DELAY) {
printpulses ();
currentpulse = -1;
return;
}
}

// We didn't time out so lets store the reading.
pulses [currentpulse += 1] = delays;

delays = 0;

// Similar to above, except waiting for a transition HIGH.
while ((IRpin_PIN & IRpinmask) == 0) {
// Pin is still LOW.

// Delay.
delays += RESOLUTION;
delayMicroseconds (RESOLUTION);

if (delays >= MAX_DELAY) {
printpulses ();
currentpulse = -1;
return;
}
}

pulses [currentpulse += 1] = delays;

// We read a high-low pulse successfully, continue!
}

bool is_even (int x) {
return ! (x & 1);
}

void printpulses (void) {
if (currentpulse < 0)
return;

Serial.println("\nReceived:\n H->L L->H");

for (uint8_t i = 0; i <= currentpulse; i += 1) {
if (pulses [i] < 10)
Serial.print (" ");
else {
if (pulses [i] < 100)
Serial.print (" ");
else {
if (pulses [i] < 1000)
Serial.print (" ");
else {
if (pulses [i] < 10000)
Serial.print (" ");
}
}
}
Serial.print (pulses [i], DEC);
Serial.print (" usec ");
Serial.print (is_even (i) ? "" : "\n");
}
}

[dcutler1958]

Sadly my last post lost all code indentations.

[dcutler1958]

Here's the final updated version with indentation....

In this latest version, I added an infinite loop to eliminate the cost of a procedure call and return while sensing transitions, I also eliminated the 2 dimensional array to simplify/minimize the array calculation that holds transition durations which should eliminate an add and a multiply operation. Array elements with even indices denote durations for transition from HIGH to LOW and elements with odd indices denote LOW to HIGH transition durations. I eliminated all the global variables, except for the transition duration table and changed the names of some variables to better match their function. I changed the test for timeout in each transition loop to potentially eliminate an unnecessary branch to a branch.

I also "simplified" the code to columnize the output nicely.

Code: Select all

// Raw IR decoder sketch!
//
// This sketch/program uses an Arduno and a GP1UX311QS to decode IR received.
// This can be used to make a IR receiver (by looking for a particular code)
// or transmitter (by pulsing an IR LED at ~38KHz for the durations detected.
//
// Code is public domain, check out www.ladyada.net and adafruit.com
// for more tutorials!
//
// We need to use the 'raw' pin reading methods because timing is
// very important here and the digitalRead() procedure is too slow.
//
// Digital pin #4 is the same as Pin D4 see
// http://arduino.cc/en/Hacking/PinMapping168 for 'raw' pin mapping.

enum { IRpin = 4 } ;

// Maximum number of transitions that can be recorded.
// We will store up to 100 pulse pairs (this is a lot).
enum { MAX_TRANSITIONS = 200 } ;
uint16_t transitions [MAX_TRANSITIONS];

void setup (void) {
   Serial.begin (115200);
   Serial.println ("Ready to decode IR!");
}

void loop (void) {
   uint16_t delays;
   int16_t idx = -1;

   // Mask to access IR input quickly.
   enum { IRpinmask = _BV (IRpin) };

   // The maximum transition time we'll listen for.  65 milliseconds is a long time.
   enum { MAX_DELAY = 65000 } ;  // Milliseconds

   // What our timing resolution should be, larger is better as it's
   // more 'precise' but too large and you wont get accurate timing.
   enum { RESOLUTION = 20 } ;  // Milliseconds
 
   for (;;) {
      delays = 0;

      // Wait for a transition from HIGH to LOW.
      while ((PIND & IRpinmask))  {  // Using "digitalRead (IRpin)" is too slow.
         // Pin is still HIGH, delay.
         delayMicroseconds (RESOLUTION);
         if ((delays += RESOLUTION) < MAX_DELAY)
            continue;
         
         // Pulse is too long, we 'timed out'.  NOTHING was received or the transmitted
         // code is complete.  Print what we've captured so far, and then reset.
         printtransitions (idx);
         return;
      }
 
      // We didn't time out, store the reading.
      transitions [idx += 1] = delays;
   
      delays = 0;
 
     // Similar to above, except waiting for a transition HIGH.
     while (!(PIND & IRpinmask)) {
        // Pin is still LOW, delay.
        delayMicroseconds (RESOLUTION);
        if ((delays += RESOLUTION) < MAX_DELAY)
           continue;

        printtransitions (idx);
        return;
     }

   // We read a high-low-high pulse successfully, continue!
   transitions [idx += 1] = delays;
   }
}

void printtransitions (int16_t idx) {
   if (idx < 0)
      return;
 
   Serial.println ("\nReceived:\n   H->L       L->H");
   
   for (uint8_t i = 0; i <= idx; i += 1) {
      uint32_t j = transitions [i];
     
      while ((j *= 10) < 100000)  // Columnize values nicely
         Serial.print (" ");
                 
      Serial.print (transitions [i], DEC);
      Serial.print (i & 1 ? " usec\n" : " usec ");  // Newline after printing duration for L->H transition.
   }
}


[adafruit_support_rick]

My next thought is to subtract a "fudge factor" to the call to delayMicrosceonds() to compensate for the time elapsed in actual code execution. Something like the equivalent of:
delays += RESOLUTION;
delayMicroseconds (RESOLUTION - 1);
and see how it works.

The timing really isn't all that critical here. IR remotes have a lot of slop. You might actually wind up with better accuracy if you *reduced* RESOLUTION to 50 or 100us, since you'd be reducing the contribution of loop execution to the total time measurement.

The code at line 70 of NECremote.cpp to verify that the 4-bytes of data read is valid is
if ((data[0] == (~data[1] & 0xFF)) && (data[2] == (~data[3] & 0xFF))) {
return data[2];
}
and does not work with my remote. The first two bytes sent by my remote are always 0x00 and 0xBF (even before I modified the code.) The third byte sent identifies the key depressed and the last byte is the 1's complement value of that key identifier.

Yeah. The whole IR "standard" is more like the Wild West. The original spec allowed for 256 device IDs and 256 codes (why would you EVER need more than 256 device id's??? ). It didn't take long for manufacturers to decide to forget about the check-byte and start using 16-bit IDs. And you can find other sneakier schemes out there for avoiding conflicts with other people's boxes.

After fixing the first part of the check, I found the one's complement check "(data[2] == (~data[3] & 0xFF))" fails when key 0 is depressed. I've printed out the values for data[2] and (~data[3] & 0xFF)) and they match in the output but the comparison invariably fails.

Weird. Time to dig into the code generation, after all!

BTW, you may notice in the code that I prefer declaring an enum to name a literal rather than using a #define. The first reason for doing so is to ensure the literal expression is a static expression at compile-time. The second reason is mainly a habit I've acquired working on large code systems because most modern debuggers can display the value of an enum but none can display the resulting expression value of a #define (except as the raw text of the define body.) The third reason is that enum literals are declared using the C/C++ scoping rules and #defines are not.

Interesting. I'm partial to enums myself, but I still use #defines. I shall re-evaluate some of those uses.

I also noticed your allergy to the '++' operator. Any particular reason for that?

[dcutler1958]

I do have an aversion to the ++ and -- operators.

I'm about to date myself here and go way back into the past when C and Unix were something Kernighan and Richie (K&R) were playing with at Bell Labs using a PDP 8 system. Trying to get a fully running Unix OS on a machine with 12 bits of addressing was definitely a challenge.

I never worked on a PDP 8 but I did work a lot on PDP 11-10s and PDP 11-70s. I believe all these systems had modes to pre- and post-increment (also pre- and post-decrement) the source register of many instructions. I'm certain it was a tremendous savings to be able to use these instruction modes as part of the original C language. However, I don't think many other architectures support these modes. So, in spite of K&R at the time claiming C was a portable language, they clearly relied on tricks like these to achieve their goals (not a bad thing but not really portable.) Likewise, I think break was added to switches as a performance improvement and code-saver to factor out common sequences of code and save space and time. Today, I think that the way the break statement works is much more dangerous than it is useful.

Modern optimization techniques look at these pre- and post-increment/decrement side-effects as a complication. It's very often the case that when you see ++ in the users source code, that operation has been transformed internally by the compiler to a canonical form that is easy for various optimization techniques to work with without having consider the side effects of the operators. For example. something like a simple byte check,

Code: Select all

     if (*x++ = '\0') ..


would be normalized to

Code: Select all

    temp = *x;
    x = x + 1;
    if (temp == '\0') ...


Using modern optimizing compilers, writing clever code that fools the optimizer and prevents it from performing optimizations may cost more than the clever code itself. I try to be straightforward with the code I write, first. I can tune it later if necessary.

After thinking about this, I tried the following code sequences in the raw listener sketch:

Code: Select all

     var [i++] = value;

     var [i = i + 1] = value;

     i++;                   // this is actually an expression, not a statement.
     var [i] = value;

     i += 1;
     var [i] = value;


but I saw no change in the size of the sketch which leads me to believe the compiler in the Arduino IDE normalizes the code and there is no benefit to using any single one of these paradigms over the other. I left the code as shown in the last example.

I also tend to fully parenthesize expressions and use more than the usual share of curly brackets. I work with several programming languages and I no longer remember the precedence rules for them all (also in C, a couple of the rules are wrong in my opinion ). It's just safer to put them (parenthesis) into the code so they are explicit. Parts of the company I work for require explicit parentheses to ensure the code works precisely as it appears in the source. The curly brackets often make it easier to determine at a glance which code is subordinate to other code. I'm also anal about indentation, even to the point of removing tab characters in the source code with spaces because tab characters are not portable from system to system.

Many of these habits have come out of being burned over the years.

Oh, that problem with one's complement of value 0x00 literally disappeared and I can no longer reproduce it. I'm hoping it was the side-effect of coding at 2am when I should be asleep.

I have been working with the NECremote module to see if I can get the code to work when a timeout is specified. I'll start another reply for that work.

[dcutler1958]

I took the code for NECremote.h and NECremote.cpp and made the following changes:

- Changed return type of "readNECbit()" to "int8_t" from "uint8_t" because it returns negative values.

- Removed code in "measurePulse()" to determine value of "statemask", changed formal parameter from "boolean state" to "uint8_t statemask" and then pass 0 and "_irpinmask" rather than 0 and 1 as actual parameters. This eliminates the need for a locally defined variable and eliminates an if-then-else statement or tertiary operator.

- Removed unnecessary initializations of "timeout" and "maxwaiting" in class initializer.

- Only initialize "maxwaiting" if maxwaitseconds is non-zero.

- Moved "_irpinreg" from "measurePulse()" to private area of the class and initialize it once in the class initializer rather than every time "measurePulse()" is invoked.

- Replaced code sequences of

Code: Select all

      if (maxwaiting < 0)
         timedout = true; // timed out waiting
     
      if (timedout)
         return -1;


with

Code: Select all

      if (maxwaiting < 0) {
         timedout = true; // timed out waiting
         return -1;
      }


in "listen()"

- Placed common delay code in "measurePulse()" in a define named "DO_DEFINE".

- Eliminated variable "timeout"; it's not needed.

- Fix check for valid 4 byte data entry to recognize the adafruit remote control I purchased.

- Placed out of range check for separating zero pulse right after getting that pulse in "readNECbit()".

- Declared constant expressions as enums to ensure they are static expressions.

- Changed type of "listen()" formal parameter maxwaitseconds from int16_t to uint16_t (no negative seconds).

- Removed the code to account and check for timeouts from "listen()" It is duplicate accounting and is already performed in "measurePulse()".

- Added defines IN_RANGE AND OUTRANGE for clarity and to factor out duplicate source code.

- Delay initializing "pulse" in "measurePulse()" until we've recognized the expected state.

In spite of all these changes I cannot get the code to work reliably when a timeout is specified. Thinking the code may be executing longer than DURATION milliseconds, I removed the internal function calls and turned them into defines (classic time/space tradeoff). At the end of this reply is the resulting code (I've left the formal functions in the code in case I want to revert to that code).

The code of interest is in the DO_DELAY define:

Code: Select all

#define DO_DELAY                    \
   delayMicroseconds (RESOLUTION);
#if 0
   if (timing) {                    \
      maxwaiting -= RESOLUTION;     \
      if (maxwaiting < 0)           \
         return -1;                 \
   }
#endif


This is the only place where timing is accounted for and this define is used in two places. If I turn the timing check off, the code works as expected. If I turn it on, hardly a key depressed on the remote control is recognized. I'm at the point of thinking that the timing check code is just too long and that using some other timeout method like a hardware defined timer and handler would be a better choice.

NECremote.h:

Code: Select all

// Basic timer-less NEC mini-remote library

// banned license

#if ARDUINO >= 100
#include "Arduino.h"
#else
#include "WProgram.h"
#endif

// what our timing resolution should be, larger is better
// as its more 'precise' - but too large and you wont get
// accurate timing
#define RESOLUTION 20

class NECremote {
public:
   NECremote (uint8_t pin);
   int16_t listen (uint16_t maxwaitseconds = 0);
   
private:
   uint8_t _irpin;
   uint8_t _irpinport;
   uint8_t _irpinmask;
   
   volatile uint8_t* _irpinreg;

   uint8_t timing;
   int32_t maxwaiting;
   
   // uint16_t measurePulse(boolean state);
   // int8_t readNECbit();
};


NECremote.cpp

Code: Select all

// Basic timer-less NEC mini-remote library

// banned license

#include "NECremote.h"
#include "pins_arduino.h"
#include "wiring_private.h"

NECremote::NECremote(uint8_t pin) {
   _irpin = pin;
   _irpinport = digitalPinToPort(_irpin);
   _irpinmask = digitalPinToBitMask(_irpin);
   _irpinreg = portInputRegister(_irpinport);

   timing = 0;
}

#define IN_RANGE(val, lb, ub) (((val) >= (lb)) && ((val) <= (ub)))
#define OUTRANGE(val, lb, ub) (((val) < (lb)) || ((val) > (ub)))

#define DO_DELAY                    \
   delayMicroseconds (RESOLUTION);
#if 0
   if (timing) {                    \
      maxwaiting -= RESOLUTION;     \
      if (maxwaiting < 0)           \
         return -1;                 \
   }
#endif

#if 0
uint16_t NECremote::measurePulse (uint8_t statemask) {
   uint16_t pulse;
   
   // Wait for the state to change.
   
   // while (((IRpin_PIN >> IRpin) & 0x1) != state) {
   while ((*_irpinreg & _irpinmask) != statemask) {
      DO_DELAY
   }
   
   pulse = 0;
   
   // In the proper state, keep track of the pulse duration.
   while ((*_irpinreg & _irpinmask) == statemask) {
      DO_DELAY
      pulse += 1;
   }
   
   return pulse;
}
#endif

#define measurePulse(statemask, counter)             \
   while ((*_irpinreg & _irpinmask) != statemask) {  \
      DO_DELAY                                       \
   }                                                 \
   counter = 0;                                      \
   while ((*_irpinreg & _irpinmask) == statemask) {  \
      DO_DELAY                                       \
      counter += 1;                                  \   
      }

#if 0
int8_t NECremote::readNECbit (void) {
   int16_t p1;
   int16_t p2;
#endif

   enum { ZERO_MIN_LO = 400 / RESOLUTION } ;
   enum { ZERO_MAX_HI = 700 / RESOLUTION } ;
   
   enum { ONE_MIN_LO = 1400 / RESOLUTION } ;
   enum { ONE_MAX_HI = 1800 / RESOLUTION } ;

#if 0 
   measurePulse (0, p1);
   if (IN_RANGE (p1, ZERO_MIN_LO, ZERO_MAX_HI)) {
      measurePulse (_irpinmask, p2);
      if (IN_RANGE (p2, ZERO_MIN_LO, ZERO_MAX_HI))
         return 0; // valid 0 bit
      if (IN_RANGE (p2, ONE_MIN_LO, ONE_MAX_HI))
         return 1; // valid 1 bit
   }
   
   return -1;
}
#endif

#define readNECbit(b)                                \
   measurePulse (0, p1);                             \
   if (IN_RANGE (p1, ZERO_MIN_LO, ZERO_MAX_HI)) {    \
      measurePulse (_irpinmask, p2);                 \
      if (IN_RANGE (p2, ZERO_MIN_LO, ZERO_MAX_HI))   \
         b = 0;                                      \
      else                                           \
         if (IN_RANGE (p2, ONE_MIN_LO, ONE_MAX_HI))  \
            b = 1;                                   \
         else                                        \
            return -1;                               \
   }                                                 \
   else                                              \
      return -1

int16_t NECremote::listen (uint16_t maxwaitseconds) {
   uint16_t p1 = 0;
   uint16_t p2 = 0;
   
   enum { LONG_PULSE_MIN = 8000 / RESOLUTION } ;
   enum { LONG_PULSE_MAX = 10000 / RESOLUTION } ;
     
   enum { SHORT_PULSE_MIN = 4000 / RESOLUTION } ;
   enum { SHORT_PULSE_MAX = 5000 / RESOLUTION } ;
   
   if (maxwaitseconds) {
      maxwaiting = maxwaitseconds * 1000000; // to microseconds.
      timing = 1;
   }
   
   // Wait for starting pulses.
   while (OUTRANGE (p1, LONG_PULSE_MIN, LONG_PULSE_MAX) &&
          OUTRANGE (p2, SHORT_PULSE_MIN, SHORT_PULSE_MAX)) {
      measurePulse (0, p1);
      measurePulse (_irpinmask, p2);
   }

#if 0
   Serial.println ("NEC");
#endif
   
   uint8_t data [4] = { 0, 0, 0, 0 } ;
   int8_t b;
   
   for (uint8_t i = 0; i < 32; i += 1) {
      readNECbit (b);
      if (b < 0)
         return -1;
     
      data [i / 8] |= b << (i % 8);
   }

#if 0
   Serial.print ("0x");
   Serial.print(data[0], HEX);
   Serial.print(" 0x");
   Serial.print(data[1], HEX);
   Serial.print(" 0x");
   Serial.print(data[2], HEX);
   Serial.print(" 0x");
   Serial.println(data[3], HEX);
#endif

   if (((data [0] + data [1]) == 0xBF) && (data [2] == ((~data [3]) & 0xFF)))
      return data[2];

   return -1;
}


listener.ino:

Code: Select all

// Example sketch for NEC remote - public domain

#include "NECremote.h"

// Connect a 38KHz remote control sensor to the pin below
#define IRpin         4

NECremote remote(IRpin);

void setup(void) {
  Serial.begin(115200);
  Serial.println("Ready to decode IR!");
}

void loop(void) {
  // You can set the listen() time out to 'n' seconds
  // int c = remote.listen(5);  // seconds to wait before timing out!
  // Or you can wait 'forever' for a valid code
  int c = remote.listen(1);  // Without a #, it means wait forever

  switch (c) {
     case -1: Serial.println ("no code");  break;
     case  0: Serial.println ("vol-"); break;
     case  1: Serial.println ("play/pause"); break;
     case  2: Serial.println ("vol+"); break;
     case  4: Serial.println ("setup"); break;
     case  5: Serial.println ("up"); break;
     case  6: Serial.println ("stop"); break;
     case  8: Serial.println ("left"); break;
     case  9: Serial.println ("enter"); break;
     case 10: Serial.println ("right"); break;
     case 12: Serial.println ("0"); break;
     case 13: Serial.println ("down"); break;
     case 14: Serial.println ("undo"); break;
     case 16: Serial.println ("1"); break;
     case 17: Serial.println ("2"); break;
     case 18: Serial.println ("3"); break;
     case 20: Serial.println ("4"); break;
     case 21: Serial.println ("5"); break;
     case 22: Serial.println ("6"); break;
     case 24: Serial.println ("7"); break;
     case 25: Serial.println ("8"); break;
     case 26: Serial.println ("9"); break;
     default: Serial.println ("unknown code"); break;
  }
}


[adafruit_support_rick]

I think I've been burned by a lot of the same things you've been burned by.

Ahhhh…PDP-11's! I still wish everything was a PDP-11. Such a lovely instruction set. And I recall stepping through some RSX-11M drivers that were written by a guy named Dave Cutler.

I'm at the point of thinking that the timing check code is just too long and that using some other timeout method like a hardware defined timer and handler would be a better choice.

Yup. The reason I know anything about IR codes is that I had to write an IR decoder for a commercial audio amplifier a few months ago. Have a look at the Input Capture feature of the AVR timers. Briefly, you run your IR signal (with the 38KHz carrier filtered out) into an input capture pin. You set up the timer to look for an edge transition on the signal. When the timer sees an edge, it saves its current count, resets to 0, and fires an interrupt. In your ISR, you read the saved timer count, which is equal to the pulse width at the resolution of the timer.

I built an event-based system, so in the ISR I just stick the timer value into an event object and queue it. The event processor implements a state-machine to decode the signal.

[dcutler1958]

That Dave Cutler was not me.

Except for memory-mapped I/O, I think the DEC 11-series is still amazing. I still remember keying in my first assembler program and debugging it on an 11-10 using just the lights and switches on the front of the CPU (no disk, no paper tape or punched cards, no querty keyboard, no download from another system, just keying in 1's and 0's manually). There was a dedicated single-step key that would execute one instruction at a time. When you depressed the key and held it down, the lights would display the PC value. I think when you released it, the lights would display the corresponding instruction. I also remember that the CPU was a black metal box about 1' high, 1.5' wide and at least 3' deep. Just a few years later, it was roughly the size of two pizza boxes stacked one on the other.

I'll look into the event-based solution next. However, I did get the code to work (see below)!

The aha moments were remembering that for Arduino, the basic integer type int is 16-bits in length and that accessing global data is more costly than accessing local data. I suspect that the statement "x -= 20;" is much more costly when the type of x is int32_t compared to int16_t. So, I changed the code to keep track of the time in "DURATION" intervals or ticks (one of your original recommendations.) I also made the counter a local variable by using defines instead of functions. The define CHECK_TIME controls the "ticking" and recognizes when a timeout has occurred.

Code: Select all

#define CHECK_TIME if (!(ticks -= 1)) return -1


I also changed the type of ticks to uint32_t to be more descriptive.

I kind of cheated on the wait-forever option when invoking listen(). If you specify 0 for the number of seconds before timing out, I set the number of ticks before timing out to be 0xFFFFFFFF. I think this is more than reasonable. To do a real wait forever option would mean having two sets of defines where one set invokes CHECK_TIME as is, and the other set uses an empty version of CHECK_TIME.

I also set the maximum user-specified timeout limit to be 255 (uint8_t) seconds which is more than enough.

One thing I dislike with the solution is the complex multi-statement defines. Normally I do the following with multi-statement defines:

Code: Select all

#define StartMultiStmtDef do {
#define EndMultiStmtDef } while (false)

#define MYDEFINE(...)
   StartMultiStmtDef   \
   ...                          \
   EndMultiStmtDef


Optimizers will remove the useless do { } while (false) and I can now use the define subordinate to a control statement without concern. Also, I can terminate the define invocation with a semicolon which makes it look like a function call in all contexts. For example:

Code: Select all

   if (...)
      MYDEFINE(...);


but I don't think the do-while loop will be removed by the Wiring compiler.

Here's the resulting working code:

NECremote.h:

Code: Select all

// Basic timer-less NEC mini-remote library

// banned license

#if ARDUINO >= 100
#include "Arduino.h"
#else
#include "WProgram.h"
#endif

// what our timing resolution should be, larger is better
// as its more 'precise' - but too large and you wont get
// accurate timing
#define RESOLUTION 20

class NECremote {
public:
   NECremote (uint8_t pin);
   int8_t listen (uint8_t maxwaitseconds = 0);

private:
   uint8_t _irpin;
   uint8_t _irpinport;
   uint8_t _irpinmask;
   volatile uint8_t* _irpinreg;

#if 0
   uint16_t measurePulse(boolean state);
   int8_t readNECbit();
#endif


NECremote.cpp:

Code: Select all

// Basic timer-less NEC mini-remote library

// banned license

#include "NECremote.h"
#include "pins_arduino.h"
#include "wiring_private.h"

NECremote::NECremote(uint8_t pin) {
   _irpin = pin;
   _irpinport = digitalPinToPort(_irpin);
   _irpinmask = digitalPinToBitMask(_irpin);
   _irpinreg = portInputRegister(_irpinport);
}

#define IN_RANGE(val, lb, ub) (((val) >= (lb)) && ((val) <= (ub)))
#define OUTRANGE(val, lb, ub) (((val) < (lb)) || ((val) > (ub)))

#define CHECK_TIME if (!(ticks -= 1)) return -1

#if 0
uint16_t NECremote::measurePulse (uint8_t statemask) {
   uint16_t pulse;
   
   // Wait for the state to change.
   
   // while (((IRpin_PIN >> IRpin) & 0x1) != state) {
   while ((*_irpinreg & _irpinmask) != statemask) {
      delayMicroseconds (RESOLUTION);
      CHECK_TIME;
   }
   
   pulse = 0;
   
   // In the proper state, keep track of the pulse duration.
   while ((*_irpinreg & _irpinmask) == statemask) {
      delayMicroseconds (RESOLUTION);
      CHECK_TIME;
      pulse += 1;
   }
   
   return pulse;
}
#endif

#define measurePulse(statemask, counter)             \
   while ((*_irpinreg & _irpinmask) != statemask) {  \
      delayMicroseconds (RESOLUTION);                \
      CHECK_TIME;                                    \
   }                                                 \
   counter = 0;                                      \
   while ((*_irpinreg & _irpinmask) == statemask) {  \
      delayMicroseconds (RESOLUTION);                \
      CHECK_TIME;                                    \
      counter += 1;                                  \   
      }

#if 0
int8_t NECremote::readNECbit (void) {
   int16_t p1;
   int16_t p2;
#endif

   enum { ZERO_MIN_LO = 400 / RESOLUTION } ;
   enum { ZERO_MAX_HI = 700 / RESOLUTION } ;
   
   enum { ONE_MIN_LO = 1400 / RESOLUTION } ;
   enum { ONE_MAX_HI = 1800 / RESOLUTION } ;

#if 0
   p1 = measurePulse (0);
   if (OUTRANGE (p1, ZERO_MIN_LO, ZERO_MAX_HI))
      return -1;
   
   p2 = measurePulse (_irpinmask);
   if (IN_RANGE (p2, ZERO_MIN_LO, ZERO_MAX_HI))
      return 0;  // valid 0 bit
   
   if (IN_RANGE (p2, ONE_MIN_LO, ONE_MAX_HI))
      return 1;  // valid 1 bit
   
   return -1;
}
#endif

#define readNECbit(b)                             \
   measurePulse (0, p1);                          \
   if (OUTRANGE (p1, ZERO_MIN_LO, ZERO_MAX_HI))   \
      return -1;                                  \
   measurePulse (_irpinmask, p2);                 \
   if (IN_RANGE (p2, ZERO_MIN_LO, ZERO_MAX_HI))   \
      b = 0;                                      \
   else                                           \
      if (IN_RANGE (p2, ONE_MIN_LO, ONE_MAX_HI))  \
         b = 1;                                   \
      else                                        \
         return -1

int8_t NECremote::listen (uint8_t maxwaitseconds) {
   uint16_t p1 = 0;
   uint16_t p2 = 0;
   
   uint32_t ticks = maxwaitseconds ?
      maxwaitseconds * (1000000 / RESOLUTION) : 0xFFFFFFFF;
   
   enum { LONG_PULSE_MIN = 8000 / RESOLUTION } ;
   enum { LONG_PULSE_MAX = 10000 / RESOLUTION } ;
     
   enum { SHORT_PULSE_MIN = 4000 / RESOLUTION } ;
   enum { SHORT_PULSE_MAX = 5000 / RESOLUTION } ;
   
   // Wait for starting pulses.
   while (OUTRANGE (p1, LONG_PULSE_MIN, LONG_PULSE_MAX) &&
          OUTRANGE (p2, SHORT_PULSE_MIN, SHORT_PULSE_MAX)) {
#if 0
      p1 = measurePulse (0);
      p2 = measurePulse (_irpinmask);
#endif
      measurePulse (0, p1);
      measurePulse (_irpinmask, p2);
   }

#if 0
   Serial.println ("NEC");
#endif
   
   uint8_t data [4] = { 0, 0, 0, 0 } ;
   int8_t b;
   
   for (uint8_t i = 0; i < 32; i += 1) {
#if 0
      b = readNECbit ();
#endif
      readNECbit (b);
      if (b < 0)
         return -1;
     
      data [i / 8] |= b << (i % 8);
   }

#if 0
   Serial.print ("0x");
   Serial.print(data[0], HEX);
   Serial.print(" 0x");
   Serial.print(data[1], HEX);
   Serial.print(" 0x");
   Serial.print(data[2], HEX);
   Serial.print(" 0x");
   Serial.println(data[3], HEX);
#endif

   if (((data [0] + data [1]) == 0xBF) && (data [2] == ((~data [3]) & 0xFF)))
      return data[2];

   return -1;
}


listener.ino:

Code: Select all

// Example sketch for NEC remote - public domain

#include "NECremote.h"

// Connect a 38KHz remote control sensor to the pin below
#define IRpin         4

NECremote remote(IRpin);

void setup(void) {
  Serial.begin(115200);
  Serial.println("Ready to decode IR!");
}

void loop(void) {
  int c = remote.listen (5);  // Without a #, or specifying 0 means wait forever

  switch (c) {
     case -1: Serial.println ("no code"); break;
     case  0: Serial.println ("vol-"); break;
     case  1: Serial.println ("play/pause"); break;
     case  2: Serial.println ("vol+"); break;
     case  4: Serial.println ("setup"); break;
     case  5: Serial.println ("up"); break;
     case  6: Serial.println ("stop"); break;
     case  8: Serial.println ("left"); break;
     case  9: Serial.println ("enter"); break;
     case 10: Serial.println ("right"); break;
     case 12: Serial.println ("0"); break;
     case 13: Serial.println ("down"); break;
     case 14: Serial.println ("undo"); break;
     case 16: Serial.println ("1"); break;
     case 17: Serial.println ("2"); break;
     case 18: Serial.println ("3"); break;
     case 20: Serial.println ("4"); break;
     case 21: Serial.println ("5"); break;
     case 22: Serial.println ("6"); break;
     case 24: Serial.println ("7"); break;
     case 25: Serial.println ("8"); break;
     case 26: Serial.println ("9"); break;
     default: Serial.println ("unknown code"); break;
  }
}


[adafruit_support_rick]

I actually wrote my first PDP-11 assembler program on a manual typewriter. The CS department didn't have an 11 or an 11 simulator, but the prof wanted us to go thru the process anyway. I worked with some 11/35's and 11/45's, but never got to key in any programs. However, I did have to work on a PDP-15 at a customer site, and I had to use the toggles there. Trouble was, some of the lights were burned out, and some of the toggles didn't work. We were using a logic analyzer to get some performance measurements of their production code, and fortunately all I had to do was toggle in some branches and/or counters here and there, but even that was a challenge when I couldn't really tell if I was in the right place and had toggled in the right code.

Most of my 11 work was on LSI's, where you had Console ODT instead of toggles. Much easier. I became the go-to guy for the HW guys, because I was able to write little machine code test programs into Console ODT on the fly - that's how simple and logical the instruction set was. These days, it's more likely that I have to work with banned like 8051, and I just want to cry. Haven't done any AVR assembler yet…

The code looks cool. Have you tried it with different remotes, or just the one? They all have their own little timing quirks.

Code: Select all

      readNECbit (b);
      if (b < 0)
         return -1;


Don't think you need the if stmt there - the macro already handles the return. Although, I suppose, it would be a little cleaner to leave the return here and take it out of the macro...

[adafruit_support_bill]

I still remember keying in my first assembler program and debugging it on an 11-10 using just the lights and switches on the front of the CPU (no disk, no paper tape or punched cards, no querty keyboard, no download from another system, just keying in 1's and 0's manually).

Used to have to boot an 11-10 that way. Eventually got around to soldering all those 1's and 0's into a diode boot board.