Pages: 1 [2] 3 4   Go Down
Author Topic: PinChangeInt library- To attach interrupts to multiple Arduino (Uno/Due) pins  (Read 17436 times)
0 Members and 1 Guest are viewing this topic.
Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 211
Posts: 13479
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset


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,

- addPin would be slightly complexer to keep the list sorted.     - http://stackoverflow.com/questions/4825030/c-add-to-linked-list-in-sorted-order -
- addPin could detect that the pin already has in ISR() attached.
- delPin does not need to change as it already can delete from the middle of the list

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
Logged

Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

Belgium
Offline Offline
Edison Member
*
Karma: 68
Posts: 1897
Arduino rocks; but with my plugin it can fly rocking the world ;-)
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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
Logged

Do not PM me a question unless you are prepared to pay for consultancy.
Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -

Belgium
Offline Offline
Edison Member
*
Karma: 68
Posts: 1897
Arduino rocks; but with my plugin it can fly rocking the world ;-)
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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.
Code:
PCintPort::attachInterrupt(14, interruptroutine,CHANGE);
PCintPort::attachInterrupt(15, interruptroutine,CHANGE);
Makes pin 15 work
but
Code:
PCintPort::attachInterrupt(15, interruptroutine,CHANGE);
Does not.
Best regards
Jantje
Logged

Do not PM me a question unless you are prepared to pay for consultancy.
Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -

0
Offline Offline
Jr. Member
**
Karma: 4
Posts: 87
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

0
Offline Offline
Jr. Member
**
Karma: 4
Posts: 87
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset


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,

- addPin would be slightly complexer to keep the list sorted.     - http://stackoverflow.com/questions/4825030/c-add-to-linked-list-in-sorted-order -
- addPin could detect that the pin already has in ISR() attached.
- delPin does not need to change as it already can delete from the middle of the list

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

Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 211
Posts: 13479
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

Dubai, UAE
Offline Offline
Edison Member
*
Karma: 22
Posts: 1675
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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
Logged


Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 211
Posts: 13479
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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
Code:
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
« Last Edit: November 04, 2012, 03:20:09 pm by robtillaart » Logged

Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 211
Posts: 13479
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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?
Code:
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
}
Logged

Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

Dubai, UAE
Offline Offline
Edison Member
*
Karma: 22
Posts: 1675
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Duane B

rcarduino.blogspot.com

Logged


Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 211
Posts: 13479
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

If you have the new numbers can you post them?
Logged

Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

Dubai, UAE
Offline Offline
Edison Member
*
Karma: 22
Posts: 1675
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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
Logged


0
Offline Offline
Jr. Member
**
Karma: 4
Posts: 87
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Code:
    // 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:
Code:
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!
Logged

Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 211
Posts: 13479
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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)
Logged

Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

Dubai, UAE
Offline Offline
Edison Member
*
Karma: 22
Posts: 1675
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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 -

Code:
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 -

http://rcarduino.blogspot.com/2012/09/how-to-view-arduino-assembly.html

Duane B
Logged


Pages: 1 [2] 3 4   Go Up
Jump to: