-
Notifications
You must be signed in to change notification settings - Fork 19
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
Feature/add tf publisher #63
Conversation
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.
Only some minor comments.
@@ -198,7 +203,21 @@ protected void init(NodeMainExecutor nodeMainExecutor) { | |||
manager.requestPermission(device, mUsbPermissionIntent); | |||
} | |||
|
|||
// Attempt a connection to ROS master | |||
checkRosMasterConenction(); |
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.
nit: checkRosMasterConnection
/** | ||
* Node that publishes a specified set of transforms between frames at a given rate to /tf. | ||
*/ | ||
public class ExtrinsicsPublisherNode extends AbstractNodeMain { |
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 rename: ExtrinsicsTfPublisherNode?
Time time = Time.fromMillis(System.currentTimeMillis()); | ||
for (TransformStamped tfs : tfsList) { | ||
Header header = tfs.getHeader(); | ||
header.setStamp(time); |
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 it be done with one line: tfs.getHeader().setStamp(time)
?
Thanks for your review @PerrineAguiar, the comments were addressed. |
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.
Great work @jubeira , I only left very minor comments, I am totally OK with this.
|
||
// map --> odom transformation | ||
addTransformation( | ||
new Transform( |
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.
tiny nit: add a comment i.e.: "using hardcoded position x=5, y=5 assuming a 10x10 empty map" or something like that maybe?
import org.ros.rosjava_geometry.Transform; | ||
import org.ros.rosjava_geometry.Vector3; | ||
|
||
// TODO: these transformations should be configurable with a YAML file or a similar mechanism. |
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.
add a class comment explaining what this class does.
import org.ros.rosjava_geometry.Vector3; | ||
|
||
// TODO: these transformations should be configurable with a YAML file or a similar mechanism. | ||
public class DefaultRobotTfPublisherNode extends ExtrinsicsTfPublisherNode { |
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.
class comment please
); | ||
|
||
// device --> base_footprint transformation | ||
// Taken from tango_extrinsics_publisher; this transformation accounts dock inclination and placement |
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 we remove the reference to this now non-existing custom script and instead add a brief description indicating how this was calculated?
import tf2_msgs.TFMessage; | ||
|
||
/** | ||
* Node that publishes a specified set of transforms between frames at a given rate to /tf. |
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 add a TODO (+ issue?) to upstream a contribution like this? (rosjava static TF publisher)
Thanks for your reviews, comments were addressed. |
This PR adds a node that is able to publish a set of transforms in /tf topic.
The actual transformations to publish are defined in separate classes, which would eventually be removed (the transformations could be configured in a YAML file and the node could read from the Parameter Server what to publish).
If that is a correct step in the future, I will open a ticket for that. The helper scripts may also be removed in the future.
Status indicators for this feature are pending (see #57).