-
Notifications
You must be signed in to change notification settings - Fork 137
Add publish_tf rosparam functionality and updated launch file accordingly #154
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
Add publish_tf rosparam functionality and updated launch file accordingly #154
Conversation
Pulling in changes from main repo
This also addresses issue #148 . @elisabethwelburn this is the code you'll want. |
Hey thanks for doing this if only everyone on the internet was as helpful as you ! :-) I found a solution that also works for the original fiducial_slam code is to set the tf_publish_interval to 0 as well :) |
I don't think that solves the entire issue. Setting tf_publish_interval to
0 only stops the re-publishing of the last known post transform. If
fiducial_slam detects a new fiducial it will publish the new tf. This is
line 494 or so, the call to publishTf().
…On Tue, Feb 19, 2019 at 10:06 AM elisabethwelburn ***@***.***> wrote:
Hey thanks for doing this if only everyone on the internet was as helpful
as you ! :-) I found a solution that also works for the original
fiducial_slam code is to set the tf_publish_interval to 0 as well :)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#154 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AmLdCqcWOwhSatP_39GwCJIrQ4KDWbLVks5vPCD9gaJpZM4bDF05>
.
|
Thats a good point but my understanding is that it doesn't register the new fidcials if you set it as a read only map so all fiducials are pre-defined within the global frame upon starting up the ROS node? If it's performing SLAM as well then sure it'll mess up your tfs |
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.
Thanks for submitting the pull request, sorry it took so long for me to get to it.
I have some minor comments, once those are patched up I can merge this.
fiducial_slam/src/map.cpp
Outdated
@@ -195,6 +195,7 @@ Map::Map(ros::NodeHandle &nh) : tfBuffer(ros::Duration(30.0)){ | |||
nh.param<std::string>("base_frame", baseFrame, "base_link"); | |||
|
|||
nh.param<float>("tf_publish_interval", tfPublishInterval, 1.0); | |||
nh.param<bool>("publish_tf", publishPoseTf, 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.
Please default to true.
fiducial_slam/src/map.cpp
Outdated
publishTf(); | ||
|
||
if (publishPoseTf) | ||
{ |
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: Move the brace to the end of the previous line for if statements
@@ -511,7 +516,7 @@ void Map::publishTf() | |||
void Map::update() | |||
{ | |||
ros::Time now = ros::Time::now(); | |||
if (havePose && tfPublishInterval != 0.0 && | |||
if (publishPoseTf && havePose && tfPublishInterval != 0.0 && |
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.
Is the tf publish interval check redundant here?
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.
Almost but not exactly. The tfPublishInterval is used when you want fiducial_slam to re-publish the TF when a fiducial isn't detected. For example, you may want TF's to be published only when a fiducial is detected so you'd set publishPoseTf=True and tfPublishInterval=0.0.
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.
@rohbotics Need anything else to keep the ball rolling?
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.
Sorry for the delay on merging this.
Thanks for your contribution, I found the same bug again and remembered this PR. Sorry for the making you wait so long. |
-adds check on rosparam publish_tf when publishing rosparam pose
-updated fiducial_slam.launch to include publish_tf and tf_publish_interval