Ros2 bt manager to hpp #461
Conversation
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
WalkthroughThe pull request introduces several changes to the Husarion UGV project. A new "Unit testing" section has been added to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
README.md(1 hunks)husarion_ugv_manager/CMakeLists.txt(7 hunks)husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_manager.hpp(2 hunks)husarion_ugv_manager/src/behavior_tree_manager.cpp(0 hunks)
💤 Files with no reviewable changes (1)
- husarion_ugv_manager/src/behavior_tree_manager.cpp
🔇 Additional comments (1)
husarion_ugv_manager/CMakeLists.txt (1)
64-65: CMakeLists.txt updates correctly reflect source file changes
The removal of src/behavior_tree_manager.cpp from the executable and test definitions appropriately aligns with the relocation of the BehaviorTreeManager implementation to the header file.
Also applies to: 80-81, 154-162, 184-185, 193-194, 230-231, 250-251
husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_manager.hpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_manager.hpp (3)
110-113: Usestring_viewfor the error message literalFor better performance, consider using
string_viewfor the error message literal since it's a compile-time constant string.- throw std::invalid_argument( - "Invalid type for blackboard entry. Valid types are: bool, int, unsigned, float, double, " - "const char*, std::string"); + static constexpr std::string_view error_msg = + "Invalid type for blackboard entry. Valid types are: bool, int, unsigned, float, double, " + "const char*, std::string"; + throw std::invalid_argument(std::string(error_msg));
93-114: Consider using a type map for more maintainable type checkingThe current implementation uses a series of if-else statements for type checking. Consider using a map of type handlers for better maintainability and extensibility.
+ template<typename T> + void SetBlackboardValue( + BT::Blackboard::Ptr blackboard, const std::string & name, const std::any & value) const + { + blackboard->set<T>(name, std::any_cast<T>(value)); + } inline BT::NodeConfig CreateBTConfig(const std::map<std::string, std::any> & bb_values) const { BT::NodeConfig config; config.blackboard = BT::Blackboard::create(); + static const std::map<const std::type_info *, + std::function<void(BT::Blackboard::Ptr, const std::string &, const std::any &)>> type_handlers = { + {&typeid(bool), [this](auto bb, auto & name, auto & val) { + SetBlackboardValue<bool>(bb, name, val); }}, + {&typeid(int), [this](auto bb, auto & name, auto & val) { + SetBlackboardValue<int>(bb, name, val); }}, + // Add other types similarly... + }; for (auto & [name, value] : bb_values) { - const std::type_info & type = value.type(); - if (type == typeid(bool)) { - config.blackboard->set<bool>(name, std::any_cast<bool>(value)); - } else if (type == typeid(int)) { - // ... existing type checks + auto handler = type_handlers.find(&value.type()); + if (handler != type_handlers.end()) { + handler->second(config.blackboard, name, value); + } else { throw std::invalid_argument("Invalid type for blackboard entry..."); } } return config; }
Line range hint
71-79: Consider thread safety for concurrent accessThe methods
TickOnce,TickExactlyOnce,TickWhileRunning, andGetTreeStatusaccess shared state (tree_status_) without synchronization. If these methods might be called from different threads, consider adding thread safety mechanisms.Consider:
- Adding mutex protection for shared state
- Documenting thread safety guarantees in the class documentation
- Making
tree_status_atomic if simple atomic operations are sufficient
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
README.md(1 hunks)husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_manager.hpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
husarion_ugv_manager/include/husarion_ugv_manager/behavior_tree_manager.hpp
Show resolved
Hide resolved
Description
BehaviorTreeManagerto .hpp to use this class in other packagestest_shutdown_single_host_nodebecause it needs ssh configuration.Requirements
Tests 🧪
Summary by CodeRabbit
New Features
husarion_ugvpackage.Bug Fixes
Documentation
Refactor
BehaviorTreeManagerclass.Chores