-
Notifications
You must be signed in to change notification settings - Fork 418
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
Dnae adas/serialized ipm #973
base: rolling
Are you sure you want to change the base?
Conversation
updated from #819 |
7b8b5b5
to
2cecb4a
Compare
Has anyone reviewed the PR already? |
No, we have not sorry. We'll get to it as soon as we can. |
@ivanpauno @wjwwood Just a small after-Easter question: Any updates? This PR is blocking the rosbag2 PR. |
Sorry, but I don't think that I will be able to do a review before |
@DensoADAS can you please rebase this branch? The connected PR for rosbag2 please as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uff, so that's quite a thing to review :)
I think I understood a fair amount of it to move forward on it.
My main concern would be that the publisher implementation gets too heavily overloaded which makes it quite hard to introspect. What about we introduce a GenericPublisher
or SerializedPublisher
similar to what we currently have in rosbag2? That would make it a bit easier to distinguish what type of messages are being sent. It also makes it a bit more straight forward to design an API which is tailored to it.
typename PublisherT = rclcpp::Publisher<MessageT, AllocatorT>, | ||
typename NodeT> | ||
std::shared_ptr<PublisherT> | ||
create_publisher( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do I need this overload if both methods are templated by MessageT
? The call to const auto type_support = *rosidl_typesupport_cpp::get_message_type_support_handle<MessageT>();
could be done in the original function or even within the create_publisher_factory
function, couldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this allows an instantion with type "rcl_serialized_message_t" for generic publishers without previously known message type (e.g. for rosbag2)
Example:
auto publisher = rclcpp::create_publisher<rcl_serialized_message_t>( node, "/topic", *rosidl_typesupport_cpp::get_message_type_support_handle<test_msgs::msg::Strings>(), rclcpp::QoS(10));
|
||
namespace rclcpp | ||
{ | ||
namespace experimental |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why should that be in experimental
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a definition for "experimental"? Should we move it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot the rationale why we introduced "experimental". But I see the Serialization
class being easily usable outside of the context of the intraprocess communication. Therefore, I'd propose putting it in the regular rclcpp
namespace.
public: | ||
virtual ~SerializationBase() {} | ||
|
||
virtual std::shared_ptr<rcl_serialized_message_t> serialize_message(const void * message) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I like that a raw pointer (void *
) is getting converted into a shared pointer. What does that mean in terms of ownership? Is the raw pointer no longer owned and directly converted into the shared pointer? What if the message was original allocated on the stack and passed in via &message
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I understand your concern. The Serialization class hides the rmw functions. It's difficult to avoid the void* without knowing the actual content (cannot be templated here). For the current PR we skip changing this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem here is not passing the message in via a void *
. The problem I see here is mainly that it's getting converted to a shared pointer which has implicit assumptions about ownership.
what about changing the signature to something like this:
virtual void serialize_message(const void * ros_message, rcl_serialize_message_t * serialized_message)
input and output are clearly defined and the encapsulation into a shared pointer can be done outside of this funciton.
That also goes along well with the deserialize_message
function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message is not converted in this sense. The resulting shared pointer is a new object. (And I want to prevent objects without cleanup functions as much as possible, so I prioritized preventing memory loss)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I had mentioned this somewhere else already, but what about using the SerializedMessage
for this?
virtual void serialize_message(const void * ros_message, SerializedMessage & serialized_message);
virtual void deserialize_message(const SerializedMessage & serialized_message, void * ros_message);
virtual void deserialize_message( | ||
const rcl_serialized_message_t & serialized_message, | ||
void * msg) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the signature of serialize
should be the same as deserialize
IntraProcessManager::add_subscription(SubscriptionIntraProcessBase::SharedPtr subscription) | ||
IntraProcessManager::add_subscription( | ||
SubscriptionIntraProcessBase::SharedPtr subscription, | ||
const bool is_serialized) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need that parameter here. It's part of the subscription base: https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/subscription_base.hpp#L203-L205
IntraProcessManager::add_publisher(rclcpp::PublisherBase::SharedPtr publisher) | ||
IntraProcessManager::add_publisher( | ||
rclcpp::PublisherBase::SharedPtr publisher, | ||
const bool is_serialized) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see an easy way to set a flag on the publisher base similar to the subscription base which indicates that it's a serialized publisher. Bummer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That being said, what happens if I take the same publisher and publish first a serialized message and then a regular MsgT
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: I just browsed through the tests to see what's happening on the user side. What about we don't call a create_publisher
function, but rather a create_serialized_publisher
which implements the publisherbase interface (sets the is_serialized
parameter) and only allows a call to publish with serialized messages.
// a publisher and a subscription with different content type can't communicate | ||
if (sub_info.is_serialized != pub_info.is_serialized) { | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just when I thought I understood what's happening the intraprocess manager, I read this comment and I am confused again. ;-)
I thought the four different variants are so that a non-serialized subscription could obtain values published from a serialized message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DensoADAS can you comment on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the point here is that here always the serialized and non-serialized publishers are added to the interprocess manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this make sense though? Either the publisher is a serialized
one or not, am I right?
That is, I think we can just only add one id plus the flag whether it's serialized or not.
9f6d560
to
02571ac
Compare
@Karsten1987 Thank you for your review. I've implemented most of your comments (some stuff like raw pointers need more time) We'd appreciate it if the PR could make it in foxy. Have a nice, healthy and relaxing weekend! |
02571ac
to
d8b65bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another round of review
public: | ||
virtual ~SerializationBase() {} | ||
|
||
virtual std::shared_ptr<rcl_serialized_message_t> serialize_message(const void * message) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem here is not passing the message in via a void *
. The problem I see here is mainly that it's getting converted to a shared pointer which has implicit assumptions about ownership.
what about changing the signature to something like this:
virtual void serialize_message(const void * ros_message, rcl_serialize_message_t * serialized_message)
input and output are clearly defined and the encapsulation into a shared pointer can be done outside of this funciton.
That also goes along well with the deserialize_message
function
|
||
namespace rclcpp | ||
{ | ||
namespace experimental |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot the rationale why we introduced "experimental". But I see the Serialization
class being easily usable outside of the context of the intraprocess communication. Therefore, I'd propose putting it in the regular rclcpp
namespace.
rclcpp/include/rclcpp/publisher.hpp
Outdated
@@ -191,34 +192,42 @@ class Publisher : public PublisherBase | |||
get_subscription_count() > get_intra_process_subscription_count(); | |||
|
|||
if (inter_process_publish_needed) { | |||
auto shared_msg = this->do_intra_process_publish_and_return_shared(std::move(msg)); | |||
auto shared_msg = this->do_intra_process_publish_and_return_shared( | |||
std::move( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't think we need that line break here.
// a publisher and a subscription with different content type can't communicate | ||
if (sub_info.is_serialized != pub_info.is_serialized) { | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DensoADAS can you comment on this?
@Karsten1987 another update |
I took the liberty to rebase your branch.
|
…ge to rcl_serialized_message_t and vice versa Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
… for addind a deleter to ease up memory handling further features: * copy constructor (allowing static memory allocation, e.g. if for static memory allocation in device driver) * destructor Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
* added flag for serialized communication * check for flag in "can_communicate" Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
* distinguish between content type and callback type * use "SerializedContainer" for serialized messages for memory deletion * added specialized methods for combinations of (un)serialized content/callback * automatically (de)serialize messages * allowed communication types are now (MessageT==CallbackMessageT || MessageT==SerializedContainer || CallbackMessageT==rcl_serialized_message_t) Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
… serialized communication in publisher base and subscriber base (adapted waitables) Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
* implemented second communication channel for serialized intra process messages extended "Publisher" for serialized messages: * pass message type by argument * added constructor for backwards compatibility (moved common code in separate methods "init_setup") * added allocator for serialized messages Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
* implemented second communication channel for serialized intra process messages * extend publish to handle "rcl_serialized_message_t" * changed behaviour of serialized message publishing by taking ownership of message (for deletion) Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
…ssage type * backwards compatible * allows creation of publisher with type "rcl_serialized_message_t" Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Co-Authored-By: Karsten Knese <Karsten1987@users.noreply.github.com> Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
* initiazlize variables Co-Authored-By: Karsten Knese <Karsten1987@users.noreply.github.com> Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
0c53533
to
2411642
Compare
* create_publisher to create_generic_publisher * create_subscription to create_generic_subscription Signed-off-by: Joshua Hampp <j.hampp@denso-adas.de>
@Karsten1987 We do not have an osx machine available. From your error log I assume there is a problem with overloading the "create_subscription" function and template deduction. Therefore I renamed the "generic" one (create_generic_subscription/create_generic_publisher). Could you test this? Thank you. |
@DensoADAS not sure if you've seen this, but I've opened a PR towards your original branch which addresses this: DensoADAS#1 |
So I've just played around with a bit more and I actually think we should re-architecture this PR, maybe break it up into a few smaller ones even. 1.) Introduce a C++ version for the serialization. That includes the 2.) Create a template specialization for the publisher, which is instantiated only for 3.) The same thing could technically be done for the subscription class as well. If the user wants to retrieve the data in a serialized way, The changes in 2.) might have drawbacks as to two different publisher instances have to be used when trying to publish ROS messages vs serialized messages. However, I generally also don't really see a big use-case for this in the first place. I could see how one wants to use one or the other publisher - c.f. rosbags when not being able to depend on the message types during compile time, but never really the mixed version. |
@Karsten1987 I replaced this PR by 3 new ones |
Signed-off-by: Noy <noy@xtend.me>
Description:
Extended rclcpp to allow serialized intra process communication. Therefore, we added a second connection to the intraprocess manager to the publisher and subscriber for serialized messages. To ease up the memory management, the rcl_serialized_message_t is wrapped in SerializedContainer with a delete to prevent memory loss. To prevent double free the ownership of the serialized message is passed to the publisher. The conversion between serialized and deserialized messages is done in the SubscriptionIntraProcess on demand. The changes also allow general purpose publisher and subscriber as needed e. g. for rosbag2.
Possible applications:
• direct recording of serialized data without serialization (with IPM)
• non-templated publisher of serialized messages (with IPM)
Features:
• added serialization and deserialization functions in intraprocess manager
• implemented publish of serialized messages (call by value and unique pointer) in Publisher and PublisherLifecycle
• claiming ownership of serialized messages by publish(rcl_serialized_message_t)
• create_publisher and create_subscription support now general message type of rcl_serialized_message_t by passing message_type (so non-templated version is possible)
• added unit tests: test_intra_process_communication