Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new realtime_buffer-like class that works for reading and writing to … #73

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Prev Previous commit
Next Next commit
cpplint fixes
  • Loading branch information
guru-florida committed Apr 5, 2021
commit 601f8a797899300a99762266b55b21794a7c3b4e
109 changes: 70 additions & 39 deletions include/realtime_tools/realtime_barrier.hpp
Original file line number Diff line number Diff line change
@@ -1,9 +1,43 @@
//
// Created by guru on 2/22/21.
//

#ifndef REALTIME_TOOLS__REALTIME_BARRIER_H
#define REALTIME_TOOLS__REALTIME_BARRIER_H
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's name this file .h for consistency with the rest. I know the ROS2 style is to prefer .hpp for C++ stuff, but I think matching the rest of the repo is probably the higher priority for now

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Ros2 style is to prefer .hpp I think if anything the other files should be migrated over to .hpp in a seperate PR. I'm not totally committed to it, but that's my thoughts.

* Copyright (c) 2021 Colin F. MacKenzie, Inc.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
*
* * Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* * Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
* * Neither the name of the Willow Garage, Inc. nor the names of its
* contributors may be used to endorse or promote products derived from
* this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
* LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
* CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
* SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
* CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*/

/*
* Publishing ROS messages is difficult, as the publish function is
* not realtime safe. This class provides the proper locking so that
* you can call publish in realtime and a separate (non-realtime)
* thread will ensure that the message gets published over ROS.
*
* Author: Colin F. MacKenzie
*/

#ifndef REALTIME_TOOLS__REALTIME_BARRIER_HPP_
#define REALTIME_TOOLS__REALTIME_BARRIER_HPP_

#include <mutex>
#include <thread>
@@ -12,7 +46,7 @@
namespace realtime_tools
{

///@brief Attempt a lock on a resource but fail if already locked
/// @brief Attempt a lock on a resource but fail if already locked
class try_lock : public std::unique_lock<std::mutex>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should stick with CamelCase for these. Even though they're pretty thin wrappers around STL stuff, and STL stuff uses snake_case, I find it very disorienting to have user-defined classes that don't match the normal naming conventions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for the structs, template params, using aliases, etc below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer consistency as well. I did default to STL snake_case because it was based on unique_lock but your argument is sound so it can be changed to TryLock.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these should go in an internal namespace.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't say that these are "internal" symbols. They are part of the calling API in some cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, good call. I was thinking they were exclusively buried within the Realtime and NonRealtime structs as that locking_strategy alias, but I missed that the user could theoretically manually specify the locking strategy instead.

{
public:
@@ -21,7 +55,7 @@ class try_lock : public std::unique_lock<std::mutex>
{}
};

///@brief Lock a resource or wait for it to be free
/// @brief Lock a resource or wait for it to be free
class wait_lock : public std::unique_lock<std::mutex>
{
public:
@@ -35,28 +69,28 @@ class wait_lock : public std::unique_lock<std::mutex>
}
};

///@brief default options for realtime acess to resources
/// @brief default options for realtime acess to resources
struct realtime
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wrap these in a barrier_lock_strategy or similar namespace. Having public types realtime_tools::Realtime and realtime_tools::NonRealtime isn't ideal, a more meaningful realtime_tools::barrier_lock_strategy::Realtime/NonRealtime is better IMO

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds extra verbosity like you mentioned earlier. A whole namespace for 2 symbols. Perhaps in a library called realtime_tools we shouldn't reserve Realtime and NonRealtime to something so simple. So perhaps they should just be called RealtimeLock and NonRealtimeLock (or Locking).

{
using lock_strategy = try_lock;
};

///@brief default options for non-realtime access to resources
/// @brief default options for non-realtime access to resources
struct non_realtime
{
using lock_strategy = wait_lock;
};


///@brief Implements the underlying mechanisms for communicating between threads.
/// @brief Implements the underlying mechanisms for communicating between threads.
/// MemoryBarrier implements a double buffer swapping mechanism for interchanging data between
/// threads. It allocates two T objects on the heap. When MemoryBarrier::swap() is called the two
/// pointers are swapped. Generally one pointer is accessed from the non-realtime thread and the
/// other is used by the realtime thread. Only one-way communication, in either direction, is
/// possible. For two-way communication two separate MemoryBarriers can be used as a pair.
///
/// You should not use this class directly, you will work with ReadBarrier, WriteBarrier and
// DirectAccess classes only.
/// DirectAccess classes only.
///
/// General strategy:
/// For non-RT writing to RT thread, non-RT thread writes but doesnt swap, RT swaps and reads.
@@ -65,7 +99,7 @@ template<class T>
class MemoryBarrier
{
public:
///@brief Provides direct access to data
/// @brief Provides direct access to data
/// Should only be used from within the realtime thread. Default template args specify
/// DirectAccess should only try to lock the resource and on failure the DirectAccess object
/// would be equal to nullptr and therefore evaluate to false.
@@ -81,22 +115,21 @@ class MemoryBarrier
explicit DirectAccess(MemoryBarrierType & mem_barrier) noexcept
: lock_strategy(mem_barrier.mutex_), mem_(&mem_barrier), obj_(nullptr)
{
obj_ = _get(); // will return nullptr if we dont own the lock yet
obj_ = _get(); // will return nullptr if we dont own the lock yet
}

template<class X>
explicit DirectAccess(X & mem_barrier) noexcept
: lock_strategy(mem_barrier.memory().mutex_), mem_(&mem_barrier.memory()), obj_(nullptr)
{

obj_ = _get(); // will return nullptr if we dont own the lock yet
obj_ = _get(); // will return nullptr if we dont own the lock yet
}

template<class X>
explicit DirectAccess(X * mem_barrier) noexcept
: lock_strategy(mem_barrier->memory().mutex_), mem_(&mem_barrier->memory()), obj_(nullptr)
{
obj_ = _get(); // will return nullptr if we dont own the lock yet
obj_ = _get(); // will return nullptr if we dont own the lock yet
}


@@ -230,7 +263,6 @@ class MemoryBarrier
obj_ = mem_->nrt_ :
nullptr;
}

};


@@ -254,17 +286,15 @@ class MemoryBarrier
move.new_data_available_ = false;
}

/*MemoryBarrier(const MemoryBarrier<T, read_locking_strategy, write_locking_strategy>& copy)
: in_(new T(*copy.in_)), out_(new T(*copy.out_)), polarity_(copy.polarity_), new_data_available_(copy.new_data_available_) {} }*/

// allow moving but not copying
MemoryBarrier(const MemoryBarrier<T> &) = delete;

MemoryBarrier & operator=(const MemoryBarrier<T> &) = delete;


// todo: perhaps we need a new_data on both nrt and rt side
// true if new data is available, depends on if the memory is being used for Read from RT or Write to RT mode
// true if new data is available, depends on if the memory is being used for Read
// from RT or Write to RT mode
Comment on lines +296 to +297
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like "Indicates to the consumer that new data is available"? Alternativey we can just drop this comment, it's a pretty self-explanatory function.

inline bool new_data_available() const
{return new_data_available_;}

@@ -301,7 +331,7 @@ class MemoryBarrier
friend class DirectAccess<non_realtime>;
};

///@brief Create a barrier for reading data from a realtime thread
/// @brief Create a barrier for reading data from a realtime thread
/// ReadBarrier implements reading data from a realtime thread using a MemoryBarrier. For example,
/// this is used to transfer state data out of hardware interfaces. The default constructor will
// create a new memory barrier for use. There is an alternate constructor if you want to create
@@ -311,7 +341,6 @@ class ReadBarrier
{
public:
using MemoryBarrierType = MemoryBarrier<T>;
//using DirectAccessType = typename T::DirectAccess<>;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to the DirectAccessType alias here? Using the full decltype(my_barrier)::MemoryBarrierType::DirectAccess<> each time is a bit cumbersome, it would be nice to have the decltype(my_barrier)::DirectAccessType

Copy link
Member

@matthew-reynolds matthew-reynolds May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we move DirectAccess out of MemoryBarrier and into the top-level realtime_tools namespace, as I suggested below, this comment is irrelevant.

ReadBarrier() noexcept
: mem_(new MemoryBarrierType()), owns_mem(true)
@@ -330,14 +359,17 @@ class ReadBarrier
}
}

//inline operator MemoryBarrier&() { return *mem_; }

///@brief Access the underlying MemoryBarrier object
/// @brief Access the underlying MemoryBarrier object
inline MemoryBarrierType & memory()
{return *mem_;}

///@brief Get current value from non-realtime side without affecting the barrier.
/// Copy the current data into dest. No swap is performed and the new_data_available flag is unaffected.
/// @brief Returns true if new data is available to read
inline bool new_data_available() const
{return mem_->new_data_available();}

/// @brief Get current value from non-realtime side without affecting the barrier.
/// Copy the current data into dest. No swap is performed and the new_data_available flag is
/// unaffected.
bool current(T & dest)
{
typename MemoryBarrierType::template DirectAccess<non_realtime, locking_strategy> direct(*mem_);
@@ -349,7 +381,7 @@ class ReadBarrier
}
}

///@brief Read data out of the realtime thread
/// @brief Read data out of the realtime thread
/// Swap RT buffer for non-RT. Copy the new data into val and reset the new_data_available flag.
// todo: since this read also swaps, should it be called read_and_swap() or pop() or pull()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need a rename. The consumer should always try to swap, I think that's well enough understood.

bool pull(T & dest)
@@ -361,7 +393,7 @@ class ReadBarrier
dest = *direct;
return true;
} else {
return false; // failed to read
return false; // failed to read
}
}

@@ -371,7 +403,7 @@ class ReadBarrier
};


///@brief Create a barrier for writing data to a realtime thread
/// @brief Create a barrier for writing data to a realtime thread
/// WriteBarrier implements writing data to a realtime thread from a non-realtime thread using a
/// MemoryBarrier. For example, this is used to transfer commands to a hardware interface. The
/// default constructor will create a new memory barrier for use. There is an alternate
@@ -381,23 +413,22 @@ class WriteBarrier
{
public:
using MemoryBarrierType = MemoryBarrier<T>;
//using DirectAccessType = DirectAccess<T>;

WriteBarrier() noexcept
: mem_(new MemoryBarrierType())
{
}

WriteBarrier(MemoryBarrierType & mem_barrier) noexcept
explicit WriteBarrier(MemoryBarrierType & mem_barrier) noexcept
: mem_(&mem_barrier)
{
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WriteBarrier is missing the owns_mem and destructor from ReadBarrier

///@brief Access the underlying MemoryBarrier object
/// @brief Access the underlying MemoryBarrier object
inline MemoryBarrierType & memory()
{return *mem_;}

///@brief Get current value from non-realtime side without affecting the barrier.
/// @brief Get current value from non-realtime side without affecting the barrier.
/// Copy the current data into dest. No swap is performed and the new_data_available flag
/// is unaffected.
bool current(T & dest)
@@ -411,7 +442,7 @@ class WriteBarrier
}
}

///@brief Write data into the realtime thread
/// @brief Write data into the realtime thread
/// Copy the new data from val into non-RT buffer then swap RT buffer for non-RT and set the
// new_data_available flag.
bool push(const T & data)
@@ -431,7 +462,7 @@ class WriteBarrier
MemoryBarrierType * mem_;
};

}
} // namespace realtime_tools


#endif // REALTIME_TOOLS__REALTIME_BARRIER_H
#endif // REALTIME_TOOLS__REALTIME_BARRIER_HPP_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be 2 spaces after end of line, before comment. I think this fix is needed in a handful of places

14 changes: 6 additions & 8 deletions test/realtime_barrier_tests.cpp
Original file line number Diff line number Diff line change
@@ -149,10 +149,9 @@ TEST(ReadBarrier, write_from_RT_to_nRT)
// write state to non-RT
ReadBarrier<DefaultConstructable> buffer;
decltype(buffer)::MemoryBarrierType::DirectAccess<> da_writer(buffer);
// typeof(state) is MemoryBarrier<StateData>::DirectAccess<realtime, try_lock>
da_writer->number_ = 8; // using smart_ptr-like semantics
da_writer.new_data_available(true); // indicate to non-RT there is new state data
da_writer.reset(); // unlock the state barrier
da_writer->number_ = 8; // using smart_ptr-like semantics
da_writer.new_data_available(true); // indicate to non-RT there is new state data
da_writer.reset(); // unlock the state barrier
}

TEST(ReadBarrier, read_on_nRT)
@@ -162,10 +161,9 @@ TEST(ReadBarrier, read_on_nRT)

// repeat RT write
decltype(buffer)::MemoryBarrierType::DirectAccess<> da_writer(buffer);
// typeof(state) is MemoryBarrier<StateData>::DirectAccess<realtime, try_lock>
da_writer->number_ = 8; // using smart_ptr-like semantics
da_writer.new_data_available(true); // indicate to non-RT there is new state data
da_writer.reset(); // unlock the state barrier
da_writer->number_ = 8; // using smart_ptr-like semantics
da_writer.new_data_available(true); // indicate to non-RT there is new state data
da_writer.reset(); // unlock the state barrier

// now test the read from nRT thread
// get the data from RT thread and write into our user