SCXML implementation of BT Control Nodes #58

MarcoLm993 · 2024-10-08T15:13:07Z

In this PR we introduce a new, modular way of handling Behavior Trees.
In the process of doing this, the old way of sending bt_ticks and the related responses got deprecated, in favor of custom tags that can be found in various examples (the easiest being the... battery drainer: https://github.com/convince-project/AS2FM/pull/58/files#diff-a1c8ee81a504867984b355d25488e8119194f227f5ee5bc3973a530205a382e3)
Be aware that the old plugin name mismatch (Sequences being ReactiveSequences and Fallbacks being ReactiveFallbacks) got fixed as well, so the BT.xml (or policy.xml) files need to be updated as well.

Please let me know if there is anything you find off in the PR :)

EnricoGhiorzi · 2024-10-09T08:51:40Z

Sorry @MarcoLm993, that was a mistake! Anyway, feel free to request a review from me when the PR is ready.

pyproject.toml

Signed-off-by: Marco Lampacrescia <[email protected]>

…handling Signed-off-by: Marco Lampacrescia <[email protected]>

Signed-off-by: Marco Lampacrescia <[email protected]>

MarcoLm993 · 2024-10-15T15:15:02Z

For @m-morelli : for the RosCon delib. workshop from Christian, we made a tag of AS2FM (to avoid creating confusion with the new changes). In case you need it, it is the following: https://github.com/convince-project/AS2FM/tree/2024_delib_ws

Signed-off-by: Marco Lampacrescia <[email protected]>

EnricoGhiorzi

This is a heroic effort, thank you for your work! As I am unfamiliar with the codebase and even with Python, I tried to mostly get a high-level view of the code, but it all make sense to me. There are just a couple minor remarks but those are non-blocking for merging.

I do have a few general comments that I am going to report here to start a discussion, but they are not meant to be acted on in the context of this PR:

The BT translation creates a SCXML machine per node, as opposed to one per tree. I expect this to have a significant negative impact on the performance of verification, and it might be worth tracking this in case future issues arise.
The BT has no Halt semantics (yet). Are we going to need it? Critically, which Halt semantics we adopt exactly might depend on which BT runtime we use.
I generally worry that, in the process of (SCXML) code generation, we might introduce name-clashing. The code in the PR preserves naming faithfully, as far as I can tell. This (might) create issues in the future, for example when introducing subtrees (nodes from different subtrees might generate SCXML machines with the same name). I suggest decorating generated names with something characterizing their scope, e.g. tree_name-node_name, to make IDs unique. This could also help downstream toolchain components, such as SCAN or STORM.

docs/source/howto.rst

docs/source/installation.rst

src/as2fm/scxml_converter/scxml_entries/scxml_bt_ticks.py

MarcoLm993 · 2024-10-18T06:56:23Z

This is a heroic effort, thank you for your work! As I am unfamiliar with the codebase and even with Python, I tried to mostly get a high-level view of the code, but it all make sense to me. There are just a couple minor remarks but those are non-blocking for merging.

I do have a few general comments that I am going to report here to start a discussion, but they are not meant to be acted on in the context of this PR:

The BT translation creates a SCXML machine per node, as opposed to one per tree. I expect this to have a significant negative impact on the performance of verification, and it might be worth tracking this in case future issues arise.

The BT has no Halt semantics (yet). Are we going to need it? Critically, which Halt semantics we adopt exactly might depend on which BT runtime we use.

I generally worry that, in the process of (SCXML) code generation, we might introduce name-clashing. The code in the PR preserves naming faithfully, as far as I can tell. This (might) create issues in the future, for example when introducing subtrees (nodes from different subtrees might generate SCXML machines with the same name). I suggest decorating generated names with something characterizing their scope, e.g. tree_name-node_name, to make IDs unique. This could also help downstream toolchain components, such as SCAN or STORM.

Hi Enrico, thanks for the review!

Switching to a modular BT expansion (one SCXML per node) will for sure introduce some more communication overhead, but it is definitely going to help in terms of maintainability, since the approach of generating a single state machine for the whole BT was going to require a lot of maintenance and was preventing users to introduce their own controllers, that is something that might be required at some point.
Halt semantics are something we should introduce at some point, at least from resetting running nodes in case of timeouts or something like that. However, we did not see them as a very urgent thing and kept postponing their implementation.
This is a fair thing to worry about: at the moment SubTrees are not supported but, I think, preventing the name clashing there is possible if we keep the ID counter consistent over the expanded subtrees (i.e. we do no start from 1000 each time we expand a new subtree). It might get problematic if we start having multiple Behavior Trees instances (that would have also independent ticks), but I believe this is also an implementation detail we will need to figure out when we introduce them: the problem is that nothing prevents people from using the same tree name, as far as I know. This is why a different ID generation strategy is required.

Signed-off-by: Marco Lampacrescia <[email protected]>

ct2034

I am fine with these. But please look at my three comments

ct2034 · 2024-10-25T10:53:17Z

src/as2fm/jani_generator/scxml_helpers/scxml_event.py

@@ -105,6 +105,7 @@ def must_be_skipped_in_jani_conversion(self):
    def is_bt_response_event(self):
        """Check if the event is a behavior tree response event (running, success, failure).
        They may have no sender if the plugin does not implement it."""
+        # TODO: Remove it when deprecated support for running, success, failure BT events is removed


Because with the new handling we have a single bt_response event per BT node (from here https://github.com/convince-project/AS2FM/pull/58/files#diff-8021c3394c29f60e934c8bf2dff081894800c6ccead20ed8b4550085cfe4ef9cR276), containing the response result as a param. This means that, with the new version, bt_response events are expected to be always sent, and there is no reason for skipping them anymore (that was the main reason for this method to exist)

ct2034 · 2024-10-25T10:59:22Z

src/as2fm/scxml_converter/scxml_entries/bt_utils.py

-        assert (
-            port_name not in RESERVED_BT_PORT_NAMES
-        ), f"Error: Port name {port_name} is reserved in BT"
+        # All port IDs are valid


Before these IDs were referring to special fields from btlib, and therefore could not be used.
Now they are referring to the ports that are automatically filled by the processor, and hence they could be set even if they aren't declared (and used) in the scxml model of the BT plugin.

ct2034 · 2024-10-25T11:01:25Z

src/as2fm/scxml_converter/scxml_entries/scxml_transition.py

@@ -116,16 +117,30 @@ def get_executable_body(self) -> ScxmlExecutionBody:
        """Return the executable content of this transition."""
        return self._body if self._body is not None else []

-    def instantiate_bt_events(self, instance_id: str):
+    def instantiate_bt_events(self, instance_id: int, children_ids: List[int]) -> "ScxmlTransition":


Suggested change

def instantiate_bt_events(self, instance_id: int, children_ids: List[int]) -> "ScxmlTransition":

def instantiate_bt_events(self, instance_id: int, children_ids: List[int]) -> "List[ScxmlTransition]":

Right, for some inherited classes, but here we always return self... Maybe we should check it once again...

I made sure that all classes inheriting from ScxmlTransition (and ScxmlTransition itseldf) return a List on that method.

Signed-off-by: Marco Lampacrescia <[email protected]>

EnricoGhiorzi marked this pull request as ready for review October 9, 2024 08:48

EnricoGhiorzi marked this pull request as draft October 9, 2024 08:49

ct2034 reviewed Oct 9, 2024

View reviewed changes

pyproject.toml Show resolved Hide resolved

MarcoLm993 force-pushed the feature/scxml-for-bt-control-nodes branch from 5c96eff to 2e2ce9d Compare October 11, 2024 12:04

MarcoLm993 added 13 commits October 14, 2024 09:07

Initial verasion of BT control nodes scxml

b5ea3be

Signed-off-by: Marco Lampacrescia <[email protected]>

Second iteration of scxml_control_nodes

5cb16d3

Signed-off-by: Marco Lampacrescia <[email protected]>

Rename BT Ports scxml declarations

1e353c3

Signed-off-by: Marco Lampacrescia <[email protected]>

One more iteration over the BT Control node SCXML

7fd7fc6

Signed-off-by: Marco Lampacrescia <[email protected]>

STart implementing new scxml bt tags

8bd28ed

Signed-off-by: Marco Lampacrescia <[email protected]>

First skeleton on new BT tags

02b7454

Signed-off-by: Marco Lampacrescia <[email protected]>

Prepare for testing new bt functionality

019d6c3

Signed-off-by: Marco Lampacrescia <[email protected]>

Prepare example using new format

045f5f7

Signed-off-by: Marco Lampacrescia <[email protected]>

Start implementing new BT SCXML generator

1f8f6ff

Signed-off-by: Marco Lampacrescia <[email protected]>

Integrate bt_children ids in the scxml root

f86b904

Signed-off-by: Marco Lampacrescia <[email protected]>

Fix package not found in ipython

f7e8f90

Signed-off-by: Marco Lampacrescia <[email protected]>

Move controllers scxml to another folder

f11c651

Signed-off-by: Marco Lampacrescia <[email protected]>

Check type of BtTickChild id

dab8efe

Signed-off-by: Marco Lampacrescia <[email protected]>

MarcoLm993 force-pushed the feature/scxml-for-bt-control-nodes branch from 2e2ce9d to dab8efe Compare October 14, 2024 07:07

MarcoLm993 added 8 commits October 14, 2024 14:53

Continue new import of bt plugins

64e1ed4

Signed-off-by: Marco Lampacrescia <[email protected]>

Install and import plugins in resources

44651f8

Signed-off-by: Marco Lampacrescia <[email protected]>

Continue integration

a9bf8de

Signed-off-by: Marco Lampacrescia <[email protected]>

Implement missing conversions

bc16c96

Signed-off-by: Marco Lampacrescia <[email protected]>

First scxml without errors

5d2f706

Signed-off-by: Marco Lampacrescia <[email protected]>

Substitution of xml escape sequence and re-order ecmascript entities …

f93c9e3

…handling Signed-off-by: Marco Lampacrescia <[email protected]>

First jani result

54ea01e

Signed-off-by: Marco Lampacrescia <[email protected]>

Various fixes

e00f599

Signed-off-by: Marco Lampacrescia <[email protected]>

MarcoLm993 force-pushed the feature/scxml-for-bt-control-nodes branch from 407b836 to e00f599 Compare October 15, 2024 08:23

MarcoLm993 added 2 commits October 15, 2024 11:19

Fix bug and improve variables naming

dc64bac

Signed-off-by: Marco Lampacrescia <[email protected]>

Yet another bug

dd6f908

Signed-off-by: Marco Lampacrescia <[email protected]>

MarcoLm993 added 3 commits October 15, 2024 15:50

Adjust BT implementations

02307be

Signed-off-by: Marco Lampacrescia <[email protected]>

Fix ReactiveFallback control node

8c91364

Signed-off-by: Marco Lampacrescia <[email protected]>

Documentation

8164302

Signed-off-by: Marco Lampacrescia <[email protected]>

MarcoLm993 marked this pull request as ready for review October 15, 2024 15:07

MarcoLm993 requested review from m-morelli, EnricoGhiorzi and SofiaFaraci October 15, 2024 15:07

MarcoLm993 added 6 commits October 15, 2024 17:48

Remove deprecated use of importlib's path

29e7a56

Signed-off-by: Marco Lampacrescia <[email protected]>

Remove unused btlib

5301d90

Signed-off-by: Marco Lampacrescia <[email protected]>

Remove old dependencies

bb267d7

Signed-off-by: Marco Lampacrescia <[email protected]>

Fix font path in toml file

46478bd

Signed-off-by: Marco Lampacrescia <[email protected]>

Test for reactive sequence

aeafe3d

Signed-off-by: Marco Lampacrescia <[email protected]>

Add test for reactive fallbacks

5cb1a43

Signed-off-by: Marco Lampacrescia <[email protected]>

MarcoLm993 requested a review from ct2034 October 17, 2024 07:23

MarcoLm993 added 4 commits October 17, 2024 11:50

Implement BT sequence model and update test plugin for counting ticks

ad544f4

Signed-off-by: Marco Lampacrescia <[email protected]>

Test for sequence controller

0a61598

Signed-off-by: Marco Lampacrescia <[email protected]>

Implement Fallback control

6eb2c4c

Signed-off-by: Marco Lampacrescia <[email protected]>

Add test for fallback bt controller

c5cb74e

Signed-off-by: Marco Lampacrescia <[email protected]>

EnricoGhiorzi approved these changes Oct 17, 2024

View reviewed changes

docs/source/howto.rst Show resolved Hide resolved

docs/source/installation.rst Outdated Show resolved Hide resolved

src/as2fm/scxml_converter/scxml_entries/scxml_bt_ticks.py Show resolved Hide resolved

MarcoLm993 added 2 commits October 18, 2024 13:04

Small TODO

18712ea

Signed-off-by: Marco Lampacrescia <[email protected]>

Prevent extra self-loops in the BT root jani automaton

2a3c64c

Signed-off-by: Marco Lampacrescia <[email protected]>

SofiaFaraci approved these changes Oct 18, 2024

View reviewed changes

MarcoLm993 mentioned this pull request Oct 23, 2024

Feature/add missing fields in plain scxml #63

Merged

ct2034 approved these changes Oct 25, 2024

View reviewed changes

Always return a List of transitions when instantiating BT events

e5d3666

Signed-off-by: Marco Lampacrescia <[email protected]>

MarcoLm993 merged commit 6a8f1cc into main Oct 25, 2024
4 checks passed

MarcoLm993 deleted the feature/scxml-for-bt-control-nodes branch October 25, 2024 12:19

MarcoLm993 mentioned this pull request Oct 29, 2024

BT control nodes as modular SCXML #39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SCXML implementation of BT Control Nodes #58

SCXML implementation of BT Control Nodes #58

MarcoLm993 commented Oct 8, 2024 •

edited

Loading

EnricoGhiorzi commented Oct 9, 2024

MarcoLm993 commented Oct 15, 2024

EnricoGhiorzi left a comment

MarcoLm993 commented Oct 18, 2024

ct2034 left a comment

ct2034 Oct 25, 2024

MarcoLm993 Oct 25, 2024

ct2034 Oct 25, 2024

MarcoLm993 Oct 25, 2024

ct2034 Oct 25, 2024

MarcoLm993 Oct 25, 2024 •

edited

Loading

MarcoLm993 Oct 25, 2024

	def instantiate_bt_events(self, instance_id: int, children_ids: List[int]) -> "ScxmlTransition":
	def instantiate_bt_events(self, instance_id: int, children_ids: List[int]) -> "List[ScxmlTransition]":

SCXML implementation of BT Control Nodes #58

SCXML implementation of BT Control Nodes #58

Conversation

MarcoLm993 commented Oct 8, 2024 • edited Loading

EnricoGhiorzi commented Oct 9, 2024

MarcoLm993 commented Oct 15, 2024

EnricoGhiorzi left a comment

Choose a reason for hiding this comment

MarcoLm993 commented Oct 18, 2024

ct2034 left a comment

Choose a reason for hiding this comment

ct2034 Oct 25, 2024

Choose a reason for hiding this comment

MarcoLm993 Oct 25, 2024

Choose a reason for hiding this comment

ct2034 Oct 25, 2024

Choose a reason for hiding this comment

MarcoLm993 Oct 25, 2024

Choose a reason for hiding this comment

ct2034 Oct 25, 2024

Choose a reason for hiding this comment

MarcoLm993 Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

MarcoLm993 Oct 25, 2024

Choose a reason for hiding this comment

MarcoLm993 commented Oct 8, 2024 •

edited

Loading

MarcoLm993 Oct 25, 2024 •

edited

Loading