Standard bt plugins support #627
Conversation
WalkthroughAdds a utility to build BT::RosNodeParams from a BehaviorTree blackboard and updates six BT plugin node constructors to use it; registers those nodes with BehaviorTree.CPP; augments tests to cover both blackboard-based and parameterized loading; exports plugin libraries in CMake and adds two LED animation IDs to the LightsManager blackboard. Changes
Sequence Diagram(s)sequenceDiagram
participant Factory as BT Factory
participant PluginCtor as Plugin Constructor
participant Utils as behavior_tree_utils
participant BB as Blackboard
participant Node as rclcpp::Node (shared_ptr)
Note over Factory,PluginCtor: New flow for constructors with (name, conf)
Factory->>PluginCtor: instantiate(name, conf)
PluginCtor->>Utils: CreateRosNodeParamsFromBlackboard(conf)
Utils->>BB: access NodeConfig blackboard
BB-->>Utils: blackboard ptr
Utils->>BB: lookup "node" key
BB-->>Node: return rclcpp::Node::SharedPtr (or null)
alt node valid
Utils->>PluginCtor: return BT::RosNodeParams(with node)
PluginCtor->>PluginCtor: initialize with extracted params
else missing/null
Utils-->>PluginCtor: throw (runtime_error / BT::RuntimeError)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
husarion_ugv_manager/test/test_behavior_tree_utils.cpp (1)
155-169: Consider ROS lifecycle management in test fixtures.The test calls
rclcpp::init()andrclcpp::shutdown()inline. If multiple tests initialize ROS or if test execution order varies, this could lead to conflicts. Consider using a test fixture withSetUp()andTearDown()methods to manage the ROS lifecycle consistently, or check if ROS is already initialized before callinginit().husarion_ugv_manager/test/plugins/condition/test_check_string_msg.cpp (1)
47-49: Consider documenting the Initialize() pattern.The empty constructor with a separate
Initialize()method is a common pattern, but it would benefit from a brief comment explaining why initialization is deferred (to support both test setup patterns).Example documentation:
public: + // Empty constructor; call Initialize() to set up node registration and publisher TestCheckStringMsg() {}husarion_ugv_manager/include/husarion_ugv_manager/plugins/action/call_set_led_animation_service_node.hpp (1)
34-38: Consider documenting the constructor's purpose.The new constructor enables usage in non-ROS plugin libraries (as per PR objectives), but this isn't documented. Consider adding a brief comment explaining when to use this constructor vs. the parameterized one.
Example documentation:
+ /** + * @brief Constructor for use in non-ROS plugin libraries. + * Extracts ROS node parameters from the blackboard. + * @param name Node name + * @param conf Node configuration containing blackboard with "node" entry + */ CallSetLedAnimationService(const std::string & name, const BT::NodeConfig & conf)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
husarion_ugv_manager/CMakeLists.txt(1 hunks)husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_utils.hpp(1 hunks)husarion_ugv_manager/include/husarion_ugv_manager/plugins/action/call_set_bool_service_node.hpp(1 hunks)husarion_ugv_manager/include/husarion_ugv_manager/plugins/action/call_set_led_animation_service_node.hpp(1 hunks)husarion_ugv_manager/include/husarion_ugv_manager/plugins/action/call_trigger_service_node.hpp(1 hunks)husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_bool_msg.hpp(1 hunks)husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_joy_msg.hpp(1 hunks)husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_string_msg.hpp(1 hunks)husarion_ugv_manager/src/lights_manager_node.cpp(1 hunks)husarion_ugv_manager/src/plugins/action/call_set_bool_service_node.cpp(1 hunks)husarion_ugv_manager/src/plugins/action/call_set_led_animation_service_node.cpp(1 hunks)husarion_ugv_manager/src/plugins/action/call_trigger_service_node.cpp(1 hunks)husarion_ugv_manager/src/plugins/condition/check_bool_msg.cpp(1 hunks)husarion_ugv_manager/src/plugins/condition/check_joy_msg.cpp(1 hunks)husarion_ugv_manager/src/plugins/condition/check_string_msg.cpp(1 hunks)husarion_ugv_manager/test/plugins/action/test_call_set_bool_service_node.cpp(2 hunks)husarion_ugv_manager/test/plugins/action/test_call_set_led_animation_service_node.cpp(2 hunks)husarion_ugv_manager/test/plugins/action/test_call_trigger_service_node.cpp(2 hunks)husarion_ugv_manager/test/plugins/condition/test_check_bool_msg.cpp(3 hunks)husarion_ugv_manager/test/plugins/condition/test_check_joy_msg.cpp(5 hunks)husarion_ugv_manager/test/plugins/condition/test_check_string_msg.cpp(8 hunks)husarion_ugv_manager/test/test_behavior_tree_utils.cpp(1 hunks)husarion_ugv_manager/test/utils/plugin_test_utils.hpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
husarion_ugv_manager/test/plugins/condition/test_check_string_msg.cpp (3)
husarion_ugv_manager/test/plugins/condition/test_check_bool_msg.cpp (4)
msg(51-51)msg(51-51)Initialize(57-61)Initialize(57-57)husarion_ugv_manager/test/plugins/condition/test_check_joy_msg.cpp (4)
msg(52-52)msg(52-52)Initialize(62-66)Initialize(62-62)husarion_ugv_manager/test/utils/plugin_test_utils.hpp (2)
RegisterNodeWithoutParams(239-242)RegisterNodeWithoutParams(239-239)
husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_bool_msg.hpp (1)
husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_utils.hpp (2)
CreateRosNodeParamsFromBlackboard(89-105)CreateRosNodeParamsFromBlackboard(89-89)
husarion_ugv_manager/test/plugins/condition/test_check_bool_msg.cpp (3)
husarion_ugv_manager/test/plugins/condition/test_check_string_msg.cpp (5)
data(50-50)msg(51-51)msg(51-51)Initialize(57-61)Initialize(57-57)husarion_ugv_manager/test/plugins/condition/test_check_joy_msg.cpp (4)
msg(52-52)msg(52-52)Initialize(62-66)Initialize(62-62)husarion_ugv_manager/test/utils/plugin_test_utils.hpp (2)
RegisterNodeWithoutParams(239-242)RegisterNodeWithoutParams(239-239)
husarion_ugv_manager/test/plugins/action/test_call_set_bool_service_node.cpp (1)
husarion_ugv_manager/test/utils/plugin_test_utils.hpp (2)
RegisterNodeWithoutParams(239-242)RegisterNodeWithoutParams(239-239)
husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_string_msg.hpp (1)
husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_utils.hpp (2)
CreateRosNodeParamsFromBlackboard(89-105)CreateRosNodeParamsFromBlackboard(89-89)
husarion_ugv_manager/include/husarion_ugv_manager/plugins/action/call_set_bool_service_node.hpp (1)
husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_utils.hpp (2)
CreateRosNodeParamsFromBlackboard(89-105)CreateRosNodeParamsFromBlackboard(89-89)
husarion_ugv_manager/test/plugins/condition/test_check_joy_msg.cpp (3)
husarion_ugv_manager/test/plugins/condition/test_check_bool_msg.cpp (2)
Initialize(57-61)Initialize(57-57)husarion_ugv_manager/test/plugins/condition/test_check_string_msg.cpp (2)
Initialize(57-61)Initialize(57-57)husarion_ugv_manager/test/utils/plugin_test_utils.hpp (2)
RegisterNodeWithoutParams(239-242)RegisterNodeWithoutParams(239-239)
husarion_ugv_manager/test/utils/plugin_test_utils.hpp (2)
husarion_ugv_manager/test/plugins/decorator/test_tick_after_timeout_node.cpp (3)
plugin_name(30-31)BuildBehaviorTree(34-50)BuildBehaviorTree(34-36)husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_manager.hpp (2)
tree_(93-93)tree_(96-96)
husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_joy_msg.hpp (1)
husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_utils.hpp (2)
CreateRosNodeParamsFromBlackboard(89-105)CreateRosNodeParamsFromBlackboard(89-89)
husarion_ugv_manager/test/test_behavior_tree_utils.cpp (1)
husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_utils.hpp (2)
CreateRosNodeParamsFromBlackboard(89-105)CreateRosNodeParamsFromBlackboard(89-89)
husarion_ugv_manager/include/husarion_ugv_manager/plugins/action/call_trigger_service_node.hpp (1)
husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_utils.hpp (2)
CreateRosNodeParamsFromBlackboard(89-105)CreateRosNodeParamsFromBlackboard(89-89)
husarion_ugv_manager/test/plugins/action/test_call_set_led_animation_service_node.cpp (1)
husarion_ugv_manager/test/utils/plugin_test_utils.hpp (2)
RegisterNodeWithoutParams(239-242)RegisterNodeWithoutParams(239-239)
husarion_ugv_manager/test/plugins/action/test_call_trigger_service_node.cpp (1)
husarion_ugv_manager/test/utils/plugin_test_utils.hpp (2)
RegisterNodeWithoutParams(239-242)RegisterNodeWithoutParams(239-239)
husarion_ugv_manager/include/husarion_ugv_manager/plugins/action/call_set_led_animation_service_node.hpp (1)
husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_utils.hpp (2)
CreateRosNodeParamsFromBlackboard(89-105)CreateRosNodeParamsFromBlackboard(89-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run unit tests hardware build type
- GitHub Check: Run unit tests simulation build type
🔇 Additional comments (31)
husarion_ugv_manager/CMakeLists.txt (1)
318-318: Correct export of plugin libraries for external consumption.This addition properly exports all BT plugin libraries using the standard ROS2/ament mechanism. This enables downstream packages (particularly those without ROS plugin infrastructure support, like nav2) to link against these plugins directly and use them when a node reference is provided via the BehaviorTree blackboard. The placement is correct and consistent with the existing export pattern.
husarion_ugv_manager/src/lights_manager_node.cpp (1)
146-147: LGTM—enum values confirmed.Verification passed. Both
GOAL_FAILED(line 17) andFLOOD_LIGHT(line 18) are properly defined inhusarion_ugv_msgs/msg/LEDAnimation.msg. The additions follow the existing pattern correctly and pose no issues.husarion_ugv_manager/src/plugins/action/call_set_bool_service_node.cpp (1)
50-54: LGTM!The BT factory registration complements the existing ROS plugin registration, enabling the node to be loaded in systems that don't support ROS plugin libraries. The registration is correctly structured and consistent with the PR objectives.
husarion_ugv_manager/src/plugins/condition/check_bool_msg.cpp (1)
36-40: LGTM!The BT factory registration follows the same pattern as other nodes in the PR, enabling dual loading paths (ROS plugin and standard BT plugin).
husarion_ugv_manager/src/plugins/action/call_trigger_service_node.cpp (1)
45-49: LGTM!Consistent BT factory registration pattern applied.
husarion_ugv_manager/src/plugins/condition/check_string_msg.cpp (1)
44-48: LGTM!Registration pattern is consistent with other nodes in the PR.
husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_joy_msg.hpp (1)
37-41: LGTM!The new constructor enables blackboard-based initialization by delegating to the base class with parameters extracted via
CreateRosNodeParamsFromBlackboard. This pattern is consistent with similar changes in other condition nodes (CheckBoolMsg, CheckStringMsg) and supports the PR objective of enabling standard BT plugin loading.husarion_ugv_manager/test/plugins/action/test_call_set_bool_service_node.cpp (2)
46-53: LGTM!Test renamed for clarity to distinguish ROS plugin loading from standard BT plugin loading paths.
55-65: LGTM!The new test exercises the standard BT plugin loading path using blackboard-based initialization. The test correctly:
- Registers the node without explicit RosNodeParams
- Creates a blackboard and injects the ROS node pointer
- Verifies tree creation succeeds with blackboard-based initialization
This complements the existing ROS plugin test and validates the dual loading capability introduced in this PR.
husarion_ugv_manager/src/plugins/condition/check_joy_msg.cpp (1)
94-98: LGTM!BT factory registration is consistent with other nodes in the PR.
husarion_ugv_manager/test/test_behavior_tree_utils.cpp (2)
126-132: LGTM!The test correctly verifies that a null blackboard throws
std::runtime_errorwith the expected message.
134-142: LGTM!The test properly validates that attempting to retrieve a missing "node" key from the blackboard results in
BT::RuntimeError.husarion_ugv_manager/test/plugins/action/test_call_set_led_animation_service_node.cpp (2)
53-62: LGTM!Renaming the test to
GoodLoadingCallSetLedAnimationServiceRosPluginclearly distinguishes it from the new standard plugin loading test, improving test clarity.
64-76: LGTM!The new test properly validates the blackboard-based plugin loading path, registering the node without parameters and injecting the ROS node via the blackboard. This complements the ROS plugin loading test and ensures both initialization paths work correctly.
husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_string_msg.hpp (1)
37-41: LGTM!The new constructor enables blackboard-based initialization by extracting ROS node parameters from the BehaviorTree blackboard. This allows the node to be registered with the BT factory without requiring explicit
RosNodeParams, complementing the existing constructor and supporting use cases where ROS plugin libraries are not available.husarion_ugv_manager/src/plugins/action/call_set_led_animation_service_node.cpp (1)
70-75: LGTM!The
BT_REGISTER_NODESblock enables standard BehaviorTree.CPP factory-based plugin registration, allowing the node to be loaded in environments that don't support ROS plugin libraries. This complements the existingCreateRosNodePluginregistration and achieves the PR's objective of supporting standard BT plugins.husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_bool_msg.hpp (1)
38-42: LGTM!The new constructor enables blackboard-based initialization, consistent with the pattern used across other plugin nodes in this PR. This allows
CheckBoolMsgto be registered with the BT factory and initialized from a blackboard-injected ROS node.husarion_ugv_manager/include/husarion_ugv_manager/plugins/action/call_trigger_service_node.hpp (1)
33-37: LGTM!The new constructor enables blackboard-based initialization for the trigger service action node, consistent with the pattern applied across other nodes in this PR. This supports factory-based registration and initialization without explicit
RosNodeParams.husarion_ugv_manager/include/husarion_ugv_manager/plugins/action/call_set_bool_service_node.hpp (1)
33-37: LGTM!The new constructor enables blackboard-based initialization for the SetBool service action node, maintaining consistency with the pattern used across all plugin nodes in this PR.
husarion_ugv_manager/test/plugins/condition/test_check_joy_msg.cpp (4)
49-66: LGTM!Extracting the initialization logic into a dedicated
Initialize()method improves test organization and allows explicit control over when node registration and publisher creation occur. This pattern is consistent with similar condition node tests.
80-96: LGTM!The test refactoring adds comprehensive coverage for both ROS plugin loading (with explicit parameters) and standard BT plugin loading (with blackboard-injected node). The renamed test clearly distinguishes between the two approaches.
98-138: LGTM!The tests are correctly updated to call
Initialize()before executing test logic, ensuring proper setup of the node and publisher.
140-204: LGTM!The
OnTickBehaviortest is correctly updated to use theInitialize()method while preserving its comprehensive validation of various message patterns and expected outcomes.husarion_ugv_manager/test/plugins/condition/test_check_string_msg.cpp (2)
70-86: LGTM! Good coverage of both plugin loading paths.These tests properly verify both ROS plugin loading (with parameters) and standard plugin loading (with blackboard). The blackboard setup correctly injects the node reference as required by the new constructor pattern.
90-90: Dismiss the review comment; no redundant Initialize() calls detected.Verification shows that each test method calls
Initialize()exactly once. The concern about redundant calls is unfounded—the current code already follows the pattern of callingInitialize()a single time per test method. No changes are required.Likely an incorrect or invalid review comment.
husarion_ugv_manager/include/husarion_ugv_manager/plugins/action/call_set_led_animation_service_node.hpp (1)
25-25: LGTM! Necessary include for the new constructor.The include is required for
behavior_tree_utils::CreateRosNodeParamsFromBlackboardused in the new constructor.husarion_ugv_manager/test/plugins/condition/test_check_bool_msg.cpp (2)
47-61: LGTM! Consistent Initialize() pattern.The Initialize() pattern matches the implementation in other test files (test_check_string_msg.cpp, test_check_joy_msg.cpp), providing a consistent approach across the test suite.
70-86: LGTM! Comprehensive plugin loading tests.Both test cases properly verify the dual loading paths:
GoodLoadingCheckBoolMsgRosPlugin: Uses parameterized registrationGoodLoadingCheckBoolMsgPlugin: Uses blackboard-based registrationhusarion_ugv_manager/test/plugins/action/test_call_trigger_service_node.cpp (2)
44-51: LGTM! Improved test naming clarity.The rename from
GoodLoadingCallTriggerServicePlugintoGoodLoadingCallTriggerServiceRosPluginclearly distinguishes this as the ROS plugin loading path test.
53-63: LGTM! Proper blackboard-based plugin loading test.The test correctly:
- Registers the node without parameters
- Creates a blackboard and injects the ROS node
- Verifies tree creation with the blackboard
This validates the standard plugin loading path enabled by the new constructor.
husarion_ugv_manager/test/utils/plugin_test_utils.hpp (1)
183-189: LGTM! Clean backward-compatible enhancement.The optional
blackboardparameter with a default value maintains backward compatibility while enabling explicit blackboard injection for testing both plugin loading paths. The implementation correctly passes the blackboard tofactory_.createTreeFromText.
husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_utils.hpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_utils.hpp(1 hunks)husarion_ugv_manager/test/test_behavior_tree_utils.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- husarion_ugv_manager/test/test_behavior_tree_utils.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run unit tests simulation build type
- GitHub Check: Run unit tests hardware build type
- GitHub Check: pre-commit / pre-commit
🔇 Additional comments (1)
husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_utils.hpp (1)
104-106: Past review comment successfully addressed.The nullptr validation for the node retrieved from the blackboard has been correctly implemented, preventing undefined behavior when constructing
BT::RosNodeParams.
Description
This makes it possible to include our custom BT nodes in other packages, that does not support ROS plugin libs (e.g. nav2). It requires to pass
nodeto the blackboard.Requirements
.reposTests 🧪
Summary by CodeRabbit
New Features
Chores
Tests