Rtde recipe publishing #35
Conversation
ur_robot_driver/CMakeLists.txt
Outdated
| @@ -103,6 +103,7 @@ add_library(ur_robot_driver | |||
| src/ur/dashboard_client.cpp | |||
| src/ur/tool_communication.cpp | |||
| src/rtde/rtde_writer.cpp | |||
| src/ros/data_field_publisher.cpp | |||
There was a problem hiding this comment.
Please add this to the actual node and not to the non-ros library. Please also see #34 that might get merged before this.
| class DataPackagePublisher | ||
| { | ||
| public: | ||
| DataPackagePublisher() = delete; | ||
|
|
||
| DataPackagePublisher(const std::vector<std::string>& recipe, ros::NodeHandle& nh) : recipe_(recipe) | ||
| { | ||
| for (auto str : recipe) | ||
| { | ||
| publishers_.push_back(DataFieldPublisher::createFromString(str, nh)); | ||
| } | ||
| } | ||
|
|
||
| void publishData(const std::unique_ptr<DataPackage>& data_package) | ||
| { | ||
| for (auto const& i : publishers_) | ||
| { | ||
| i->publish(data_package); | ||
| } | ||
| } | ||
|
|
||
| private: | ||
| std::vector<std::string> recipe_; | ||
| std::list<std::unique_ptr<DataFieldPublisher>> publishers_; | ||
| }; |
There was a problem hiding this comment.
No Doxygen documentation for this class?
|
A high-level comment: publishing raw Best practice suggests correctly typed topics should be used wherever possible. Could the rationale for publishing these raw values be clarified, perhaps here in this issue or in some other place? |
| * | ||
| * \returns True if the realtime publisher could publish the data. | ||
| */ | ||
| virtual bool publish(const std::unique_ptr<DataPackage>& data_package) = 0; |
There was a problem hiding this comment.
Would passing a plain DataPackage const& work here? If there is no strong need for DataFieldPublishers to have access to the pointer, a plain reference would seem better.
| private: | ||
| DataT data_; | ||
| std::string data_field_identifier_; | ||
| std::unique_ptr<realtime_tools::RealtimePublisher<MsgT>> pub_; |
There was a problem hiding this comment.
Similar question here: pub_ could be made a direct member of the class, instead of wrapping it in a unique_ptr.
Is pub_ ever shared outside of this class?
| { | ||
| if (data_package->getData(data_field_identifier_, data_)) | ||
| { | ||
| if (pub_) |
There was a problem hiding this comment.
When would this evaluate to false?
Is it possible for publish(..) to be called before pub_ is initialised?
| { | ||
| for (size_t i = 0; i < N; i++) | ||
| { | ||
| pub_->msg_.data[i] = data_[i]; |
There was a problem hiding this comment.
If data_ is contiguous, perhaps std::copy could be used.
| @@ -0,0 +1,110 @@ | |||
| #include "ur_robot_driver/ros/data_field_publisher.h" | |||
There was a problem hiding this comment.
I have not checked other files: should this #include be moved below the license?
There was a problem hiding this comment.
uh, that slipped through my eyes. Of course, thank you.
| using _vector3d_publisher = ArrayDataPublisher<double, std_msgs::Float64MultiArray, 3>; | ||
| using _vector6d_publisher = ArrayDataPublisher<double, std_msgs::Float64MultiArray, 6>; | ||
| using _vector6int32_publisher = ArrayDataPublisher<int32_t, std_msgs::Int32MultiArray, 6>; | ||
| using _vector6uint32_publisher = ArrayDataPublisher<uint32_t, std_msgs::UInt32MultiArray, 6>; |
There was a problem hiding this comment.
The *MultiArray messages need the stride and size configured (among other things) to correct values, otherwise Subscribers can't deserialise them properly.
Does ArrayDataPublisher do that?
| if (DataPackage::isType<bool>(data_field_identifier)) | ||
| { | ||
| return std::unique_ptr<DataFieldPublisher>(new _bool_publisher(data_field_identifier, nh)); | ||
| } |
There was a problem hiding this comment.
Could templating be used here? Ctor of all DataFieldPublishers seems identical.
| ROS_ERROR_STREAM(rtde_interface::DataPackage::isType<double>("actual_q")); | ||
| ROS_ERROR_STREAM(rtde_interface::DataPackage::isType<vector6d_t>("actual_q")); |
There was a problem hiding this comment.
Are these lines included intentionally?
The idea is that if users are interested in getting additional data from the RTDE interface they can do so without code modifications, but by only specifying them in the recipe. Publishing raw data is the only option that we see without explicitly preparing a publisher for each possible RTDE data field and instanciating them on-demand. We consider this more of an easy-access power-functionality instead of something out of the box. |
Unfortunately I expect this -- ie: raw values from RTDE recipies -- to become the default way people will start interacting with the driver over a ROS API. As I believe one of the goals of this driver is to be an example of how to properly integrate a UR in a ROS environment, it would be nice if we could avoid including anti-patterns in it. |
You are probably right... Thanks for never letting me get away with laziness :-) We'll take that into consideration and improve this PR. |
That was not my intention, I don't want to be a gatekeeper here. It's easy for me to write "this is not a nice solution", but it would be nice if I could provide alternatives then. Perhaps an issue where the requirements and constraints could be listed could be used to drive some discussion? Dynamic message generation would be possible, as would |
I meant that in a positive way. The main reason we implemented it this was was laziness on implementing this for each message field. This dropped out of an internal requirement rather than belonging to the original plan.
We've been discussing a bit and also came to this conclusion. We would leave simple datatypes as is, e.g. |
|
So, we took the time to have a closer look at all the published data from RTDE: Any thoughts on this? |
|
Markdown version: RTDE datatypes to ROS
|
|
Some first comments:
|
|
Thank you for your input.
From our point of view, this goes against the idea of this PR which is providing an easy interface to request single RTDE fields. Requesting one of the above will require requesting all the others, as well.
The
Yes, as we suggested. But I guess that comment was mainly referring to 1. and 2.
Same reasoning as 1.
Yes, we could easily publish TF frames, as well. However, I also like the idea of putting this into a separate ros controller...
I agree, that would be a better solution.
Both suggestions sound superior. We'll do some more investigation and pick the appropriate one from those.
Not sure, if I understand that correctly. Why would there be a lot of elements? It should be exactly 32. Publishing a plain |
t-schnell
force-pushed
the
rtde_recipe_publishing
branch
from
296b42b
to
dba9ced
Compare
|
Initial rework towards new structure/message types is done, should be critically looked over though - some questions about specific data fields remain and I have added them as comments directly in the code. |
|
There is a typo in data_field_publisher.cpp (target_movement should be target_moment). |
| * \param nh The used ROS node handle | ||
| */ | ||
| DirectDataPublisher(const std::string& data_field_identifier, ros::NodeHandle& nh) | ||
| : data_field_identifier_(data_field_identifier), pub_(nh, "rtde_data/" + data_field_identifier_, 1) |
There was a problem hiding this comment.
I think, I would not factor this in, here. Leave this to the caller that is creating this object. This way, the user would create this with a node handle living in the rtde_data namespace if desired.
| * \param nh The used ROS node handle | ||
| */ | ||
| ArrayDataPublisher(const std::string& data_field_identifier, ros::NodeHandle& nh) | ||
| : data_field_identifier_(data_field_identifier), pub_(nh, "rtde_data/" + data_field_identifier_, 1) |
There was a problem hiding this comment.
same comment about node handle as above
| class JointTemperaturePublisher : public DataFieldPublisher | ||
| { | ||
| public: | ||
| /*! | ||
| * \brief Creates a JointTemperaturePublisher object. | ||
| * | ||
| * \param data_field_identifier The string identifier of the data field to publish | ||
| * \param nh The used ROS node handle | ||
| */ | ||
| JointTemperaturePublisher(const std::string& data_field_identifier, ros::NodeHandle& nh) | ||
| : data_field_identifier_(data_field_identifier), pub_(nh, "rtde_data/" + data_field_identifier_, 1) | ||
| { | ||
| pub_.msg_ = std_msgs::Duration(); | ||
| } | ||
|
|
||
| /*! | ||
| * \brief Publishes the relevant data field from a data package. | ||
| * | ||
| * \param data_package The given data package to publish from | ||
| * | ||
| * \returns True if the realtime publisher could publish the data. | ||
| */ | ||
| virtual bool publish(const DataPackage& data_package) | ||
| { | ||
| if (data_package.getData(data_field_identifier_, data_)) | ||
| { | ||
| if (pub_.trylock()) | ||
| { | ||
| pub_.msg_.data.fromSec(data_); | ||
| pub_.unlockAndPublish(); | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| private: | ||
| double data_; | ||
| std::string data_field_identifier_; | ||
| realtime_tools::RealtimePublisher<std_msgs::Duration> pub_; | ||
| }; |
There was a problem hiding this comment.
I think, this is a copy&paste error, as it is just the timestamp publishing function again. Did you forget to implement this?
| using _bool_publisher = DirectDataPublisher<bool, std_msgs::Bool>; | ||
| using _uint8_publisher = DirectDataPublisher<uint8_t, std_msgs::UInt8>; | ||
| using _uint32_publisher = DirectDataPublisher<uint32_t, std_msgs::UInt32>; | ||
| using _uint64_publisher = DirectDataPublisher<uint64_t, std_msgs::UInt64>; | ||
| using _int32_publisher = DirectDataPublisher<int32_t, std_msgs::Int32>; | ||
| using _double_publisher = DirectDataPublisher<double, std_msgs::Float64>; | ||
| using _string_publisher = DirectDataPublisher<std::string, std_msgs::String>; |
| return std::unique_ptr<DataFieldPublisher>( | ||
| new ArrayDataPublisher<double, ur_rtde_msgs::JointCurrents, 6>(data_field_identifier, nh)); | ||
| } | ||
| else if (data_field_identifier == "target_movement") |
There was a problem hiding this comment.
| else if (data_field_identifier == "target_movement") | |
| else if (data_field_identifier == "target_moment") |
As mentioned by @macsmitty
| "actual_digital_input_bits", | ||
| "actual_digital_output_bits", | ||
| "analog_io_types", | ||
| "tool_analog_input_types" }; |
There was a problem hiding this comment.
robot_mode and safety_mode should also go here.
ur_rtde_msgs/package.xml
Outdated
| <!-- One maintainer tag required, multiple allowed, one person per tag --> | ||
| <!-- Example: --> | ||
| <!-- <maintainer email="[email protected]">Jane Doe</maintainer> --> | ||
| <maintainer email="[email protected]">Tristan Schnell</maintainer> |
There was a problem hiding this comment.
Are you going to actively maintain this? Otherwise it might be good to set me as maintainer while keeping you listed as the author.
ur_rtde_msgs/package.xml
Outdated
| <?xml version="1.0"?> | ||
| <package format="2"> | ||
| <name>ur_rtde_msgs</name> | ||
| <version>0.0.0</version> |
There was a problem hiding this comment.
A version different to 0 might be nice, although not released, yet.
|
Hi,
best regards |
This issue is exactly the (main) reason why this isn't merged, yet. |
|
Is this still under development? We want to have the actual TCP position data and this PR seems to be what we want. |
|
I am just reading the TCP via the topic /ur_hardware_interface/rtde_data/target_TCP_pose. I get a standard messagetype with the position vector and the quaternion describing the orientation. This works fine. But there is no timestamp available. I also tried to change the sample rate without luck. Only streaming with 500Hz works for me. |
The actual TCP pose is already available inside the tf system as as And yes, this is still on my ToDo-List, it just always gets too much over-prioritized :-( |
|
Hello everyone and thank you for your contribution. I am new in ROS and currently involved in a project with an UR3e. I have couple of questions I would be super grateful if you´d help me clarify:
Thanks so much in advance for your time and help! |
When using this branch adding variables to |
@fmauch Thank you so much for your response. What about the variables that are already specified in rtde_output_recipe.txt inside the official package, Can I access to them somehow (either through topics or by printing them in the terminal) without using this branch? Thanks again! |
|
@RisMmi Most data is available through running controllers. You can try running |
|
This PR hasn't made any progress for quite some time and will be closed soon. Please comment if it is still relevant. |
Implements automatic publishing of requested RTDE data to ROS topics.