Skip to content

Fix multi-signal emission in case of fused functor data source callbacks #293

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix multi-signal emission in case of fused functor data source callbacks #293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
619fce2
signals: fix multi-signal emission in case of fused functor data sour…
Oct 9, 2014
41cd372
internal: fixed const-correctness for internal::DataStore<T> and avoi…
meyerj Jan 6, 2017
f6f11f6
internal: fixed conversion operator return type in internal::AStore<T…
meyerj Jan 10, 2017
8850590
internal: replaced internal::DataStore<T> by internal::AStore<T> alre…
meyerj Jan 10, 2017
a5857b2
internal: remove unneccessary boost::ref() and .get() calls in BindSt…
meyerj Jan 10, 2017
ca0c42b
internal: fixed evil using namespace std in FusedFunctorDataSource.hpp
meyerj May 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 19 additions & 17 deletions rtt/internal/BindStorage.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,27 +64,29 @@ namespace RTT
template<class T>
struct AStore
{
typedef T arg_type;
T arg;
AStore() : arg() {}
AStore(T t) : arg(t) {}
AStore(AStore const& o) : arg(o.arg) {}

T& get() { return arg; }
void operator()(T a) { arg = a; }
operator T() { return arg;}
operator T&() { return arg; }
Copy link
Member Author

Choose a reason for hiding this comment

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

@psoetens Was there a good reason why the operator did not return a reference in the original code? I think not: The object is already copied at least once when creating the AStore<T> instance or via operator()(T a). So no reason to copy it again to read it. For references we use the specialization below instead.

Even without the reference return type the object was not copied for the actual operation call because before this patch .get() was used to access it in BindStorageImpl<ToBind> below.

Copy link
Member

Choose a reason for hiding this comment

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

I remember that I tested this code with objects which did a cout in a copy constructor to count the number of copies made, and then introduced references to reduce the number of copies. That said, I don't remember if there was a formal place in the architecture where a copy 'must' be made, but the AStore seems to be the best place to do it, if required. AStore was meant to mimic the classical C++ function argument behavior.

};

template<class T>
struct AStore<T&>
{
typedef T& arg_type;
T* arg;
AStore() : arg( &NA<T&>::na() ) {}
AStore(T& t) : arg(&t) {}
AStore(AStore const& o) : arg(o.arg) {}

T& get() { return *arg; }
void operator()(T& a) { arg = &a; }
operator T&() { return *arg;}
operator T&() { return *arg; }
};

template<class T>
Expand Down Expand Up @@ -287,7 +289,7 @@ namespace RTT
typename Signal<ToBind>::shared_ptr msig;
#endif

BindStorageImpl() : vStore(boost::ref(retv)) {}
BindStorageImpl() : vStore(retv) {}
BindStorageImpl(const BindStorageImpl& orig) : mmeth(orig.mmeth), vStore(retv)
#ifdef ORO_SIGNALLING_OPERATIONS
, msig(orig.msig)
Expand Down Expand Up @@ -335,10 +337,10 @@ namespace RTT
void store(arg1_type t1) { a1(t1); }
void exec() {
#ifdef ORO_SIGNALLING_OPERATIONS
if (msig) (*msig)(a1.get());
if (msig) (*msig)(a1);
#endif
if (mmeth)
retv.exec( boost::bind(mmeth, boost::ref(a1.get()) ) );
retv.exec( boost::bind(mmeth, a1 ) );
Copy link
Member Author

@meyerj meyerj May 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

To be checked: Does boost::bind actually copy the AStore<T> instance and indirectly the argument to the functor object or does it invoke operator T&() here and only stores a reference?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like boost::bind is always copying the arguments itself, and only invokes the conversion operator at the time the functor is actually called: https://stackoverflow.com/questions/11255144/why-does-boostbind-store-arguments-of-the-type-passed-in-rather-than-of-the-ty

In this case the patch in a5857b2 (this PR) and a2ccd79 (in toolchain-2.9) is actually wrong. The .get() can be removed, but the boost::ref() call should have been kept.

else
retv.executed = true;
}
Expand Down Expand Up @@ -375,10 +377,10 @@ namespace RTT
void store(arg1_type t1, arg2_type t2) { a1(t1); a2(t2); }
void exec() {
#ifdef ORO_SIGNALLING_OPERATIONS
if (msig) (*msig)(a1.get(), a2.get());
if (msig) (*msig)(a1, a2);
#endif
if (mmeth)
retv.exec( boost::bind(mmeth, boost::ref(a1.get()), boost::ref(a2.get()) ) );
retv.exec( boost::bind(mmeth, a1, a2 ) );
else
retv.executed = true;
}
Expand Down Expand Up @@ -417,10 +419,10 @@ namespace RTT
void store(arg1_type t1, arg2_type t2, arg3_type t3) { a1(t1); a2(t2); a3(t3); }
void exec() {
#ifdef ORO_SIGNALLING_OPERATIONS
if (msig) (*msig)(a1.get(), a2.get(), a3.get());
if (msig) (*msig)(a1, a2, a3);
#endif
if (mmeth)
retv.exec( boost::bind(mmeth, boost::ref(a1.get()), boost::ref(a2.get()), boost::ref(a3.get()) ) );
retv.exec( boost::bind(mmeth, a1, a2, a3 ) );
else
retv.executed = true;
}
Expand Down Expand Up @@ -460,10 +462,10 @@ namespace RTT
void store(arg1_type t1, arg2_type t2, arg3_type t3, arg4_type t4) { a1(t1); a2(t2); a3(t3); a4(t4); }
void exec() {
#ifdef ORO_SIGNALLING_OPERATIONS
if (msig) (*msig)(a1.get(), a2.get(), a3.get(), a4.get());
if (msig) (*msig)(a1, a2, a3, a4);
#endif
if (mmeth)
retv.exec( boost::bind( mmeth, boost::ref(a1.get()), boost::ref(a2.get()), boost::ref(a3.get()), boost::ref(a4.get()) ) );
retv.exec( boost::bind( mmeth, a1, a2, a3, a4 ) );
else
retv.executed = true;
}
Expand Down Expand Up @@ -505,10 +507,10 @@ namespace RTT
void store(arg1_type t1, arg2_type t2, arg3_type t3, arg4_type t4, arg5_type t5) { a1(t1); a2(t2); a3(t3); a4(t4); a5(t5);}
void exec() {
#ifdef ORO_SIGNALLING_OPERATIONS
if (msig) (*msig)(a1.get(), a2.get(), a3.get(), a4.get(), a5.get());
if (msig) (*msig)(a1, a2, a3, a4, a5);
#endif
if (mmeth)
retv.exec( boost::bind( mmeth, boost::ref(a1.get()), boost::ref(a2.get()), boost::ref(a3.get()), boost::ref(a4.get()), boost::ref(a5.get()) ) );
retv.exec( boost::bind( mmeth, a1, a2, a3, a4, a5 ) );
else
retv.executed = true;
}
Expand Down Expand Up @@ -552,10 +554,10 @@ namespace RTT
void store(arg1_type t1, arg2_type t2, arg3_type t3, arg4_type t4, arg5_type t5, arg6_type t6) { a1(t1); a2(t2); a3(t3); a4(t4); a5(t5); a6(t6);}
void exec() {
#ifdef ORO_SIGNALLING_OPERATIONS
if (msig) (*msig)(a1.get(), a2.get(), a3.get(), a4.get(), a5.get(), a6.get());
if (msig) (*msig)(a1, a2, a3, a4, a5, a6);
#endif
if (mmeth)
retv.exec( boost::bind( mmeth, boost::ref(a1.get()), boost::ref(a2.get()), boost::ref(a3.get()), boost::ref(a4.get()), boost::ref(a5.get()), boost::ref(a6.get()) ) );
retv.exec( boost::bind( mmeth, a1, a2, a3, a4, a5, a6 ) );
else
retv.executed = true;
}
Expand Down Expand Up @@ -601,10 +603,10 @@ namespace RTT
void store(arg1_type t1, arg2_type t2, arg3_type t3, arg4_type t4, arg5_type t5, arg6_type t6, arg7_type t7) { a1(t1); a2(t2); a3(t3); a4(t4); a5(t5); a6(t6); a7(t7);}
void exec() {
#ifdef ORO_SIGNALLING_OPERATIONS
if (msig) (*msig)(a1.get(), a2.get(), a3.get(), a4.get(), a5.get(), a6.get(), a7.get());
if (msig) (*msig)(a1, a2, a3, a4, a5, a6, a7);
#endif
if (mmeth)
retv.exec( boost::bind( mmeth, boost::ref(a1.get()), boost::ref(a2.get()), boost::ref(a3.get()), boost::ref(a4.get()), boost::ref(a5.get()), boost::ref(a6.get()), boost::ref(a7.get()) ) );
retv.exec( boost::bind( mmeth, a1, a2, a3, a4, a5, a6, a7 ) );
else
retv.executed = true;
}
Expand Down
73 changes: 65 additions & 8 deletions rtt/internal/CreateSequence.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
#include <boost/fusion/mpl.hpp>
#include <boost/utility/enable_if.hpp>

#include "BindStorage.hpp"
#include "DataSource.hpp"
#include "Exceptions.hpp"
#include "../FactoryExceptions.hpp"
Expand All @@ -65,9 +66,6 @@ namespace RTT
namespace bf = boost::fusion;
namespace mpl = boost::mpl;


template <class T> struct incomplete;

/**
* Helper class for extracting the bare pointer from a shared_ptr
* data source. Used in create_sequence::data() to unwrap the shared_ptr;
Expand Down Expand Up @@ -123,7 +121,10 @@ namespace RTT
typedef typename ds_type::element_type element_type;

ds_type a =
boost::dynamic_pointer_cast< element_type >( DataSourceTypeInfo<ds_arg_type>::getTypeInfo()->convert(*front) );
boost::dynamic_pointer_cast< element_type >( *front );
if ( ! a ) {
a = boost::dynamic_pointer_cast< element_type >( DataSourceTypeInfo<ds_arg_type>::getTypeInfo()->convert(*front) );
}
if ( ! a ) {
//cout << typeid(DataSource<ds_arg_type>).name() << endl;
ORO_THROW_OR_RETURN(wrong_types_of_args_exception( argnbr, tname, (*front)->getType() ), ds_type());
Expand All @@ -135,8 +136,10 @@ namespace RTT
template<class ds_arg_type, class ads_type>
static ads_type assignable(std::vector<base::DataSourceBase::shared_ptr>::const_iterator front, int argnbr, std::string const& tname )
{
typedef typename ads_type::element_type element_type;

ads_type a =
boost::dynamic_pointer_cast< AssignableDataSource<ds_arg_type> >( *front ); // note: no conversion done, must be same type.
boost::dynamic_pointer_cast< element_type >( *front ); // note: no conversion done, must be same type.
if ( ! a ) {
ORO_THROW_OR_RETURN(wrong_types_of_args_exception( argnbr, tname, (*front)->getType() ), ads_type());
}
Expand Down Expand Up @@ -194,6 +197,11 @@ namespace RTT
*/
typedef typename mpl::front<List>::type arg_type;

/**
* The argument storage type
*/
typedef AStore<arg_type> arg_store_type;

/**
* The data source value type of an assignable data source is non-const, non-reference.
*/
Expand Down Expand Up @@ -223,12 +231,18 @@ namespace RTT
typedef bf::cons<ads_type, atail_type> atype;

typedef typename tail::data_type arg_tail_type;
typedef typename tail::data_store_type arg_store_tail_type;

/**
* The joint T data type of head and tail.
*/
typedef bf::cons<arg_type, arg_tail_type> data_type;

/**
* The joint T data storage type of head and tail.
*/
typedef bf::cons<arg_store_type, arg_store_tail_type> data_store_type;

/**
* Converts a std::vector of DataSourceBase types into a boost::fusion Sequence of DataSources
* of the types given in List. Will throw if an element of the vector could not
Expand Down Expand Up @@ -279,7 +293,32 @@ namespace RTT
*/
static void set(const data_type& in, const atype& seq) {
AssignHelper<atype, data_type>::set(seq, in);
return tail::set( bf::pop_front(in), bf::pop_front(seq) );
tail::set( bf::pop_front(in), bf::pop_front(seq) );
}

/**
* Sets the values of a sequence of AssignableDataSource<T>
* data sources ot the values contained in \a in using set().
* @param in The values to write.
* @param seq The receiving assignable data sources. Because
* the datasources are shared pointers, it's ok to work on the
* temporary copies of seq.
*/
static void load(const data_store_type& in, const atype& seq) {
AssignHelper<atype, data_store_type>::set(seq, in);
tail::load( bf::pop_front(in), bf::pop_front(seq) );
}

/**
* Stores the values of a sequence of data_type into
* a data_store_type sequence for later retrieval during load.
* We must return the resulting sequence by value, since boost fusion
* returns temporaries, which we can't take a reference to.
* @param in The values to store
* @return The receiving AStore sequence.
*/
static data_store_type store(const data_type& in ) {
return data_store_type(bf::front(in), tail::store(bf::pop_front(in)));
}

/**
Expand All @@ -289,7 +328,7 @@ namespace RTT
*/
static void update(const type&seq) {
UpdateHelper<arg_type>::update( bf::front(seq) );
return tail::update( bf::pop_front(seq) );
tail::update( bf::pop_front(seq) );
}

/**
Expand Down Expand Up @@ -347,6 +386,9 @@ namespace RTT
typedef typename remove_cr<arg_type>::type ds_arg_type;
typedef bf::cons<arg_type> data_type;

typedef AStore<arg_type> arg_store_type;
typedef bf::cons<arg_store_type > data_store_type;

/**
* The type of a single element of the vector.
*/
Expand Down Expand Up @@ -385,13 +427,20 @@ namespace RTT

static void update(const type&seq) {
UpdateHelper<arg_type>::update( bf::front(seq) );
return;
}

static void set(const data_type& in, const atype& seq) {
AssignHelper<atype, data_type>::set(seq, in);
}

static void load(const data_store_type& in, const atype& seq) {
AssignHelper<atype, data_store_type>::set(seq, in);
}

static data_store_type store(const data_type& in ) {
return data_store_type( bf::front(in) );
}

/**
* Copies a sequence of DataSource<T>::shared_ptr according to the
* copy/clone semantics of data sources.
Expand Down Expand Up @@ -421,6 +470,7 @@ namespace RTT
struct create_sequence_impl<List, 0> // empty mpl list
{
typedef bf::vector<> data_type;
typedef bf::vector<> data_store_type;

// the result sequence type is a cons of the last argument in the vector.
typedef bf::vector<> type;
Expand Down Expand Up @@ -455,6 +505,13 @@ namespace RTT
return;
}

static void load(const data_store_type& in, const atype& seq) {
return;
}

static data_store_type store(const data_type& in ) {
return data_store_type();
}

/**
* Copies a sequence of DataSource<T>::shared_ptr according to the
Expand Down
7 changes: 5 additions & 2 deletions rtt/internal/FusedFunctorDataSource.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,9 @@ namespace RTT
typename boost::function_types::parameter_types<Signature>::type> SequenceFactory;
typedef typename SequenceFactory::atype DataSourceSequence;
boost::shared_ptr<base::ActionInterface> mact;
// We need the arg_cache to store data similar to BindStorage,
// such that we can safely access it during execute().
typename SequenceFactory::data_store_type arg_cache;
DataSourceSequence args;
ExecutionEngine* subscriber;
/**
Expand Down Expand Up @@ -512,8 +515,7 @@ namespace RTT
if ( subscriber ) {
// asynchronous
shared_ptr sg = this->cloneRT();
SequenceFactory::set( seq, sg->args );

sg->arg_cache = SequenceFactory::store(seq);
sg->self = sg;
if ( subscriber->process( sg.get() ) ) {
// all ok
Expand All @@ -530,6 +532,7 @@ namespace RTT
}

void executeAndDispose() {
SequenceFactory::load( this->arg_cache, this->args );
mact->execute();
dispose();
}
Expand Down