Are there hidden dangers when using callbacks with member pointers?

Question

In Arduino projects, I often find that I need to have some class call a callback function when something happens, for instance when a pin reaches a certain value, or when a certain amount of time has elapsed. I can't use the STL, so I've written a small class that lets me attach a static member function with a reference to the object it is a member of, which then calls the actual instanced callback. When I call the call() function, it executes the attached callbacks.

Here's the class:

class CallbackCaller
{
    public:
    using instance_pointer_t = void*;
    using instance_callback_t = void(*)(instance_pointer_t);

    private:
    instance_pointer_t instance;
    instance_callback_t instanceCallback;


    public:
    void attach(instance_callback_t callback, instance_pointer_t instance);
    void call();
};

instanceCallback will hold a pointer to a static function in the class. instance holds a pointer to the class instance that the method in instanceCallback is a (static) member of.

Here's the implementation:

#include "CallbackCaller.h"

void CallbackCaller::attach(instance_callback_t callback, instance_pointer_t instance)
{
    this->instanceCallback = callback;
    this->instance = instance;
}

void CallbackCaller::call()
{
    if (this->instanceCallback && this->instance)
    {
        this->instanceCallback(this->instance);
    }
}

I attach a callback function via attach(), passing a pointer to the static member function and a pointer to the class instance. When it's time to execute the callback, I call call().

The idea is that a class would use it like so:

class Test
{
    CallbackCaller callbackCaller;

    public:
    void actualMemberCallback()
    {
        Serial.println("Member callback called!");
    }

    static void staticCallback(Test instance)
    {
        instance.actualMemberCallback();
    }

    void begin()
    {
        // Attach the static "staticCallback" function, which will receive a pointer to this object's instance as an argument, which it can then use to call the instance's member function.
        this->callbackCaller.attach((CallbackCaller::instance_callback_t)staticCallback, (CallbackCaller::instance_pointer_t)this);    
    }

    void raiseCallback()
    {
        // In a real world scenario this function would be called when a certain condition is met that warrants the callback.
        this->callbackCaller.call();
    }
};

This appears to work as intended:

Test test;
test.begin();
test.raiseCallback();    

This will produce "Member callback called!" in the serial monitor.

However, function pointers, void pointers, and pointers in general can be tricky. I'm wondering if there's some sort of trap I'm walking into? Is this a valid way of calling member functions that won't lead to unforeseen consequences down the road?

Answer 1

You've almost got an idiomatic C-style callback implementation, however staticCallback is incorrect, and trying to force it with casts has lead you into undefined behaviour. Because your example actualMemberCallback doesn't use this, you have unfortunately had the worst symptom of UB, appearing to work.

static void staticCallback(void *instance)
{
    static_cast<Test *>(instance)->actualMemberCallback();
}

Having fixed that, you no longer need to reinterpret_cast when calling attach.

A more C++ way would be to accept function pointers and function objects, and have Test::begin attach a lambda. CallbackCaller is approximately std::function<void()>.

class CallbackCaller
{
    struct BaseCallback
    {
        virtual ~BaseCallback() {}
        virtual void invoke() = 0;
    };
    template <typename F>
    struct Callback : BaseCallback
    {
        Callback(F f) : f(f) {}
        F f;
        void invoke() override { f(); }
    };
    BaseCallback * callback = nullptr; // Should be std::unique_ptr, at which point you can remove the constructors and destructors
    public:
    CallbackCaller() {}
    CallbackCaller(const CallbackCaller&) = delete;
    CallbackCaller& operator=(const CallbackCaller&) = delete;
    ~CallbackCaller() { delete callback; }
    template <typename F>
    void attach(F f) { callback = new Callback<F>{ f }; }
    void call() { if (callback) callback->invoke(); }
};

void Test::begin()
{
    this->callbackCaller.attach([this]{ actualMemberCallback(); });    
}