1
\$\begingroup\$

Here is a class I wrote to provide a user a way to have a callback function executed at some defined frequency:

Task.hpp

#pragma once

#include <memory>
#include <functional>

class Task
{
public:
    Task(const double& _hertz, const std::function<void()>& _callback);
    ~Task();

    void SetCallback(const std::function<void()>& _callback);

    void Start();
    void Pause();
    void Exit();

    void SetFrequency(const double& _hertz);
    bool IsRunning() const;

    unsigned long GetOverrunCount() const;

private:
    struct Internal;
    std::unique_ptr<Internal> impl;
};

Task.cpp

#include "Task.hpp"

// misc
#include "Chrono.hpp"

// std
#include <thread>
#include <mutex>
#include <atomic>

struct Task::Internal
{
public:

    Internal(const double& _m_freq, const std::function<void()>& _callback):
        m_frequency(_m_freq),
        m_callback(_callback)
    {

    }

    ~Internal()
    {
        Exit();
    }

    void SetCallback(const std::function<void()>& _callback)
    {
        std::lock_guard<std::mutex> _lock(m_thread_mutex);

        m_callback = _callback;
    }

    void SetFrequency(const double& _hertz)
    {
        m_frequency = _hertz;
    }

    void Start()
    {
        std::lock_guard<std::mutex> _lock(m_thread_mutex);

        // Already have a thread
        if (m_thread != nullptr)
        {
            return;
        }

        m_thread = std::make_unique<std::thread>(&Internal::Task, this);

        m_pause = false;
        m_run = true;
    }

    void Pause()
    {
        m_run = false;
        m_pause = true;
    }

    void Exit()
    {
        std::lock_guard<std::mutex> _lock(m_thread_mutex);

        // We don't have a thread
        if (m_thread == nullptr)
        {
            return;
        }

        m_run = false;
        m_exit = true;

        // wait for exit
        m_thread->join();

        // Destory thread object
        m_thread.reset(nullptr);
    }

    bool IsRunning() const
    {
        return m_run;
    }

    unsigned long GetOverrunCount() const
    {
        return m_overrun_count;
    }

protected:

private:
    std::atomic<double> m_frequency;
    std::atomic<bool> m_exit{ false };
    std::atomic<bool> m_pause{ true };
    std::atomic<bool> m_run{ false };
    std::atomic<unsigned long> m_overrun_count{ 0 };

    std::unique_ptr<std::thread> m_thread{ nullptr };
    std::function<void()> m_callback;
    std::mutex m_thread_mutex;

    void Task()
    {
        // run until told to m_exit
        while (m_exit == false)
        {
            // Calculate the period from freq
            long long periodInUs = (long long)((1.0 / m_frequency) * 1000.0 * 1000.0);

            // start time of the task
            auto start = ChronoHelper::GetTimeNow();

            // we are not paused
            if (m_pause == false)
            {
                std::lock_guard<std::mutex> _lock(m_thread_mutex);

                // the callback has been set
                if (m_callback != nullptr)
                {
                    m_callback();
                }
            }

            // get the amount of time left needed to sleep
            TimeVar end = ChronoHelper::GetTimeNow();
            long long executionTime = ChronoHelper::DurationInMicroSeconds(start, end);
            long long toSleep = periodInUs - executionTime;

            if (toSleep > 0)
            {
                std::this_thread::sleep_for(std::chrono::microseconds(toSleep));
            }
            else
            {
                m_overrun_count++;
            }
        }
    };
};

Task::Task(const double& _m_freq, const std::function<void()>& _callback): 
    impl(new Internal(_m_freq, _callback))
{

}

Task::~Task()
{

}

void Task::Start()
{
    impl->Start();
}

void Task::Pause()
{
    impl->Pause();
}

void Task::Exit()
{
    impl->Exit();
}

bool Task::IsRunning() const
{
    return impl->IsRunning();
}

void Task::SetFrequency(const double& _hertz)
{
    impl->SetFrequency(_hertz);
}

void Task::SetCallback(const std::function<void()>& _callback)
{
    impl->SetCallback(_callback);
}

unsigned long Task::GetOverrunCount() const
{
    return impl->GetOverrunCount();
}

I made std::thread dynamically allocated because I did not like that I can't control when std::thread starts executing. This way, I limit the thread from running unnecessarily until the user calls Start(). It will, however, still be ticking when the user calls Pause(), which is a little jarring.

I was also considering having this class maintain a list of callbacks rather than a single callback.

\$\endgroup\$
3
\$\begingroup\$

Really nice use of an opaque type! (though I personally think it's overkill in this particular instance, but that's a matter of opinion).

I made std::thread dynamically allocated because I did not like that I can't control when std::thread starts executing.

You don't have to, a default-created std::thread serves the same purpose.

std::thread my_thread; //does nothing
my_thread = std::thread(&Internal::Task, this); // starts the thread

That being said, your code exhibits a severe lack of RAII, Start() and Exit() describe a clear bookended process that is best handled by a constructor/destructor pair.

The main problem this causes is that your class has very unclear invariants.

What if someone class Start() twice in a row? What if someone calls Pause() before calling Start()? Your code has to handle all these cases precisely because you have no control over the order of calls of the public interface.

However! You do know that it will be constructed once, and destructed once. If you move Start() and Exit() to the constructor/destructor, you'll suddenly see that the amount of code needed to implement the class become drastically smaller.

Edit:

This way, I limit the thread from running unnecessarily until the user calls Start().

This feeling is misguided. What you see as flexibility is actually complexity. The simpler the behavior and interface of class is, the easier it is to work with.

Delaying the creation of an object until you need it is easy to do in C++. In fact, it's the preferred way of doing things in general.

\$\endgroup\$
1
\$\begingroup\$

In addition to what Frank noted,

I assume that all the forwarding functions in the CPP file are just to prevent the definition of Internal from being known to the header file. What does that buy you?


const double& _hertz,

Why use that instead of the types from <chrono>? You are losing the strong type with units by just going with a float.

    m_frequency(_m_freq),
    m_callback(_callback)

Use the uniform initialization syntax for member initializers.

    std::lock_guard<std::mutex> _lock(m_thread_mutex);

Did you know that you can now write:

    std::lock_guard _lock(m_thread_mutex);

it is less error prone and easier to change the actual mutex type.


    if (m_thread != nullptr)

Don’t make explicit tests against nullptr! Use the contextual conversion to bool provided by the smart pointer type (or built-in for raw pointers).

The precondition can be written using a compact form. Even though you have the lock before it, it still makes sense to treat it as a precondition check.

if (!m_thread)  return;

    while (m_exit == false)

LOL! You know that m_exit is already of type bool… comparing a bool against true/false is just silly.

while (!m_exit)

is the proper idiomatic way to write it.


Writing identifiers with a leading underscore is a bad idea, and is generally dissuaded as a style. Note that it is legal for member names if (and only if) the next character is not a capital letter or another underscore.

\$\endgroup\$
2
  • \$\begingroup\$ The forwarding functions belong to the "partial implementation" idiom. It cleans up the header file a lot. My original implementation of this class was with WinAPI threads. When I stumbled on std::thread, I rewrote the class and everything that includes Task.hpp did not need to be recompiled. Although, none of those are that ground breaking. As for the m_exit == false, I agree. It reads easier in my mind to see "X is False" rather than "Not X". Is there a formal document that describes a widely accepted c++ coding style? I would like that make that a habit. \$\endgroup\$ Jul 1 '18 at 3:48
  • \$\begingroup\$ You can start with the C++ Standard Guidelines. You want x==false to be true, so you need to write (x==false)==true? Or do you want that to be is true so you need (x==false)==true)==true? Contextual bool as “OK to use” is deep seated and influenced subsequent language features as well as native idioms. \$\endgroup\$
    – JDługosz
    Jul 1 '18 at 21:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.