Create MessageMemoryStrategy for subscribers #64
Conversation
jacquelinekay
added
in review
|
|
||
| std::shared_ptr<void> borrow_message() | ||
| { | ||
| return message_; |
There was a problem hiding this comment.
What happens when this is called from multiple threads, i.e. when queue_size is greater than 1 and a multithreaded executor is used?
jacquelinekay
added
in progress
| std::shared_ptr<MessageT> message_; | ||
|
|
||
| }; | ||
|
|
There was a problem hiding this comment.
I agree, it should be possible to make a solid reset function using the generated code. Though it probably needs to be a free function that takes the message as an argument, to avoid collisions with a message field called reset. Either that or the reset function would need to contain __ somewhere.
|
summary of changes: Generalized |
jacquelinekay
added
in review
| protected: | ||
| std::array<MessageT, size> msg_pool_; | ||
| std::array<bool, size> msg_used_; | ||
| std::array<std::shared_ptr<MessageT>, size> msg_ptrs_; |
There was a problem hiding this comment.
Since all three arrays use the same index wouldn't it make sense to only have one array which contains a struct with three members?
| throw std::runtime_error("Tried to access message that was still in use! Abort."); | ||
| } | ||
| pool[current_index].msg_ptr_->~MessageT(); | ||
| new (pool[current_index].msg_ptr_.get())MessageT; |
There was a problem hiding this comment.
Does this compile? I've always called the constructor explicitly, it would be like this:
new (pool[current_index].msg_ptr_.get()) MessageT();(note the trailing ()) If this compiles, then no worries, I just didn't know you could do that.
There was a problem hiding this comment.
Yep, it does compile. actually, what bothers me is that uncrustify always removes the space between the memory location and the name of the class, regardless of trailing parentheses:
new (pool[current_index].msg_ptr_.get()) MessageT; -> new (pool[current_index].msg_ptr_.get())MessageT;
I'm not going to mess with that though.
There was a problem hiding this comment.
Yeah, that does bother me too; it makes it look like a c-style cast or something.
|
This is looking great! +1 |
|
+1 |
jacquelinekay
force-pushed
the
msg_memory_strategy
branch
from
f95b07c to
71681f1
Compare
|
jenkins: http://ci.ros2.org/job/ros2_batch_ci_linux/79/ @dirk-thomas, any more comments? Otherwise I will merge this by the end of the day. |
|
+1 to squash and merge. |
jacquelinekay
force-pushed
the
msg_memory_strategy
branch
from
71681f1 to
74a216a
Compare
Create MessageMemoryStrategy for subscribers
Complete support for rmw listener APIs
require CMake 3.5
* ros2GH-64 Rearrange default plugins build to use public headers Also already links write integration test against the default plugins. * ros2GH-64 Remove after_write_action Query the underlying db directly in tests to determine the amount of recorded messages. * ros2GH-64 Add convenience getter for single line SQL result * ros2GH-64 Add visibility macros to enable linking on Windows * ros2GH-64 Remove second sqlite exception class (it is not needed) * ros2GH-64 Fix hanging rosbag2_read_integration_test * ros2GH-64 Always log sqlite return code * ros2GH-64 Improve opening of sqlite database - Always open db with threading mode multi-thread. This forbids sharing database connections across threads. Db access from multiple threads is possible when each thread uses its own connection. - Open sqlite db accordingly to given io flags. Readonly open works only with already existing database. - Set journal mode pragma to WAL (write ahead log) and synchronous pragma to NORMAL. This should yield good write performance. - Small fix: use *.db3 as db name in tests. * ros2GH-64 Fix package test dependencies * ros2GH-64 Fix cppcheck error * ros2GH-64 Fix asserting typesupport in test (varies on architectures) * Cleanup - consistently use const ref of string instead of string for function arguments - simplify package dependencies - minor formatting * Make play integration test compile on Mac * Fix sqlite_wrapper_integration_test
* ros2GH-118 Rename rosbag2_storage::TopicMetadata to TopicInformation and rosbag2_storage::TopicwithType to TopicMetadata - The former TopicWithTye struct will be enlarged to contain also the rmw serialization format relative to the topic. This is why the name 'TopicMetadata' is now better suited for it. * ros2GH-17 Rename timestamp to time_stamp for consistency in types * Fix renaming of TopicWithType to TopicMetadata * formatting * pass by const ref
To avoid
newwhenever a message is received by the subscriber, abstract out the memory strategy for subscribers.MessageMemoryStrategyis templated on the message type and gets passed to the subscriber constructor.Example usage in ros2/examples#39