[ROS2 Porting] Meeting 2020/11/30

[mitsudome-r]

1. Go over previous meeting minutes.

• Investigate what would happen if we add gcc options and linters to a package -> @nnmm
  • From Josh: We use ament_lint_common which runs all of the linters listed here: https://github.com/ament/ament_lint/blob/master/ament_lint_common/package.xml
  • flake8 is giving an error. @nnmm will look deeper into it.
• @mitsudome-r asking about ROS2 driver for Livox lidar.
  • Currently, it wouldn't be a blocker for this week's integration test.

2. Discuss current blockers

• Parameters not available anymore in ROS2:
  • for now, these will be commented out to move forward with porting.
  • @mitsudome-r will check impact and make sure that they can be left out
• Nodelet Gone:
  • @TakaHoribe to test if he can port the nodelet to ros2.
• @fred-apex-ai will create and Issue on https://github.com/tier4/autoware_launcher.iv.universe/issues for anything that was left out from first ROS2 porting.

3. Other discussion:

• Tier IV will test integration on a vehicle this weekend.
  • [ROS2 Porting] Meeting 2020/11/30 #133 (comment)

[fred-apex-ai]

Current blockers

parameters not available anymore in ros2

<!-- velodyne_driver/DriverNodelet -->
<param name="scan_phase" value="$(var scan_phase)"/>
<param name="sensor_timestamp" value="$(var sensor_timestamp)" />

nodelet intention and missing parameters

also the cloud nodelet itself was turned into transform or convert node. Which one do we need?

<!-- velodyne_pointcloud/CloudNodelet -->
<param name="num_points_threshold" value="$(var num_points_threshold)"/>
<param name="invalid_intensity" value="$(var invalid_intensity)"/>
<param name="scan_phase" value="$(var scan_phase)"/>

nodelet gone

<!-- gone from velodyne repo -->
<node pkg="nodelet" exec="nodelet" name="$(var manager)_fix_distortion" args="load velodyne_pointcloud/InterpolateNodelet $(var manager)">
    <remap from="velodyne_points_ex" to="mirror_cropped/pointcloud_ex" />
    <remap from="velodyne_points_interpolate" to="rectified/pointcloud" />
    <remap from="velodyne_points_interpolate_ex" to="rectified/pointcloud_ex" />
  </node>

[nnmm]

Ament linters

How to use ament linters

See this document. The linters are executables wrapped by cmake packages.

Results for the pose_initializer package

I added that to the pose_inializer package and ran

colcon build --packages-up-to pose_initializer
colcon test --packages-select pose_initializer
colcon test-result --verbose

ament_cmake_copyright

It complains for all source files that it doesn't find a copyright notice, although there is one, because it doesn't understand /* */.
This can be fixed by running ament_copyright --add-missing 'Autoware Foundation' apache2 (or whatever license holder is appropriate) and then deleting the existing license. That's probably not too hard to do in a batch operation as long as the existing licenses all look the same.

ament_cppcheck

It prints

- cppcheck
  <<< failure message
    -- run_test.py: invoking following command in '/home/nikolai.morin/t4/src/autoware/pilot.auto/localization/util/pose_initializer':
     - /opt/ros/foxy/bin/ament_cppcheck --xunit-file /home/nikolai.morin/t4/build/pose_initializer/test_results/pose_initializer/cppcheck.xunit.xml --include_dirs /home/nikolai.morin/t4/src/autoware/pilot.auto/localization/util/pose_initializer/include
    [include/pose_initializer/pose_initializer_core.h:33]: (error: syntaxError) Code 'classPoseInitializer:' is invalid C code. Use --std or --language to configure the language.
    1 errors
    -- run_test.py: return code 1
    -- run_test.py: verify result file '/home/nikolai.morin/t4/build/pose_initializer/test_results/pose_initializer/cppcheck.xunit.xml'
  >>>

This is not very readable – there is a lot of irrelevant stuff in there. The cause of the error is that the file is assumed to be C and not C++ because of the file name extension. This can easily be fixed in a batch rename operation.

ament_cpplint

This one has a lot of output, so I split it into error categories:

Requiring header guards instead of #pragma once

I didn't find a tool to generate those automatically, so it would potentially mean a moderate amount of work for very little gain, at least where #pragma once is already there.

Ordering of #includes

The pose_initializer_core.hpp has the following includes:

#include <memory>
#include <string>

#include <rclcpp/rclcpp.hpp>
// more includes

On the rclcpp line, cpplint outputs this very misleading message: Found C system header after C++ system header. Should be: pose_initializer_core.h, c system, c++ system, other.
Here's what's wrong with that:

• there is no pose_initializer_core.h, we're already in the header file
• rclcpp/rclcpp.hpp is not a C system header. cpplint thinks angle brackets = system header.
• I don't think the order should be like that: If we include system headers after the headers of other Autoware/in-house packages, we can guard a bit better against missing includes in those. See this article for the rationale. It is a very frequent problem in the codebase that there are superfluous and missing headers.

Changing angle brackets of .h or .hpp includes to quotes should be not too much work, given that C system headers are rarely used in the codebase. But reordering the includes is more work.

Include what you use

Tells you to add missing C++ system headers. A moderate amount of work, because it is manual and probably affects most packages, but done in a few minutes per package. It doesn't bring that much of a quality improvement because it doesn't tell you everything that's used – e.g. it didn't pick up on ostream, and if a build breaks because of this, it will be pretty obvious and easy to fix.

TODOs

There are a bunch of Missing username in TODO; it should look like "// TODO(my_username): Stuff. – a very good check in my opinion, since the code base has many // TODO comments that do not even explain what the TODO is. When the code base transitions to AutowareAuto, it should be self-contained and not rely on access to the original authors.

Fixing that check would probably require pinging a lot of the original authors. Before that, we should clarify whether we should put GitHub usersnames, GitLab usernames, email addresses or real names in there.

Other checks

I didn't see documentation on what things cpplint actually checks, maybe we need to read the source code. Several other things in there seem like a moderate amount of work.

ament_uncrustify

This had no output, because I think I already ran it on the original code.

I remember being annoyed with ament_uncrustify before, because ament_uncrustify --reformat doesn't fix all the things it complains about, but I can't find a specific example.

ament_xmllint

I think this pointed out important fixes in a previous package I ported, but for this one it pointed out nothing.

Further linters

• ament_pclint: This is a commercial tool
• ament_clang_format: This is a configurable formatter that ships with a preset for the Google Style guide. I used it before and liked it, but I don't know how it compares to uncrustify.

Alternatives

clang-tidy

The ament_clang_tidy package seems to do nothing, but it can be used manually:

• Build with --cmake-args -DCMAKE_EXPORT_COMPILE_COMMANDS=1
• In the root directory, execute clang-tidy <cpp file paths> -p build/compile_commands.json

It is sometimes slow, but can be extremely valuable. In pose_initializer it found nothing, but that is a tiny package. In surround_obstacle_checker however it pointed out that in rclcpp::Duration(0.5), 0.5 is rounded to 0 – it accepts nanoseconds, not seconds, so this was a bug. It also pointed out an unnecessary copy, an unused argument to a function, and the use of potentially-moved-from objects.

Address sanitizer, UB sanitizer, thread sanitizer

In theory, you could compile your tests with these, just add --cmake-args -DCMAKE_CXX_FLAGS="-fsanitize=<address/undefined/thread>" to colcon build. However, they all report errors already for the minimal pub-sub example. I don't know if this is due to actual bugs or false positives. At least TSan can not be used without recompiling ROS2:

ThreadSanitizer generally requires all code to be compiled with -fsanitize=thread. If some code (e.g. dynamic libraries) is not compiled with the flag, it can lead to false positive race reports, false negative race reports and/or missed stack frames in reports depending on the nature of non-instrumented code.

Therefore I don't think they can be recommended without further investigation (e.g. if it's possible to disable errors coming from outside the current package).

todocheck

https://github.com/preslavmihaylov/todocheck links TODO comments with GitLab issues, which would be even better than just having usernames in there. That's difficult to do while we're still transitioning from GitHub to GitLab though, and it would also be a lot of work.

Summary

There are four code linters/formatters: ament_cppcheck, ament_cpplint, ament_uncrustify and ament_clang_format. While it is in my opinion a bit awkward to have four separate tools for that, and they are mutually incompatible for some corner cases (1 and 2), they generally help. Getting linters to pass takes probably less than 10 minutes for a small package like pose_initializer, but adds up to a lot of work for the whole code base.

Not that satisfying these linters does not mean at all that the code follows the style guide that we use, which is a slightly modified Google C++ Style guide (because that's what AutowareAuto uses, because that's what ROS uses). There are things that are addressed by none of them, e.g. that code should live in a namespace, which we should still do.

clang-tidy is more powerful since it actually builds your code. I don't know if it's fast enough to run as part of all tests, but it seems worth at least to go through all errors it reports once and fixing them – my impression is that the ratio of effort to quality is pretty good. Maybe @fred-apex-ai can comment more on it since he has used it in the past.

My suggestion is to try to batch-migrate license comments, header filenames, and #include style. We should probably also track all TODO comments in an issue to add explanations and assignees. Then we can go through all packages and fix lint issues and run clang-tidy. And of course, there is also tests, a good overall design, build quality, and documentation etc. that we should try to improve as part of code quality efforts.

[fred-apex-ai]

Requiring header guards instead of #pragma once
I didn't find a tool to generate those automatically, so it would potentially mean a moderate amount of work for very little gain, at least where #pragma once is already there.

@nnmm There is a good tool that I have used for this purpose in the past and on at least one package during porting until I decided this should be done in a batch operation later. It's https://github.com/cgmb/guardonce. THe workflow would be to first convert everything to #pragma once to have a common ground, then convert to include guards based on the path in the repo. But we can/should do that in autoware.auto when the position in the directory structure would change again. Pseudocode

for each header:
    guard2once header
   once2guard header -p "format based on file name"