Faster isr

[xxxajk]

Before:

Sketch uses 3,606 bytes (11%) of program storage space. Maximum is 32,256 bytes.

   text    data     bss     dec     hex filename
   3496     110    1169    4775    12a7 /tmp/build3790807526441479191.tmp/spi_speed_test.cpp.elf

After:

Sketch uses 3,606 bytes (11%) of program storage space. Maximum is 32,256 bytes.

   text    data     bss     dec     hex filename
   3496     110    1169    4775    12a7 /tmp/build3790807526441479191.tmp/spi_speed_test.cpp.elf

The checking if the target ISR is set is removed
and traded in for setting the initial array values and an empty function.
This gives you a speed gain when an ISR comes in, without increasing the size of the binary.

[matthijskooijman]

You probably wanted to submit this against the ide-1.5.x b ranch, nog master?

[xxxajk]

yes, but the file is the same in both branches.

On Mon, Nov 3, 2014 at 9:07 AM, Matthijs Kooijman [email protected]
wrote:

You probably wanted to submit this against the ide-1.5.x b ranch, nog
master?

—
Reply to this email directly or view it on GitHub
#2408 (comment).

Visit my github for awesome Arduino code @ https://github.com/xxxajk

[xxxajk]

in short, it is a fix for BOTH 1.0 and 1.5

[cmaglie]

Hi @xxxajk

I can now see your commit (but only because @matthijskooijman has commented after it).
In general when opening a pull request, please, keep an eye on the branch you're currently trying to merge on, otherwise it's very difficult to spot your fix between 250+ unrelated commits and it's really a pity to get it lost under a lot of "noise"!

BTW the fix is here:
xxxajk@5d03b57

and is very good IMHO, I'm for merging it.

[NicoHood]

Looking good. You could change the "nothings" to different functions, make them weak and an aliasof "nothing". Then the user could overwrite this function directly. Not sure if it makes sense.

example: https://github.com/NicoHood/PinChangeInterrupt/blob/dev/PinChangeInterrupt.cpp#L34

[xxxajk]

weak aliasing would be very awesome. I constantly run into a brick wall when wanting to use a custom ISR v.s. what is provided.

[xxxajk]

Also, FWIW I think all the arduino-provided ISRs should be weak aliases, not just pin interrupts. This would allow someone to do advanced programming, especially important when writing a shield library.

[NicoHood]

Not sure if its possible to implement an ISR itself weak. But if it is, why not do it? There is nothing to lose i think. That is a great idea! (thanks, i will add this to my PCINT library as well if possible).

[matthijskooijman]

Looking good. You could change the "nothings" to different functions, make them weak and an aliasof "nothing". Then the user could overwrite this function directly. Not sure if it makes sense.

Not sure what the point of this would be. The "nothing" functions are only set in the array while the corresponding interrupt is disabled, so it would never be called. Also, if you want your custom function to be called when an interrupt happens, you should just use attachInterrupt, that's what its for?

Declaring ISRs as weak is probably a good idea, but unrelated to this PR. If you want to see this happen, please prepare a pullrequest and test if this works as expected (or is there already a PR for this?)

[NicoHood]

Nevermind what I wrote last time.

The PR looks good to me, it will just not work for every MCU. (like the U2 Series which has tons of normal pin interrupts)
Maybe we can make the array with the "nothing"s inside more flexible with constructs like this?
http://stackoverflow.com/questions/16605043/call-a-c-preprocessor-macro-multiple-times-through-a-variable

[Chris--A]

@NicoHood, there is a way to make the array flexible without resorting to macros. Simply let the array elements initialize themselves regardless of how many are present, as opposed to generating code to explicitly initialize each element.

These changes slot into the changes proposed by @xxxajk

This requires changing Winterrupts.c to WInterrupts.cpp

static void nothing(void) {}

struct cbHandler{
  cbHandler() : ptr(nothing) {}
  void operator =( const voidFuncPtr in ) volatile { ptr = in; }
  void operator ()() volatile { ptr(); }
  voidFuncPtr ptr;
};

static volatile cbHandler intFunc[EXTERNAL_NUM_INTERRUPTS] = {}; //Use default initialization.

Here is a comparison: xxxajk/Arduino-1@faster-isr...Chris--A:patch-1

[NicoHood]

That looks complicated but sounds good if it works. Changing to cpp should be tested of course.

[matthijskooijman]

@Chris--A, I think that if you add a constexpr declaration on the cbHandler constructor, the compiler can even optimize away the constructor calls and just put the initialized memory in the .data section (instead of still running all constructors during startup, which I think will happen without constexpr). Did you look at the resulting code size and assembler output?

[matthijskooijman]

(Note that constexpr needs C++11 support, which currently needs an hourly build)

[Chris--A]

@NicoHood
The header file is wrapped with safeguards so everything should still work fine (C code can still use the functions).

#ifdef __cplusplus
extern "C"{
#endif

//...

#ifdef __cplusplus
} // extern "C"
#endif

@matthijskooijman
I'm having a look at its performance and size now. constexpr will not do anything here, it simply would allow using an instance of the object in a constant expression (C++98 would generate an error like: xx cannot be used in a constant expression). The code is inline by default.

There is something different happening which I'll try investigate, however I've also noticed only the function pointer in the cbHandler object needs to be volatile, not the entire array.

I've also noticed that @xxxajk's original code does not currently work with an Uno. EXTERNAL_NUM_INTERRUPTS is set to two, and the array specifically initializes at least three.

[matthijskooijman]

@Chris--A, I actually believe constexpr is needed to allow compile-time initialization of the array. I previousrly wrote a piece on this thing that I think might be appropriate here: http://www.stderr.nl/Blog/Software/Cpp/ConstantInitialization.html

I wonder if another approach might be easier and more readable here, though:

static volatile voidFuncPtr intFunc[EXTERNAL_NUM_INTERRUPTS] = {
    #if EXTERNAL_NUM_INTERRUPTS > 0
    nothing,
    #endif
    #if EXTERNAL_NUM_INTERRUPTS > 1
    nothing,
    #endif
    #if EXTERNAL_NUM_INTERRUPTS > 2
    nothing,
    #endif

    etc..
}

It's easier to read than fiddling with a struct, but won't work with any
number of interrupts.

One midway might be a simpler struct:

struct cbHandler{
  cbHandler() : ptr(nothing) {}
  voidFuncPtr ptr;
};

And then just access this as intFunc[0].ptr = foo. Not sure if this
helps much, though.

I just noticed that all this code is in a .c file, meaning we could use
"designated initializers" (which I believe is a gcc extension not
supported in C++):

static volatile voidFuncPtr intFunc[EXTERNAL_NUM_INTERRUPTS] = {
    [ 0 ... (EXTERNAL_NUM_INTERRUPTS - 1) ] = nothing
}

(untested)

[NicoHood]

@matthijskooijman The first solution is so simple that I never ever would have thought of this :D
I like it, its less complicated and for the thing we use it, its enough. I'd add up to 8 definitions, if its more an error. That should work and we can merge the PR without much problems soon.

[Chris--A]

@matthijskooijman

Unfortunately constexpr does not always help with moving the initialization into raw loads in the .data section. It is still emitted as runtime code using __do_global_ctors as the usage of the object is not constant, if the array was also constexpr then the class would be initialized during compile time. A constexpr constructor does not guarantee that the compiler will do the desired optimizations, the code using it will dictate whether to force it or give it the option. In a compile time constant expression, the run-time data should be redundant and removed altogether, as in no RAM is initialized for an object at startup, its just hard values accessed inline directly from flash.

The usage code outputs the same though.

From your article:

class Foo {
public:
  constexpr Foo(int x) { }
  Foo(long x) { }
};

Foo a = Foo(1);
Foo b = Foo(1L);

We define a Foo class, which has two constructors: one accepts an int and is constexpr and one accepts a long and is not constexpr. Above, this means that a will be const-initialized, while b is not.

This is not quite correct, as only the temporary anonymous Foo(1) is a constexpr. a is not a constexpr and can still produce code if the compiler thinks its too complicated/can't guarantee constness. You can see this in play with an extraction of my proposal:

typedef void (*voidFuncPtr)(void);

void nothing(){}

class Foo {
public:
  constexpr Foo() : ptr(nothing) { }
  volatile voidFuncPtr ptr;
};

Foo a[2];

void setup() {
  GPIOR0 = (uint16_t) a[0].ptr;
}

void loop() {}

Without constexpr on the array Foo a[2], run-time initialization is performed. When added, you can see the code size drop and the values are inlined as would be preferred. When used as a template parameter (or other constant expression, like the static_assert's you used for testing), the code must be static initialized and cannot be initialized at run-time (forced, otherwise compile fail). However as the data needs to be writable, it cannot be a constexpr variable.

---

The struct was designed to not require 'fiddling' with the code. However the designated initializer approach you used is nice and simple and the file can stay a .c file. As a C++ programmer I have not used these, so thanks for the new insight, and yeah, its not supported. From what I've read and assumed, constructors are C++'s way of dealing with abstracted initialization.

[cmaglie]

I'm looking at this one.

What is the maximum for NUM_EXTERNAL_INTERRUPT? It looks like 8 is the max for the Arduino boards, but maybe some 3rd party boards may define more external int pins?

static volatile voidFuncPtr intFunc[EXTERNAL_NUM_INTERRUPTS] = {
#if EXTERNAL_NUM_INTERRUPTS > 8
    #warning There are more than 8 external interrupts. Some callbacks may not be initialized.
#endif
#if EXTERNAL_NUM_INTERRUPTS > 7
    nothing,
#endif
#if EXTERNAL_NUM_INTERRUPTS > 6
    nothing,
#endif
#if EXTERNAL_NUM_INTERRUPTS > 5
    nothing,
#endif
#if EXTERNAL_NUM_INTERRUPTS > 4
    nothing,
#endif
#if EXTERNAL_NUM_INTERRUPTS > 3
    nothing,
#endif
    nothing,
    nothing,
    nothing
};

As said by @matthijskooijman even if the array is not initialized it should not be a problem when the attachInterrupt function is used properly, so I just opted for a warning instead of an error.

[NicoHood]

The Uno only has 2 INTs. So your paste is wrong for the uno and I also would add the possibility for a single interrupt as well (maybe some attinys only have a single INT).

The 16u2 has 8 Interrupts:
https://github.com/NicoHood/HoodLoader2/wiki/Arduino-Uno-Mega-16u2-Pinout

Also some boards might not have any interrupts (maybe some attinys) where the array would just be empty. It doesnt hurt to add a definition here, so in any case this doesnt cause a problem.

Where is EXTERNAL_NUM_INTERRUPTS defined?

After your warning you can still add a single nothing because you at least have one more, so 9 is still supported. A warning is okay I think.

static volatile voidFuncPtr intFunc[EXTERNAL_NUM_INTERRUPTS] = {
#if EXTERNAL_NUM_INTERRUPTS > 8
    #warning There are more than 8 external interrupts. Some callbacks may not be initialized.
    nothing,
#endif
#if EXTERNAL_NUM_INTERRUPTS > 7
    nothing,
#endif
#if EXTERNAL_NUM_INTERRUPTS > 6
    nothing,
#endif
#if EXTERNAL_NUM_INTERRUPTS > 5
    nothing,
#endif
#if EXTERNAL_NUM_INTERRUPTS > 4
    nothing,
#endif
#if EXTERNAL_NUM_INTERRUPTS > 3
    nothing,
#endif
#if EXTERNAL_NUM_INTERRUPTS > 2
    nothing,
#endif
#if EXTERNAL_NUM_INTERRUPTS > 1
    nothing,
#endif
#if EXTERNAL_NUM_INTERRUPTS > 0
    nothing,
#endif
};

[cmaglie]

Right, it's defined here: https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/wiring_private.h#L55
so the maximum is really 8.

@NicoHood
I thought it was defined in the variant files, but I was wrong.
I guess I can go with your last paste, if there are no other concerns.

[NicoHood]

Oh. Maybe we should move this to the variants file with this patch?

I think I also missed this in the 16u2 USB-Core fix. Can you please add the 16u2 definitions here as well?
https://github.com/NicoHood/HID/blob/master/avr/cores/hid/wiring_private.h#L75-L77

As said, the variants file seems to be a better place. And for compatibility you can add a
#ifndef maybe? Not sure if this works, because the variants file is included at the end of the Arduino.h file.

[cmaglie]

The number of external interrupts is a CPU-bounded value so, thinking again at this, it seems more obvious to keep things as they are.

Can you please add the 16u2 definitions here as well?

Ok

[NicoHood]

Yeah that sounds okay to me then. Will you open a new PR or merge it directly? Please cc us the commits.

Note:
I do not know if the other interrupt settings are working for the u2 series. I think I looked at this half a year ago and it looked good. The 16u2 on the uno board only has 7 broken out pins of which none is an interrupt. And since nearly noone will use the u2 standalone I think its okay to live with that if it wont work. But I remember that I checked that, so it should be okay.

[xxxajk]

Even if the pin is not broken out, you can still use the pin for a
soft-ware IRQ.
Try it...
Set the pin for output, and toggle; you'll get an IRQ.

On Mon, Aug 31, 2015 at 9:56 AM, Nico [email protected] wrote:

Yeah that sounds okay to me then. Will you open a new PR or merge it
directly? Please cc us the commits.

Note:
I do not know if the other interrupt settings are working for the u2
series. I think I looked at this half a year ago and it looked good. The
16u2 on the uno board only has 7 broken out pins of which none is an
interrupt. And since nearly noone will use the u2 standalone I think its
okay to live with that if it wont work. But I remember that I checked that,
so it should be okay.

—
Reply to this email directly or view it on GitHub
#2408 (comment).

Visit my github for awesome Arduino code @ https://github.com/xxxajk

[NicoHood]

Why didnt you add

#if EXTERNAL_NUM_INTERRUPTS > 0
    nothing,
#endif

This can now cause problems if the MCU doesnt have an external interrupt. Attiny 13 seems to have no pin interrupts.

@xxxajk Sounds like a good idea, thanks. I'll test it.

[cmaglie]

Oops... cut & paste errors sorry. Fixed here: 8720384

[NicoHood]

I think you can add an error if the number is >8.
It is not implemented in the IDE an if there is an MCU that really has more than 8 interrupts you should see the error as reminder that you also need to fix this array. This wont happen anyways but if it does you will get a notice. Thats better I think.

https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/WInterrupts.c#L70
https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/wiring_private.h#L55-L64

Another thing I want to discuss here (before opening another ticket):

What about a function disable/enableInterrupt? Sometimes you want to turn off an interrupt, similar as detaching. But you do not want to save the MODE and the userfunction all the time, so you need a function without this parameter. This also speeds up the deactivation a bit.

Simply the functions would not set the userfunction again/remove it and uses the old mode. I am not sure if the mode is still saved after you disable/enable the interrupt.

Would this be an idea? I need this feature for my IRLremote to temporary turn it off, cause the interrupts ma cause errors on some code parts.

[matthijskooijman]

@NicoHood See #2159 for enable/disableInterrupt, best continue discussing about this there.