Lights auto mode #610
Lights auto mode #610
Conversation
WalkthroughAdds a new Behavior Tree condition plugin CheckStringMsg (std_msgs/String) and integrates it into lights behavior trees, build, config, and tests; expands user animation documentation and usage examples; documents a new lights_manager subscriber; and removes joystick support from LightsManagerNode. Changes
Sequence Diagram(s)sequenceDiagram
participant LM as LightsManagerNode
participant BT as Behavior Tree
participant Check as CheckStringMsg (BT Node)
participant Source as twist_mux_controller/source (std_msgs/String)
participant LED as SetLedAnimation Service
LM->>BT: Tick
BT->>BT: Evaluate e_stop_state
alt e_stop active
BT->>LED: SetEStopAnimation
LED-->>BT: OK
else e_stop inactive
BT->>Check: onTick(topic=Source, data=expected)
Check->>Source: subscribe / await message
Source-->>Check: String(data)
Check-->>BT: SUCCESS or FAILURE
alt manual (SUCCESS)
BT->>LED: SetManualActionAnimation (if not already set)
LED-->>BT: OK
else autonomous (FAILURE)
BT->>LED: SetManualActionAnimation (AUTONOMOUS)
LED-->>BT: OK
BT->>LED: SetReadyAnimation
LED-->>BT: OK
end
end
BT-->>LM: Status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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
🔭 Outside diff range comments (1)
husarion_ugv_manager/test/test_lights_behavior_tree.cpp (1)
123-141: Add a focused test covering the new CheckStringMsg-based gating.Current tests don’t publish the string topic. Add a test that publishes representative sources (e.g., “manual”, “autonomous”) and asserts the expected animation or branch selection in the BT.
I can draft a new test case that:
- Creates a
std_msgs::msg::Stringpublisher on the configured topic.- Publishes “manual” and “autonomous” in separate runs.
- Asserts the corresponding LED animation IDs or state-specific outputs.
Would you like me to provide a concrete test snippet?
🧹 Nitpick comments (13)
husarion_ugv_lights/CONFIGURATION.md (4)
119-119: Heading level inconsistency: align with surrounding sections.All nearby sections under “LED Animations” use third-level headers (###). Consider changing “## Defining Animations” to “### Defining Animations” for structural consistency.
-## Defining Animations +### Defining Animations
195-201: Example should reference user-defined animation IDs, not reserved system IDs.You’re introducing user animations but the service call uses
id: 0(reserved/system). Use one of the example user IDs (e.g., 21–24) to make the example actionable.-ros2 service call /lights/set_animation husarion_ugv_msgs/srv/SetLEDAnimation "{animation: {id: 0, param: ''}, repeating: true}" +ros2 service call /lights/set_animation husarion_ugv_msgs/srv/SetLEDAnimation "{animation: {id: 21, param: ''}, repeating: true}"
189-193: Clarify path expectation for user_animations file.If the file lives in the workspace or a package, consider hinting that an absolute path is required or show an alternative using
$(find <pkg>)/path.yamlfor consistency with earlier examples.-ros2 launch husarion_ugv_bringup bringup.launch user_animations_file:=/my_awesome_user_animations.yaml +ros2 launch husarion_ugv_bringup bringup.launch user_animations_file:=/absolute/path/to/my_awesome_user_animations.yaml +# or, if stored in a package: +# ros2 launch husarion_ugv_bringup bringup.launch user_animations_file:=$(find <your_pkg>)/config/my_awesome_user_animations.yaml
125-182: Minor YAML hygiene suggestion: quote hex color values to avoid parser ambiguity.While many YAML parsers accept
0x..., quoting improves portability and readability.- color: 0xffff00 + color: "0xffff00"husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_string_msg.hpp (2)
31-41: Expose QoS as optional inputs (follow-up to FIXME).Since source topics may use different durability/reliability (e.g., transient_local for latched state), add optional ports (e.g., qos_reliability, qos_durability, qos_depth) or a single qos_profile enum/str, and forward them to the underlying subscription QoS in the .cpp.
I can propose a patch (header + .cpp) if you confirm the desired QoS knobs and defaults.
47-51: Consider ergonomics: allow multiple accepted values or case-insensitive match.Depending on upstream publishers (e.g., twist_mux naming), supporting a comma-separated list or case-insensitive compare can make the condition more robust.
Would you like a variant that accepts
data_list(CSV) andcase_sensitive(bool, default false)?husarion_ugv_manager/README.md (1)
34-35: Fix Markdown type annotation formatting.Missing trailing asterisk in the type formatting.
-- `twist_mux_controller/source` [*std_msgs/String]: Source of the command velocity. +- `twist_mux_controller/source` [*std_msgs/String*]: Source of the command velocity.husarion_ugv_manager/CMakeLists.txt (1)
170-175: Unit test target added for CheckStringMsg (LGTM) — minor improvement possible.Compiling the plugin source directly into the test is fine. Optionally, consider linking against the built shared library instead to validate the exported plugin artifact as used in production.
Apply this diff if you prefer linking the shared lib (keeping source in test is also acceptable):
ament_add_gtest( ${PROJECT_NAME}_test_check_string_msg - test/plugins/condition/test_check_string_msg.cpp - src/plugins/condition/check_string_msg.cpp) + test/plugins/condition/test_check_string_msg.cpp) list(APPEND plugin_tests ${PROJECT_NAME}_test_check_string_msg) +target_link_libraries(${PROJECT_NAME}_test_check_string_msg check_string_msg_bt_node)husarion_ugv_manager/behavior_trees/lights.xml (1)
110-139: State selection logic via nested IfThenElse is correct — consider naming and topic value verification.
- Logic: manual → MANUAL_ACTION, autonomous → AUTONOMOUS_ACTION, else → READY. Skipped entirely when e_stop_state is true. This meets the auto-mode goal.
- Nit: The action name "SetManualActionAnimation" is used in both manual and autonomous branches; consider renaming the second to "SetAutonomousActionAnimation" for clarity (cosmetic only).
- Verify: Ensure twist_mux_controller/source indeed publishes "manual" and "autonomous" literals. If upstream publishes arbitrary source names, consider mapping upstream values to these tokens or matching multiple allowed values.
Would you like a small mapping decorator or a parameterized list of aliases (e.g., manual: ["teleop", "joy", ...], autonomous: ["nav2", ...]) to make this more robust?
husarion_ugv_manager/src/plugins/condition/check_string_msg.cpp (2)
20-20: Remove unused include.behavior_tree_utils.hpp is not used here.
Apply this diff:
-#include "husarion_ugv_manager/behavior_tree_utils.hpp"
25-32: Optional: Add case-insensitive or trim support via a toggle.If upstream may vary casing or include incidental whitespace, consider optional normalization (e.g., ports: case_insensitive=true, trim=true). Not required for current usage.
husarion_ugv_manager/test/plugins/condition/test_check_string_msg.cpp (2)
117-128: "Mixed letters" test is an exact-case match, not case-insensitive.The test name could imply case-insensitive matching; however, it verifies identical case. Consider renaming to avoid confusion.
Apply this diff:
-TEST_F(TestCheckStringMsg, SuccessOnMatchWithMixedLetters) +TEST_F(TestCheckStringMsg, SuccessOnExactMatchWithMixedCase)
44-60: Publisher QoS and topic naming (optional).
- If the plugin node’s subscription uses the default QoS, test’s default QoS is sufficient. If the plugin expects transient_local or reliability changes, mirror QoS in the test publisher.
- Minor: Consider using a more representative topic name (e.g., "twist_mux_controller/source") in tests to mimic production.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (12)
husarion_ugv_lights/CONFIGURATION.md(1 hunks)husarion_ugv_manager/CMakeLists.txt(2 hunks)husarion_ugv_manager/README.md(1 hunks)husarion_ugv_manager/behavior_trees/LightsBT.btproj(1 hunks)husarion_ugv_manager/behavior_trees/lights.xml(2 hunks)husarion_ugv_manager/config/lights_manager.yaml(1 hunks)husarion_ugv_manager/include/husarion_ugv_manager/lights_manager_node.hpp(0 hunks)husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_string_msg.hpp(1 hunks)husarion_ugv_manager/src/lights_manager_node.cpp(0 hunks)husarion_ugv_manager/src/plugins/condition/check_string_msg.cpp(1 hunks)husarion_ugv_manager/test/plugins/condition/test_check_string_msg.cpp(1 hunks)husarion_ugv_manager/test/test_lights_behavior_tree.cpp(1 hunks)
💤 Files with no reviewable changes (2)
- husarion_ugv_manager/include/husarion_ugv_manager/lights_manager_node.hpp
- husarion_ugv_manager/src/lights_manager_node.cpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
husarion_ugv_manager/src/plugins/condition/check_string_msg.cpp (1)
husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_string_msg.hpp (2)
last_msg(43-43)CheckStringMsg(37-41)
⏰ 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 simulation build type
- GitHub Check: Run unit tests hardware build type
🔇 Additional comments (14)
husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_string_msg.hpp (2)
31-33: Good use of RosTopicSubNode for String gating.Class shape and inheritance look appropriate for a ROS 2 BT condition node consuming std_msgs/String.
47-51: Verify that providedBasicPorts includes BT::RosTopicSubNode base ports (e.g. "topic_name")I couldn't find a definition of providedBasicPorts in this repository, so I can't confirm it merges the base ports required by BT::RosTopicSubNode (notably "topic_name"). Please verify.
Files to check:
- husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_string_msg.hpp
- husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_bool_msg.hpp
- husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_joy_msg.hpp
- husarion_ugv_manager/include/husarion_ugv_manager/plugins/action/call_set_bool_service_node.hpp
- husarion_ugv_manager/include/husarion_ugv_manager/plugins/action/call_set_led_animation_service_node.hpp
- husarion_ugv_manager/include/husarion_ugv_manager/plugins/action/call_trigger_service_node.hpp
Suggested change if providedBasicPorts does NOT include the base ports:
- Explicitly merge the base ports from the ROS topic/service node into the returned PortsList, for example:
static BT::PortsList providedPorts()
{
auto ports = BT::RosTopicSubNode::providedPorts(); // base ports (topic_name etc.)
// add this node's ports
ports.insert(/* local BT::InputPort entries /.begin(), / ... */.end());
return ports;
}Tests and behavior tree XML reference "topic_name" (see tests under test/plugins/condition and behavior_trees/lights.xml), so missing the base port can cause runtime port lookup failures. Please confirm or adjust accordingly.
husarion_ugv_manager/config/lights_manager.yaml (1)
21-23: LGTM: plugin registered for runtime usage.Adding
check_string_msg_bt_nodetoros_plugin_libsaligns with the new condition node; no issues spotted.husarion_ugv_manager/test/test_lights_behavior_tree.cpp (1)
132-135: LGTM: test configuration loads the new plugin.Including
check_string_msg_bt_nodeensures the BT can instantiate the node during tests.husarion_ugv_manager/behavior_trees/LightsBT.btproj (1)
12-15: Add CheckStringMsg ports to Groot model (LGTM).Ports align with usage in lights.xml and the ROS2 plugin. Consistent with other node definitions.
husarion_ugv_manager/CMakeLists.txt (1)
61-64: Registering check_string_msg_bt_node in plugin_libs (LGTM).The target and inclusion in plugin_libs look correct and will be exported/installed with the other BT plugins.
husarion_ugv_manager/behavior_trees/lights.xml (1)
159-163: CheckStringMsg model entry (LGTM).Ports match the plugin expectations and LightsBT.btproj. No issues.
husarion_ugv_manager/src/plugins/condition/check_string_msg.cpp (3)
34-34: Latch behavior (LGTM).Returning true ensures last received message is available across ticks; aligns with typical gating conditions.
38-39: Plugin registration (LGTM).Correct usage of CreateRosNodePlugin with the public ID "CheckStringMsg".
25-32: Provided ports already include 'topic_name' — no change requiredCheckStringMsg::providedPorts() uses providedBasicPorts({...}), the behaviortree_ros2 helper that merges base ports (including "topic_name") with additional ports.
Files inspected:
- husarion_ugv_manager/include/husarion_ugv_manager/plugins/condition/check_string_msg.hpp — providedPorts() calls providedBasicPorts({ BT::InputPortstd::string("data", ...) })
- Same pattern present in check_bool_msg.hpp and check_joy_msg.hpp
- Tests and XML reference "topic_name" (husarion_ugv_manager/test/plugins/condition/*, husarion_ugv_manager/behavior_trees/lights.xml)
[result: providedBasicPorts merges base ports as intended]
husarion_ugv_manager/test/plugins/condition/test_check_string_msg.cpp (4)
68-76: No message case covered (LGTM).Asserts FAILURE when no message arrives; good negative coverage.
78-89: Exact match success (LGTM).Covers the happy path and messaging flow via tickOnce → publish → tickWhileRunning.
91-115: String content edge cases covered (LGTM).Spaces and dots are validated; good to see content-agnostic matching.
130-148: State change cycles covered (LGTM).Verifies transitions success → failure → success. Solid behavior coverage.
Description
Requirements
.reposTests 🧪
Summary by CodeRabbit