Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ros2 panther manager #246

Merged
merged 19 commits into from
Mar 26, 2024
Merged

Ros2 panther manager #246

merged 19 commits into from
Mar 26, 2024

Conversation

KmakD
Copy link
Contributor

@KmakD KmakD commented Mar 14, 2024

Most of the work is done, can't be merged as it is waiting for plugin PR first. Plugin PR should include merging newest ros2-devel branch.
Things to finish after plugins are added:

  • remap fan_enable, aux_enable etc. (should be /hardware/name) services controller launch
  • update bringup launch
  • remove dummy_scheduler
  • add shutdown_hosts.yaml
  • change ExpectThrowWithDesription to EXPECT_TRUE(IsMessageThrown) in tests

@KmakD KmakD requested a review from pawelirh March 20, 2024 08:17
@KmakD KmakD changed the base branch from ros2-devel to ros2-manager-plugins March 20, 2024 10:37
Copy link
Contributor

@pawelirh pawelirh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some doubts regarding the architecture (coupling between different behavior trees). I'm not sure if at this exact moment, we want to pay attention to such a discussion. However, let me mention a few aspects.

  • The states (received messages) are updated on the blackboard frequently.
  • It's hard to scale. A new BT, or modification of an existing one demands changes in the core node and pulls after unit test modifications.
  • Single responsibility for the node is not kept, since the node in this example is responsible for creating and configuring the BT and blackboard.
  • Why do we push to have several independent trees, since they are in the end using the same resources and executing interchangeably (SingleThreadedExecutor)?

panther_manager/config/Panther106BT.btproj Outdated Show resolved Hide resolved
panther_manager/src/manager_bt_node.cpp Show resolved Hide resolved
panther_manager/src/manager_bt_node.cpp Outdated Show resolved Hide resolved
panther_manager/src/manager_bt_node.cpp Outdated Show resolved Hide resolved
panther_manager/test/test_manager_bt_node.cpp Outdated Show resolved Hide resolved
panther_manager/test/test_manager_bt_node.cpp Outdated Show resolved Hide resolved
panther_manager/test/test_manager_bt_node.cpp Outdated Show resolved Hide resolved
@KmakD
Copy link
Contributor Author

KmakD commented Mar 21, 2024

* Why do we push to have several independent trees, since they are in the end using the same resources and executing interchangeably (SingleThreadedExecutor)?

My mistake, this should be MultiThreadedExecutor. It's fixed. The idea behind two separate BTs is that safety is a crucial part of system and should be handled independently, we don't want deadlock or some sleep behavior in lights affect safety features.

panther_manager/main.cpp Outdated Show resolved Hide resolved
panther_manager/src/main.cpp Outdated Show resolved Hide resolved
panther_manager/src/manager_bt_node.cpp Show resolved Hide resolved
@KmakD KmakD marked this pull request as ready for review March 25, 2024 13:34
@KmakD KmakD requested a review from pawelirh March 25, 2024 13:34
@KmakD KmakD requested a review from pawelirh March 26, 2024 07:48
Copy link
Contributor

@pawelirh pawelirh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👌🏼

@KmakD KmakD changed the base branch from ros2-manager-plugins to ros2-devel March 26, 2024 10:43
@KmakD KmakD merged commit 8d06a2b into ros2-devel Mar 26, 2024
@KmakD KmakD deleted the ros2-panther-manager branch March 26, 2024 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants