Recommended strategy for using of PinChangeInt to read RC receiver pulses about arduino-pinchangeint HOT 26 CLOSED

[deleted comment]

[deleted comment]

[deleted comment]

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.

[deleted comment]

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.

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!  

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.

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

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.

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

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

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

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

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.

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

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.

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.

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.

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.

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.

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.

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.

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.

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.

Agreed.  Thanks for the great work!