attachInterrupt implementation remarks

[matthijskooijman]

In this repo, some additions to attachInterrupt have been made to allow passing an extra argument. See https://github.com/arduino/ArduinoCore-API/blob/master/api/Interrupts.h#L19-L34

I have a few remarks/questions about his:

• I do not really like the use of a container in dynamic memory here and wonder if that is really needed. On second reading, I see that passing a pointer to attachInterrupt already skips the wrapper lambda and dynamic memory, it is just other types that need it. Technically, the wrapper lambda and container only serve to convert a pointer to a reference argument, which I suspect is actually a no-op on most architectures (but that's probably hard to guarantee...).
• The implementation currently seems to not free the container on detachInterrupt, which leads to memory leaks. I can't quickly think of an easy way to fix this either, without modifying the underlying interrupt handler table to somehow mark these interrupts as dynamic.
• The argument is passed by reference and then a pointer to it is taken and stored in the container. I'm wondering if this API is understood well enough, or if people would end up passing local variables resulting in all kinds of breakage. Possibly people expect to be able to pass the argument by value, so perhaps that would be a better way to implement this? As an optimization, values that are exactly as big as a pointer could be passed without the wrapper and container, but that must assume things about the architecture-specific calling conventions, so might not be acceptable here.
• AFAICS the API repo provides attachInterrupt() and the underlying core provides attachInterruptParam. However, the attachInterrupt() method now accepts a parameter, so the naming distinction is weird. I guess the distinction is really between the sketch-visible attachInterrupt() API and the internal API-to-core attachInterruptParam() API. So perhaps the latter should be renamed to attachInterruptInternal() or something similar? There might be other APIs where something like this also happens that could benefit from a clear naming convention as well. Update: Seems that only a C++ overload for attachInterrupt with param is implemented, so a core needs to implement both with and without param versions. Also, attachInterruptParam() should probably be user-visible, to allow using it from within C code.
• AFAICS there is no attachInterrupt() without the extra parameter defined here, so I assume the core is expected to define that directly? For consistency, it might be better to have all sketch-visible versions defined here, and define only "Internal" versions in the core (where attachInterrupt() without the extra param defined here will then simply call the equivalent internal version without adding anything). Alternatively, the core could define only a param version which is then used without the non-param version, but that either requires an extra wrapper lambda (passing the original function in the extra param) resulting in extra runtime delay, or passing a param to a function that does not accept it (which is probably acceptable on register-based architectures, but cannot be guaranteed everywhere. There has been some discussion about this in Support extra parameter on attachInterrupt() ArduinoCore-avr#58 and Support extra parameter on attachInterrupt() Arduino#4519). So perhaps the current approach (letting the core define both a param and param-less version) is actually the best, since then the core can decide to handle both in the same way if acceptable on that architecture, or store the validity of the argument separately if needed).

[matthijskooijman]

For another core (stm32duino/Arduino_Core_STM32#892 (comment)) I was making a list of possible implementations for combining a callback with and without argument. Since I thought this might be more generally, I'm reproducing the list below. Most of these things are more useful for implementations in individual cores, but the last option affects the API by using a std::function argument (which can accept a normal function pointer as well) instead of having a version with an extra argument (since std::function can accept a lambda that captures the extra argument as well).

1. keep a flag about whether the callback accepts the additional argument or not and then support both. This adds a bit of overhead, but this could probably be just a single uint8_t with flags per HardwareTimer instance.

2. simply always pass the extra argument, even to functions that do not accept it. I think that the calling convention will always put this extra argument in a register, which will then just be unused by the callee, and should be harmless. See Support extra parameter on attachInterrupt() Arduino#4519 (comment) and onwards.

3. support parameter-less calls using an extra function call (e.g. having a global function that accepts a void* and then calls the function inside that void*, and then set this global function as the callback with the original user-supplied callback as the void*). See Support extra parameter on attachInterrupt() Arduino#4519 (original version before any force pushes).

4. similar to the previous one, but instead of calling the global function, just call the void* argument directly. See Support extra parameter on attachInterrupt() Arduino#4519 (comment)

   This option seems quite elegant, but it might not be faster in practice than the extra call from Rename RealtimeClock to ArduinoTime #3 (it seems that at least on AVR, a few years ago, the compiler compiled Functions for taking the mean and standard deviation of an array of numbers? #4 slower than Rename RealtimeClock to ArduinoTime #3, see Support extra parameter on attachInterrupt() Arduino#4519 (comment))

5. use std::function, which can wrap normal function pointers, as well as lambda functions. This means that instead of adding an extra void* argument, you can use a lambda capture to achieve the same. e.g. Instead of:

   attachInterrupt(my_func, &MyObj);
   

   you could use:

   attachInterrupt([MyObj&]() { my_func(&MyObj) });
   

   (or you could of course just put the code for my_func inside the lambda)

   One other alternative is to use std::bind before pass to attachInterrupt to bind the argument:

   attachInterrupt(std:bind(&my_func, &MyObj));
   

   Note that in both of the latter cases, a std::function object would then be created in the call to attachInterrupt. All of this is enabled by simply replacing the function pointer argument with a std::function argument.

   One caveat is that std::function might do dynamic allocation. It is guaranteed to not allocate when wrapping a function or method pointer, but the gcc implementation seems to do exactly that, so any lambda captures that are bigger than a single pointer will cause dynamic allocation (which should be ok as an alternative for the single void*, approach, and allows more captures at the cost of dynamic allocation).

   It seems the std::function approach is already used for the global attachInterrupt in STM32 and Teensy: Support extra parameter on attachInterrupt() ArduinoCore-avr#58 (comment)

   The big problem with std::function is that AVR does not supply it, but maybe something equivalent could be provided by Arduino there?

Of all these options, I would bed inclined to use option 5, using std::function. It has a bit more runtime overhead, but it is a lot more flexible (also allowing lambdas), backward compatible and consistent with the top-level attachInterrupt function. It might use dynamic allocation, but not if you just need a single void* argument.

Alternatively, the current approach of requiring C-compatible attachInterrupt and attachInterruptParam could stay, but individuals (non-AVR) cores could choose to use std::function as the underlying implementation (optionally exposing this to sketches as well) and create a lambda that captures the argument to convert to a std::function internally. This might actually be the best option, I think.

As suggested in the original issue, I would also recommend removing the overload with a reference param since it is complicated and probably leads to hard-to-debug surprises for users.