-
Notifications
You must be signed in to change notification settings - Fork 47
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
base: rolling
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some things that came up when spot comparing the generated files with the ones from https://github.com/mikaelarguedas/tb3_demo/blob/857e1c62ba06476cda4ba359fdf59d31ba488f91/policies/sqlite_playground/extract_policy_from_discovery_db.py
How's it now? See example output: |
There was a problem hiding this 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
29ae5fa
to
1854b7e
Compare
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
The template can be debugged better commenting back the secondary transforms: sros2/sros2/sros2/policy/templates/dds/demangle.xsl Lines 204 to 215 in 1854b7e
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.
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? |
You can diff from last pastebin using the fix output here: https://pastebin.com/WYvxNTbg |
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 👍
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.
Yep that's the approach I had in mind (using FQNs) before comparing to common assets. |
@mikaelarguedas and I chatted offline about adding a |
Signed-off-by: ruffsl <[email protected]>
Signed-off-by: ruffsl <[email protected]>
Signed-off-by: ruffsl <[email protected]>
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]>
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]>
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]>
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]>
Signed-off-by: ruffsl <[email protected]>
Update schema and transforms as well as generating script Signed-off-by: ruffsl <[email protected]>
Signed-off-by: ruffsl <[email protected]>
a67dbc7
to
850fedc
Compare
Related: #112