Recommended strategy for using of PinChangeInt to read RC receiver pulses

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Connect Arduino Uno or Nano to an RC receiver
2. Each receiver channel is connected to a separate digital input pin on the 
Arduino
3. Attempt to read the pulsewidth for each channel in the background of the 
main control code using PinChangeInt

This is not a defect but just a request for guidance on how to best use this 
library.  The posted ReadReceiver Arduino sketch detaches and reattaches 
interrupts in the Arduino Loop function.  To me, this opens up an opportunity 
for missing pulses and does not seem like a good strategy. It also uses the 
timer1 library for measuring the pulsewidths.  This will cause a conflict will 
driving servos, so my preference is to simply use the micros function to 
determine the pulsewidth, leveraging the already utilized Timer0.  I would 
store the millis time on the pulse rise, and subtract this from the millis time 
on the pulse drop.

The strategies I am considering fall into two options:  use the same userFunc 
for all pin changes, or a separate userFunc for each pin change.  In the former 
case, this matches your examples, but I then need to determine in the sketch 
which pin change caused the interrupt by querying the arduinoPin variable.  
Having this extra logic in the ISR seems like an unnecessary performance hit, 
since this logic is already evaluated in the library code.  

My currently preferred approach is the later:  have a separate userFunc for 
each pin.  In reading over the library code, it appears that this is supported. 
 Namely, that there is a linked list between pins and user functions.  In this 
case, I would use the millis function to measure the pulsewidth in each 
userFunc, and I only need to evaluate whether the pin is high or low to 
properly manage the logic to read the pulsewidth.  

It appears that my second approach should work, but I don’t see it in any of 
your examples.  Am I on the right track?

Thanks in advance.

Original issue reported on code.google.com by steve.ha...@gmail.com on 4 Jan 2012 at 4:34

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

I developed a sketch (attached) that tested my preferred approach to reading an 
RC Receiver.  It appears to be working fine.  It was tested under Arduino 1.0 
and 0023 with a Nano, a GWS RC servo, and two RC servos to mirror out what was 
read.  It is my understanding that the Arduino digitalRead function is many 
times slower than an AVR direct port data register read, so there are several 
comments in the sketch about how to handle that.  That portion could be 
improved to make the code more portable.

Original comment by steve.ha...@gmail.com on 5 Jan 2012 at 8:07

Attachments:

• ReadReceiverPinChangeIntV1_3.ino

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Yes, you can attach as many userFunc's to as many pins as you have pins and 
memory space.  So your basic idea is sound.

I do have a couple of comments:

- To figure out the PORT programmatically and check the pin state, you can do 
this:

volatile uint8_t 
*pin_InputPort=portInputRegister(digitalPinToPort(arduino_pin));
uint8_t pin_mask = digitalPinToBitMask(arduino_pin);

Then to check  the state:

if (*pin_InputPort & pin_mask) {
  do stuff;
}

Also, if you are using only 2 pins then External Interrupts (using 
attachInterrupt()) are faster.  You use attachInterrup() in much the same way 
as PCintPort::attachInterrupt(), This may or may not be an issue for you; check 
out my speed tests for more information.

Original comment by m...@schwager.com on 6 Jan 2012 at 3:42

[GoogleCodeExporter]

Got it, thanks for those comments.  Thinking out loud, I'm wondering if your 
library could return the state of the pin that fired the interrupt?   You are 
doing this state check already and redoing it at the sketch level seems a bit 
redundant.

Agreed on using the external interrupts if one was only reading two channels.  
This was just an initial test.  I will be reading 5 channels from my receiver 
so I can send both pilot commands and gain updates over the RC link.  Your 
library makes this possible!  

Original comment by steve.ha...@gmail.com on 6 Jan 2012 at 6:11

[GoogleCodeExporter]

Returning the state of the pin would be difficult.  I think I would have to do 
this:

if (     p->mode == CHANGE) 
  PCintPort::mode=CHANGE;
  PCintPort::arduinoPin=p->arduinoPin;
  p->PCintFunc();
else if ((p->mode == RISING)  &&  (curr & p->mask))
  PCintPort::mode=RISING;
  PCintPort::arduinoPin=p->arduinoPin;
  p->PCintFunc();
else if ((p->mode == FALLING) && !(curr & p->mask)) )
  PCintPort::mode=FALLING;
  PCintPort::arduinoPin=p->arduinoPin;
  p->PCintFunc();

I have to duplicate the settings within each if  or else if section because I 
don't want to slow things down by checking state more than once.

I may try this out to see what the effect is on code size and speed.  ...No 
promises, though, as I'm quite busy these days.

Original comment by m...@schwager.com on 2 Feb 2012 at 5:27

[GoogleCodeExporter]

Ooops.  I'm silly.  I don't have to do all that complicated stuff; it's really 
quite simple.  I'll look into it.

Original comment by m...@schwager.com on 3 Feb 2012 at 12:10

[GoogleCodeExporter]

Cool!   I was hoping so.   I'm doing this pin state check in the sketch,
but it dirties up the sketch with hardware dependent port bit reads.

Original comment by steve.ha...@gmail.com on 3 Feb 2012 at 12:22

[GoogleCodeExporter]

Version 1.5 is ready.  I'm posting it now.

Original comment by m...@schwager.com on 4 Feb 2012 at 3:06

[GoogleCodeExporter]

Thanks very much!   I should be able to try it tomorrow.

Original comment by steve.ha...@gmail.com on 4 Feb 2012 at 4:05

[GoogleCodeExporter]

I just completed my tests but the pinState variable is not changing for 3 of 
the 4 receiver channels I am monitoring.  I have a working sketch that reads 
the pin states directly via a port register read within the userFuncs.  This 
method still works with PinChangeInt v1.5.  

To test the pinState feature, I first changed my sketch to use thepinState 
variable instead of port register reads.  In that case, there was no pulses 
getting measured.  I then took a step back and decided to Serial.print the 
pinState var.  

In the background, I am using the servo library to send pulses out to 4 servos. 
 Maybe all of this interrupt activity is causing issues?  This could be the 
case, but it doesn't explain why bit reads work and pinState checks don't.

Attached is my sketch.  It correctly reads 4 receiver channels and commands 4 
servos to mimic what is received.  I have hardware tested two of the servo 
outputs.  Within my userFunc's, I am also printing the pinState variable.  I 
have confirmed that my sketch and hardware work for the pins that are showing 
no pinState variable changes.

Hopefully I've just made a simple mistake?

Here is an example userFunc:
//userFunc for pin 1
void handlePin1()  
{  
  Serial.print("1: ");
  Serial.println(PCintPort::pinState, BIN);
  if(pin1state)  
    pulseStart1=micros(); // we got a positive edge  
  else  
    pulseTime1=micros()-pulseStart1; // Negative edge: get pulsewidth  
}  

Here is the serial output 
[PinNumber]: [state]

1: 1
1: 1
3: 1
2: 1
3: 1
2: 1
4: 1
4: 0
1: 1
1: 1
3: 1
2: 1
3: 1
2: 1
4: 1
4: 0
1: 1
1: 1
3: 1
2: 1
3: 1
2: 1
4: 1
4: 0

Original comment by steve.ha...@gmail.com on 4 Feb 2012 at 10:03

Attachments:

• ReadReceiver4CH.ino

[GoogleCodeExporter]

FYI: my testing was with Arduino 1.0 and a nano. 

Original comment by steve.ha...@gmail.com on 4 Feb 2012 at 11:04

[GoogleCodeExporter]

I think I have a bug.  On the line that says
    PCintPort::pinState=curr & changedPins ? HIGH : LOW; // version 1.5
change it to
    PCintPort::pinState=curr & p->mask ? HIGH : LOW; // version 1.51
This is line 392 of the PinChangeInt.h file.

I still don't quite understand how your sketch broke- unless two or more pins 
changed at almost the same time.  Then the "curr" variable, which gets set from 
the Port Input Register, would perhaps reflect a pin that changed from 0 to 1 
and another that changed from 1 to 0.  curr & changedPins is true for all 
changed pins.  The second pin, which is now 0, would still return a 1 to 
pinState because of my flaw.

If we want to look at a particular pin on the Port Input Register, we have to 
AND it with its mask, not with the entire body of changed pins.  That was my 
bug.

Please change that line as mentioned and let me know how it works.

Original comment by m...@schwager.com on 5 Feb 2012 at 4:51

[GoogleCodeExporter]

...How fast are your interrupts coming in, would you say?  I noticed that if I 
did this:

  Serial.print("1: ");
  Serial.print(PCintPort::pinState, BIN);
  Serial.print(" ");
  Serial.println(pin1state, BIN);

then my pinState and the pin1state may- every once in a while, say 5% of the 
time- not agree.  But I am using a pushbutton with a good deal of bounce, so I 
think that it's possible.  Because the Serial.print may take milliseconds to 
complete, and this is enough time for a different reading to happen between the 
interrupt and the sampling of the state of the port input register.

Original comment by m...@schwager.com on 5 Feb 2012 at 5:14

[GoogleCodeExporter]

Thanks!  I was reading that line in pinchangeint.h as well and wondering if
it was correct.  My pulses are coming in at about 1.5 msec width.  The
pulses should be serialized across pins, but will be close where one pulse
ends and the next pulse on the next pin begins.

Yes, the serial command is slow.  I hated putting that in the ISR.
However, even before that, I had no luck with the pinState reading.

I will check that line change tomorrow (?) and post back.

Original comment by steve.ha...@gmail.com on 5 Feb 2012 at 5:32

[GoogleCodeExporter]

On line 363, I have put debug code in like this:

        // DEBUG
        // Serial.print("C:"); Serial.println(curr,BIN);        // reading from the port's input register
        // Serial.print("H:"); Serial.println(changedPins,BIN); // the pin(s) that have changed
        lastPinView = curr;

...Just in front of that "lastPinView=curr;" line.

If you still have problems, run your code with this section- it may help me 
debug it.

The Serial commands should not affect the determination of the changed pins and 
such, as those things are determined first thing in the interrupt handler.

Original comment by m...@schwager.com on 5 Feb 2012 at 4:50

[GoogleCodeExporter]

Your recommendation for line 392 fixed it!  It now has this code:

PCintPort::pinState=curr & p->mask ? HIGH : LOW; // version 1.51

Upon updating this line and recompiling, I was able to see the proper pinState 
changes for each channel.

I performed a final test of this change with the attached sketch and verified 
that all four channels read and command a servo correctly.  It also seems that 
the servos are less jittery now than my previous version that relied on 
PinChangeInt with port bit reads in the userFuncs.  Maybe this is due to the 
reduced overhead within the userFunc ISRs?  Either way, thanks a bunch.  It 
sure did clean up the sketch a lot.  :-)

At this point, since it's working, I'm going to assume the debug code you 
posted above in #18 is not necessary.

Original comment by steve.ha...@gmail.com on 6 Feb 2012 at 1:53

Attachments:

• ReadReceiver4CHpinState.ino

[GoogleCodeExporter]

Awesome!  It makes sense that this would be less jittery.  There could 
theoretically be a lot of time (relatively) between the interrupt and your test 
of the pin status, after the interrupt.  Because it will go through the entire 
interrupt routine, from the PCintPort::PCint() call to the user's attached 
function.  But now, the pin state is determined very close to the time at which 
the interrupt takes place.

I'm glad it's working (whew!).  And I'm glad I took on this task- it is 
necessary to check the pin state at the beginning of the interrupt, for anyone 
who wants to know what their pin state was at the interrupt.  And still it's 
not perfect, because after the interrupt takes place, it takes 4 clock cycles 
to enter the Interrupt SubRoutine (ISR).  The compiler generates a whole pile 
of push instructions, like this:

ISR(PCINT2_vect) {
     31e:       1f 92           push    r1
     320:       0f 92           push    r0
     322:       0f b6           in      r0, 0x3f        ; 63
     324:       0f 92           push    r0
     326:       11 24           eor     r1, r1
     328:       df 92           push    r13
     32a:       ef 92           push    r14
     32c:       ff 92           push    r15

Not to mention the cycle(s) necessary to read the register.  I count 22 
assembler push instructions alone, @ 2 cycles apiece.  Plus various and sundry 
other instructions... so if we have, let's say 32 instructions (to be Computer 
Geekly about it) @ 2 cycles per instruction average after the interrupt, just 
for compiler housekeeping, that means it takes 4 microseconds (64 * 62.5 
nanoseconds @ 16 MHz clock rate) just to begin the ISR proper.

The state the library measures is some X microseconds after the interrupt 
actually takes place, where 1 < X < 10 microseconds, more or less.

Original comment by m...@schwager.com on 6 Feb 2012 at 5:27

[GoogleCodeExporter]

BTW, don't forget to put

#define NO_PORTB_PINCHANGES // to indicate that port b will not be used for pin 
change interrupts
#define NO_PORTC_PINCHANGES // to indicate that port c will not be used for pin 
change interrupts

ahead of your #include PinChangeInt.h.  I estimate you will save about 10% of 
your interrupt time (about 3 microseconds), per interrupt.

Original comment by m...@schwager.com on 6 Feb 2012 at 1:34

[GoogleCodeExporter]

Got it.  Thanks for the reminder. I've got that on my task list.  I will
report back if it reduces the jitter even more.   It's good to know that it
has that large of an impact.

Original comment by steve.ha...@gmail.com on 6 Feb 2012 at 2:52

[GoogleCodeExporter]

Gentlemen.... I added another issue and fix for 1.4 and sent it off attached to 
my other posting.  Either it is in your spam or you do not need it. Take a 
look; the fix is generic and should help should you want to interrupt a pin on 
a different port.

Original comment by Kestr...@att.net on 9 Feb 2012 at 1:13

[GoogleCodeExporter]

There is indeed a bug in my code.  Thanks to Ron for finding it.  The 
attachInterrupt switch statements are missing the "break" statements.  I 
believe they should look like this:

    switch (portNum) {
#ifndef NO_PORTB_PINCHANGES
    case 2:
        port=&portB;
        break;
#endif
#ifndef NO_PORTC_PINCHANGES
    case 3:
        port=&portC;
        break;
#endif
#ifndef NO_PORTD_PINCHANGES
    case 4:
        port=&portD;
        break;
#endif
    }

...I continue to investigate.

Original comment by m...@schwager.com on 13 Feb 2012 at 6:00

[GoogleCodeExporter]

I am going to close this one out as Done.  This is an interesting discussion 
but the actual bugs in versions prior to 1.6beta are covered in another issue.

Thanks again to Kestrel6 for finding a bug in the code and to Steve for his 
enhancement request.

Original comment by m...@schwager.com on 25 Feb 2012 at 7:09

• Changed state: Done

[GoogleCodeExporter]

Agreed.  Thanks for the great work!

Original comment by steve.ha...@gmail.com on 25 Feb 2012 at 1:18