PinChangeInt library- To attach interrupts to multiple Arduino (Uno/Mega) pins

robtillaart:
Looked at the PCint() code and came up with this.

    // if fastmask eq zero there will be no irq in the linked list, optimization!

if (fastMask != 0)
    {
}

Thanks, Rob. That looks like a really good idea; I'm going to change it and run a speed test. I note that the if statement, above, is certainly (in my mind) not needed, because of the portPCMask. Unless the programmer is creating Pin Change Interrupts outside of this library, any pin that interrupts will by definition mean that fastmask is non-zero.

Also, I have simplified the boolean a little bit so rather than calling it "fastmask", I rather like to leave it at "changedpins" since that is what the bitmask represents. Hence we have:

uint8_t changedPins = PCintPort::curr ^ lastPinView & (portRisingPins & (PCintPort::curr | portFallingPins));

We'll see if these optimizations make the code any faster; the compiler is pretty good at getting rid of cruft I think; whether it was able to optimize the invariants, I think the speed test will tell.

Thanks for the optimizations!

Calling it changedPins is better, just as you say.

don't know if the boolean expression is optimized right,

(portRisingPins & (PCintPort::curr | portFallingPins)
equals ??
(portRisingPins & PCintPort::curr ) | ( portFallingPins & ~PCintPort::curr )
??

because of the ~ (negation) you may not apply de Morgans law? or did I miss something?.
Could you print both expressions to debug to check this? (no free arduino nearby)

This is the pinchangeint inner loop prior to the optimisations being discussed here, for those watching the discussion, this is the most important part to optimise because it is potentially executed many times on each interrupt -

A lot of the code is relating to the if statements -

			thisChangedPin=p->mask & changedPins;
     340:	6b 81       	ldd	r22, Y+3	; 0x03
     342:	8f 2d       	mov	r24, r15
     344:	86 23       	and	r24, r22
			if (thisChangedPin) {
     346:	71 f1       	breq	.+92     	; 0x3a4 <_ZN9PCintPort5PCintEv+0x86>
				if ((thisChangedPin & portRisingPins & PCintPort::curr ) ||
     348:	48 2f       	mov	r20, r24
     34a:	50 e0       	ldi	r21, 0x00	; 0
     34c:	f8 01       	movw	r30, r16
     34e:	85 81       	ldd	r24, Z+5	; 0x05
     350:	20 91 44 01 	lds	r18, 0x0144
     354:	90 e0       	ldi	r25, 0x00	; 0
     356:	84 23       	and	r24, r20
     358:	95 23       	and	r25, r21
     35a:	30 e0       	ldi	r19, 0x00	; 0
     35c:	82 23       	and	r24, r18
     35e:	93 23       	and	r25, r19
     360:	89 2b       	or	r24, r25
     362:	69 f4       	brne	.+26     	; 0x37e <_ZN9PCintPort5PCintEv+0x60>
     364:	86 81       	ldd	r24, Z+6	; 0x06
     366:	20 91 44 01 	lds	r18, 0x0144
     36a:	90 e0       	ldi	r25, 0x00	; 0
     36c:	48 23       	and	r20, r24
     36e:	59 23       	and	r21, r25
     370:	30 e0       	ldi	r19, 0x00	; 0
     372:	20 95       	com	r18
     374:	30 95       	com	r19
     376:	42 23       	and	r20, r18
     378:	53 23       	and	r21, r19
     37a:	45 2b       	or	r20, r21
     37c:	99 f0       	breq	.+38     	; 0x3a4 <_ZN9PCintPort5PCintEv+0x86>
		        	(thisChangedPin & portFallingPins & ~PCintPort::curr )) 
			{
					// Trigger interrupt if the bit is high and it's set to trigger on mode RISING or CHANGE
					// Trigger interrupt if the bit is low and it's set to trigger on mode FALLING or CHANGE
						#ifndef NO_PIN_STATE
						PCintPort::pinState=PCintPort::curr & p->mask ? HIGH : LOW;
     37e:	20 91 44 01 	lds	r18, 0x0144
     382:	40 e0       	ldi	r20, 0x00	; 0
     384:	86 2f       	mov	r24, r22
     386:	90 e0       	ldi	r25, 0x00	; 0
     388:	30 e0       	ldi	r19, 0x00	; 0
     38a:	82 23       	and	r24, r18
     38c:	93 23       	and	r25, r19
     38e:	89 2b       	or	r24, r25
     390:	09 f0       	breq	.+2      	; 0x394 <_ZN9PCintPort5PCintEv+0x76>
     392:	41 e0       	ldi	r20, 0x01	; 1
     394:	40 93 46 01 	sts	0x0146, r20
						#endif
						#ifndef NO_PIN_NUMBER
						PCintPort::arduinoPin=p->arduinoPin;
     398:	8c 81       	ldd	r24, Y+4	; 0x04
     39a:	80 93 45 01 	sts	0x0145, r24
						#endif
						#ifdef PINMODE
						PCintPort::pinmode=p->mode;
						#endif
						p->PCintFunc();
     39e:	e8 81       	ld	r30, Y
     3a0:	f9 81       	ldd	r31, Y+1	; 0x01
     3a2:	09 95       	icall
				}
			}
			p=p->next;
     3a4:	0d 80       	ldd	r0, Y+5	; 0x05
     3a6:	de 81       	ldd	r29, Y+6	; 0x06
     3a8:	c0 2d       	mov	r28, r0
		uint8_t changedPins = PCintPort::curr ^ lastPinView;
		//changedPins &= portPCMask;
		lastPinView = PCintPort::curr;

		PCintPin* p = firstPin; // 3427
		while (p) {
     3aa:	20 97       	sbiw	r28, 0x00	; 0
     3ac:	49 f6       	brne	.-110    	; 0x340 <_ZN9PCintPort5PCintEv+0x22>
				}
			}
			p=p->next;

Disassembled by following this procedure -

Duane B

Big change, from 108 instructions to 54 - cut the time in half !

     358:	e5 2e       	mov	r14, r21
     35a:	ff 24       	eor	r15, r15
     35c:	1c c0       	rjmp	.+56     	; 0x396 <_ZN9PCintPort5PCintEv+0x78>
     35e:	8b 81       	ldd	r24, Y+3	; 0x03
     360:	28 2f       	mov	r18, r24
     362:	30 e0       	ldi	r19, 0x00	; 0
     364:	c7 01       	movw	r24, r14
     366:	82 23       	and	r24, r18
     368:	93 23       	and	r25, r19
     36a:	89 2b       	or	r24, r25
     36c:	89 f0       	breq	.+34     	; 0x390 <_ZN9PCintPort5PCintEv+0x72>
	  #ifndef NO_PIN_STATE
	  PCintPort::pinState=PCintPort::curr & p->mask ? HIGH : LOW;
     36e:	80 91 44 01 	lds	r24, 0x0144
     372:	40 e0       	ldi	r20, 0x00	; 0
     374:	90 e0       	ldi	r25, 0x00	; 0
     376:	82 23       	and	r24, r18
     378:	93 23       	and	r25, r19
     37a:	89 2b       	or	r24, r25
     37c:	09 f0       	breq	.+2      	; 0x380 <_ZN9PCintPort5PCintEv+0x62>
     37e:	41 e0       	ldi	r20, 0x01	; 1
     380:	40 93 46 01 	sts	0x0146, r20
	  #endif
	  #ifndef NO_PIN_NUMBER
	  PCintPort::arduinoPin=p->arduinoPin;
     384:	8c 81       	ldd	r24, Y+4	; 0x04
     386:	80 93 45 01 	sts	0x0145, r24
	  #endif
	  #ifdef PINMODE
	  PCintPort::pinmode=p->mode;
	  #endif
	  p->PCintFunc();
     38a:	e8 81       	ld	r30, Y
     38c:	f9 81       	ldd	r31, Y+1	; 0x01
     38e:	09 95       	icall
      }
      p = p->next;

Great work Rob and greynomad

Duane B

rcarduino.blogspot.com

The update in action reading RC Signals -

The red line is before, the green line is after. They both represent the maximum variation between two consecutive RC Pulses in 10 seconds periods. There are three channels, when one channel goes high, the adjacent channel goes low meaning we will often need to service two interrupts in one call to the ISR, the improved efficiency has a visible effect in the graph.

Full explanation of the graph towards the end of this -

Duane B

rcarduino.blogspot.com

what are the average numbers of blue, red and green? (I do not fully understand the test As I did not dived into it yet :slight_smile:

Hi,
The test is very application specific and not a good way to measure pinchangeint performance, however it does illustrate that there is a improvement.

A quick look at the assembly suggests that the instructions have been reduced from 104 to 52 - a saving of 50%.

My application is reading RC Signals, when one pin toggles, the adjacent pin also toggles, this means that the pin change int library will be servicing two pins each time it is called so the saving is very significant in this particular application.

Duane B

rcarduino.blogspot.com

Great work Rob and greynomad

Much as I'd love to take the credit you probably meant GreyGnome :slight_smile:


Rob

Fixed, thanks

Duane B

They both represent the maximum variation between two consecutive RC Pulses in 10 seconds periods.

What is the unit of the Y-axis ?

X Axis is sample number where a sample is a period of 10 seconds.

Y Axis is the largest deviation between two consecutive RC Signals in milliseconds encountered in the 10 second sample period - in an ideal world it would all be 0.

The signals are being read using the modified PinChangeInt code.

As I say, its very application specific but between my new servo library and other optimizations (blue line to red line) and the modified PinChangeInt (red line to green line) the improvement is quite dramatic.

Duane B

rcarduino.blogspot.com

Duane
I guess this also explains the peeks I have we discussed easy to use Remote Control (RC) library - Libraries - Arduino Forum.
as I still use the blue line and the whole scope is about 1 minute and have 6 rc channels to read that comes close.
Or am I missing something?
Best regards
Jantje

Jante,
No idea what was causing the peaks, why don't you try the code here including my new servo library -

I could use a friendly tester.

I would suggest testing outputs instead of the measured signal - thats the hard part getting highly consistent output pulses - the new servo library helps with this.

As this is now off topic I will start a new topic on the library in a day or two when I have a chance to document it a little more, but Jante, you should be able to find your way around it and have a go at testing in the meantime - if you can find the time.

Duane B

@GreyGnome
did you have time to look at my remark in #28 regarding your optimization proposal of the boolean expression.

there is a diff in the formulas when rising = 0, falling = 1 and curr = 0

robtillaart:
@GreyGnome
did you have time to look at my remark in #28 regarding your optimization proposal of the boolean expression.

there is a diff in the formulas when rising = 0, falling = 1 and curr = 0

Yes- sorry. I have been a busy busy busy little beaver! Geez, what a week!

You're right- my optimization was messed up because I made a mistake in my truth table. I think your expression is about the best that we can do. Second- there are more dragons lying about than I had bargained for! I spent the better part of a week's free time wrestling with a big bug in my posted code; I said: PCintPort::curr ^ lastPinView & (portRisingPins... and I should have said: (PCintPort::curr ^ lastPinView) & (portRisingPins...

...Operator precedence. It's not just a good idea, it's the law! OMG what a debugging session I had.

The extra nice thing is, because of my debugging, I uncovered a significant flaw in my code! This is a big deal if you have fast interrupts, and it is real nasty when you have, for example, switches that bounce (don't they all?). The end of the PCint() function should look like this:

   #ifndef DISABLE_PCINT_MULTI_SERVICE
        pcifr = PCIFR & PCICRbit;
        PCIFR = pcifr;  // clear the interrupt if we will process it (no effect if bit is zero)
        PCintPort::curr=portInputReg; // BUG! ...Fixed in 2.11beta. Without this, we get really weird interrupt behavior. Sheesh.
                                      // This fix slows down the code, though. Even with only 1 pin interrupting.
    } while(pcifr);
    #endif

...don't worry about the speed slowdown mentioned in the comment. With your fix, robtillaart, my tests speed up by a couple of micros. Sadly, that PCintPort::curr=portInputReg will be run even if pcifr is 0. But it's either that or an if statement, in any event. If you don't need it, best to define DISABLE_PCINT_MULTI_SERVICE.

I hope to post version 2.11beta soon. Right now I think I've uncovered/fixed/optimised/edited/compiled everything necessary, but the code is in a bit of a shambles right now so give me a day or two to clean up. In the meantime, there is that damn bug that you can fix in v. 2.01beta by copying from my snippet above.

Hi,
There are two current forum topics where pinchangeint is being used to read incoming rc signals. In both topics, the ops would like to detach pin change interrupts and later reattach them to enter and exit autonomous and RC Modes. I have suggested that this is not the best approach due to the memory leak in detach. As an alternative, would you consider adding an enable function that will set or clear the relevant bit for given pin in the pin change port masks allowing the user to disable pin changes for one more pins and reenable them later without memory leaks.

Duane B

DuaneB:
Hi,
There are two current forum topics where pinchangeint is being used to read incoming rc signals. In both topics, the ops would like to detach pin change interrupts and later reattach them to enter and exit autonomous and RC Modes. I have suggested that this is not the best approach due to the memory leak in detach. As an alternative, would you consider adding an enable function that will set or clear the relevant bit for given pin in the pin change port masks allowing the user to disable pin changes for one more pins and reenable them later without memory leaks.

Duane B

Memory leaks! Yikes! Could you tell me more about that issue? On the face of it I don't mind such a "pause" function, but moreso I am not comfortable with such a bug. It should be fixed.

Hi,

I assume its a memory leak as my understanding is that delete does not free memory in Arduino and the current implementation of detach seems to assume that delete works.

Duane B

DuaneB:
I assume its a memory leak as my understanding is that delete does not free memory in Arduino and the current implementation of detach seems to assume that delete works.

Yes, I ran a test and noticed that my memory got gobbled up pretty quickly. Drat!!! ...The problem is not merely in delete(), because delete just calls free(). The problem is that free() doesn't seem to work.

Anyway, regardless of that I like your idea of having detachInterrupt() simply disable the pin from interrupting, rather than using the delete() operator. I am working on it doing just that.

attachInterrupt() will return -1 on error, 0 if the pin has already been added to the list (the pin will be enabled if it is called and had previously been created already), and 1 if a new pin is created.

Perfect, I will post a link to this topic from the two related threads so that the ops are aware of this and can follow the progress.

Thanks

Duane B