ros-drivers / vrpn_client_ros

VRPN ROS Client
http://wiki.ros.org/vrpn_client_ros
60 stars 78 forks source link

VRPN Tracker May Have Multiple Sensors #1

Closed karlun closed 8 years ago

karlun commented 8 years ago

A VRPN "tracker" can have multiple tracked frames, each being a separate "sensor" in the VRPN API. In our project we have two sensors on one tracker and need to be able to read these off separately. The attached patch adds a parameter "append_sensor_id" that when set to true makes the client append the sensor id to the tracker name before submitting the pose to the tf topic.

sensor_id.patch.txt

paulbovbel commented 8 years ago

@karlun, thanks for the contribution. Can you please submit it as a pull request?

paulbovbel commented 8 years ago

It probably makes sense to publish to 'sensor_num/pose' when append_sensor_id is true. (https://github.com/clearpathrobotics/vrpn_client_ros/blob/indigo-devel/src%2Fvrpn_client_ros.cpp#L117).

This would require creating a vector of shared_ptr rather than one publisher per Tracker.

karlun commented 8 years ago

Yes, I agree that the pose should follow the pattern, but as you say that requires a vector of publishers and what I needed at the moment is a tf.

I also made a pull request of the patch.

paulbovbel commented 8 years ago

I put together a prototype extending on your PR (https://github.com/clearpathrobotics/vrpn_client_ros/tree/process-sensor-id). This implements the full set of changes I'd like to see if this package is going to support processing sensor IDs. Could you please give it a whirl with your hardware, I don't have access to anything a the moment.

karlun commented 8 years ago

@paulbovbel, that didn't work. I get an exception because it seems that ROS does not allow the / character in the topic. I think that it has to be in the namespace of the node handle instead. I see two alternatives:

1) create a separate namespace for each sensor, e.g.

NodeHandle (tracker->output_nh_, std::to_string(tracker_pose.sensor)).advertise(...)

resuting in something like "/Tracker0/5/pose", or

2) append the sensor id directly onto the namespace, e.g.

NodeHandle (tracker->nh, tracker->tracker_name + "_" + std::to_string(tracker_pose.sensor)).advertise(...);

resulting in something like "/Tracker0_5/pose".

I'm not sure if ROS likes the use of temporary NodeHandle objects, but I do think that that's fine since they afaik work as a singleton facade.

All code is for illustration purposes only and has never been compiled or tested.

karlun commented 8 years ago

I tested alternative 1 from by previous comment. After some adjustments it seems to work fine both with and without process_sensor_id set to true, so I made a pull request (#3). Do proof read, I'm pretty new with ROS. :-)

paulbovbel commented 8 years ago

@karlun, can you please provide the exception? ROS definitely allows '/' in namespaces, and I may have borked something else.

paulbovbel commented 8 years ago

You're right, looks like a nodehandle is lightweight, so this LGTM.

karlun commented 8 years ago

Taking a second look I now see that it was not the slash but the number that was invalid. Sorry for being to much in a rush when testing. The exception I got was

terminate called after throwing an instance of 'ros::InvalidNameException' what(): Character [0] is not valid as the first character in Graph Resource Name [0/pose]. Valid characters are a-z, A-Z, / and in some cases ~.

I'm not sure if this is important now since the solution with a separate node handle worked fine, but it does open up for alternative implementations, such as using sensor0/pose instead of just 0/pose