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

from the 2.01 beta?

void PCintPort::addPin(uint8_t arduinoPin, uint8_t mode,PCIntvoidFuncPtr userFunc)
{
	// Create pin p:  fill in the data.  This is no longer in another method (saves some bytes).
	PCintPin* p=new PCintPin;
	p->arduinoPin=arduinoPin;
	p->mode = mode;
	p->next=NULL;
	p->mask = digitalPinToBitMask(arduinoPin); // the mask

#ifdef DEBUG
	Serial.print("createPin. pin given: "); Serial.print(arduinoPin, DEC);
	int addr = (int) p;
	Serial.print(" instance addr: "); Serial.println(addr, HEX);
	Serial.print("userFunc addr: "); Serial.println((int)p->PCintFunc, HEX);
#endif

	// pin created

	if (p == NULL) return;

The test for p == NULL should be directly after PCintPin* p=new PCintPin;
as one cannot access the members of p when p == NULL

void PCintPort::detachInterrupt(uint8_t arduinoPin)
{
	PCintPort *port;
#ifdef DEBUG
	Serial.print("detachInterrupt: "); Serial.println(arduinoPin, DEC);
#endif
	uint8_t portNum = digitalPinToPort(arduinoPin);
	if (portNum == NOT_A_PORT) return;
	port=lookupPortNumToPort(portNum);
        if (port == NULL)                                             // <<<<<<<<<<<<<<<<<<<<<<< handle error? from code point it is possible.
	port->delPin(digitalPinToBitMask(arduinoPin));
}

You use a linked list for the PCintPin objects.
you could keep this list in sorted order to speed up the average search time of void PCintPort::PCint()
On average the nr of search steps would be half of the current implementation,

it might be more robust to add a return value for addPin and delPin that indicates the #pins added /delete. addPin might return -1 if pin exist.
These values can be propagated to attachInterrupt/detachIRQ.

sofar my 2 cents
Rob

Hi
I tested the library with a mega. I can confirm following ports to work
15,A8,A9,A10,A11,A12,A13,A14,A15
I did not succeed in getting port 14 to work.
Thanks for the great work.
Best regards
Jantje

Jantje:
Hi
I tested the library with a mega. I can confirm following ports to work
15,A8,A9,A10,A11,A12,A13,A14,A15
I did not succeed in getting port 14 to work.
Thanks for the great work.
Best regards
Jantje

playing a bit more I came to the conclusion you need to use 14 and 15 to get 15 to work.

PCintPort::attachInterrupt(14, interruptroutine,CHANGE);
PCintPort::attachInterrupt(15, interruptroutine,CHANGE);

Makes pin 15 work
but

PCintPort::attachInterrupt(15, interruptroutine,CHANGE);

Does not.
Best regards
Jantje

The test for p == NULL should be directly after PCintPin* p=new PCintPin;
as one cannot access the members of p when p == NULL

Fixed. Nice catch, thanks!
-GreyGnome

robtillaart:
You use a linked list for the PCintPin objects.
you could keep this list in sorted order to speed up the average search time of void PCintPort::PCint()
On average the nr of search steps would be half of the current implementation,

it might be more robust to add a return value for addPin and delPin that indicates the #pins added /delete. addPin might return -1 if pin exist.
These values can be propagated to attachInterrupt/detachIRQ.

Thanks for the feedback, Rob.

I had considered the sorted linked list, but then I realized that sorting on pin number may not be what the user wants. It only speeds up the average search time if you can guarantee an even distribution of interrupts. I would rather let the programmer decide in which order he or she decides to add pins.

Having addPin() and attachInterrupt() return values seems pretty easy and somewhat smart, so I will look into doing that.

I would rather let the programmer decide in which order he or she decides to add pins.

You have a point, and it should be made clear in the documentation that adding the most interrupted pin as first gives a better overall performance as it is found fastest.

On the subject of letting the programmer take responsibilty it would be nice to add an option to skip all of the rising/falling tests inside the inner loop of the ISR, it adds a few cycles in the most repeated part of the ISR code.

Duane B

rcarduino.blogspot.com

reviewed again,

#define PCINT_VERSION 2020 // This number MUST agree with the version number, above.

In the Add function, this line
p->PCintFunc=userFunc;
must be moved upwards so that when adding the new PCint object to the list, the function is available. Now there is still a race condition possible, that p is added to the list but the function is not in place.

I think

			if (thisChangedPin) {
				if ((thisChangedPin & portRisingPins & PCintPort::curr ) ||
		        	(thisChangedPin & portFallingPins & ~PCintPort::curr )) {

can be optimized as [ portRisingPins & PCintPort::curr ] and [ portFallingPins & ~PCintPort::curr] are invariant in the loop

Rob

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

  • precalculated fastMask to simplify and speed up the mask testing in the inner while loop.
  • thisChangedPin optimized away

The var changedPins can be optimized away but probably the compiler will see that too.

Can you give it a try?

void PCintPort::PCint() 
{
  #ifdef FLASH
  if (*led_port & led_mask) *led_port&=not_led_mask;
  else *led_port|=led_mask;
  #endif
	
  #ifndef DISABLE_PCINT_MULTI_SERVICE
  uint8_t pcifr;
  do {
  #endif
    // get the pin states for the indicated port.
    uint8_t changedPins = PCintPort::curr ^ lastPinView;
    lastPinView = PCintPort::curr;
	
    uint8_t fastMask = changedPins & ((portRisingPins & PCintPort::curr ) | ( portFallingPins & ~PCintPort::curr ));  // bitwise OR is needed here
    // if fastmask eq zero there will be no irq in the linked list, optimization!
    if (fastMask != 0)
    {
      PCintPin* p = firstPin; 
      while (p) {
        if (p->mask & fastMask){
	  #ifndef NO_PIN_STATE
	  PCintPort::pinState=PCintPort::curr & p->mask ? HIGH : LOW;
	  #endif
	  #ifndef NO_PIN_NUMBER
	  PCintPort::arduinoPin=p->arduinoPin;
	  #endif
	  #ifdef PINMODE
	  PCintPort::pinmode=p->mode;
	  #endif
	  p->PCintFunc();
      }
      p = p->next;
      }  // while(p)
    }
    #ifndef DISABLE_PCINT_MULTI_SERVICE
    pcifr = PCIFR & PCICRbit;
    PCIFR = pcifr;	// clear the interrupt if we will process it (no effect if bit is zero)
  } while(pcifr);
  #endif
}

Excellent, thats going to save a lot of cycles, thanks

Duane B

rcarduino.blogspot.com

If you have the new numbers can you post them?

I actually commented these tests out in my pinchangeint.cpp a long time back - but its nice to see that you have been able to eliminate them while retaining the functionality.

I might find time tonight to dissassemble the before and after versions for a comparison

Duane B

rcarduino.blogspot.com

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