Feature/add missing fields in plain scxml #63

MarcoLm993 · 2024-10-23T14:51:53Z

This addresses the requests from @EnricoGhiorzi in #59

It will be turned into an actual PR once the BT SCXML one (#58) will be merged.
Before that happens, we can use it to better align on SCAN's actual requirements

MarcoLm993 · 2024-10-23T14:53:06Z

src/as2fm/jani_generator/scxml_helpers/top_level_interpreter.py

+            "w",
+            encoding="utf-8",
+        ) as f:
+            f.write(scxml_model.as_xml_string(data_type_as_argument=False))


This makes sure that the data types are stored using the comment structure.

That's great, thanks!

src/as2fm/jani_generator/scxml_helpers/top_level_interpreter.py

MarcoLm993 · 2024-10-23T14:57:33Z

src/as2fm/scxml_converter/scxml_entries/scxml_executable_entries.py

+            else_body = add_targets_to_scxml_send(entry.get_else_execution(), events_to_automata)
+            new_body.append(ScxmlIf(if_conditionals, else_body))
+        elif isinstance(entry, ScxmlSend):
+            target_automata = events_to_automata.get(entry.get_event(), {"NONE"})


@EnricoGhiorzi on this one I am not entirely sure: there are cases in which we send an event, but we define no transition for it.
This might be desired in case the developer wants to expose a specific information from a single automaton only for referring to it in a property.
For now, I decided to add a "NONE" target for such cases, but we can find other alternatives

I see the use for such a feature, but I worry about aligning the model with the implementation. Would this be how the real system implements such a feature? In other words, is it possible to monitor a topic that has a publisher but no subscribers? (I guess the monitor itself is a subscriber, but that is not modelled.)

Reguarding the "NONE" syntax, I looked up the SCXML specification but is provided no insight: there is no way to send an event into the void, and having a non-existing target would raise an error.

Another option could be to automatically create such a "NONE" automaton and just let it process any event it receives.

To monitor a topic, you need to subscribe to it, and as you mentioned above, the monitor will do exactly that.
If you are fine with it, you can assume to have the NONE automaton that acts as a receiver for all events (though I am not sure it makes sense to auto-generate it: maybe it is easier to have a script removing the send events with a NONE target)

maybe it is easier to have a script removing the send events with a NONE target

I don't get it: if we remove these sends, the events will not be observable in SCAN either. I think we either need to send the events into the void (as it is now) and allow that in SCAN, or to generate a target NONE automaton, either at AS2FM or SCAN level, possibly consistently with the actual implementation. @SofiaFaraci might have some insight on this.

My comment about (optionally) removing the NONE events was only for executing system outside of SCAN. Otherwise, I would be happy to keep it as is and allow the NONE target in SCAN, if that works for you.

I think we either need to send the events into the void (as it is now) and allow that in SCAN, or to generate a target NONE automaton, either at AS2FM or SCAN level, possibly consistently with the actual implementation. @SofiaFaraci might have some insight on this.

In the actual implementation there is no need for a target for the send tag, the events are directly handled by the generated C++ code of the SMs that sends the event.

EnricoGhiorzi

Thank you for this PR, it addresses so many of the issues we have with the format! There are a couple of points that I commented on but the code looks generally good.

I run the code on @SofiaFaraci's model and it works, minus adding the simplest NONE.scxml automaton to receive the messages sent to it. I think that's the main issue to address, either on AS2FM side or on SCAN side.

This PR also addresses the same topic as the branch feature/get-closer-to-standard-scxml, maybe we could add those commits to this PR too?

src/as2fm/jani_generator/scxml_helpers/top_level_interpreter.py

EnricoGhiorzi · 2024-10-23T21:21:20Z

src/as2fm/jani_generator/scxml_helpers/top_level_interpreter.py

+            "w",
+            encoding="utf-8",
+        ) as f:
+            f.write(scxml_model.as_xml_string(data_type_as_argument=False))


That's great, thanks!

src/as2fm/scxml_converter/scxml_entries/scxml_data.py

src/as2fm/scxml_converter/scxml_entries/scxml_executable_entries.py

EnricoGhiorzi · 2024-10-23T22:12:52Z

src/as2fm/scxml_converter/scxml_entries/scxml_executable_entries.py

+            else_body = add_targets_to_scxml_send(entry.get_else_execution(), events_to_automata)
+            new_body.append(ScxmlIf(if_conditionals, else_body))
+        elif isinstance(entry, ScxmlSend):
+            target_automata = events_to_automata.get(entry.get_event(), {"NONE"})


I see the use for such a feature, but I worry about aligning the model with the implementation. Would this be how the real system implements such a feature? In other words, is it possible to monitor a topic that has a publisher but no subscribers? (I guess the monitor itself is a subscriber, but that is not modelled.)

Reguarding the "NONE" syntax, I looked up the SCXML specification but is provided no insight: there is no way to send an event into the void, and having a non-existing target would raise an error.

Another option could be to automatically create such a "NONE" automaton and just let it process any event it receives.

MarcoLm993 · 2024-10-24T07:46:11Z

Thank you for this PR, it addresses so many of the issues we have with the format! There are a couple of points that I commented on but the code looks generally good.

I run the code on @SofiaFaraci's model and it works, minus adding the simplest NONE.scxml automaton to receive the messages sent to it. I think that's the main issue to address, either on AS2FM side or on SCAN side.

This PR also addresses the same topic as the branch feature/get-closer-to-standard-scxml, maybe we could add those commits to this PR too?

The only change from that branch is this: 27ab9c2

I will have a look at renaming the event fields and to allow _event.origin in the transition (though we are not planning to really make use of that feature for now: are you?)

EnricoGhiorzi · 2024-10-24T09:56:00Z

I will have a look at renaming the event fields and to allow _event.origin in the transition (though we are not planning to really make use of that feature for now: are you?)

Internally SCAN already supports _event.origin but there is no need to add that to AS2FM if it is not necessary (that is, if use-case developers don't need it). It could also potentially be a can of worms, so I would advise caution (for example, it makes it difficult to figure out at compile time which automata can potentially receive a certain event).

EnricoGhiorzi

Looks all good!

MarcoLm993 · 2024-10-24T12:26:47Z

I just fixed the remaining conversion problems and updated the test files. It should be good to go!

MarcoLm993 · 2024-10-25T11:24:44Z

After some discussion with Enrico, it seems there is a problem with verifying n-to-n communication entries (that is the standard with topics) and we opted for exporting, for plain scxml, an additional state machine that gets all the published messages and sends them to all subscribers.
~~This is not expected to affect the conversion to Jani, since there we do not distinguish between difference sources of a message~~
I had a second thought on it, and maybe it makes sense to model this as a separated state machine for Jani as well, since that way we could model the message queuing in a better and more explicit way. I will make an issue for this, and it will be discussed and addressed in another PR.

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

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

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

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

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

ct2034 · 2024-10-30T13:43:56Z

test/scxml_converter/test_utils.py

        elem.attrib.clear()
-        for k in sorted(elem.keys()):
-            elem.set(k, elem.get(k))
+        for k in sorted(elem_attributes.keys()):


MarcoLm993 requested review from ct2034 and EnricoGhiorzi October 23, 2024 14:51

MarcoLm993 commented Oct 23, 2024

View reviewed changes

src/as2fm/jani_generator/scxml_helpers/top_level_interpreter.py Show resolved Hide resolved

MarcoLm993 commented Oct 23, 2024

View reviewed changes

EnricoGhiorzi requested changes Oct 23, 2024

View reviewed changes

EnricoGhiorzi approved these changes Oct 24, 2024

View reviewed changes

MarcoLm993 force-pushed the feature/add-missing-fields-in-plain-scxml branch from 15ab218 to b1bc59a Compare October 25, 2024 11:44

MarcoLm993 mentioned this pull request Oct 25, 2024

Additional state machine for topics handling #64

Open

Base automatically changed from feature/scxml-for-bt-control-nodes to main October 25, 2024 12:19

MarcoLm993 added 14 commits October 25, 2024 14:22

Implement storage of data types as comment for plain scxml

3465c59

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

Move plain jani file generation to separate function

7a6689a

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

Add method to retrieve all events received by an automaton in plain s…

d2dfab5

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

Implement scxml send with substitution

c3c4261

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

Create copies of the transition bodies when generating multiple insta…

f827c07

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

Generate copies instead of modifying lists in place

028dbbc

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

Additional check in test

2b1489e

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

Some renaming from review

bbff834

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

Fix attribute sorting in test xml

46cc214

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

Prepare scxml utils for new event data prefix

8ce74d7

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

Start adjusting tests for new scxml events format

971501e

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

Continue adaptation to new event format

7282e77

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

Address wrong events conversion in Jani

cb16d2d

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

Add sorting to loaded files, for test reproducibility

eae7ad6

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

MarcoLm993 force-pushed the feature/add-missing-fields-in-plain-scxml branch from b1bc59a to eae7ad6 Compare October 25, 2024 12:23

MarcoLm993 marked this pull request as ready for review October 25, 2024 12:25

ct2034 approved these changes Oct 30, 2024

View reviewed changes

MarcoLm993 merged commit b5699a6 into main Oct 30, 2024
4 checks passed

MarcoLm993 deleted the feature/add-missing-fields-in-plain-scxml branch October 30, 2024 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/add missing fields in plain scxml #63

Feature/add missing fields in plain scxml #63

MarcoLm993 commented Oct 23, 2024

MarcoLm993 Oct 23, 2024

EnricoGhiorzi Oct 23, 2024

MarcoLm993 Oct 23, 2024

EnricoGhiorzi Oct 23, 2024

MarcoLm993 Oct 24, 2024

EnricoGhiorzi Oct 24, 2024

MarcoLm993 Oct 24, 2024

SofiaFaraci Oct 24, 2024

EnricoGhiorzi left a comment

EnricoGhiorzi Oct 23, 2024

EnricoGhiorzi Oct 23, 2024

MarcoLm993 commented Oct 24, 2024

EnricoGhiorzi commented Oct 24, 2024

EnricoGhiorzi left a comment

MarcoLm993 commented Oct 24, 2024

MarcoLm993 commented Oct 25, 2024 •

edited

Loading

ct2034 Oct 30, 2024

Feature/add missing fields in plain scxml #63

Feature/add missing fields in plain scxml #63

Conversation

MarcoLm993 commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnricoGhiorzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoLm993 commented Oct 24, 2024

EnricoGhiorzi commented Oct 24, 2024

EnricoGhiorzi left a comment

Choose a reason for hiding this comment

MarcoLm993 commented Oct 24, 2024

MarcoLm993 commented Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

MarcoLm993 commented Oct 25, 2024 •

edited

Loading