Autoware code style and guidelines

[isamu-takagi]

Autoware's code style is based on this page, but I think it needs some customization. So I'd like to discuss the changes of code style for Autoware here.

[isamu-takagi]

Function names are supposed to use snake_case. However, the following exceptions are needed when inheriting classes of external projects such as Qt for rviz plugins.

In a class that inherits a class from an external project, follow that naming convention.

[isamu-takagi]

autowarefoundation/autoware-documentation#225

[isamu-takagi]

According to this PR, constants using constexpr should be lower_snake_case.

[isamu-takagi]

autowarefoundation/autoware-documentation#225

[kenji-miyake]

A rationale for the include order: https://llvm.org/docs/CodingStandards.html#include-style

[isamu-takagi]

autowarefoundation/autoware-documentation#225

[kenji-miyake]

Private header files should be placed under src/, not include/.
What files to regard as private header files can be discussed. For normal use cases, most header files in an application package (that defines several nodes) can be private.

Reference: https://docs.ros.org/en/rolling/How-To-Guides/Ament-CMake-Documentation.html#adding-files-and-headers

if you are building a library, put all headers which should be usable by clients and therefore must be installed into a subdirectory of the include folder named like the package,
while all other files (.c/.cpp and header files which should not be exported) are inside the src folder.

[isamu-takagi]

Created the PR.
autowarefoundation/autoware-documentation#224

[kenji-miyake]

[suffixes of build artifacts]

• {node_name}: Node executable
• {node_name}_core: Core library (ROS independent)
• {node_name}_component: Node component lib
• {node_name}_node: Node lib (Not recommended)

Related: autowarefoundation/autoware.universe#1561 (comment)

[xmfcx]

@kenji-miyake This is totally unrelated topic but, how do you think we should handle logging debug/info messages for the ROS independent libraries?

We use RCLCPP_INFO, DEBUG, INFO, WARN, ERROR, FATAL if we have a rclcpp::Node but what should we do if we don't have it?

Should we use https://github.com/google/glog or something similar?

[kenji-miyake]

We need to define the style for acronyms in CamelCase.

• UserID or UserId
• RoutingAPI or RoutingApi
• etc.

Related:

• https://stackoverflow.com/questions/15526107/acronyms-in-camelcase
• https://google.github.io/styleguide/cppguide.html

Google's guide says:

For the purposes of the naming rules below, a "word" is anything that you would write in English without internal spaces. This includes abbreviations, such as acronyms and initialisms. For names written in mixed case (also sometimes referred to as "camel case" or "Pascal case"), in which the first letter of each word is capitalized, prefer to capitalize abbreviations as single words, e.g., StartRpc() rather than StartRPC().

Microsoft's guide says:

When using acronyms, use Pascal case or camel case for acronyms more than two characters long. For example, use HtmlButton or htmlButton. However, you should capitalize acronyms that consist of only two characters, such as System.IO instead of System.Io.

I personally think following Google's guide is simple.

I've tried to find a Clang-Tidy rule to fix the naming automatically, but I couldn't find it.
https://releases.llvm.org/14.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/readability-identifier-naming.html

[kenji-miyake]

Related: ros2/launch_ros follows PEP8 ros2/launch_ros#326

[kenji-miyake]

@isamu-takagi -san has proposed that "Since we can't distinguish the words if two acronyms are written successively, it's better to use RosApi rather than ROSAPI".
And as mentioned in the ROS 2's style guidelines, it's important also to consider consistency between programming languages, so I'll check it below.

Although Python follows PEP8, Rust says the following in https://rust-lang.github.io/api-guidelines/naming.html#casing-conforms-to-rfc-430-c-case.

In UpperCamelCase, acronyms and contractions of compound words count as one word: use Uuid rather than UUID, Usize rather than USize or Stdin rather than StdIn.

Considering Autonomous Driving should be a safe system, I feel it's better to align with Rust, which is a safe language, rather than Python.

So my opinion is, it's good to use the style of RosApi.

@xmfcx @yukkysaito @mitsudome-r Do you agree with this comment?

[kenji-miyake]

There was a good example: HDDSMARTInfoItem. cc @ito-san
https://github.com/autowarefoundation/autoware.universe/blob/3a4845c8b8d2115067579a33e2243732c1f6b6af/system/system_monitor/include/system_monitor/hdd_monitor/hdd_monitor.hpp#L111

It caused a spell-check error.

system/system_monitor/src/hdd_monitor/hdd_monitor.cpp:80:20 Unknown word (HDDSMART)

[isamu-takagi]

autowarefoundation/autoware-documentation#225

[xmfcx]

@kenji-miyake I always use CamelCase for acronyms, I didn't know there was a standard for it :D I am on the HddSmartInfoItem side.

[kenji-miyake]

We should discuss the guideline for ROS interface (topic/service/action) names.
autowarefoundation/autoware.universe#1431 (comment)

[kenji-miyake]

About the ROS message guidelines.
autowarefoundation/autoware_adapi_msgs#16 (comment)

[kenji-miyake]

Takagi-san's proposal.
autowarefoundation/autoware_adapi_msgs#16 (comment)

For an example, refer to https://github.com/autowarefoundation/autoware_adapi_msgs/blob/ac51d9cd88154dfc3d1de8ef8a0b3b4658bfee57/autoware_adapi_v1_msgs/planning/msg/SteeringFactor.msg.

@xmfcx @mitsudome-r @yukkysaito Do you agree?

[kenji-miyake]

TODO: Consider guidelines for message type names and field names.
tier4/tier4_autoware_msgs#61 (review)

[kenji-miyake]

About the naming convention for enum members.

ROS 2 uses UpperCamelCase, but it's not documented in the style guide.
https://docs.ros.org/en/rolling/The-ROS2-Project/Contributing/Code-Style-Language-Versions.html

rclcpp/rclcpp/include/rclcpp/qos.hpp
34:enum class HistoryPolicy
35-{
36-  KeepLast = RMW_QOS_POLICY_HISTORY_KEEP_LAST,
--
42:enum class ReliabilityPolicy
43-{
44-  BestEffort = RMW_QOS_POLICY_RELIABILITY_BEST_EFFORT,
--
51:enum class DurabilityPolicy
52-{
53-  Volatile = RMW_QOS_POLICY_DURABILITY_VOLATILE,
--
60:enum class LivelinessPolicy
61-{
62-  Automatic = RMW_QOS_POLICY_LIVELINESS_AUTOMATIC,
--
69:enum class QoSCompatibility
70-{
71-  Ok = RMW_QOS_COMPATIBILITY_OK,

In the ROS 1 guide, it uses UpperCamelCase, so I guess ROS 2 inherited this.
http://wiki.ros.org/CppStyleGuide

enum class Choise
{
    Choice1,
    Choice2,
    Choice3
};
Choise c = Choise::Choice1;

And Google uses UpperCamelCase with the prefix k.
https://google.github.io/styleguide/cppguide.html#Enumerator_Names

enum class UrlTableError {
  kOk = 0,
  kOutOfMemory,
  kMalformedInput,
};

In C++ Core Guidelines, ALL_CAPS is forbidden.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#enum5-dont-use-all_caps-for-enumerators

[kenji-miyake]

lowercase (snake_case or lowerCamelCase?) in cppreference.com. (and also in examples of C++ Core Guidelines)
https://en.cppreference.com/w/cpp/language/enum

[kenji-miyake]

CamelCase in Microsoft.
https://learn.microsoft.com/en-us/cpp/cpp/enumerations-cpp?view=msvc-170

[isamu-takagi]

autowarefoundation/autoware-documentation#225

[kenji-miyake]

@xmfcx Let's discuss https://github.com/orgs/autowarefoundation/discussions/2824#discussioncomment-3967388 in another thread.

how do you think we should handle logging debug/info messages for the ROS independent libraries?

We use RCLCPP_INFO, DEBUG, INFO, WARN, ERROR, FATAL if we have a rclcpp::Node but what should we do if we don't have it?

Should we use https://github.com/google/glog or something similar?

I personally think we can depend on rclcpp/logging.hpp in ROS-independent libraries, which is easy to remove later if necessary.
But I believe it's good to receive feedback from AutoCore members, who are working on integration with AutoCore.OS.

@Sharrrrk So, do you have any requests to logging in ROS-independent libraries.

[xmfcx]

Thank you, I will reproduce it and see if I have any other concerns. So far it looks well, thanks for showing this.

[xmfcx]

@kenji-miyake Ok I've tested it and just using RCLCPP_INFO(rclcpp::get_logger("dummy_name"), "log text"); logs into the console and it has equivalent functionality to the glog as far as I can see.

Using a common logging node

A global common name (e.g. autoware_library_logger) or A global library-specific name (e.g. foo_library_logger) can be used.

Do you think we should create a global logger node for libraries to output to /rosout?

We can launch it before the other nodes, if it runs, we can see logs on /rosout, if not, we will see them only on the console.

A node named: common_logging_rosout_node

Advantages

• see the logs on /rosout

Disadvantages

• if multiple instances are run together, on the console, different library outputs won't be differentiated (all will have same node name)
• we optionally launch common_logging_rosout_node, which will cause extra network traffic for /rosout

What to do?

Should we recommend logging for:

• Parameterized logger name input like here
• "library_name" for logger name
• "common_logging_rosout_node" for the logger name

[kenji-miyake]

Do you think we should create a global logger node for libraries to output to /rosout?

Ah, I'm sorry. I've checked the behavior, but it seems to be impossible to use a global logger node.
If we specify the same name, it can't find the corresponding node's logger.
For example, even if I change the logger name to talker and run ros2 run demo_nodes_cpp talker, it's not shown in /rosout.

@xmfcx Could you also check it on your side, please?

[xmfcx]

@kenji-miyake

I've also tried but came to same conclusion.

Logger get_logger(const std::string & name);

this method creates a logger which is not associated to any node.

Also I've did a bit of research,

Here: https://discourse.ros.org/t/ros2-logging/6469 under FAQ:

Why not send all logs to the /rosout topic instead of only those sent to loggers associated with nodes?

• Because of the way ROS 2 associates DDS concepts with ROS Nodes, there didn’t seem to be a clean way to setup a general topic on a process that wasn’t associated with an existing Node. Since a process can also contain multiple ROS Nodes, we didn’t think it would be good design to just pick one to have everything published to.

So this is a design decision made in the ROS2 it seems.

Also as far as I know, there is no way to access another node's logger just by the node's name.

I know that we can use SyncParametersClient to access another node's parameters by name as explained in this ros answer. But not its logger.

[xmfcx]

So we have 2 options to recommend:

1. Parameterized logger object like here
2. "library_name" for logger name

Or we can also just leave it optional.