Added functionality to pass custom parameter to HardwareTimer callback

[ramboerik]

Closes #891

[ramboerik]

I cherry picked the changes from the older version of arduino_core that platformio uses so I haven't been able to test the changes for current version.

[ABOSTM]

Hi @ramboerik ,
Thank you for this pull request. I run HardwareTimer non regression test and it is passed. 👍
Just missing few comments to keep code coherent and easy to read/maintain.
Also there is an issue returned by travis/Astyle. Can you run astyle: https://github.com/stm32duino/wiki/wiki/Astyle
By the way, can you also update wiki page with the new API you added:
https://github.com/stm32duino/wiki/wiki/HardwareTimer-library#API

[ramboerik]

All comments fixed and wiki is updated.

[ABOSTM]

@ramboerik , thanks for the update.
But still missing few comments.
Also for wiki update, you added new API, but you keep the old prototypes for attachInterrupt(..) without the new parameter.

[ABOSTM]

Thanks a lot @ramboerik.
LGTM

[ramboerik]

Hi @uzi18

is this a request? I could of course add an example usage somewhere, maybe on the wiki page?

[ABOSTM]

@ramboerik , if you go for a new example, it can be added here:
https://github.com/stm32duino/STM32Examples/tree/master/examples/Peripherals/HardwareTimer
And then wiki can be updated to point to the new example:
https://github.com/stm32duino/wiki/wiki/HardwareTimer-library#Examples

[matthijskooijman]

How is this parameter to be used? Normally, such a parameter is passed to the callback, but that does not seem to happen here (which would also have some complications for backward compatibility, of course). It seems these "arguments" can be accessed using getArg(), but then the callback still needs to remember its own channel number somehow?

[ramboerik]

@matthijskooijman This was originally designed for the rollover interrupt which doesn't have the channel parameter and the you can just use getArgs(). For it to work with channels you need to have one callback for each channel and from the callback use getArgs(CHANNEL_NUMBER)

Example:

#include <Arduino.h>

HardwareTimer t1(TIM1);
HardwareTimer t2(TIM2);
HardwareTimer t3(TIM3);

enum {
    CHANNEL_1 = 1,
    CHANNEL_2 = 2,
    CHANNEL_3 = 3,
    CHANNEL_4 = 4,
};

class CParam{
    public:
        int pin = 0;
        CParam(int pin) : pin(pin) {
            pinMode(pin, OUTPUT);
        }
};


void rollover_cb(HardwareTimer *timer){
    CParam* arg = (CParam*)timer->getArg();
    if(arg == nullptr) return;
    digitalWrite(arg->pin, !digitalRead(arg->pin));
}

void channel_cb1(HardwareTimer *timer){
    CParam* arg = (CParam*)timer->getArg(CHANNEL_1);
    if(arg == nullptr) return;
    digitalWrite(arg->pin, !digitalRead(arg->pin));
}

void channel_cb2(HardwareTimer *timer){
    CParam* arg = (CParam*)timer->getArg(CHANNEL_2);
    if(arg == nullptr) return;
    digitalWrite(arg->pin, !digitalRead(arg->pin));
}

void setup(){
    // heap alloc, scope will disappear
    CParam* param1 = new CParam(LED_BUILTIN);
    CParam* param2 = new CParam(LED_BLUE);
    CParam* param3 = new CParam(LED_RED);

    t1.attachInterrupt(rollover_cb, param1);
    t1.attachInterrupt(CHANNEL_1, channel_cb1, param2);
    t1.attachInterrupt(CHANNEL_2, channel_cb2, param3);

    t1.setOverflow(5, HERTZ_FORMAT);
    t1.setCaptureCompare(CHANNEL_1, 20, PERCENT_COMPARE_FORMAT);
    t1.setCaptureCompare(CHANNEL_2, 40, PERCENT_COMPARE_FORMAT);
    t1.resume();
}

void loop(){}

A bit cumbersome to use? yes.
Breaks comparability? no.
I agree that a breaking change would be the most easy to use and understand but that might upset someone 😄 I guess it's up to the maintainers to chose direction.

[ABOSTM]

@ramboerik
After discussion with @fpistm we agree to break compatibility in order to ease usage and maintenance.
So we would prefer that parameter are passed to the callback.
What is the opinion of our HardwareTimer main user ? @LinoBarreca @matthijskooijman @MagoKimbra @cparata

[MagoKimbra]

I agree with you if the solution is truly intelligent, better to do it well than to be forced by compatibility.
Best Regards.
MagoKimbra

[cparata]

Ok for me to pass the param in the callback prototype. It is clearer even if it breaks the compatibility.
Best Regards,
Carlo

[fpistm]

@ramboerik
could you udapte your PR to pass parameter to the callback?

Thanks in advance

[fpistm]

@ramboerik
could you udapte your PR to pass parameter to the callback?

Thanks in advance

[ramboerik]

okay, it's on my todo list! shall i open a new PR or continue on this one?

[fpistm]

okay, it's on my todo list! shall i open a new PR or continue on this one?

Thanks @ramboerik
you can continue on this one we will squash it when finished.

[LinoBarreca]

@fpistm sorry, I was away for some job interviews. I agree with you too.

[fpistm]

@fpistm sorry, I was away for some job interviews. I agree with you too.

No worries @LinoBarreca 😉
We are all very busy 😜
Thanks for the feedback

[matthijskooijman]

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/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/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/Arduino#4519 (comment)

   This option seems quite elegant, but it might not be faster in practice than the extra call from Uploader tool wrong path? #3 (it seems that at least on AVR, a few years ago, the compiler compiled micros() is going backwards #4 slower than Uploader tool wrong path? #3, see Support extra parameter on attachInterrupt() arduino/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() arduino/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?

6. The official ArduinoCore-API repo (which is intended to become the main definition of what the "Arduino API" means) has a separate attachInterruptParam function (for C, which does not support function overloading). See https://github.com/arduino/ArduinoCore-API/blob/398e70f188e2b861c10d9ffe5e2bfcb6a4a4f489/api/Common.h#L122-L123 It does not specify any implementation for this API, of course (and the only core that is based on ArduinoCore-API, megaAVR, does not seem to implement attachInterruptParam yet.

ArduinoCore-API also contains some implementation to allow passing not just void* arguments, but references to any object (which will be internally converted to void* and then back to a reference on the call), but I do not like the implementation much (it uses dynamic memory which I think is not really needed, and without a good way to clean up) and I'm not so sure that implicitly passing references to arbitrary objects is a good idea (it hides pointer complexity, but makes it easy for users to pass references to short-lived objects with all of the hard to debug fallout from that).

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.

[ramboerik]

I went with a minimal change in line with # 5 described above. I changed the interrupt callback from being a raw pointer into std::function. If anyone want to pass arguments to the callbacks it's easy to wrap with std::bind.

class abc{
    private:
        std::function<void(int)> cb_with_args = [&](int freq) {
            blink_red_led(freq);
        }

        callback_function_t cb_without_args = []() {
           blink_blue_led();
        }

    public:
       abc(){
           attachInterrupt(std::bind(cb_with_args, 500)); 
           attachInterrupt(cb_without_args); 
       }
};

[ABOSTM]

Hi @ramboerik,
Thanks for this update.
There is some missing part. Due to API change, all sources that use HardwareTimer interrupt callback are impacted. This is the case for:

• SoftwareSerial (it corresponds to the issue reported by PIO Continuous Integration)

/github/workspace/libraries/SoftwareSerial/src/SoftwareSerial.cpp: In member function 'void SoftwareSerial::setSpeed(uint32_t)':
/github/workspace/libraries/SoftwareSerial/src/SoftwareSerial.cpp:135:45: error: no matching function for call to 'HardwareTimer::attachInterrupt(void (*)(HardwareTimer*))'
  135 |       timer.attachInterrupt(&handleInterrupt);

• Probably the same for c:\Arduino\arduino_BugFix3\portable\packages\STM32\hardware\stm32\1.8.0\libraries\Servo\src\stm32\Servo.cpp

There is also the HardwareTimer examples and NonReg, but I propose to handle that part on my side.

[ABOSTM]

LGTM

[ABOSTM]

Thanks @ramboerik .
I updated the examples, and created a new one specific for callback with parameter.
You can review/comment:
stm32duino/STM32Examples#23

[fpistm]

This would be fine to also update the Wiki, to warn about API change btw core version.

[fpistm]

@matthijskooijman
Please, can we have your feedback before merging?

[matthijskooijman]

Code looks great to me, nice and clean diff. I left one nitpick inline.

The commit history is a bit of a mess, though, so this should probably be squashed into a single commit during merge.

[matthijskooijman]

I would remove the !- nullptr here, just let the callback be implicitly converted to bool by the if. This does exactly the same, but is a bit more future-proof (support for != null will be removed in C++20 for some reason) and does not suggest that the callback is actually a pointer (which it is not, though it sort of works like one).

[fpistm]

The commit history is a bit of a mess, though, so this should probably be squashed into a single commit during merge.

Yes it will be squashed.
Thanks for the tips.