feat: add autoware_command_gate and its tests#1012
feat: add autoware_command_gate and its tests#1012sasakisasaki wants to merge 2 commits intoautowarefoundation:mainfrom
autoware_command_gate and its tests#1012Conversation
|
Thank you for contributing to the Autoware project! 🚧 If your pull request is in progress, switch it to draft mode. Please ensure:
|
|
|
||
| EXPECT_TRUE(outputs.status.success); | ||
| EXPECT_EQ(outputs.status.code, 0); | ||
| EXPECT_EQ(outputs.status.message, "Switched to STOP"); |
There was a problem hiding this comment.
Perhaps we can omit tests for message check as it is kind of cosmetic.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1012 +/- ##
==========================================
+ Coverage 56.70% 56.83% +0.13%
==========================================
Files 375 378 +3
Lines 23276 23367 +91
Branches 10969 11005 +36
==========================================
+ Hits 13198 13280 +82
- Misses 7276 7282 +6
- Partials 2802 2805 +3
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Junya Sasaki <junya.sasaki@tier4.jp>
c4d4b94 to
7779e16
Compare
| namespace spec | ||
| { | ||
| struct ChangeToStop | ||
| { | ||
| using Service = autoware_adapi_v1_msgs::srv::ChangeOperationMode; | ||
| static constexpr char name[] = "/api/operation_mode/change_to_stop"; | ||
| }; | ||
|
|
||
| struct ChangeToAutonomous | ||
| { | ||
| using Service = autoware_adapi_v1_msgs::srv::ChangeOperationMode; | ||
| static constexpr char name[] = "/api/operation_mode/change_to_autonomous"; | ||
| }; | ||
|
|
||
| struct OperationModeState | ||
| { | ||
| using Message = autoware_adapi_v1_msgs::msg::OperationModeState; | ||
| static constexpr char name[] = "/api/operation_mode/state"; | ||
| }; | ||
|
|
||
| struct GearCommand | ||
| { | ||
| using Message = autoware_vehicle_msgs::msg::GearCommand; | ||
| static constexpr char name[] = "/control/command/gear_cmd"; | ||
| }; | ||
| } // namespace spec | ||
|
|
||
| namespace system | ||
| { | ||
| struct OperationModeState | ||
| { | ||
| using Message = autoware_adapi_v1_msgs::msg::OperationModeState; | ||
| static constexpr char name[] = "/system/operation_mode/state"; | ||
| }; | ||
| } // namespace system |
There was a problem hiding this comment.
These specs are duplicated from here:
Only the GearCommand is unique. Should they be reused instead?
| rclcpp::QoS state_qos(depth); | ||
| state_qos.reliable(); | ||
| state_qos.transient_local(); |
There was a problem hiding this comment.
QoS is manually constructed as rclcpp::QoS(1).reliable().transient_local() instead of using the spec-defined QoS (e.g. get_qos<T>()). Functionally correct today but will drift if specs change.
|
|
||
| target_include_directories(${PROJECT_NAME}_node | ||
| PUBLIC | ||
| include |
There was a problem hiding this comment.
| include |
There is no include folder.
| ament_add_gtest(test_ros_integration | ||
| test/integration/test_ros_integration.cpp | ||
| ) |
There was a problem hiding this comment.
| ament_add_gtest(test_ros_integration | |
| test/integration/test_ros_integration.cpp | |
| ) | |
| ament_add_ros_isolated_gtest(test_ros_integration | |
| test/integration/test_ros_integration.cpp | |
| ) |
I didn't test it but this might be a better way to do this as test numbers grow.
| <depend>autoware_vehicle_msgs</depend> | ||
| <depend>rclcpp</depend> |
There was a problem hiding this comment.
| <depend>autoware_vehicle_msgs</depend> | |
| <depend>rclcpp</depend> | |
| <depend>autoware_vehicle_msgs</depend> | |
| <depend>builtin_interfaces</depend> | |
| <depend>rclcpp</depend> |
command_gate_mode_builder.hpp directly includes <builtin_interfaces/msg/time.hpp> but it's not a direct <depend>. Works transitively but violates package isolation.
| foreach(target ${PROJECT_NAME}_node test_command_gate_mode_builder test_ros_integration) | ||
| if(TARGET ${target}) | ||
| target_compile_options(${target} PRIVATE -O0 -g --coverage) | ||
| target_link_options(${target} PRIVATE --coverage) |
There was a problem hiding this comment.
Consider removing this entire ENABLE_COVERAGE block (lines 63-100).
No other package in autoware_core has custom coverage CMake targets. The workspace already handles coverage collection:
- Build-time: CI uses the
coverage-gccmixin which injects--coverageviaCMAKE_CXX_FLAGSglobally to all packages. - Test-time: CI runs
colcon lcov-resultto capture gcov data workspace-wide. - Upload: The resulting
total_coverage.infois uploaded to Codecov.
This custom ENABLE_COVERAGE option is never set by CI (defaults to OFF), so the coverage_unit and coverage_integration targets are never created in CI. When the workspace-level --coverage flag is active, gcov data is already generated for this package's tests and collected by colcon lcov-result without any per-package opt-in.
The one argument for keeping it is local developer convenience (per-package HTML reports), but that's achievable with the standard workspace commands and is not a convention any other package follows. Keeping it risks future packages cargo-culting this pattern, fragmenting the build.
xmfcx
left a comment
There was a problem hiding this comment.
Thanks for the work on this, the package is well-structured and the test coverage is solid (both unit and integration tests for all four modes is great). The separation of the builder logic from the ROS node is clean, and the gating via launch_command_gate: false is the right call for avoiding conflicts with universe.
A few things to address before this can be merged:
-
Use existing interface specs (
autoware_command_gate.cpp:36-68): Four of the five locally-declared structs (ChangeToStop,ChangeToAutonomous,OperationModeState,system::OperationModeState) already exist inautoware_adapi_specs/operation_mode.hppandautoware_component_interface_specs/system.hpp. Reusing them avoids drift if topic names or QoS change upstream.GearCommandis the only one that's genuinely new. -
QoS from specs (
autoware_command_gate.cpp:84-86): Once the specs are used, the QoS (depth, reliability, durability) comes for free instead of being manually constructed. -
Remove phantom
includedir (CMakeLists.txt:14): Theincludedirectory doesn't exist. -
Add
builtin_interfacestopackage.xml: Directly included but not declared as a dependency. -
Remove custom coverage block (
CMakeLists.txt:63-100): No other package in autoware_core does this. The workspace-levelcoverage-gccmixin andcolcon lcov-resultalready handle everything. See inline comment for details. -
Consider
ament_add_ros_isolated_gtestfor the integration test to match the project convention.
The inline comments have more details on each point. Happy to discuss any of these if you have questions!
Description
Add the
autoware_command_gatepackage to the autoware.core control stack. This package provides a minimal gateway that exposes operation-mode change services (API and system) and publishes matching operation-mode state and gear commands. STOP requests map to gearPARK, AUTONOMOUS requests map to gearDRIVE, and LOCAL/REMOTE requests emit gearNONE. The launch file starts the node asautoware_command_gate_exe.Related links
This PR is the split PR that of:
autoware_command_gateand its tests #871How was this PR tested?
For tests, I used a branch based on this PR.
I tested using the planning simulation and scenario simulation workflow. Please see README, all the steps I tested are written there.
Quick Note:
README.md./api/operation_mode/state,/system/operation_mode/state, and/control/command/gear_cmdwere published.Notes for reviewers
/api/operation_mode/stateand/system/operation_mode/statewith reliable, transient-local QoS and emits a gear command on each service call.NONE).Interface changes
Topic changes
Additions and removals
/api/operation_mode/stateautoware_adapi_v1_msgs/msg/OperationModeState/system/operation_mode/stateautoware_adapi_v1_msgs/msg/OperationModeState/control/command/gear_cmdautoware_vehicle_msgs/msg/GearCommandService changes
Additions and removals
/api/operation_mode/change_to_stopautoware_adapi_v1_msgs/srv/ChangeOperationMode/api/operation_mode/change_to_autonomousautoware_adapi_v1_msgs/srv/ChangeOperationMode/system/operation_mode/change_operation_modeautoware_system_msgs/srv/ChangeOperationModeROS Parameter Changes
Additions and removals
Effects on system behavior
autoware_command_gatepackage and emitting consistent operation-mode state and gear outputs for API and system requests.