feat: add autoware_control_center

[xmfcx]

Description

This PR introduces:

• autoware_control_center node
  • Capabilities:
    • Register/Deregister
    • Publish node reports
• autoware_control_center_msgs
  • Messages
    • NodeReport(s)
    • NodeStatusActivity
    • NodeStatusOperational
    • ResultDeRegistration
    • ResultRegistration
  • Services
    • Deregister
    • Register

Related links

Parent Issue

• Implement Autoware Node & Control Center #210

Parent PR:

• feat: add autoware_control_center and autoware_node #84

How was this PR tested?

Notes for reviewers

Please let me know if you have any questions, I'd be happy to explain.

Interface changes

None.

Effects on system behavior

None.

[github-actions]

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

• You've checked our contribution guidelines.
• Your PR follows our pull request guidelines.
• All required CI checks pass before marking the PR ready for review.

[codecov]

Codecov Report

Attention: Patch coverage is 72.49191% with 85 lines in your changes missing coverage. Please review.

Project coverage is 71.34%. Comparing base (4cb18f5) to head (256cace).
Report is 52 commits behind head on main.

Files with missing lines	Patch %	Lines
...utoware_control_center/src/control_center_main.cpp	55.81%	11 Missing and 8 partials ⚠️
...toware_control_center/test/test_cc_registering.cpp	57.77%	3 Missing and 16 partials ⚠️
...utoware_control_center/src/control_center_node.cpp	83.56%	4 Missing and 8 partials ⚠️
common/autoware_node/src/node.cpp	57.14%	7 Missing and 5 partials ⚠️
common/autoware_node/test/test_an_registering.cpp	78.26%	0 Missing and 10 partials ⚠️
...mmon/autoware_control_center/test/test_utility.hpp	78.57%	3 Missing and 6 partials ⚠️
common/autoware_node/test/test_utility.hpp	78.57%	1 Missing and 2 partials ⚠️
...mmon/autoware_control_center/test/test_cc_init.cpp	92.30%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #191      +/-   ##
==========================================
- Coverage   78.75%   71.34%   -7.42%     
==========================================
  Files          11       11              
  Lines         193      335     +142     
  Branches       73      199     +126     
==========================================
+ Hits          152      239      +87     
- Misses         11       34      +23     
- Partials       30       62      +32     

Flag	Coverage Δ
differential	71.34% <72.49%> (?)
total	?

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[youtalk]

Before implementing, a PR should be created for message definitions only. Otherwise, the implementation may end up being wasted.
Additionally, autoware_core is not a suitable place for message definitions. If they are exposed externally, they should be in autoware_msgs, and if used internally only, they should be in autoware_internal_msgs.

[xmfcx]

Since this is in development stage and this is not used by anything yet, and won't be used anytime soon, let's keep it here. And once we solidify the implementation of autoware control center, then we can move it to autoware internal messages.

[youtalk]

Autoware Core is designed as a place to store only the minimal necessary functions and packages. Therefore, we don’t want it to be a place where unused features are added for the time being.
Wouldn’t it be better to develop on a feature branch?

[xmfcx]

@youtalk -san, I could develop this in a completely isolated repository too. But one way or another, it will be merged to Core. The reason it's being added here this way is to allow you to review it feature by feature.

Even in Core, we should allow for (and aim for) iterative development. I don't understand why you don't review it at its current stage.

I had also previously made it all in a single PR too, then you said to split the PRs feature by feature. Now I'm doing it to make it easier for the reviewers to understand.

What if this was a readily available external package? What if this was an existing Universe package?

We take them in without much questions or understanding how they work into the Core. No questions asked.

But for this package, I am ready here to explain and include you as part of the development process. But you are making this very difficult for me and I'm seriously losing my motivation to develop this package.

[mitsudome-r]

I think we just need to agree on the plans about migration to use this Autoware Node and Autoware Control Center node.
Our intention is that we use this as a base class for all the nodes in Autoware Core, but in order to ease the PR review process we can first merge it as a unused package.

@xmfcx Could you create an issue ticked to track the required steps and timeline for Autoware base class integration?

[xmfcx]

• @mitsudome-r created the steps in chronological order in Implement Autoware Node & Control Center #210

👍 Please let me know if this suits you and please provide your feedback.

[youtalk]

If development is not done step by step or feature by feature, the implemented program may end up being wasted.

For example, the heartbeat you are trying to add here would be more compatible with ROS 2 and would not require reimplementation if we use diagnostics_updater.
https://docs.ros.org/en/humble/p/diagnostic_updater/generated/classdiagnostic__updater_1_1Heartbeat.html

[xmfcx]

This PR is only for ACC, registration and deregistration, node reports.

Only extra is the heartbeat message is also here but I can remove it and we can discuss it when I create the pr for heartbeat after this is merged.

[xmfcx]

For example, the heartbeat you are trying to add here would be more compatible with ROS 2 and would not require reimplementation if we use diagnostics_updater.

I've specifically avoided https://github.com/ros2/common_interfaces/blob/humble/diagnostic_msgs/msg for heartbeats because they are very heavy.

https://github.com/ros/diagnostics/blob/83bf46f0839af793e66d3ff5dc2f2b44679bfd4d/diagnostic_updater/include/diagnostic_updater/update_functions.hpp#L429

The heartbeat message used there is an instance of diagnostic_updater::DiagnosticStatusWrapper which is an instance of diagnostic_msgs/msg/DiagnosticStatus.msg.

It contains:

# Level of operation enumerated above.
byte level
# A description of the test/component reporting.
string name
# A description of the status.
string message
# A hardware unique string.
string hardware_id
# An array of values associated with the status.
KeyValue[] values

Has 3 strings and a potential array of string pairs. And it is expected to be part of diagnostic_msgs/msg/DiagnosticArray.msg since the time stamp is in the header there.

Compared to this, my message is smaller and static in size:

# Time stamp without frame string
builtin_interfaces/Time stamp

# 1 byte
unique_identifier_msgs/UUID uuid_node

# 2 bytes
uint16 sequence_number

# 1+1=2 bytes
autoware_control_center_msgs/NodeStatusActivity status_activity
autoware_control_center_msgs/NodeStatusOperational status_operational

ACC/AN system and the existing established ros diagnostics system is expected to interact with ACC in the future. But not for the ACC-AN communication.

We can discuss this in more detail when I make the HB PR.

[youtalk]

The diagnostics is a mechanism that has been adopted since ROS 1, and there are visualization tools available, making its implementation well-established. We want to avoid redeveloping a similar mechanism just to reduce message size slightly.

How about first trying an implementation using the diagnostics, and if performance issues arise, then considering a custom implementation?

[youtalk]

This is big PR and not a porting from Universe. So, if we don’t develop it little by little, we won’t be able to review it properly.

[xmfcx]

The diagnostics is a mechanism that has been adopted since ROS 1, and there are visualization tools available, making its implementation well-established. We want to avoid redeveloping a similar mechanism just to reduce message size slightly.

ACC can publish diagnostics status messages in addition to the node reports to communicate with existing ROS 2 tooling.

But ANs will talk to ACC with the minimal communication bandwidth overhead with this protocol.

This way, existing tooling can be used while keeping the actual internal communication lightweight and fast.

[xmfcx]

This is big PR and not a porting from Universe. So, if we don’t develop it little by little, we won’t be able to review it properly.

I did not add heartbeat feature in this PR. How much smaller can I make it?

The only feature implemented here is the Registration / Deregistration feature, that's it.

[youtalk]

If we use the diagnostics for the heartbeat, is this PR necessary? From reading #210, it mainly seemed to be about the heartbeat.

Again if I were to seriously review this PR, it would take me several hours. Except for simple replacements or porting cases, I can only review PRs with fewer than a few hundred lines.

[xmfcx]

If we use the diagnostics for the heartbeat, is this PR necessary?

No. This PR wouldn't be necessary. You could achieve similar features with these autoware.universe
/system nodes like:

• autoware_topic_state_monitor
  • Subscribes to topics to monitor. If topic is large, it has very large overhead. To be replaced with monitored subscribers.
• autoware_diagnostic_graph_aggregator
  • Subscribes to multiple diagnostic_msgs/msg/DiagnosticArray
  • Error states are aggregated with this node
  • Uses this autoware_launch/config/system/diagnostics
    /autoware-main.yaml
  • https://github.com/autowarefoundation/autoware_launch/blob/ea7b92d7338afd4282114742f98bf109ce474806/autoware_launch/config/system/diagnostics/control.yaml#L35-L38 uses some messages as heartbeat

The issue is that because they are heavy, we cannot make it available for every node. The https://github.com/ros2/common_interfaces/blob/humble/diagnostic_msgs/msg/DiagnosticStatus.msg doesnt allow you to add non-string values, you cannot have incrementing numbers to track if you skipped a heartbeat (unless you use string to store numbers :( )

I will prepare a simple benchmark to measure how much overhead (in terms of memory, bandwidth, cpu usage) and (in terms of how much the existing tools slow down with ros2 topic list etc too.)

Case for making this a base class

If we make this a base class and standardize the interface between ACC and AN, this heartbeat feature will be enabled by default for all of our nodes. So it won't be possible to forget implementing heartbeat monitoring feature. As long as we make sure all nodes inherit from this base class, they will register and publish heartbeats.

I can see a world where we make this into a library to include in nodes (though this would create its own publishers and services) and tracking them with tests. But this way of base class is the easiest to implement and enforce this new system.

Reviewing PRs with >100 lines

Can you propose how I should split this PR even more?

Anything I take out from this PR makes it dysfunctional. Or I can remove READMEs and you won't know how to use it.
Most the files are boilerplate cmakelists, package.xml, Apache2.0 header text etc.

I think you are exaggerating the magnitude of this PR.

But really, I'm open to cutting this down even more. But I don't know how.