Closed ruffsl closed 3 years ago
@ruffsl just remove the nav2_
before bringup_launch.py
. I hate redundancy. ros2 launch nav2_bringup ...
so yes, we know its nav2_
already.
My next attack is going to be at the package naming with nav2_...
(its in the navigation2 metapackage, we get it). That will wait until after Foxy (mhm. or before foxy so that master and foxy look similar...). If you have thoughts on that, let me know.
Side note: I didn't know this org existed. You may want to consider merging this in with https://github.com/ros-security so that all the security stuff is all in 1 place.
My next attack is going to be at the package naming
The only nav2 package that differs there is the meta package navigation2
, no?
I think the nav2_
prefixing of the other navigation2 sub package's names are fine.
https://github.com/ros-planning/navigation2/blob/c64ee48b06d3ed24c55b7aaf6089017a080057b2/navigation2/package.xml#L4
Side note: I didn't know this org existed. You may want to consider merging this in with https://github.com/ros-security so that all the security stuff is all in 1 place.
This org/repo predates the other buy quite a bit. Not sure why we didn't just rename this org.
I think the nav2_ prefixing of the other navigation2 sub package's names are fine.
Its just an unimportant detail and adds a bunch of extra typing every time you want to launch something. I think that was initially done because OR at the time had a partially-straight port of Navigation in ROS2 so there were naming collisions. That's not been true since Bouncy.
Given the flat package namespace in ros distros, I think it helps keep things organized and uniquely identifiable. Many of the existing package names would be too vague or colliding if nav2_
is stripped.
Also, I think it helps discoverability when all related packages are similarly prefixed, like when installing:
# hmm, what navigation package are available for my distro?
sudo apt install ros-foxy-nav2-<tab tab>
...
Most of these are designed to be able to be used separately so I think it sends the wrong message. Its in the navigation2 metapackage, so that makes alot of sense.
But for instance specific plugins or methods aren't "core" to nav2 so the prefix seems odd. If we had 5 controller plugins, are they all the nav2 stack or are they implementations to be used by it? In that way, I think nav2 should be removed from most packages other than things that are "core" to nav2 like nav2_{planner, controller, utils, common, core, bringup}*
Anyhow this is super off topic
I doubt that's the actual reason
I seem to recall that being the reason at the time. I did some of those ports.
that grouping helps discoverability
Alright, seems to be a common theme, I can live with it then. I'm mostly concerned about it due to the aforementioned plugin issue about "what is navigation2" when you have many options. I'm also concerned a little about AMCL, which gives the impression that you must use lidar localization or navigation to use the stack, which is a fundamental misconception many people have that I want to aggressively dispel. So much so that I have the full intent of kicking AMCL and map server out of the stack the minute I can show and document a compelling production-ready visual or depth-based positioning demo (https://github.com/ros-planning/navigation2/issues/1342). The main blocker there is the existence / setup of an open-source production(ish) grade non-laser slam. When that happens, I'd like it not to bare the nav2_
prefix to make the point that its not part of, nor required to run the stack. It has no API connection to anything else, other than providing TF transformations (the new repo would be something like navigation_auxiliary
or navigation_2d
or something).
I seem to recall that being the reason at the time. I did some of those ports.
oh interesting, I guess thats the reason then :+1:
RE:prefixing
It makes a lot of sense to want to convey what are the "essential parts" and the "plug-able parts". Prefixing is very visual but also pretty limited so it could be interesting to explore other ways of doing so.
Not sure how it could help in practice but an interesting feature brought by package format 3 is the notion of group. So maybe all navigation planners could declare in their package.xml that they are members of the "navigation_planners" group etc
Currently except from cathin_pkg (so colcon) I'm not sure how many tools can leverage that information, but if index.ros.org and others (maybe bloom could add this to the deb info) could use it, then it would be a non-prefix based way to allow discovery.
For example adding the osrf apt repos seems unnecessary and we should rely on gazebo being provided in the ros repos.
That's just temporary until these are sorted:
This PR removse all the non-tb3 parts of the demo, is it intentional?
I wanted to simplify its focus back to sros2, plus I didn't want to maintain those parts of the demo, as they didn't seem active. Although, I've tagged a release that we can reference for posterity:
If you don't need this demo now I'd encourage you to hold off until at least desktop is released and we should have quite less changes needed in this PR.
This is more or less to test the current state of foxy before anything is released, as I wanted to automate the build setup to use with SROS2 on larger ROS graphs, rather than borking my host install.
I've updated this with the foxy release. It still uses an underlay as navigation2 and gazebo_ros_package haven't been released.
@mikaelarguedas , if you'd like to test the Dockerfile locally, it looks like the nav2 launch files are more up-to-date than whats in the tb3 gazebo repos. E.g this works fine:
ros2 launch nav2_bringup tb3_simulation_launch.py
Looks like there are now a number of nodes with duplicate names :<
@mikaelarguedas , I've updated the build to install nav2 from foxy release packages, and install slam_toolbox in place of cartographer. Then I auto generated the sros2 policy from running nav2 and slam_toolbox in unsecured mode using the sros generate policy CLI. I then cleaned up the policy using xinclude macros and action tags. The demo doesn't launch just yet, as I think the transform_listener_impl_***
node seem to append a UUID number to it's unique node name, but at least it doesn't seem to have reported using any private/relative namespaces that would change.
Perhaps you'd like to take a look at and catch if I've missed anything thus far.
Nice!
I needed https://github.com/ros-swg/turtlebot3_demo/pull/34/commits/1d76c2a951ffd68667496a51fad7395670754f78 to be able to compile the dockerfile as some of the dependencies have been recently released and were not in the rosdep cache of the official foxy image
https://github.com/ros-swg/turtlebot3_demo/pull/34/commits/5d624aafff00065a8b787f30097b90493ae66b23 allowed me to resolve most of the permission issues preventing from launching the demo. The remaining issues being addressed in https://github.com/ros-swg/turtlebot3_demo/pull/34/commits/1b8329beb492f62db2acbc5af857539d8d87dd35
However I'm haven't been able to build a map with either the secure or unsecure configuration. It seems that no map or map transforms are published. Are there additional steps needed with slam toolbox that were not necessary with cartographer ?
However I'm haven't been able to build a map with either the secure or unsecure configuration.
I've been able to build and save maps with the unsecure configuration on my workstation. Are you running into dropped lifecycle messages, as I still can't get even the nav2 with pre-loaded map started with the secure configuration, just gazebo with a blank rviz panel. I might try a different rmw other than fast-rtps to see if its still the blocker.
Are there additional steps needed with slam toolbox that were not necessary with cartographer ?
See my recent commits that update the map server cli snippet.
It seems even adding a unrestricted privileges to the default enclave results in the same inoperable behavior:
<enclaves>
<enclave path="/">
<profiles>
<profile node="admin" ns="/">
<services reply="ALLOW" request="ALLOW">
<service>*</service>
</services>
<topics publish="ALLOW" subscribe="ALLOW">
<topic>*</topic>
</topics>
</profile>
...
@mikaelarguedas I gave it a try using DDS security with RTI connext, but ran into some other QoS issues, like so
MIGGenerator_addData:serialize buffer too small
So I added a custom QoS profile with logging, but I'm still not sure why nav2 stack fails to start. The DDS log reveal little:
root@85fad59a6ce1:/opt/ros# grep error /tmp/log.xml
[1595562113.215598] [CREATE Participant]RTI_Security_CertHelper_verifyCert:X509_verify_cert returned 0 with error 20: unable to get local issuer certificate
[1595562113.432589] [CREATE Participant]RTI_Security_CertHelper_verifyCert:X509_verify_cert returned 0 with error 20: unable to get local issuer certificate
Wow... After thinking about why the MIGGenerator_addData:serialize buffer too small
error was originating from just the security channel write event:
[planner_server-8] MIGGenerator_addData:serialize buffer too small
[planner_server-8] COMMENDAnonWriterService_write:!add DATA to MIG
[planner_server-8] PRESPsWriter_writeCommend:!anonw->write
[planner_server-8] PRESPsWriter_writeInternal:!failed to write sample in Commend
[planner_server-8] PRESParticipant_onSecurityChannelWriteEvent:!security channel write dds.sec.auth message
it recently dawned on me that the lengthy DDS permissions.xml file generated from the combined single default context was perhaps exceeding the max UDP packet size, and given security handshake for exchanging permissions is over a builtin topic. Per the rti docs above:
The builtin topic writers, however, do not fragment the builtin topic information. If large type-code information needs to be sent, it is essential to increase the transport-buffer settings to be larger than the maximum serialized typecode size. This increased size needs to be applied to all the transports you will use (i.e., UDPv4, UDPv6 and Shared Memory).
This issue probably didn't exhibit before foxy, as the size of a permissions file was relatively much smaller given the one-to-one node profile to dds permissions mapping. Thus signed permission.p7s files never encroached the ~64KB UDP limit.
As a comparison, here are two examples where only the admin profiles, like in https://github.com/ros-swg/turtlebot3_demo/pull/34#issuecomment-663283358 is used in the default enclave, vs that currently committed that includes profiles for each node in the one default enclave.
root@29d9ef66b542:/opt/ros# ls -alh keystore/enclaves/
total 42K
drwxr-xr-x 2 root root 10 Jul 24 13:47 .
drwxr-xr-x 5 root root 5 Jul 24 13:47 ..
-rw-r--r-- 1 root root 473 Jul 24 13:47 cert.pem
-rw-r--r-- 1 root root 2.9K Jul 24 13:47 governance.p7s
-rw-r--r-- 1 root root 1.4K Jul 24 13:47 governance.xml
lrwxrwxrwx 1 root root 30 Jul 24 13:47 identity_ca.cert.pem -> ../public/identity_ca.cert.pem
-rw-r--r-- 1 root root 241 Jul 24 13:47 key.pem
-rw-r--r-- 1 root root 2.8K Jul 24 13:47 permissions.p7s
-rw-r--r-- 1 root root 1.3K Jul 24 13:47 permissions.xml
lrwxrwxrwx 1 root root 33 Jul 24 13:47 permissions_ca.cert.pem -> ../public/permissions_ca.cert.pem
root@039b0a878ba0:/opt/ros# ls -alh keystore/enclaves/
total 58K
drwxr-xr-x 2 root root 10 Jul 24 21:10 .
drwxr-xr-x 5 root root 5 Jul 24 21:10 ..
-rw-r--r-- 1 root root 473 Jul 24 21:10 cert.pem
-rw-r--r-- 1 root root 2.9K Jul 24 21:10 governance.p7s
-rw-r--r-- 1 root root 1.4K Jul 24 21:10 governance.xml
lrwxrwxrwx 1 root root 30 Jul 24 21:10 identity_ca.cert.pem -> ../public/identity_ca.cert.pem
-rw-r--r-- 1 root root 241 Jul 24 21:10 key.pem
-rw-r--r-- 1 root root 96K Jul 24 21:10 permissions.p7s
-rw-r--r-- 1 root root 93K Jul 24 21:10 permissions.xml
lrwxrwxrwx 1 root root 33 Jul 24 21:10 permissions_ca.cert.pem -> ../public/permissions_ca.cert.pem
I am not sure if FastRTPS can fragmente the permissions.xml file for security channel write event during the Secure DDS handshake, but if it doesn't and is silently failing instead, this may explain why nav2 fails to start with the current security setup.
After switching between the two enclaves above with Connext and FastRTPS, here's what I'm seeing:
Exploring the idea of reducing permissions file size by simplifying the rules of a participant by using wildcard:
diff --git a/sros2/sros2/policy/templates/dds/permissions.xsl b/sros2/sros2/policy/templates/dds/permissions.xsl
index e99f535..d5951f1 100644
--- a/sros2/sros2/policy/templates/dds/permissions.xsl
+++ b/sros2/sros2/policy/templates/dds/permissions.xsl
@@ -194,9 +194,7 @@
<xsl:variable name="fqn">
<xsl:apply-templates select="."/>
</xsl:variable>
- <topic>rq<xsl:value-of select="$fqn"/>/_action/cancel_goalRequest</topic>
- <topic>rq<xsl:value-of select="$fqn"/>/_action/get_resultRequest</topic>
- <topic>rq<xsl:value-of select="$fqn"/>/_action/send_goalRequest</topic>
+ <topic>rq<xsl:value-of select="$fqn"/>/_action/*</topic>
</xsl:for-each>
</topics>
</publish>
@@ -206,11 +204,8 @@
<xsl:variable name="fqn">
<xsl:apply-templates select="."/>
</xsl:variable>
- <topic>rr<xsl:value-of select="$fqn"/>/_action/cancel_goalReply</topic>
- <topic>rr<xsl:value-of select="$fqn"/>/_action/get_resultReply</topic>
- <topic>rr<xsl:value-of select="$fqn"/>/_action/send_goalReply</topic>
- <topic>rt<xsl:value-of select="$fqn"/>/_action/feedback</topic>
- <topic>rt<xsl:value-of select="$fqn"/>/_action/status</topic>
+ <topic>rr<xsl:value-of select="$fqn"/>/_action/*</topic>
+ <topic>rt<xsl:value-of select="$fqn"/>/_action/*</topic>
</xsl:for-each>
</topics>
</subscribe>
@@ -223,11 +218,8 @@
<xsl:variable name="fqn">
<xsl:apply-templates select="."/>
</xsl:variable>
- <topic>rr<xsl:value-of select="$fqn"/>/_action/cancel_goalReply</topic>
- <topic>rr<xsl:value-of select="$fqn"/>/_action/get_resultReply</topic>
- <topic>rr<xsl:value-of select="$fqn"/>/_action/send_goalReply</topic>
- <topic>rt<xsl:value-of select="$fqn"/>/_action/feedback</topic>
- <topic>rt<xsl:value-of select="$fqn"/>/_action/status</topic>
+ <topic>rr<xsl:value-of select="$fqn"/>/_action/*</topic>
+ <topic>rt<xsl:value-of select="$fqn"/>/_action/*</topic>
</xsl:for-each>
</topics>
</publish>
@@ -237,9 +229,7 @@
<xsl:variable name="fqn">
<xsl:apply-templates select="."/>
</xsl:variable>
- <topic>rq<xsl:value-of select="$fqn"/>/_action/cancel_goalRequest</topic>
- <topic>rq<xsl:value-of select="$fqn"/>/_action/get_resultRequest</topic>
- <topic>rq<xsl:value-of select="$fqn"/>/_action/send_goalRequest</topic>
+ <topic>rq<xsl:value-of select="$fqn"/>/_action/*</topic>
</xsl:for-each>
</topics>
</subscribe>
Looks like there are now a number of nodes with duplicate names :<
FYI, here is a relevant ticket touching on the large number of nodes in nav2: https://github.com/ros-planning/navigation2/issues/816
@mikaelarguedas , touching back on this; today I rebuilt the docker image using the latest ros:foxy
image and change the policy to only use <profile node="admin" ns="/">
for debugging, but encounter the classic error message about fastrtps not being built with security:
# ros2 run turtlebot3_teleop teleop_keyboard
[INFO] [1614032513.627008872] [rcl]: Found security directory: /opt/ros/overlay_ws/../keystore/enclaves
>>> [rcutils|error_handling.c:108] rcutils_set_error_state()
This error state is being overwritten:
'This Fast-RTPS version doesn't have the security libraries
Please compile Fast-RTPS using the -DSECURITY=ON CMake option, at /tmp/binarydeb/ros-foxy-rmw-fastrtps-shared-cpp-1.2.4/src/participant.cpp:248, at /tmp/binarydeb/ros-foxy-rcl-1.1.10/src/rcl/node.c:276'
with this new error message:
'rcl node's rmw handle is invalid, at /tmp/binarydeb/ros-foxy-rcl-1.1.10/src/rcl/node.c:428'
rcutils_reset_error() should be called after error handling to avoid this.
<<<
[ERROR] [1614032513.643556666] [rcl]: Failed to fini publisher for node: 1
Traceback (most recent call last):
File "/opt/ros/overlay_ws/install/turtlebot3_teleop/lib/turtlebot3_teleop/teleop_keyboard", line 11, in <module>
load_entry_point('turtlebot3-teleop', 'console_scripts', 'teleop_keyboard')()
File "/opt/ros/overlay_ws/build/turtlebot3_teleop/turtlebot3_teleop/script/teleop_keyboard.py", line 131, in main
node = rclpy.create_node('teleop_keyboard')
File "/opt/ros/foxy/lib/python3.8/site-packages/rclpy/__init__.py", line 141, in create_node
return Node(
File "/opt/ros/foxy/lib/python3.8/site-packages/rclpy/node.py", line 164, in __init__
self.__handle = Handle(_rclpy.rclpy_create_node(
_rclpy.RCLError: Unknown error creating node: rcl node's rmw handle is invalid, at /tmp/binarydeb/ros-foxy-rcl-1.1.10/src/rcl/node.c:428
Did FastRTPS get re-released into Foxy without building with -DSECURITY=ON
?! :scream:
With 66a44ec62e1bee353eeec0824dec4b8944f76ea2 I've added option to use rmw_connextdds
, but it seems to crash when launching something larger like the navigation stack. So far, no rmw implementation is working with security, even when relaxing the permissions with d46b623aa1039b90793b736f71d8a8d1c2ce60f5 .
rmw_connextdds
w/o securityrmw_connextdds
w securityDemo now seems to work with the default rmw on foxy binaries, but only without security enabled. Will follow up on tracking down the ROS2 issues with security in separate tickets.
@mikaelarguedas looks like this now building agents foxy, all be it from a number of forks.
Looks like the navigaiton launch files a have changed some:
@SteveMacenski , which launch files should should external packages now use for navigation2?