Skip to content
This repository has been archived by the owner on Apr 24, 2023. It is now read-only.

Ground truth #861

Closed
wants to merge 12 commits into from
Closed

Ground truth #861

wants to merge 12 commits into from

Conversation

nzhongathan
Copy link

@nzhongathan nzhongathan commented Dec 7, 2021

Description

Takes the main.cpp file for the ground_truth node and splits it into a .cpp and header file for a ground truth republished node.

This PR does the following:

  • Creates a header file for the ground truth node
  • Creates the ground truth node

Fixes #739

Testing steps (If relevant)

  1. Ground truth republisher still works properly

Self Checklist

  • I have formatted my code using make format
  • I have tested that the new behavior works

Copy link
Contributor

@VAM7686 VAM7686 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job! Just a few minor things that need to be addressed.

With regards to testing the functionality of the node I'd like to see a few unit tests. There's a ros node testing framework that can be found here and making a few tests should be pretty simple. A good example of some unit tests are the differential drive unit tests which test the differential drive node.

// since it also publishes the same transform
// publish transform for tf if there has not been a update from the
// localization node in the last second since it also publishes the same
// transform
if (std::abs(msg->header.stamp.toSec() - g_last_estimate.toSec()) > 1.0)
{
static tf::TransformBroadcaster br;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of having this as a static variable we could move this to the header file?

Comment on lines 96 to 97
static tf::TransformBroadcaster br;
static tf::TransformListener tf_listener;
Copy link
Contributor

@VAM7686 VAM7686 Dec 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also probably a good idea to keep these in the header file. Would probably need to rename the broadcasters to differentiate them since they broadcast two different transforms.

@VAM7686 VAM7686 removed the request for review from JasonGibson274 December 17, 2021 22:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite the igvc_gazebo/ground_truth node to be a C++ class
3 participants