-
Notifications
You must be signed in to change notification settings - Fork 412
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
*_raw function #388
*_raw function #388
Conversation
24b4382
to
da27a04
Compare
CallbackT && callback, | ||
size_t qos_history_depth, |
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.
Can you speak a little about why you had to change the order of the arguments to this 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.
i can't really explain why, but with the modifications done in subscription_traits
, I get template deduction errors without this change. Somehow it can't really differentiate between the two function overloads. With the change done here, it works though.
I am happy to discuss/debug that further and try to resolve this. I just don't know how :)
rclcpp/src/rclcpp/executor.cpp
Outdated
subscription->get_subscription_handle(), | ||
reinterpret_cast<rcl_message_raw_t *>(message.get()), &message_info); | ||
auto rcl_ptr = message.get(); | ||
(void) rcl_ptr; |
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.
What is this for?
@@ -297,11 +297,15 @@ class LifecycleNode::LifecycleNodeInterfaceImpl | |||
return RCL_RET_ERROR; | |||
} | |||
|
|||
// TODO(karsten1987): Set this back to true whenever we have c-raw publisher | |||
constexpr bool publish_update = 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.
rcl
has a raw publish right? Or do you mean for C types (as opposed to C++ types)?
Shouldn't we support C types now before merging this? How much more work is that?
rclcpp/src/rclcpp/executor.cpp
Outdated
if (subscription->is_raw()) { | ||
ret = rcl_take_raw( | ||
subscription->get_subscription_handle(), | ||
reinterpret_cast<rcl_message_raw_t *>(message.get()), &message_info); |
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 think the memory allocated by rcl_take_raw
(rmw_take_raw
indirectly) is freed here, since the smart pointer will only free the memory for the rcl_message_raw_t
, which doesn't have a destructor that functions properly (it's a C struct). It should call rmw_free
(as long as the rmw_take_raw
starts using rmw_allocate
as I suggested in the other pr), so it probably needs a custom destructor that calls the right functions before freeing the rcl_message_raw_t
itself.
You might need to override the message strategy's borrow_message
and return_message
functions for the rmw_message_raw_t
.
c86fd2c
to
c7ee1f9
Compare
rclcpp/src/rclcpp/executor.cpp
Outdated
message_info.from_intra_process = false; | ||
|
||
if (subscription->is_raw()) { | ||
// TODO(karsten1987): Raise if opensplice !? |
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.
Right now, this function contains a lot of code duplication, given that big if - else
.
Ideally, we also call take_raw
to get the still serialized data from the wire and only convert it to a ros message if it's a raw callback.
The reason I didn't do it this way is that Opensplice doesn't support to get the serialized stream yet and thus would stay incompatible.
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 until we make take_raw
required it has to be this way.
rclcpp/CMakeLists.txt
Outdated
@@ -234,6 +234,17 @@ if(BUILD_TESTING) | |||
${PROJECT_NAME} | |||
) | |||
endif() | |||
ament_add_gtest(test_raw_message_allocator test/test_raw_message_allocator.cpp | |||
ENV RCL_ASSERT_RMW_ID_MATCHES=${rmw_implementation}) | |||
if(TARGET test_raw_message_allocator) |
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 curious, why if(TARGET ...)
? Is it not guaranteed that ament_add_gtest()
will create a target with that name?
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.
fair point. I simply adhered to the rest of the file. We can change that in a follow up PR
auto raw_msg = std::make_shared<rcl_message_raw_t>(); | ||
raw_msg->buffer_length = 0; | ||
raw_msg->buffer_capacity = 0; | ||
raw_msg->buffer = NULL; |
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.
*raw_msg = rmw_get_zero_initialized_raw_message()
instead?
raw_msg->buffer_capacity = 0; | ||
raw_msg->buffer = NULL; | ||
rmw_initialize_raw_message(raw_msg.get(), 0, &rcutils_allocator_); | ||
rmw_raw_message_resize(raw_msg.get(), capacity); |
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.
How about using capacity
on the line above instead of resizing?
rmw_initialize_raw_message(raw_msg.get, capacity, &rcutils_allocator_)
@@ -62,15 +80,51 @@ class MessageMemoryStrategy | |||
return std::allocate_shared<MessageT, MessageAlloc>(*message_allocator_.get()); | |||
} | |||
|
|||
virtual std::shared_ptr<rcl_message_raw_t> borrow_raw_message(unsigned int capacity) | |||
{ | |||
auto raw_msg = std::make_shared<rcl_message_raw_t>(); |
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.
Recommend using the 4th std::shared_ptr
constructor on this page to store a deleter that calls rmw_raw_message_fini()
. That makes sure it will be cleaned up even if a bug causes return_raw_message()
to not be called.
rclcpp/include/rclcpp/publisher.hpp
Outdated
if (status != RCL_RET_OK) { | ||
// *INDENT-OFF* (prevent uncrustify from making unnecessary indents here) | ||
throw std::runtime_error( | ||
std::string("failed to publish raw message: ") + rcl_get_error_string_safe()); |
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.
Could use rclcpp::exceptions::throw_from_rcl_error(status, "failed to publish raw message");
rclcpp/src/rclcpp/executor.cpp
Outdated
rcl_reset_error(); | ||
} | ||
auto void_raw_msg = std::static_pointer_cast<void>(raw_msg); | ||
subscription->handle_message(void_raw_msg, message_info); |
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.
It looks like this calls handle_message()
even when rcl_take_raw()
fails. Is this intentional? The non-raw case doesn't do that.
rclcpp/src/rclcpp/executor.cpp
Outdated
"take failed for subscription on topic '%s': %s", | ||
subscription->get_topic_name(), rcl_get_error_string_safe()); | ||
rcl_reset_error(); | ||
if (ret == RCL_RET_OK) { |
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.
Switch order RCL_RET_OK == ret
for misra?
@@ -62,15 +80,52 @@ class MessageMemoryStrategy | |||
return std::allocate_shared<MessageT, MessageAlloc>(*message_allocator_.get()); | |||
} | |||
|
|||
virtual std::shared_ptr<rcl_message_raw_t> borrow_raw_message(unsigned int capacity) |
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.
size_t
(obviously depends on changing that down the API chain too, but I think it should be)
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 chose unsigned int
because RTI only serializes this.
https://community.rti.com/static/documentation/connext-dds/5.2.3/doc/api/connext_dds/api_cpp/structDDS__DynamicData.html#ae31195afe3fe6f567ddce38e7f99980e
I can change to size_t
and cast it to unsigned for connext, but this might lead to data loss. Do you prefer this? Not sure what the best approach here is.
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.
size_t
is unsigned, and usually larger than unsigned int
. Do you mean the user could ask for something larger and that would be an issue? You could check that it isn't too big at the point where you pass the size_t
to RTI's API.
delete msg; | ||
}); | ||
*raw_msg = rmw_get_zero_initialized_raw_message(); | ||
rmw_raw_message_init(raw_msg.get(), capacity, &rcutils_allocator_); |
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.
Check the return code, and do not setup the destructor for the shared_ptr
until you know this succeeds. Make sure to throw based on the rcl ret code.
{ | ||
auto raw_msg = std::shared_ptr<rcl_message_raw_t>(new rcl_message_raw_t, | ||
[](rmw_message_raw_t * msg) { | ||
rmw_raw_message_fini(msg); |
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.
Check return code.
/// Release ownership of the message, which will deallocate it if it has no more owners. | ||
/** \param[in] msg Shared pointer to the message we are returning. */ | ||
virtual void return_message(std::shared_ptr<MessageT> & msg) | ||
{ | ||
msg.reset(); | ||
} | ||
|
||
virtual void return_raw_message(std::shared_ptr<rcl_message_raw_t> & raw_msg) | ||
{ | ||
rmw_raw_message_fini(raw_msg.get()); |
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.
Isn't this already being done in the custom destructor?
rclcpp/include/rclcpp/publisher.hpp
Outdated
{ | ||
if (store_intra_process_message_) { | ||
// not supported atm | ||
throw std::runtime_error("storing raw messages in intra process is not supported yet."); |
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.
Make sure to make an issue about this after merging, and consider putting a TODO in here.
virtual void | ||
handle_intra_process_message( | ||
rcl_interfaces::msg::IntraProcessMessage & ipm, | ||
const rmw_message_info_t & message_info) = 0; | ||
|
||
rosidl_message_type_support_t | ||
get_message_type_support_handle() const; |
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.
Should this return a const reference instead? Not sure it matters, but worth considering.
* Cheers! | ||
*/ | ||
template<typename T> | ||
struct is_raw_subscription_argument : std::false_type |
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.
Maybe if you used the public
keyword before the inheritance uncrustify would do the right thing?
template<typename T>
struct is_raw_subscription_argument: public std::false_type
{};
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.
Unfortunately, it still complains:
31: +++ include/rclcpp/subscription_traits.hpp.uncrustify
31: @@ -40 +40 @@
31: -struct is_raw_subscription_argument<rcl_message_raw_t> : public std::true_type
31: +struct is_raw_subscription_argument<rcl_message_raw_t>: public std::true_type
Good idea though!
rclcpp/src/rclcpp/executor.cpp
Outdated
message_info.from_intra_process = false; | ||
|
||
if (subscription->is_raw()) { | ||
// TODO(karsten1987): Raise if opensplice !? |
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 until we make take_raw
required it has to be this way.
rclcpp/src/rclcpp/time_source.cpp
Outdated
@@ -74,7 +74,8 @@ void TimeSource::attachNode( | |||
|
|||
auto cb = std::bind(&TimeSource::clock_cb, this, std::placeholders::_1); | |||
|
|||
clock_subscription_ = rclcpp::create_subscription<MessageT, decltype(cb), Alloc, SubscriptionT>( | |||
clock_subscription_ = rclcpp::create_subscription< | |||
MessageT, decltype(cb), Alloc, MessageT, SubscriptionT>( |
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 is bad style in my opinion because the template arguments line up with the function arguments. Instead do:
clock_subscription_ = rclcpp::create_subscription<
MessageT, decltype(cb), Alloc, MessageT, SubscriptionT
>(
node_topics_.get(),
topic_name,
std::move(cb),
...
);
Or:
clock_subscription_ =
rclcpp::create_subscription<MessageT, decltype(cb), Alloc, MessageT, SubscriptionT>(
node_topics_.get(),
topic_name,
std::move(cb),
...
);
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.
Both suggest versions don't work, but I'll go for this one then to separate the template arguments from the function arguments:
clock_subscription_ = rclcpp::create_subscription<
MessageT, decltype(cb), Alloc, MessageT, SubscriptionT
>(
node_topics_.get(),
topic_name,
std::move(cb),
rmw_qos_profile_default,
group,
false,
false,
msg_mem_strat,
allocator);
I'm going to take a crack at changing the |
The nightly packaging jobs on Linux failed in the |
I forgot to link the |
* Enforce non-null argv values on rcl_init(). Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Adds test case for null argv values on rcl_init(). Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: ketatam <mohamed.amine.ketata@tngtech.com>
Connects to ros2/demos#185