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
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.
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
}
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
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
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
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 
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
Great work Rob and greynomad
Much as I'd love to take the credit you probably meant GreyGnome
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
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