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

Pipeline for policy generation from DDS discovery data #153

Draft
wants to merge 18 commits into
base: rolling
Choose a base branch
from

Conversation

ruffsl
Copy link
Member

@ruffsl ruffsl commented Aug 6, 2019

Related: #112

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

sros2/sros2/policy/templates/dds/policy.xsl Outdated Show resolved Hide resolved
sros2/scripts/dds_sql_to_sros2_policy.py Outdated Show resolved Hide resolved
sros2/scripts/dds_sql_to_sros2_policy.py Outdated Show resolved Hide resolved
@ruffsl
Copy link
Member Author

ruffsl commented Aug 6, 2019

How's it now? See example output:
https://pastebin.com/JhCvvYVh

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

How's it now?

Better! less unnecessary permissions and it validates the schema!

Few surprising things in the result file:

  • empty profiles:
    Profiles are generated for namespaces that don't belong to them and this results in a lot of empty profiles.
    <profile node="gazebo" ns="/global_costmap"/>
    <profile node="gazebo" ns="/local_costmap"/>
  • There are some topics listed in the database that don't appear in the generated file, for example tf doesnt appear for any node in the generated policies o_O

  • clock appears sometimes absolute and sometimes relative. I believe it makes sense as when the namespace is just / the script has no way to know which one of the two it is. Something to keep in mind when adding logic to collapse permissions to higher level common objects

sros2/scripts/dds_sql_to_sros2_policy.py Outdated Show resolved Hide resolved
sros2/scripts/dds_sql_to_sros2_policy.py Show resolved Hide resolved
@ruffsl ruffsl force-pushed the dds_to_sros_pipline branch 3 times, most recently from 29ae5fa to 1854b7e Compare August 7, 2019 04:02
@ruffsl
Copy link
Member Author

ruffsl commented Aug 7, 2019

  • empty profiles:
    Profiles are generated for namespaces that don't belong to them and this results in a lot of empty profiles.

Empty profiles were unintendedly added with an erroneous combinatorial across nodes and namespaces: address in 368f4c5 . I've also added a check for when the discovery user_data from a foreign QoS configuration: see commit message in 3533ec2

  • There are some topics listed in the database that don't appear in the generated file, for example tf doesnt appear for any node in the generated policies o_O

The template can be debugged better commenting back the secondary transforms:

<xsl:variable name="profile_deduplicated">
<xsl:apply-templates mode="deduplicate"
select="ext:node-set($profile)"/>
</xsl:variable>
<xsl:variable name="profile_namespaced">
<xsl:apply-templates mode="namespace"
select="ext:node-set($profile_deduplicated)"/>
</xsl:variable>
<xsl:apply-templates mode="compress"
select="ext:node-set($profile_namespaced)"/>

Diffing the consecutive chained output from different iterations of commiting, I found some copy and paste typos in the compression. See 1854b7e . I'd like to make this a little more DRY, but it's tricky when only using XSL v1.0. Let me know if you see a better expression for things.

  • clock appears sometimes absolute and sometimes relative. I believe it makes sense as when the namespace is just / the script has no way to know which one of the two it is. Something to keep in mind when adding logic to collapse permissions to higher level common objects

Yea, I thought about this, like adding a lookup for common root level topics in the xst for such cases, but it's probably better instead to ensure the added logic to collapse permissions to higher level common objects is robust enough to handle both appearances, perhaps by expanding the fqn topic namespace using rcl logic when comparing candidate substitutions?

@ruffsl
Copy link
Member Author

ruffsl commented Aug 7, 2019

You can diff from last pastebin using the fix output here: https://pastebin.com/WYvxNTbg

@mikaelarguedas
Copy link
Member

Deduplicating the topics in the raw intermediate representation now generate the exact same topic list as https://github.com/mikaelarguedas/tb3_demo/blob/857e1c62ba06476cda4ba359fdf59d31ba488f91/policies/sqlite_playground/policy_dump_from_sqlite.xml#L1094 🎉

diff --git a/sros2/scripts/dds_sql_to_sros2_policy.py b/sros2/scripts/dds_sql_to_sros2_policy.py
index 6c602cd..801a919 100755
--- a/sros2/scripts/dds_sql_to_sros2_policy.py
+++ b/sros2/scripts/dds_sql_to_sros2_policy.py
@@ -91,9 +91,12 @@ def df_to_dds_policy(df):
                 if not pd.isna(mode):
                     topics = etree.SubElement(profile, 'raws')
                     topics.set(mode, "ALLOW")
+                    raw_topics = []
                     for dds_topic in df['dds_topic'].loc[namespace, name, mode]:
-                        topic = etree.SubElement(topics, 'raw')
-                        topic.text = dds_topic
+                        if dds_topic not in raw_topics:
+                            raw_topics.append(dds_topic)
+                            topic = etree.SubElement(topics, 'raw')
+                            topic.text = dds_topic
     return dds_policy

Comparing the second stage is a bit trickier but spot checking a few nodes everything seems to work as expected 👍

I'd like to make this a little more DRY, but it's tricky when only using XSL v1.0. Let me know if you see a better expression for things.

Yeah that would be good indeed, but like you said it looks tricky to do with XSL 1.0. We can do more investigation after we get everything running well.

like adding a lookup for common root level topics in the xst for such cases, but it's probably better instead to ensure the added logic to collapse permissions to higher level common objects is robust enough to handle both appearances, perhaps by expanding the fqn topic namespace using rcl logic when comparing candidate substitutions?

Yep that's the approach I had in mind (using FQNs) before comparing to common assets.
Not sure if rcl logic needs to be leveraged here as the concatenation of namespace and names already happens in the xsl in some places.

@ruffsl
Copy link
Member Author

ruffsl commented Aug 8, 2019

@mikaelarguedas and I chatted offline about adding a <raw> type element to the policy schema. Though I think we'll hold off on that for now until we have a more concrete use case for it. So I've just relegated that schema check to its own transport folder for now. a67dbc7

Signed-off-by: ruffsl <[email protected]>
apply template on each profile to avoided crosstalk

Signed-off-by: ruffsl <[email protected]>
Signed-off-by: ruffsl <[email protected]>
Signed-off-by: ruffsl <[email protected]>
To avoid provisioning topic to all modes
instead of the topic's respective mode as intended

Signed-off-by: ruffsl <[email protected]>
Borrowing cli petern from @mikaelarguedas

Signed-off-by: ruffsl <[email protected]>
node names should be limited to what exists
with respect to the current iterative namespace

Signed-off-by: ruffsl <[email protected]>
Drop rows with non-parcelable userdata payloads,
as we can't infer the node name or namespace from participant

Participant vs data discovery sent over separate QoS packets in DDS.
Ensure a profile is created even if no topics are discovered.
E.g. for the ros2 cli daemon only listing nodes on the network

Signed-off-by: ruffsl <[email protected]>
Signed-off-by: ruffsl <[email protected]>
Update schema and transforms
as well as generating script

Signed-off-by: ruffsl <[email protected]>
@ruffsl ruffsl force-pushed the dds_to_sros_pipline branch from a67dbc7 to 850fedc Compare September 7, 2019 00:06
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:25
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