Added status topic to indicate which mux channel is selected and if l… #20
Conversation
src/twist_mux.cpp
Outdated
| @@ -157,6 +166,13 @@ bool TwistMux::hasPriority(const VelocityTopicHandle& twist) | |||
| priority = velocity_priority; | |||
| velocity_name = velocity_h.getName(); | |||
| } | |||
| } else if(last_pub_name_ == velocity_h.getName() && | |||
| (pub_status_continuously_ || last_locked_name_ != velocity_h.getName())) { | |||
There was a problem hiding this comment.
please match the style of this file and drop that { to the next line
.gitignore
Outdated
| @@ -1 +1,2 @@ | |||
| *.pyc | |||
| /cmake-build-debug/ | |||
There was a problem hiding this comment.
please don't add custom stuff here
| @@ -82,7 +83,9 @@ class TwistMux | |||
| boost::shared_ptr<velocity_topic_container> velocity_hs_; | |||
| boost::shared_ptr<lock_topic_container> lock_hs_; | |||
|
|
|||
| ros::Publisher cmd_pub_; | |||
| ros::Publisher cmd_pub_, status_pub_; | |||
There was a problem hiding this comment.
wouldn't it be handier to add a new message that contains these two strings and publish that?
There was a problem hiding this comment.
I usually try to avoid adding custom messages for stuff like this. Unless you really believe that a new message type is the best way to go and don't like the current implementation of two topics, I'm thinking it might be better to either concatenate the strings with an easily parse-able delimiter or potentially use something like the diagnostics messages.
There was a problem hiding this comment.
I made the other couple small changes, let me know what you think the best course of action is on this. Unfortunately I no longer have a melodic setup with ROS1 so I may not be able to make + test any major modifications anytime soon.
|
I don't have a need for this feature, but I'm willing to review. |
|
Sorry I have not gotten back to this PR in a bit. Ill try to make some of the suggested modifications this weekend. |
| @@ -61,6 +63,7 @@ TwistMux::TwistMux(int window_size) | |||
|
|
|||
| /// Publisher for output topic: | |||
| cmd_pub_ = nh.advertise<geometry_msgs::Twist>("cmd_vel_out", 1); | |||
| status_pub_ = nh.advertise<std_msgs::String>("out_status", 5, true); | |||
There was a problem hiding this comment.
Just "status" is enough, "status in" is quite rare for nodes.
| { | ||
| cmd_pub_.publish(*msg); | ||
| if(pub_status_continuously_ || name != last_pub_name_){ |
| { | ||
| cmd_pub_.publish(*msg); | ||
| if(pub_status_continuously_ || name != last_pub_name_){ |
There was a problem hiding this comment.
Ah, I see here why the param is appended with continuously. Sorry I missed that part, than the parameter name does make sense!
But why publish a topic continuously when the data does not change? This is exactly what latched topics are for!
There was a problem hiding this comment.
That mostly had to do with my application at the moment of writing this. I had it going through a ROS1 bridge to ROS2 which meant latching did not work for me so I had to get around it with that solution. I can probably remove it now since that specific application is no longer something I need.
There was a problem hiding this comment.
Thanks, it's probably nicer not to modify all nodes for a shortcoming in
the bridge :)
|
Ah, sorry. I've reviewed this a while back. But apparently you have to close the review in order for the comments to show up... Main item: make the status a latched topic and you can drop the whole 'continuously' part. |
|
Any update on this? I'd like to use it. Thanks! |
|
No updates, my last comment is still valid. |
|
@chrisl8 Its not looking like I will have time to implement that anytime soon. I am also no longer using ROS1 as my team has fully transitioned our robot to ROS2 which makes it a bit harder to work on this. |
|
@Michael-Equi Which package, if any, are you using for command input mux on ROS2. @Timple Sounds good. I'll give it a try soon if I can find time. |
Added status topic to indicate which mux channel is selected and if lock is active