3

I'm trying to implement a small watchdog timer class in C++ 11 that should call some code when it expires.

Watchdog.h:

#pragma once

#include <thread>
#include <atomic>

class Watchdog
{
public:
    Watchdog();
    Watchdog(unsigned int milliseconds, std::function<void()> callback);
    ~Watchdog();

    void Start(unsigned int milliseconds, std::function<void()> callback);
    void Stop();
    void Pet();

private:
    unsigned int _interval;
    std::atomic<unsigned int> _timer;
    std::atomic<bool> _running;
    std::thread _thread;
    std::function<void()> _callback;

    void Loop();
};

Watchdog.cpp:

#include "Watchdog.h"

Watchdog::Watchdog() :
    _interval(0),
    _timer(0),
    _running(false) 
{
}

Watchdog::Watchdog(unsigned int milliseconds, std::function<void()> callback)
{
    Start(milliseconds, callback);
}

Watchdog::~Watchdog()
{
}

void Watchdog::Start(unsigned int milliseconds, std::function<void()> callback)
{
    _interval = milliseconds;
    _timer = 0;
    _callback = callback;
    _running = true;
    _thread = std::thread(&Watchdog::Loop, this);
}

void Watchdog::Stop()
{
    _running = false;
    _thread.join();
}

void Watchdog::Pet()
{
    _timer = 0;
}

void Watchdog::Loop()
{
    while (_running)
    {
        _timer++;

        if (_timer >= _interval)
        {
            _running = false;
            _callback();
        }

        std::this_thread::sleep_for(std::chrono::milliseconds(1));
    }
}

However, this thread loop seems a bit dirty to me, and std::this_thread::sleep_for is not accurate (it sleeps for at least the specified amount, meaning it can be longer than 1 ms), is there a better way to achieve this functionality?

UnTraDe
  • 3,747
  • 10
  • 36
  • 60
  • Rather use a condition variable and wait for a certain time that it's signalled. – πάντα ῥεῖ Mar 06 '16 at 08:51
  • 1
    I don't get it, why don't you just `std::this_thread::sleep_for(std::chrono::milliseconds(_interval));` ? – Gábor Buella Mar 06 '16 at 08:58
  • 2
    Also, your `Start` method can start multiple threads, there are four lines between checking `_running` and setting it. Should do it atomically instead http://en.cppreference.com/w/cpp/atomic/atomic_flag_test_and_set e.g.: `if (_running.test_and_set(_running))` – Gábor Buella Mar 06 '16 at 09:03
  • 2
    I meant: `if (std::atomic_compare_exchange_strong(_running, false, true))` http://en.cppreference.com/w/cpp/atomic/atomic_compare_exchange – Gábor Buella Mar 06 '16 at 09:07
  • @BuellaGábor If I sleep the entire `_interval` I won't be able to reset the timer by calling Pet. – UnTraDe Mar 06 '16 at 09:10
  • @BuellaGábor As for the thread syncing, you are correct. I edited the question to remove the thread sync related stuffs so the question can be more focused. Thanks! – UnTraDe Mar 06 '16 at 09:13
  • Blocking the caller of `Watchdog::stop()` for the watchdog timer interval is rather nasty. There's also a race in the destructor - the thread could run with a partially destructed `Watchdog` object. Take a look at my answer to [http://stackoverflow.com/questions/35681020/stdthread-unhanded-exception-accessing-this/35681928#35681928] - you could apply the pattern here, – marko Mar 06 '16 at 22:52

2 Answers2

1

Seems interesting, I could come up with this code. It does not compile, and you have to work a lot on it, but it shows one idea about how to do it.

std::mutex cmutex;  // needed for the condition_variable
std::condition_variable stop_condition;
std::chrono::time_point   last_pet_time;

void Watchdog::Start(unsigned int milliseconds, std::function<void()> callback)
{
    // somewhere in this method:
    last_pet_time = now();
    timeout = milliseconds;
}


void Watchdog::Stop()
{
    if (_running) {
        std::unique_lock<std::mutex> lock(cmutex);

        _running = false;
        stop_condition.notify_all(); // tell Loop() to stop

        _thread.join();
    }
}

void Watchdog::Pet()
{
    std::unique_lock<std::mutex> lock(cmutex);

    last_pet_time = now();
}


void Watchdog::Loop()
{
    std::unique_lock<std::mutex> lock(cmutex);

    while (_running                                 // was Stop() called?
           and (now() - last_pet_time) < timeout)   // was Pet() ( or Start() ) called recently?
    {
        // here the threads waits until:
        //  1. the condition_variable is notified in ::Stop()
        //  2. or the timeout expires
        //  3. or until spurious wakeup
        stop_condition.wait_for(lock, timeout);

    }
    if (_running) {
        _running = false;
        callback();
    }
}

Check: http://en.cppreference.com/w/cpp/thread/condition_variable/wait_until

Gábor Buella
  • 1,840
  • 14
  • 22
1

This should work. There are two classes really. One to handle the notification (AutoResetEvent) and the Watchdog class itself.

AutoResetEvent.h

#pragma once
#include <mutex>
class AutoResetEvent
{
private:
    bool m_ready = true;
    std::condition_variable m_condition;
    std::mutex m_mutex;
public:
    AutoResetEvent();
    ~AutoResetEvent();
    void WaitOne();
    void WaitFor(unsigned __int32 milli_secs);
    void Notify();
};

AutoResetEvent.cpp

AutoResetEvent::AutoResetEvent()
{
}


AutoResetEvent::~AutoResetEvent()
{
    Notify();
}

void AutoResetEvent::Notify()
{
    if (!m_ready)
    {
        std::unique_lock<std::mutex> locker(m_mutex);
        m_ready = true;
        m_condition.notify_all();
    }
}

void AutoResetEvent::WaitOne()
{
    std::unique_lock<std::mutex> locker(m_mutex);
    m_ready = false;
    m_condition.wait(locker, [&ready = m_ready]() {return ready; });
}

void AutoResetEvent::WaitFor(unsigned __int32 milli_secs)
{
    std::unique_lock<std::mutex> locker(m_mutex);
    m_ready = false;
    m_condition.wait_for(locker, std::chrono::milliseconds(milli_secs), [&ready = m_ready]() {return ready; });
}

Watchdog.h

#pragma once
#include "AutoResetEvent.h"
#include <functional>
class Watchdog
{
private:
    const unsigned __int32 WATCHDOG_BEEP_MS = 5*60*1000;

    bool m_active_status = false;
    bool m_new_pet = false;
    bool m_cancelled = false;

    AutoResetEvent m_reset_event;
    std::function<void(void)> m_call_back;

    void Loop();
public:
    Watchdog(std::function<void(void)> call_back);
    ~Watchdog();
    bool Start();
    void Pet();
    void Stop();

    bool IsActive() const;
};

Watchdog.cpp

    #include "stdafx.h"
    #include "Watchdog.h"

    void Watchdog::Loop()
    {
        m_active_status = true;
        m_cancelled = false;
        while (true)
        {
            m_new_pet = false;
            m_reset_event.WaitFor(WATCHDOG_BEEP_MS);
            if (!m_new_pet && !m_cancelled)
            {
                m_call_back();
                break;
            }
            if (m_cancelled)
            {
                break;
            }
        }
        m_active_status = false;
    }

    Watchdog::Watchdog(std::function<void(void)> call_back): m_call_back(call_back)
    {
    }
    Watchdog::~Watchdog()
    {
        Stop();
    }
    bool Watchdog::Start()
    {
        if (IsActive()) return false;
        std::thread loop_thread(&Watchdog::Loop, this);
        loop_thread.detach();
        return true;
    }


    void Watchdog::Pet()
    {
        if (!IsActive()) return;
        m_new_pet = true;
        m_reset_event.Notify();
    }

    void Watchdog::Stop()
    {
        if (!IsActive()) return;
        m_cancelled = true;
        m_reset_event.Notify();
    }

    bool Watchdog::IsActive() const
    {
        return m_active_status;
    }
falopsy
  • 636
  • 7
  • 16