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

Rework ros2 support #7229

Conversation

berndgassmann
Copy link
Contributor

@berndgassmann berndgassmann commented Mar 8, 2024

Description

Fixes #

Where has this been tested?

  • Platform(s): ...
  • Python version(s): ...
  • Unreal Engine version(s): ...

Possible Drawbacks


This change is Reviewable

Ensure that actually only the unreal sysroot is deployed for all C as
well C++ compilations including dependencies. That ensures the server is
NOT anymore linked against the system glibc (which is in Ubuntu22.04
incompatible with the Unreal one).
Harmonized naming split into client/server (instead of libcpp and
libstdcpp) to ensure nothing mixed up (and there were things mixed up
before!)

Refactor C# Unreal build using CarlaRules base class to provide common
functionality. Fixing windows build without ROS2 and added intitial
windows build with ROS2

Minors:
- Update osm2odr commit
- Fix windows install scripts and forward arguments for building
libcarla
- Add some helper scripts to run under Linux
@berndgassmann berndgassmann requested a review from a team as a code owner March 8, 2024 15:04
Copy link

update-docs bot commented Mar 8, 2024

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update our CHANGELOG.md based on your changes.

@berndgassmann
Copy link
Contributor Author

CARLA-ROS2-Integration.pptx

I've created a basic PowerPoint describing a bit what is implemented...

DataTypes:
  Introduced geom::AngularVelocity, geom::Velocity,geom::Acceleration,
    geom::Quaternion to make the Unreal -> Carla -> ROS2 data conversion
    more clear. Also in preparation of the Carla -> ROS2 data conversion
    afterwards.
  Added some Math functions for Quaternions.

BUGFIX:
  Fixed geom::Rotation::RotateVector() rotation directions of
    pitch and roll!
  Added RightHandedVector3D.h (temporary internal class)
  Commented TransformationMatrix access out by ifdef to prevent
    from erroneous misuse

RPC:
  Extended Control types by timestamp.

Client:
  Applied geom::AngularVelocity, geom::Velocity, geom::Acceleration
    to Actor functions
  Added Vehicle::GetAckermanControl()
  detail/Episode,Simulator: Fixed compiler warnings on std::move

Actor:
Moved Blueprint handling sources to carla/actor from carla/client to
be able to make use of that functionalilty within server when ROS2 is
activated.

Streaming:
  Moved streaming/detail/tcp/Message.h -> streaming/detail/Message.h
    to be deployed for non-tcp communication streams like ROS2.
  Moved general (tcp-independent parts) from tcp/ServerSession to
    streaming/detail/Session.h to be used for ROS2 streams. Had to
    rename a Write() function to WriteMessage because of virtual
    function overloaded with template function not possible.
  Make Dispatcher a shared_pointer to be able to access from ROS2.
  Removed EnableForROS calls, as the decision to use ROS or
    not is done via configuration. The topics are always crecated
    whenever the sensors are spawned and data-stream sessions are
    opened/attached on demanded; i.e. a ROS2 participant
    is subscribing to the topic.

Serializer:
  Provide GetHeaderOffset() functions where missing.
  Add sensor_relative_transform (Location and Quaternion parts) to
    SensorHeader to support relative TF. Keep it in the header also
    when not compiled with ROS2 to keep ROS enabled server builds
    compatible with non-ROS enabled client builds as it's not that
    much of an overhead.
  Add SerializerVectorAllocator to provide the SharedBufferView
    data 'under the hood' as vector without data copy.
  Remove move semantic from Deserialize on server builds to allow
multiple sinks (i.e. TCP-Server and ROS2 to deserialize the data without
destoying it).
  Added VehicleAckermannControl to ActorDynamicState in form of a
    union with VehicleControl to save space
  Extract sensor/data/Array.h into a ArrayConst.h base class to be
used for const data acess on ROS2 Deserialization of
RawEpisodeState.

UE4:
  Updated accordingly.

ROS2:
  Filled the most of the ROS2 interfaces.

Removed some compiler warnings

TODO: Testing of control interfaces; Testing of sensor data values; Find
out why the services are not visible in ROS2
instead of deleting the file
@berndgassmann berndgassmann force-pushed the berndgassmann/rework_ros2_support branch from a57cf4a to cac1560 Compare March 11, 2024 11:01
@berndgassmann
Copy link
Contributor Author

I got it compiled and linked under windows now; had to rename actor -> actors as windows file system is ... after that long time still not able to distiguish between LARGE letters and small letters ... incredible. I also cannot test if ROS2 works on windows... because I had to find out that: oh windows 11 is not supported by ROS2, yet. Theoretically, it should work though given that not some firewall stuff is interfering with the ports required by ROS2/DDS. But crashing when starting up on creating DdsDomainParticipantImpl() with access violation is pointing so some stuff like that. Maybe we should leave the debugging of this under Windows to someone who actually needs that there and has experience with DDS/ROS under Windows.

@berndgassmann
Copy link
Contributor Author

Maybe some comment here on what's actually missing on this now is mainly:

  • transfer the synchonization from ros-bridge to here in the sense that we don't just tick by some single instance, but rather every vehicle can decide it it wants to participate in "ticking"; but the idea would be rather a windowing (every client tells what future timestamp he will accept) than a simple tick -- that is what a simulation of multiple vehicles stacks actually require. A vehicle stack knows that while it is evaluating the sensor data there is multiple sensor data upcoming; therefore a let's day 40ms delta time into the future is for most controllers rather usual. Like this the overall simulation speed-up is massive, because the simulator can run at the maximum speed, the hardware is able to provide mostly. Even with massiv multi-vehicle setup.
    At some point the synchoniszation will come, but that portion can be merged and used before.
    The bridging parts of the old ROS bridge can then be removed.
    The supporting/scripting/controlling parts though will NOT BE REPLACED by this. The internal CARLA ROS support will not refine the topic data to shiny perfectly data. That has to be performed on ROS side in each case.
    One open of this ROS2 support -- which has to be clarified over time, I guess -- is the threading: do we need to call the publisher in an async way to not block the server, or is DDS already taking care for such. If DDS is able to block on the publish() call, then a std::async() must be placed to not block the carla server from continuing.
    I know: quite some things to be taken into account, in addition. But I won't be the one to look into these details. I implemented the initial version of the "new" ROS bridge in a C++ like python; now I had the chance to remove the loose python contracts by strong C++ implementation. The rest is up to the community. Thanks.

@Blyron
Copy link
Contributor

Blyron commented Mar 25, 2024

Hey Bernd,

We are sorry from being late on your PR. We really like your PR and we think it takes very interesting things about ROS2 functionality, but we think we need to focus the PR on ROS2. We are aware that you have introduced quite useful functionality in CARLA, but in order to be tested properly. In order to provide better feedback, first we would like to focus on ROS2 to test it and then after the merge of ROS2 on the introduction of new classes or other stuff. This other functionality could go into another PR so we can test it properly too.

Would be possible to have a PR focused in ROS2?

Apart from that we had some questions and comments about your code;

Questions:

  • Why are you using git submodules?
  • Why have you changed gcc-7? Do you think changing gcc version can be risky?
  • Are we mixing coordinate systems with Quaternions class?
  • Why have you renamed CarlaTopics from Carla...Publisher to Ue...Publisher?

Comments:

  • We are not gonna use a single clang format for the whole proyect, LibCarla is using google coding standard, but in Unreal we are gonna use Unreal's coding standard. So we think that if we want to include clang formatters they should go into a different PR.
  • We would like to know the reason of changing namespaces and some files' path.
  • The changes from MakeSafeUnitVector to MakeUnitVector needs to be reverted, we always want to make sure the unit vector is safely normalized as the location can be (0,0,0)
  • What is the purpouse of RightHandedVector?.
  • In build.cs we think we need to take care of Mac by adding IsLinux()
  • What is the purpouse in ASyncDataStreamImpl Transform RelativeTransform and Rotation?
  • Why are we making FCarlaServer inherit from ros2 interface? Does it work if we do not want ROS2 in the build? Or Are we making ROS2 mandatory dependency? We want to keep ROS2 as optional

Kind regards;,

CARLA team

@berndgassmann
Copy link
Contributor Author

Hi CARLA team,

indeed this is focussing mainly required ROS2 changes. Let's see what is obsolete and can be moved out.
Most of the build system changes are just the merging of #7151 into this branch otherwise one is NOT able to compile successfully under Ubuntu 22.04 / ROS humble.
On the build system:

  • gcc-7 was changed to gcc since that one is quite old and not available on ubuntu 22.04 system, why not just using the default gcc that is installed in the system? In Ubuntu 22.04 it's then gcc-11. That works perfectly fine.

  • clang-format file I'll remove

Then on the ROS2 changes:

  • git submodules allow to fix a specific branch/commit without the need of editing some setup files; especially since the carla-ros-msgs repository is tightly connected to the ros2 extension. It is not required at all for building, but might be useful to checkout as a reference to peek into the msg files. So checking out the msg files is optional; but the developer that wants to start some changes there is happy to have a tight connection of his CARLA PR to the carla-ros-msg PR.

  • Carla...Publisher to Ue...Publisher: the Ue...Publisher classes are derived from UePublisherBaseSensor, the other publishers not without that prefix not. That makes it easier to understand which publisher interface the classes belong to. The topics are not named differently, only the classes.

  • Are we mixing coordinate systems with Quaternions class?
    No, we don't. The quaternions are introduced because then the conversion
    A) quaternion (Unreal) -> rotation (Carla) -> quaternions (ROS)becomes
    B) quaternion (Unreal) -> quaternions (Carla) -> quaternions (ROS).
    The conversion B) is trivial. The conversion A) requires lots of sin/cos calls.
    I added the quationions in a way, that these are currently only used for ROS2. The rest of carla still uses the A) conversion. The only point is that the quaternion data is added to ActorDynamicState and like that also transferred to other clients

  • namespaces/paths: let's see what had to be changed:
    A) Coexistence of TCP and ROS2
    Message is not only used within TCP, but also for ROS2. Since Message.h is independent from TCP, I moved it outside (ROS2 is not depending on TCP tough).
    In a similar way, Session has been split into the basic parts of the session (Session.h) which are independant from TCP and are used by TCP and ROS2 and kept the TPC specidif parts in tcp/ServerSession.h.
    B) Usage of ActorAttribute/ActorBlueprint/BlueprintLibrary from within server build, therefor had to be moved out from client subfolder; new "actors" namespace was the most matching one here as we also have a "sensor" namespace. "actor" namespace was not possible due to folder clash in windows builds where files named identical also resist in "Actor/..." folder.
    C) Did I forget some?

  • The changes from MakeSafeUnitVector to MakeUnitVector needs to be reverted, we always want to make sure the unit vector is safely normalized as the location can be (0,0,0)
    -> Indeed I didn't remove the MakeSafeUnitVector functionality, I rather removed the MakeUnitVector() plain functionality since I saw in the develeopment build some usages of the MakeUnitVector() that lead to the Developement-Assert. Therefore, now ALWAYS safe unit vectors are calculated, either with the epsilon provided, or, if not with the default value of 2.0f * std::numeric_limits::epsilon(). Those happened I believe with the opendrive maps, wich also can have points that are exactly on the same position in the map or at least very very close to each other. Since there isn't the possibility of making unsafe unit vectors anymore, I decided to just rename it to MakeUnitVector(). Like this no one can accidently call the unsafe function anymore.

  • RightHandedVector: this is a temporary class that isn't visible for the user. In the current CALRA code the rotations around the pitch and roll axes were wrong!
    RightHandedVector has autoconversion functions in both directions to flip the y-axis from UE coordinate system to a right handed coordinate system. The rotation matrixes as formulated are actually rotating in a right handed coordinate system. Therefor, the UE/Carla-Vector3D has to converted to a RightHandedVector before the rotation and converted back to UE/Carla-Vector3D after the rotation. Like this the rotation is performed properly. The same is true for rotations using the quaternion. I have extended the unit test to perform roations around all axis so the proper rotation calculation for Rotator as well for Quaternions is ensured. I also commented out the RotationMatrix retrieval by ifdef ALLOW_UNSAFE_GEOM_MATRIX_ACCESS, because users generally are not aware of that problem and will most proably misuse the matrices (as it was formerly done within CARLA itself).

  • ASyncDataStreamImpl: Since ROS2 requires the relative transform to the parent (e.g. of sensors attached to the vehicle) to provide the correct TF trees, the relativeTransform + the relative.Rotation() which is the Quaternion Roation is added to the sensor header (LibCarla/source/carla/sensor/s11n/SensorHeaderSerializer.h). With some smaller changes in the interfaces now ROS2 Ue...Publishers are able to directly read the very same stream data that is already prepared for TCP transmission. That required also the switching of the move semantics on deserialization within the server build (LibCarla/source/carla/sensor/Deserializer.h). Because if the data is moved, the ROS2 publisher "eat" the data and the Buffers provided to the second reader (TCP in that case), get empty buffers.

  • Why are we making FCarlaServer inherit from ros2 interface? Does it work if we do not want ROS2 in the build? Or Are we making ROS2 mandatory dependency? We want to keep ROS2 as optional
    -> ROS2 is totally optional. We could also move the interface somewhere else and rename it into e.g. carla::rpc::RpcServerInterface maybe that's then clearer. I did only define the server functions to the interface that are required for ROS2
    Cleanup and harmonize Carla server build to use Unreal sysroot #7151

In summary: You might want to look into the general server build fixes (#7151) that the carla-server and the libraries built for the server are compiled against the Unreal-Sysroot and don't use ANYTHING from the host (maybe someone has to adapt the same for the pytorch build afterwards to make that also clean). If that one is merged we can rebase this commit (or merge the dev branch into this one) to get rid of the general build system changes.
I will rename Ros2ServerInterface and move to rpc folder as suggested. I will remove the clang file. But there is not much more that can be moved out otherwhise the ROS2 build won't work. All other changes were required to get the interface to work, I guess.

Blyron and others added 4 commits March 26, 2024 13:24
Separate installation of clang/llvm in Ubuntu is not required anymore
since the UE4-clang/llvm is used for compilation of ALL dependencies to
ensure the exact required sysroot is used.
Since the Unreal sysroot build is broken and mixes up libraries from
sysroot with libraries installed on the host, we have to enforce using
the native clang compilation and suppress the sysroot usage completely.

Still clang is using the bundled libc++.

Try to fix also libtorch compilation using the server setup (don't know
yet if that works).
If the configuration setting:
ROS2TopicVisibility=False
is set to false, actors and sensors not created via ROS interface are
not published via ROS per default. But sensors can be explicitly enabled
via EnableForROS calls on client side.

This is a useful feature to support the leaderboard where not all
information shall be published via ROS.
Fixed wrong cast on IMUData publisher leading to memory issues and
finally segfaults.
Fix some world sensor topics not under /carla/world
Moved control topics under extra /control subname
Fixed V2XEvent Deserializer constructors
Now every vehicle and every walker interface provides the possiblity to
make the simulator stop at a certain point in time in case the
controller requires more time than expected (or someone is debugging a
controller e.g. in a breakpoint).
V2XCustom Sensor can be configured by the Actors role_name Attribute to
create communication channels.
@berndgassmann berndgassmann force-pushed the berndgassmann/rework_ros2_support branch from c3e54fd to 58b9f48 Compare June 17, 2024 11:38
@berndgassmann berndgassmann force-pushed the berndgassmann/rework_ros2_support branch from 58b9f48 to 49805b0 Compare June 17, 2024 13:41
@berndgassmann berndgassmann force-pushed the berndgassmann/rework_ros2_support branch from 92f3035 to d2b59ed Compare June 24, 2024 12:12
- remove duplicated 'world' in service names
- fix Segfault in SpawnObject calls of std::sample()
- disable synchronization on generate_traffic since it's not working
properly anymore...
@berndgassmann
Copy link
Contributor Author

This PR is replaced by a step-wise (stacked) approach:
1.) #7919
2.) berndgassmann#1
3.) berndgassmann#2
the later both will be switched to merge into dev when the previous one is merged

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