From 619fce27b36bfb5abaaeb68ba9ba8fb1683747aa Mon Sep 17 00:00:00 2001 From: Peter Soetens Date: Thu, 9 Oct 2014 22:17:30 +0200 Subject: [PATCH 1/6] signals: fix multi-signal emission in case of fused functor data source callbacks When an operation was called twice in a row, and a signal callback was installed using datasource semantics (with produceSignal() ), both callbacks would most likely receive twice the same data, being equal to the last operation call's values, since before the callbacks are dispatched, the data sources are already filled in. The last write wins and the first call-back sees the data of the last. This patch fixes that by defining a data_store_type in create_sequence with a store() and a load() function. The fused signal callback can now first store the args of the operation, then dispatch and retrieve them later on in the callback function. Signed-off-by: Peter Soetens --- rtt/internal/CreateSequence.hpp | 93 +++++++++++++++++++++++-- rtt/internal/FusedFunctorDataSource.hpp | 10 ++- 2 files changed, 97 insertions(+), 6 deletions(-) diff --git a/rtt/internal/CreateSequence.hpp b/rtt/internal/CreateSequence.hpp index 5d4e05d87..46e5a1883 100644 --- a/rtt/internal/CreateSequence.hpp +++ b/rtt/internal/CreateSequence.hpp @@ -65,8 +65,39 @@ namespace RTT namespace bf = boost::fusion; namespace mpl = boost::mpl; + /** + * Store a bound argument which may be a reference, const reference or + * any other type. + * This class was required to store (const) references, after the reference object storage + * was created. Copy of RTT::internal::AStore in BindStorage.hpp. + */ + template + struct DataStore + { + typedef T arg_type; + T arg; + DataStore() : arg() {} + DataStore(T t) : arg(t) {} + DataStore(DataStore const& o) : arg(o.arg) {} + + T& get() { return arg; } + void operator()(T a) { arg = a; } + operator T() { return arg;} + }; - template struct incomplete; + template + struct DataStore + { + typedef T& arg_type; + T* arg; + DataStore() : arg( &NA::na() ) {} + DataStore(T& t) : arg(&t) {} + DataStore(DataStore const& o) : arg(o.arg) {} + + T& get() { return *arg; } + void operator()(T& a) { arg = &a; } + operator T&() { return *arg;} + }; /** * Helper class for extracting the bare pointer from a shared_ptr @@ -194,6 +225,11 @@ namespace RTT */ typedef typename mpl::front::type arg_type; + /** + * The argument storage type + */ + typedef DataStore arg_store_type; + /** * The data source value type of an assignable data source is non-const, non-reference. */ @@ -223,12 +259,18 @@ namespace RTT typedef bf::cons 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 data_type; + /** + * The joint T data storage type of head and tail. + */ + typedef bf::cons 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 @@ -279,7 +321,32 @@ namespace RTT */ static void set(const data_type& in, const atype& seq) { AssignHelper::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 + * 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::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 DataStore sequence. + */ + static data_store_type store(const data_type& in ) { + return data_store_type(bf::front(in), tail::store(bf::pop_front(in))); } /** @@ -289,7 +356,7 @@ namespace RTT */ static void update(const type&seq) { UpdateHelper::update( bf::front(seq) ); - return tail::update( bf::pop_front(seq) ); + tail::update( bf::pop_front(seq) ); } /** @@ -347,6 +414,9 @@ namespace RTT typedef typename remove_cr::type ds_arg_type; typedef bf::cons data_type; + typedef DataStore arg_store_type; + typedef bf::cons data_store_type; + /** * The type of a single element of the vector. */ @@ -385,13 +455,20 @@ namespace RTT static void update(const type&seq) { UpdateHelper::update( bf::front(seq) ); - return; } static void set(const data_type& in, const atype& seq) { AssignHelper::set(seq, in); } + static void load(const data_store_type& in, const atype& seq) { + AssignHelper::set(seq, in); + } + + static data_store_type store(const data_type& in ) { + return data_store_type( bf::front(in) ); + } + /** * Copies a sequence of DataSource::shared_ptr according to the * copy/clone semantics of data sources. @@ -421,6 +498,7 @@ namespace RTT struct create_sequence_impl // 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; @@ -455,6 +533,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::shared_ptr according to the diff --git a/rtt/internal/FusedFunctorDataSource.hpp b/rtt/internal/FusedFunctorDataSource.hpp index fad59a9e1..bd001e111 100644 --- a/rtt/internal/FusedFunctorDataSource.hpp +++ b/rtt/internal/FusedFunctorDataSource.hpp @@ -55,6 +55,9 @@ #include #include +#include +using namespace std; + namespace RTT { namespace internal @@ -475,6 +478,9 @@ namespace RTT typename boost::function_types::parameter_types::type> SequenceFactory; typedef typename SequenceFactory::atype DataSourceSequence; boost::shared_ptr 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; /** @@ -512,8 +518,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 @@ -530,6 +535,7 @@ namespace RTT } void executeAndDispose() { + SequenceFactory::load( this->arg_cache, this->args ); mact->execute(); dispose(); } From 41cd3720d8c84e69d00f5fe41aaac12ada99cfb5 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Fri, 6 Jan 2017 23:14:01 +0100 Subject: [PATCH 2/6] internal: fixed const-correctness for internal::DataStore and avoid TypeInfo usage for conversion Signed-off-by: Johannes Meyer --- rtt/internal/CreateSequence.hpp | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/rtt/internal/CreateSequence.hpp b/rtt/internal/CreateSequence.hpp index 46e5a1883..8fb3ce740 100644 --- a/rtt/internal/CreateSequence.hpp +++ b/rtt/internal/CreateSequence.hpp @@ -81,8 +81,10 @@ namespace RTT DataStore(DataStore const& o) : arg(o.arg) {} T& get() { return arg; } + T const& get() const { return arg; } void operator()(T a) { arg = a; } operator T() { return arg;} + operator T const&() const { return arg;} }; template @@ -95,8 +97,10 @@ namespace RTT DataStore(DataStore const& o) : arg(o.arg) {} T& get() { return *arg; } + T const& get() const { return *arg; } void operator()(T& a) { arg = &a; } operator T&() { return *arg;} + operator T const&() const { return *arg;} }; /** @@ -154,7 +158,10 @@ namespace RTT typedef typename ds_type::element_type element_type; ds_type a = - boost::dynamic_pointer_cast< element_type >( DataSourceTypeInfo::getTypeInfo()->convert(*front) ); + boost::dynamic_pointer_cast< element_type >( *front ); + if ( ! a ) { + a = boost::dynamic_pointer_cast< element_type >( DataSourceTypeInfo::getTypeInfo()->convert(*front) ); + } if ( ! a ) { //cout << typeid(DataSource).name() << endl; ORO_THROW_OR_RETURN(wrong_types_of_args_exception( argnbr, tname, (*front)->getType() ), ds_type()); @@ -166,8 +173,10 @@ namespace RTT template static ads_type assignable(std::vector::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 >( *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()); } From f6f11f61c75587ec9063e50d186cd1c2d7226382 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 10 Jan 2017 17:41:59 +0100 Subject: [PATCH 3/6] internal: fixed conversion operator return type in internal::AStore and internal::DataStore These types are not used in a const context and therefore const correctness is not an issue. This patch partially reverts ccf678ca53fab373ad1fd65f137993b893517e41. For consistency with the get() accessors the return type of internal::AStore and internal::DataStore has been changed to T&. Unfortunately this change alone does not fix the compile error reported in https://github.com/orocos-toolchain/rtt/pull/196. Signed-off-by: Johannes Meyer --- rtt/internal/BindStorage.hpp | 4 ++-- rtt/internal/CreateSequence.hpp | 8 ++------ 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/rtt/internal/BindStorage.hpp b/rtt/internal/BindStorage.hpp index 663ead4f4..e60a008a2 100644 --- a/rtt/internal/BindStorage.hpp +++ b/rtt/internal/BindStorage.hpp @@ -71,7 +71,7 @@ namespace RTT T& get() { return arg; } void operator()(T a) { arg = a; } - operator T() { return arg;} + operator T&() { return arg; } }; template @@ -84,7 +84,7 @@ namespace RTT T& get() { return *arg; } void operator()(T& a) { arg = &a; } - operator T&() { return *arg;} + operator T&() { return *arg; } }; template diff --git a/rtt/internal/CreateSequence.hpp b/rtt/internal/CreateSequence.hpp index 8fb3ce740..6d6e1d949 100644 --- a/rtt/internal/CreateSequence.hpp +++ b/rtt/internal/CreateSequence.hpp @@ -81,10 +81,8 @@ namespace RTT DataStore(DataStore const& o) : arg(o.arg) {} T& get() { return arg; } - T const& get() const { return arg; } void operator()(T a) { arg = a; } - operator T() { return arg;} - operator T const&() const { return arg;} + operator T&() { return arg; } }; template @@ -97,10 +95,8 @@ namespace RTT DataStore(DataStore const& o) : arg(o.arg) {} T& get() { return *arg; } - T const& get() const { return *arg; } void operator()(T& a) { arg = &a; } - operator T&() { return *arg;} - operator T const&() const { return *arg;} + operator T&() { return *arg; } }; /** From 885059053054dc5096e45a36f37662a0038832ed Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 10 Jan 2017 19:20:55 +0100 Subject: [PATCH 4/6] internal: replaced internal::DataStore by internal::AStore already defined in BindStorage.hpp Signed-off-by: Johannes Meyer --- rtt/internal/BindStorage.hpp | 2 ++ rtt/internal/CreateSequence.hpp | 41 ++++----------------------------- 2 files changed, 6 insertions(+), 37 deletions(-) diff --git a/rtt/internal/BindStorage.hpp b/rtt/internal/BindStorage.hpp index e60a008a2..eac05725c 100644 --- a/rtt/internal/BindStorage.hpp +++ b/rtt/internal/BindStorage.hpp @@ -64,6 +64,7 @@ namespace RTT template struct AStore { + typedef T arg_type; T arg; AStore() : arg() {} AStore(T t) : arg(t) {} @@ -77,6 +78,7 @@ namespace RTT template struct AStore { + typedef T& arg_type; T* arg; AStore() : arg( &NA::na() ) {} AStore(T& t) : arg(&t) {} diff --git a/rtt/internal/CreateSequence.hpp b/rtt/internal/CreateSequence.hpp index 6d6e1d949..8d8b716ad 100644 --- a/rtt/internal/CreateSequence.hpp +++ b/rtt/internal/CreateSequence.hpp @@ -51,6 +51,7 @@ #include #include +#include "BindStorage.hpp" #include "DataSource.hpp" #include "Exceptions.hpp" #include "../FactoryExceptions.hpp" @@ -65,40 +66,6 @@ namespace RTT namespace bf = boost::fusion; namespace mpl = boost::mpl; - /** - * Store a bound argument which may be a reference, const reference or - * any other type. - * This class was required to store (const) references, after the reference object storage - * was created. Copy of RTT::internal::AStore in BindStorage.hpp. - */ - template - struct DataStore - { - typedef T arg_type; - T arg; - DataStore() : arg() {} - DataStore(T t) : arg(t) {} - DataStore(DataStore const& o) : arg(o.arg) {} - - T& get() { return arg; } - void operator()(T a) { arg = a; } - operator T&() { return arg; } - }; - - template - struct DataStore - { - typedef T& arg_type; - T* arg; - DataStore() : arg( &NA::na() ) {} - DataStore(T& t) : arg(&t) {} - DataStore(DataStore const& o) : arg(o.arg) {} - - T& get() { return *arg; } - void operator()(T& a) { arg = &a; } - operator T&() { return *arg; } - }; - /** * Helper class for extracting the bare pointer from a shared_ptr * data source. Used in create_sequence::data() to unwrap the shared_ptr; @@ -233,7 +200,7 @@ namespace RTT /** * The argument storage type */ - typedef DataStore arg_store_type; + typedef AStore arg_store_type; /** * The data source value type of an assignable data source is non-const, non-reference. @@ -348,7 +315,7 @@ namespace RTT * 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 DataStore sequence. + * @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))); @@ -419,7 +386,7 @@ namespace RTT typedef typename remove_cr::type ds_arg_type; typedef bf::cons data_type; - typedef DataStore arg_store_type; + typedef AStore arg_store_type; typedef bf::cons data_store_type; /** From a5857b2aab16144ed5ddb314209529de0d06cc9c Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Tue, 10 Jan 2017 19:21:41 +0100 Subject: [PATCH 5/6] internal: remove unneccessary boost::ref() and .get() calls in BindStorage.hpp This was required without the fix in e0dd34b, but can be simplified now that the implicit conversion operator directly returns a reference. Signed-off-by: Johannes Meyer --- rtt/internal/BindStorage.hpp | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/rtt/internal/BindStorage.hpp b/rtt/internal/BindStorage.hpp index eac05725c..d80cb1a8e 100644 --- a/rtt/internal/BindStorage.hpp +++ b/rtt/internal/BindStorage.hpp @@ -289,7 +289,7 @@ namespace RTT typename Signal::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) @@ -337,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 ) ); else retv.executed = true; } @@ -377,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; } @@ -419,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; } @@ -462,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; } @@ -507,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; } @@ -554,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; } @@ -603,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; } From ca0c42bab65f2696a304ea6935fda13a902e1c6e Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 16 May 2019 20:24:09 +0200 Subject: [PATCH 6/6] internal: fixed evil using namespace std in FusedFunctorDataSource.hpp Probably a leftover from debugging e7ede9f. Signed-off-by: Johannes Meyer --- rtt/internal/FusedFunctorDataSource.hpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/rtt/internal/FusedFunctorDataSource.hpp b/rtt/internal/FusedFunctorDataSource.hpp index bd001e111..3b385abff 100644 --- a/rtt/internal/FusedFunctorDataSource.hpp +++ b/rtt/internal/FusedFunctorDataSource.hpp @@ -55,9 +55,6 @@ #include #include -#include -using namespace std; - namespace RTT { namespace internal