Skip to content
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 source code and launch file for tf2 PointStamped message publisher and listener/filter #62

Merged
merged 27 commits into from
Sep 22, 2021

Conversation

kennywangaigraphx
Copy link

I add one python node (broadcaster node of PointStamped messages) and a launch file in turtle_tf2_py, and one C++ node (message filter/listener node) in turtle_tf2_cpp.

Add Python Code of PointStamped Messages Broadcaster Node
Add launch File turtle_tf2_sensor_message.launch.py
@kurshakuz
Copy link

Hi @kennywangaigraphx! Have you tried to build the package along with your updates? I see that there are some indentation issues and I think nodes will not build. I suggest you to clone the geometry_tutorials to your colcon workspace's src, checkout to kennywangaigraphx:ros2, build it and resolve errors. Then make commit and push them to your branch. Let me know how it goes, and ask if any questions arise

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Hi @ kennywangaigraphx

Thank you for your contribution, a have a few comments:

  • Do you mind to change the title of the PR ? to something more descriptive about what your are adding to this repo ?
  • Please check the linters. Some of then are failing. You can run then locally with:
colcon test --event-handlers console-direct+ --ctest-args -R turtle_tf2_cpp

@kennywangaigraphx
Copy link
Author

Thank you @kurshakuz for your comment, I followed your suggestion and resolved some errors and successfully built the two packages. But when I followed @ahcorde to colcon test the python package, test_copyright always have two errors: install/_local_setup_util_ps1.py: copyright=Dirk Thomas (2016-2019), license=
install/_local_setup_util_sh.py: copyright=Dirk Thomas (2016-2019), license=
But the copyright statements on the top of my .py file is the same as other python packages. I don't know the reason.
As for the CPP package, when I run
colcon test --event-handlers console-direct+ --ctest-args -R turtle_tf2_cpp
it just say "No tests were found!!!", and I don't know what to do. Would you please guide me to complete the colcon test? Thank you.
Anyway, I think I should update the source code files, because now they can be built successfully.

@kennywangaigraphx kennywangaigraphx changed the title Add source code and launch file for pr #1929 Add source code and launch file for tf2 PointStamped message publisher and listener/filter Sep 14, 2021
@kennywangaigraphx
Copy link
Author

And I changed the title of this PR as "dd source code and launch file for tf2 PointStamped message publisher and listener/filter".

@ahcorde
Copy link
Contributor

ahcorde commented Sep 14, 2021

colcon test --event-handlers console-direct+ --ctest-args -R turtle_tf2_cpp
it just say "No tests were found!!!", and I don't know what to do. Would you please guide me to complete the colcon test? Thank you.

Then you should compile your code with the test enabled.

colcon build --cmake-args -DBUILD_TESTING=True --packages-select turtle_tf2_cpp <any other argument>

@kurshakuz
Copy link

@kennywangaigraphx I see that there is no update in the turtle_tf2_message_filter.cpp, as the indentation is still wrong and the copyright is not present. Can you take a look at it?

turtle_tf2_cpp/src/turtle_tf2_message_filter.cpp Outdated Show resolved Hide resolved
@@ -1,42 +1,43 @@
from glob import glob
Copy link
Contributor

Choose a reason for hiding this comment

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

all theses changes are not required

@@ -0,0 +1,67 @@
# Copyright 2015 Open Source Robotics Foundation, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Copyright 2015 Open Source Robotics Foundation, Inc.
# Copyright 2021 Open Source Robotics Foundation, Inc.

Copy link
Author

Choose a reason for hiding this comment

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

I modified the copyright year from 2015 to 2021.


self.vel_pub = self.create_publisher(Twist, '/turtle3/cmd_vel', 1)
self.sub = self.create_subscription(Pose, '/turtle3/pose', self.handle_turtle_pose, 1)
self.sub
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.sub

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I will delete that line.

@@ -0,0 +1,76 @@
#include <rclcpp/rclcpp.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation

],
},
)
from glob import glob

Choose a reason for hiding this comment

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

I also think that this PR have the same issue with CR LF end of line sequence, as your commit tries to overwrite the setup.py file. Can you also fix it in all your files? If you are still struggling with it, let me know, I will try to help

Choose a reason for hiding this comment

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

Same for new package.xml and CMakeList.txt files

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I have fixed them now.

Choose a reason for hiding this comment

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

I don't see that error being solved. Basically if you look at the "Files changed" tab, you will see that your new files are fully red, which means you are overwriting the whole file. What you should see in red is the only lines you changed in original files. Something in your editor is trying to overwrite that.

If you still not able to solve it, you either can give me the write access to your geometry_tutorials repo or I can create a new PR based on your one. Just let me know

Update CMakeLists.txt for turtle_tf2_message_filter node
Update package.xml for dependencies of turtle_tf2_message_filter.cpp
Update turtle_tf2_message_filter.cpp after checking linters.
@kennywangaigraphx
Copy link
Author

Thank you @ahcorde , and I followed your guidance to do the linters checking, and corrected some errors in the turtle_tf2_message_filter.cpp source file and CMakeLists.txt, @kurshakuz . Currently, The following tests FAILED:
1 - copyright (Failed)
3 - cpplint (Failed)
5 - lint_cmake (Failed)
7 - uncrustify (Failed)
Where: 1 - copyright has 35 errors, all are "could not find copyright notice" errors; 3 - cpplint totally has 20 errors, all are in file turtle_tf2_cpp/build/turtle_tf2_cpp/CMakeFiles/3.16.3/CompilerIdCXX/CMakeCXXCompilerId.cpp and turtle_tf2_cpp/build/turtle_tf2_cpp/CMakeFiles/3.16.3/CompilerIdCXX/CMakeCXXCompilerId.c, and Not using '--root'; 5 - lint_cmake has 160 errors, all are in cmake related files in install or build subdirectory; 7 - uncrustify has 2 errors, one is Code style divergence in file 'build/turtle_tf2_cpp/CMakeFiles/3.16.3/CompilerIdCXX/CMakeCXXCompilerId.cpp', and the other is Code style divergence in file 'build/turtle_tf2_cpp/CMakeFiles/3.16.3/CompilerIdC/CMakeCCompilerId.c'.
As shown in the check list, all 7 lint diagnostics are successful now.
It seems that now the errors are not aroused by turtle_tf2_message_filter.cpp, and I don't know how to resolve these errors, do you @ahcorde @kurshakuz have any more suggestions? Thank you.

for the copyright year from 2015 to 2021 and delete the line "self.sub".
@ahcorde
Copy link
Contributor

ahcorde commented Sep 15, 2021

I fixed something on the ros2 branch which is not fixed in your branch, you should execute the following commands to merge these new changes.

git remote add ros https://github.com/ros/geometry_tutorials/
git fetch ros
git merge ros/ros2
git push origin ros2

Another important thing. Please you should only create a PR with your new changes, for example you edited the hole CMakeLists.txt file, I think @kurshakuz give it to you a tip about how to fix this. Probably you are adding some characters because of your editor.

@kurshakuz
Copy link

@kennywangaigraphx can you now try to change the EOL to LF here in all your files?

@kennywangaigraphx
Copy link
Author

@kurshakuz I changed the EOL to LF for all my files.

@kennywangaigraphx
Copy link
Author

I fixed something on the ros2 branch which is not fixed in your branch, you should execute the following commands to merge these new changes.

git remote add ros https://github.com/ros/geometry_tutorials/
git fetch ros
git merge ros/ros2
git push origin ros2

Another important thing. Please you should only create a PR with your new changes, for example you edited the hole CMakeLists.txt file, I think @kurshakuz give it to you a tip about how to fix this. Probably you are adding some characters because of your editor.

@ahcorde I did git fetch, merge and push through Github Desktop just now, and also resolved the EOL of my files. But why I can't see the check list now?

@ahcorde
Copy link
Contributor

ahcorde commented Sep 16, 2021

I fixed something on the ros2 branch which is not fixed in your branch, you should execute the following commands to merge these new changes.

git remote add ros https://github.com/ros/geometry_tutorials/
git fetch ros
git merge ros/ros2
git push origin ros2

Another important thing. Please you should only create a PR with your new changes, for example you edited the hole CMakeLists.txt file, I think @kurshakuz give it to you a tip about how to fix this. Probably you are adding some characters because of your editor.

@ahcorde I did git fetch, merge and push through Github Desktop just now, and also resolved the EOL of my files. But why I can't see the check list now?

it's your first contribution to this repo, Github force me to trigger the CI for each commit. You should see the check list now.

@kennywangaigraphx
Copy link
Author

@ahcorde @kurshakuz Great, finally all checks have passed.

@kurshakuz
Copy link

kurshakuz commented Sep 17, 2021

@ahcorde @kurshakuz Great, finally all checks have passed.

you need to remove all .pyc files

@kennywangaigraphx
Copy link
Author

@ahcorde @kurshakuz Great, finally all checks have passed.

you need to remove all .pyc files

@kurshakuz Ok, I will remove them. So I need to delete them from local folder of my computer and commit again? Or other ways to do that?

@kennywangaigraphx
Copy link
Author

@kurshakuz I removed all the .pyc files just now.

Copy link

@kurshakuz kurshakuz left a comment

Choose a reason for hiding this comment

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

Congrats, finally all issues are resolved! Now I have reviewed the code itself and have some points to cover. Do you mind to take a look? If we fix them, the PR is ready to go

@@ -20,6 +20,15 @@ find_package(rclcpp REQUIRED)
find_package(tf2 REQUIRED)
find_package(tf2_ros REQUIRED)
find_package(turtlesim REQUIRED)
find_package(tf2_geometry_msgs REQUIRED)
find_package(message_filters REQUIRED)

Choose a reason for hiding this comment

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

can you sort these statements in an alphabetical order?

tf2
tf2_ros
tf2_geometry_msgs
message_filters

Choose a reason for hiding this comment

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

we need these ones to be sorted too

@@ -21,6 +21,8 @@
<depend>tf2</depend>
<depend>tf2_ros</depend>
<depend>turtlesim</depend>
<depend>tf2_geometry_msgs</depend>
<depend>message_filters</depend>

Choose a reason for hiding this comment

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

similarly, we store them in an alphabetical order

Comment on lines 15 to 27
#include <rclcpp/rclcpp.hpp>
#include <geometry_msgs/msg/point_stamped.hpp>

#include <tf2_ros/transform_listener.h>
#include <tf2_ros/message_filter.h>
#include <tf2_ros/buffer.h>
#include <tf2_ros/create_timer_ros.h>
#ifdef TF2_CPP_HEADERS
#include <tf2_geometry_msgs/tf2_geometry_msgs.hpp>
#else
#include <tf2_geometry_msgs/tf2_geometry_msgs.h>
#endif
#include <message_filters/subscriber.h>

Choose a reason for hiding this comment

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

Suggested change
#include <rclcpp/rclcpp.hpp>
#include <geometry_msgs/msg/point_stamped.hpp>
#include <tf2_ros/transform_listener.h>
#include <tf2_ros/message_filter.h>
#include <tf2_ros/buffer.h>
#include <tf2_ros/create_timer_ros.h>
#ifdef TF2_CPP_HEADERS
#include <tf2_geometry_msgs/tf2_geometry_msgs.hpp>
#else
#include <tf2_geometry_msgs/tf2_geometry_msgs.h>
#endif
#include <message_filters/subscriber.h>
#include <geometry_msgs/msg/point_stamped.hpp>
#include <message_filters/subscriber.h>
#include <rclcpp/rclcpp.hpp>
#include <tf2_ros/transform_listener.h>
#include <tf2_ros/message_filter.h>
#include <tf2_ros/buffer.h>
#include <tf2_ros/create_timer_ros.h>
#ifdef TF2_CPP_HEADERS
#include <tf2_geometry_msgs/tf2_geometry_msgs.hpp>
#else
#include <tf2_geometry_msgs/tf2_geometry_msgs.h>
#endif

it is also better to sort includes in a following manner

Copy link
Author

Choose a reason for hiding this comment

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

@kurshakuz Yes, I will sort them in all these files.

PoseDrawer()
: Node("turtle_tf2_pose_drawer")
{
auto node = rclcpp::Node::make_shared("tf2_ros_message_filter");

Choose a reason for hiding this comment

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

I see you are creating a Node instance two times. Is there any particular reason for that? One called "turtle_tf2_pose_drawer" and one "tf2_ros_message_filter". I see that in the code you are using node-> and this-> interchangeably, and this might be confusing to users. I recommend to stick with leaving the PoseDrawer() : Node("turtle_tf2_pose_drawer") call and using this-> on all onwards, when you need to call your node

Copy link
Author

Choose a reason for hiding this comment

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

@kurshakuz You're correct. The reason I used them interchangeably is that when I create the messagefilter object, tf2_filter_ = std::make_shared<tf2_ros::MessageFilter<geometry_msgs::msg::PointStamped>>(), the fifth argument need to be type ‘const SharedPtr&’ {aka ‘const std::shared_ptrrclcpp::Node&’}, while this pointer is type of ‘PoseDrawer*’, I don't know how to convert between them. Do you have any suggestion?

Choose a reason for hiding this comment

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

Did you do it like below?

tf2_filter_ = std::make_shared<tf2_ros::MessageFilter<geometry_msgs::msg::PointStamped>>(
      point_sub_, *tf2_buffer_, target_frame_, 10, this, buffer_timeout);

I am not quite getting it as the PoseDrawer actually inherits from the Node type. Can you try to build it as above? If it is still does not work, try looking for other APIs of MessageFilter initialization. Some of them require node clock rather than node itself:
https://docs.ros2.org/foxy/api/tf2_ros/classtf2__ros_1_1MessageFilter.html

Let me know if it still does not work

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that does not work, when I build the turtle_tf2_cpp package, it will get errors:
from /home/wangjg/geometry_tutorials/turtle_tf2_cpp/src/turtle_tf2_message_filter.cpp:15:
/usr/include/c++/9/ext/new_allocator.h:145:20: note: cannot convert ‘std::forward<PoseDrawer*>((* & __args#4))’ (type ‘PoseDrawer*’) to type ‘const SharedPtr&’ {aka ‘const std::shared_ptrrclcpp::node_interfaces::NodeLoggingInterface&’}
145 | noexcept(noexcept(::new((void *)__p)
| ^~~~~~~~~~~~~~~~~~
146 | _Up(std::forward<_Args>(__args)...)))
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/wangjg/geometry_tutorials/turtle_tf2_cpp/src/turtle_tf2_message_filter.cpp:21:
/home/wangjg/ros2_galactic_with_nav2/ros2_ws/install/tf2_ros/include/tf2_ros/message_filter.h:217:3: note: candidate: ‘template<class F, class TimeRepT, class TimeT> tf2_ros::MessageFilter<M, BufferT>::MessageFilter(F&, BufferT&, const string&, uint32_t, const SharedPtr&, std::chrono::duration<TimeRepT, TimeT>)’
217 | MessageFilter(
| ^~~~~~~~~~~~~

Copy link

@kurshakuz kurshakuz Sep 19, 2021

Choose a reason for hiding this comment

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

I just dived into that and found that currently tf2_ros::MessageFilter is lacking that capabilities. There is an open issue on that:
ros2/geometry2#95

I suggest to leave it that way for now, as we will still be able to make corresponding updates later

Copy link
Author

Choose a reason for hiding this comment

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

@kurshakuz this problem was resolved just now:
tf2_filter_ = std::make_shared<tf2_ros::MessageFilter<geometry_msgs::msg::PointStamped>>(
point_sub_, *tf2_buffer_, target_frame_, 10, this->get_node_logging_interface(),
this->get_node_clock_interface(), buffer_timeout);
I used rclcpp::node_interfaces:NodeLoggingInterface and rclcpp::node_interfaces:NodeClockInterface other than const rclcpp::Node::SharedPtr &, and it will work.

using std::placeholders::_1;
using namespace std::chrono_literals;


Choose a reason for hiding this comment

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

Suggested change


rclcpp::Clock::SharedPtr clock = std::make_shared<rclcpp::Clock>(RCL_SYSTEM_TIME);
tf2_buffer_ = std::make_shared<tf2_ros::Buffer>(node->get_clock());
tf2_buffer_->setCreateTimerInterface(create_timer_interface);

Choose a reason for hiding this comment

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

Can you please clarify why we need to create setCreateTimerInterface? I think a couple of inline comments would suffice

Copy link
Author

Choose a reason for hiding this comment

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

This is also because I create another node object except this pointer, and if I didn't setCreateTimerInterface, then when I run this node, it will raise a exception:
terminate called after throwing an instance of 'tf2_ros::CreateTimerInterfaceException'
what(): timer interface not set before call to waitForTransform
[ros2run]: Aborted
If we can resolve the two Node instance problem, I think we needn't to create setCreateTimerInterface.

Copy link
Author

Choose a reason for hiding this comment

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

@kurshakuz Still need setCreateTimerInterface after changing from two node instances to one node instance, and I added two inline comments for its purpose.

[install]
install-scripts=$base/lib/turtle_tf2_py
install_scripts=$base/lib/turtle_tf2_py

Choose a reason for hiding this comment

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

I am not sure if we need to modify these ones.

Copy link
Author

Choose a reason for hiding this comment

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

@kurshakuz yes, for rolling it would be "_", while for foxy, it may need to be "-". So does it have any influences?

request.x = float(4)
request.y = float(2)
request.theta = float(0)
self.client.call_async(request)

Choose a reason for hiding this comment

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

I think you have not seen that, as the fixing PR was made not long ago, but we have changed the way request is called in nodes. The issue was is that if the service is not available during that process, the node will got stuck in the __init__ phase and will not start spinning. What I suggest is to take a look at the update spawn syntax that we came up with and try to modify it in a similar manner:

https://github.com/ros/geometry_tutorials/blob/ros2/turtle_tf2_cpp/src/turtle_tf2_listener.cpp

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I will try to modify it in a similar manner.

Copy link
Author

Choose a reason for hiding this comment

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

@kurshakuz So I must spawn turtle3 in callback function? Then there will be problem:
self.sub = self.create_subscription(Pose, 'turtle3/pose', self.handle_turtle_pose, 1)
My callback will be called in the subscription, and require a positional argument 'msg', and when I launch this node, it will never spawn turtle3. How to resolve this problem?

Copy link
Author

Choose a reason for hiding this comment

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

@kurshakuz All other suggestion you made were resolved except for this.

Choose a reason for hiding this comment

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

I see your point. Do you think we then can make a separate timer call responsible for spawn service? Then we can make a flag that would indicate that the turtle is spawned and could start the subscription process?

Copy link
Author

Choose a reason for hiding this comment

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

@kurshakuz I have done it according to your good point, and now it will work.

sort include headers; change two node instances to one instance; change turtle3 spawning manner.
@kennywangaigraphx
Copy link
Author

@ahcorde I made some updates according to the suggestions on codes itself by @kurshakuz , and I built and run them in my computer, they can work as I expected. So would you please to start the tests and lints for 96ecf2b when you see this comment? Thank you very much.


self.timer = self.create_timer(1.0, self.on_timer)

def on_timer(self):

Choose a reason for hiding this comment

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

Can you take at the build test output?

  E     ./turtle_tf2_py/turtle_tf2_message_broadcaster.py:39:1: W293 blank line contains whitespace
  E     ./turtle_tf2_py/turtle_tf2_message_broadcaster.py:42:24: W291 trailing whitespace

Copy link
Author

Choose a reason for hiding this comment

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

@kurshakuz Sorry, I checked carefully for all the files to ensure there is no whitespace ,but I still neglected these two lines. I removed them just now.

remove whitespaces in line 39 and 42
Copy link

@kurshakuz kurshakuz left a comment

Choose a reason for hiding this comment

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

Some very last nitpicks and I think we can merge it. Great job!

seconds_type buffer_timeout(10);

tf2_buffer_ = std::make_shared<tf2_ros::Buffer>(this->get_clock());
// set timer interface before call to waitForTransform,

Choose a reason for hiding this comment

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

Suggested change
// set timer interface before call to waitForTransform,
// Create the timer interface before call to waitForTransform,


tf2_buffer_ = std::make_shared<tf2_ros::Buffer>(this->get_clock());
// set timer interface before call to waitForTransform,
// to avoid a tf2_ros::CreateTimerInterfaceException is thrown.

Choose a reason for hiding this comment

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

Suggested change
// to avoid a tf2_ros::CreateTimerInterfaceException is thrown.
// to avoid a tf2_ros::CreateTimerInterfaceException exception

Comment on lines 62 to 65
tf2_filter_->registerCallback(&PoseDrawer::msgCallback, this);
}

// Callback to register with tf2_ros::MessageFilter to be called when transforms are available

Choose a reason for hiding this comment

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

Suggested change
tf2_filter_->registerCallback(&PoseDrawer::msgCallback, this);
}
// Callback to register with tf2_ros::MessageFilter to be called when transforms are available
// Register a callback with tf2_ros::MessageFilter to be called when transforms are available
tf2_filter_->registerCallback(&PoseDrawer::msgCallback, this);
}

try {
tf2_buffer_->transform(*point_ptr, point_out, target_frame_);
RCLCPP_INFO(
this->get_logger(), "point of turtle 3 in frame of turtle 1 Position(x:%f y:%f z:%f)\n",

Choose a reason for hiding this comment

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

Suggested change
this->get_logger(), "point of turtle 3 in frame of turtle 1 Position(x:%f y:%f z:%f)\n",
this->get_logger(), "Point of turtle3 in frame of turtle1: x: %f y: %f z: %f\n",

point_out.point.z);
} catch (tf2::TransformException & ex) {
RCLCPP_WARN(
this->get_logger(), "Failure %s\n", ex.what()); // Print exception which was caught

Choose a reason for hiding this comment

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

Suggested change
this->get_logger(), "Failure %s\n", ex.what()); // Print exception which was caught
this->get_logger(), "Failure %s\n", ex.what()); // Print exception which was caught

Copy link
Author

Choose a reason for hiding this comment

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

@kurshakuz Here has 4 whitespace, I removed 2 whitespaces, and left 2 whitespaces, I think it need 2 whitespaces between comments and code, otherwise it will get wrong when linting.

Copy link
Author

Choose a reason for hiding this comment

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

@kurshakuz To ensure it can pass the test and linting, I moved the comment line to a new line before the code line.

Comment on lines 55 to 62
tf2_buffer_->setCreateTimerInterface(timer_interface);
tf2_listener_ =
std::make_shared<tf2_ros::TransformListener>(*tf2_buffer_);
point_sub_.subscribe(this, "/turtle3/turtle_point_stamped");
tf2_filter_ = std::make_shared<tf2_ros::MessageFilter<geometry_msgs::msg::PointStamped>>(
point_sub_, *tf2_buffer_, target_frame_, 100, this->get_node_logging_interface(),
this->get_node_clock_interface(), buffer_timeout);
tf2_filter_->registerCallback(&PoseDrawer::msgCallback, this);

Choose a reason for hiding this comment

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

I think it would be nice to have some empty lines between separate tools, like:

Suggested change
tf2_buffer_->setCreateTimerInterface(timer_interface);
tf2_listener_ =
std::make_shared<tf2_ros::TransformListener>(*tf2_buffer_);
point_sub_.subscribe(this, "/turtle3/turtle_point_stamped");
tf2_filter_ = std::make_shared<tf2_ros::MessageFilter<geometry_msgs::msg::PointStamped>>(
point_sub_, *tf2_buffer_, target_frame_, 100, this->get_node_logging_interface(),
this->get_node_clock_interface(), buffer_timeout);
tf2_filter_->registerCallback(&PoseDrawer::msgCallback, this);
tf2_buffer_->setCreateTimerInterface(timer_interface);
tf2_listener_ =
std::make_shared<tf2_ros::TransformListener>(*tf2_buffer_);
point_sub_.subscribe(this, "/turtle3/turtle_point_stamped");
tf2_filter_ = std::make_shared<tf2_ros::MessageFilter<geometry_msgs::msg::PointStamped>>(
point_sub_, *tf2_buffer_, target_frame_, 100, this->get_node_logging_interface(),
this->get_node_clock_interface(), buffer_timeout);
tf2_filter_->registerCallback(&PoseDrawer::msgCallback, this);

Copy link
Author

Choose a reason for hiding this comment

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

@kurshakuz Yes, you're right. I added an empty line to separate them.

Copy link
Author

Choose a reason for hiding this comment

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

@kurshakuz By the way, really it seems that we didn't use the tf2_geometry_msgs, but I tested if I remove it, it will get wrong when building the cpp package. And in the ROS1 version of this tutorial, there also had this header, so I think we need it.

Copy link
Author

Choose a reason for hiding this comment

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

The error looks like this:
try_tutorials/turtle_tf2_cpp$ colcon build --symlink-install
Starting >>> turtle_tf2_cpp
--- stderr: turtle_tf2_cpp
/usr/bin/ld: CMakeFiles/turtle_tf2_message_filter.dir/src/turtle_tf2_message_filter.cpp.o: in function geometry_msgs::msg::PointStamped_<std::allocator<void> >& tf2_ros::BufferInterface::transform<geometry_msgs::msg::PointStamped_<std::allocator<void> > >(geometry_msgs::msg::PointStamped_<std::allocator<void> > const&, geometry_msgs::msg::PointStamped_<std::allocator<void> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::chrono::duration<long, std::ratio<1l, 1000000000l> >) const': turtle_tf2_message_filter.cpp:(.text._ZNK7tf2_ros15BufferInterface9transformIN13geometry_msgs3msg13PointStamped_ISaIvEEEEERT_RKS7_S8_RKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEENSt6chrono8durationIlSt5ratioILl1ELl1000000000EEEE[_ZNK7tf2_ros15BufferInterface9transformIN13geometry_msgs3msg13PointStamped_ISaIvEEEEERT_RKS7_S8_RKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEENSt6chrono8durationIlSt5ratioILl1ELl1000000000EEEE]+0x5a): undefined reference to std::chrono::time_point<std::chrono::V2::system_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l> > > tf2::getTimestamp<geometry_msgs::msg::PointStamped<std::allocator > >(geometry_msgs::msg::PointStamped_<std::allocator > const&)'
/usr/bin/ld: turtle_tf2_message_filter.cpp:(.text.ZNK7tf2_ros15BufferInterface9transformIN13geometry_msgs3msg13PointStamped_ISaIvEEEEERT_RKS7_S8_RKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEENSt6chrono8durationIlSt5ratioILl1ELl1000000000EEEE[ZNK7tf2_ros15BufferInterface9transformIN13geometry_msgs3msg13PointStamped_ISaIvEEEEERT_RKS7_S8_RKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEENSt6chrono8durationIlSt5ratioILl1ELl1000000000EEEE]+0x7a): undefined reference to std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > tf2::getFrameId<geometry_msgs::msg::PointStamped_<std::allocator<void> > >(geometry_msgs::msg::PointStamped_<std::allocator<void> > const&)' /usr/bin/ld: turtle_tf2_message_filter.cpp:(.text._ZNK7tf2_ros15BufferInterface9transformIN13geometry_msgs3msg13PointStamped_ISaIvEEEEERT_RKS7_S8_RKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEENSt6chrono8durationIlSt5ratioILl1ELl1000000000EEEE[_ZNK7tf2_ros15BufferInterface9transformIN13geometry_msgs3msg13PointStamped_ISaIvEEEEERT_RKS7_S8_RKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEENSt6chrono8durationIlSt5ratioILl1ELl1000000000EEEE]+0xcf): undefined reference to void tf2::doTransform<geometry_msgs::msg::PointStamped<std::allocator > >(geometry_msgs::msg::PointStamped<std::allocator > const&, geometry_msgs::msg::PointStamped_<std::allocator >&, geometry_msgs::msg::TransformStamped_<std::allocator > const&)'
collect2: error: ld returned 1 exit status
make[2]: *** [CMakeFiles/turtle_tf2_message_filter.dir/build.make:192:turtle_tf2_message_filter] 错误 1
make[1]: *** [CMakeFiles/Makefile2:115:CMakeFiles/turtle_tf2_message_filter.dir/all] 错误 2
make: *** [Makefile:141:all] 错误 2

Failed <<< turtle_tf2_cpp [17.0s, exited with code 2]

Summary: 0 packages finished [17.2s]
1 package failed: turtle_tf2_cpp
1 package had stderr output: turtle_tf2_cpp

Copy link
Author

Choose a reason for hiding this comment

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

And I increased the queue size from 10 to 100:

tf2_filter_ = std::make_shared<tf2_ros::MessageFilter<geometry_msgs::msg::PointStamped>>(
point_sub_, *tf2_buffer_, target_frame_, 100, this->get_node_logging_interface(),
this->get_node_clock_interface(), buffer_timeout);

because if the queue size is small, there will have some warning outputs:

message filter dropping message: frame 'world' at time 1632119254.938 for reason 'discarding message because the queue is full'

Copy link
Author

Choose a reason for hiding this comment

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

@kurshakuz And I also remove this line under the includings:
using std::placeholders::_1;
I test we don't need that line, and it can still work as expected.

Copy link
Author

Choose a reason for hiding this comment

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

@kurshakuz I modified the cpp file according to your last nitpicks suggestion.

Choose a reason for hiding this comment

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

Thank you! Looks pretty good to me

Choose a reason for hiding this comment

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

@kurshakuz By the way, really it seems that we didn't use the tf2_geometry_msgs, but I tested if I remove it, it will get wrong when building the cpp package. And in the ROS1 version of this tutorial, there also had this header, so I think we need it.

Yes, indeed we need a tf2_geometry_msgs.hpp because it contains required conversion methods.

@kennywangaigraphx
Copy link
Author

@ahcorde I made some little updates according to linting results and suggestions of @kurshakuz , do you have any other suggestion? And please trigger testing and linting for my dd0f207 and 324b723. Thank you very much.

@ahcorde
Copy link
Contributor

ahcorde commented Sep 20, 2021

launched CI

@kennywangaigraphx
Copy link
Author

@kurshakuz @ahcorde All checks have passed.

@kennywangaigraphx
Copy link
Author

@kurshakuz @ahcorde So what should we do next?

@kurshakuz
Copy link

@kurshakuz @ahcorde So what should we do next?

In the meantime, I suggest you to add all those updates to your tutorial file. Additionally, take a look at Chris' comments there.

@ahcorde ahcorde requested a review from audrow September 21, 2021 16:07
@kennywangaigraphx
Copy link
Author

@kurshakuz Ok, I have updated my tutorial file just now according to your suggestion.

@ahcorde ahcorde merged commit 125c3dc into ros:ros2 Sep 22, 2021
@ahcorde
Copy link
Contributor

ahcorde commented Sep 22, 2021

Thank you for your contribution @kennywangaigraphx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants