Interval scheduling wrapper for Mongoose-OS C timers

Question

I've written a library for Mongoose-OS C mgos_timers. I wanted to write a wrapper to take advantage of the full potential of C++ lambdas. The original C timers allow to pass a callback function pointer.

/* Timer callback */
typedef void (*timer_callback)(void *param);

mgos_timer_id mgos_set_timer(int msecs, int flags, timer_callback cb,void *cb_arg);

This has its limitations:

The closure type for a lambda-expression with no lambda-capture has a public non-virtual non-explicit const conversion function to pointer to function having the same parameter and return types as the closure type’s function call operator. The value returned by this conversion function shall be the address of a function that, when invoked, has the same effect as invoking the closure type's function call operator.

Therefore I wanted to use an std::function.

This is the C++ wrapper:

Header:

#pragma once

#include <mgos_timers.h>
#include <functional>

namespace mgos_utils {
    class interval {
        using interval_function_t = std::function<void(void)>;
        public:
            interval() = default;
            interval& operator=(interval&& other);
            interval(int millis, interval_function_t f);
            void start();
            void stop();
            ~interval();
        private:
            bool running = false;
            mgos_timer_id id;
            int repeat_millis;
            interval_function_t function;
    };
}

The implementation:

#include <mgos.h>

#include <mgos_utils_interval.h>

#include <functional>
#include <memory>
#include <mgos_timers.h>

#define MGOS_TIMER_DO_ONCE false

namespace mgos_utils {

    using interval_function_t = std::function<void(void)>;

    interval::interval(int millis, interval_function_t f) : 
        repeat_millis(millis), function(f)
    {
        start();
    }

    void interval::start() {
        if (!running) {
            running = true;
            id = mgos_set_timer(repeat_millis, MGOS_TIMER_DO_ONCE, [](void* this_interval) {
                auto interval = reinterpret_cast<mgos_utils::interval*>(this_interval);
                if (interval->running) interval->function(); 
                // Check again as the called function might stop the interval
                if (interval->running) interval->start();
            }, this);
        } else {
            stop();
            start();
        }
    }

    void interval::stop() {
        if (running) {
            running = false;
            mgos_clear_timer(id);
        }
    }

    interval& interval::operator=(interval&& other) {
        other.stop();
        function = other.function;
        repeat_millis = other.repeat_millis;
        start();
        return *this;
    }

    interval::~interval() {
        stop();
    }
}

This is an example of its usage:

#include <mgos.h>

#include <mgos_utils_interval.h>
#include <memory>

class interval_test {
    public:
        interval_test() {
            loop = mgos_utils::interval(500, [this](){
                LOG(LL_INFO, ("Test interval v1 count %i", interval_count++));
                if (interval_count > 5) {
                    loop = mgos_utils::interval(1000, [this]() {
                        LOG(LL_INFO, ("Test interval v2 count %i", interval_count++));
                        if (interval_count > 8) {
                            LOG(LL_INFO, ("Stop test interval"));
                            loop.stop();
                        }
                    });
                }
            });
        }
    private: 
        mgos_utils::interval loop;
        int interval_count = 0;
};

std::unique_ptr<interval_test> test;

extern "C" enum mgos_app_init_result mgos_app_init(void) {

    test = std::unique_ptr<interval_test>(new interval_test());

    return MGOS_APP_INIT_SUCCESS;
}

I've implemented the move assignment operator to be able to initialize the timers at any point as in the example. Any ideas for a better and more efficient user interface?

Answer 1

I'd be inclined to write that big lambda as a private static function:

class interval {
private:
    static void callback(void* this_interval) {
        reinterpret_cast<interval*>(this_interval)->do_it();
    }

    void do_it() {
        if (running && function) {
            function(); 
        }
        // Check again as the called function might stop the interval
        if (running) {
            start();
        }
    }
};

Note that I've added a check that function isn't empty - particularly important given that's the state of a default-constructed interval. You may prefer to just let it throw std::bad_function_call - if so, that's certainly worth a comment.

---

I'd re-order the condition in start() so it doesn't need to recurse:

void interval::start() {
    if (running) { stop(); }

    running = true;
    // etc.

---

I see there's a reasonable move-assignment operator, but what about move construction? That needs to be implemented or explicitly deleted. And copy construct or assignment? If you explicitly delete or default copy/move assignment and constructor, it helps show which operations you've considered.

---

Also, be aware that if function() takes some time to run, then starting the timer after it has finished will gradually drift from a standard repeating timer. That may or may not be a concern for your use, but make sure you've thought about it!