Dds prototype #17779

arshPratap · 2021-06-15T15:53:25Z

Hello Ardupilot community!
This PR is work in progress and consists of the prototype code that aims to provide DDS functionality to Ardupilot for effective communication with ROS2 and Native DDS implementations

Added 2 IDL files

A simple Num idl to test the xrce dds functionality
AP_INS idl file for sending INS info

Added the eProsima XRCE Client lib as submodule
Provides Ardupilot an XRCE client to communicate with DDS/ROS2 through :
Serial(Main focus)
UDP(Only for testing,to be removed in final version)

Current Issues

Have tested the code through serial communication and sometimes it is found that data is not properly flushed out in the serial ports resulting in communication to XRCE agent being cut.Working on implementing a function to take care of this issue.
Have doubts related to setting up the build system to install the XRCE Client from the necessary submodules

Thanks !

ArduCopter/xrce-client.cpp

khancyr · 2021-06-15T17:01:52Z

libraries/AP_XRCE_Client/AP_XRCE_Client.cpp

+    }
+
+    if (this->use_serial) {
+        this->fd=open("/dev/ttyUSB1", O_RDWR | O_NOCTTY | O_NONBLOCK);


You should get a serial object from the hal that way you ensure that the port is configured correctly

Also this should be made available on other serial ports as well... maybe check how other libraries are implemented and the parameter system that sets them up.

ardupilot/libraries/AP_SerialManager/AP_SerialManager.cpp

Line 412 in 3386cbb

// initialise serial ports

peterbarker

Rough pass only.

ArduCopter/Copter.cpp

ArduCopter/xrce-client.cpp

peterbarker · 2021-06-17T04:54:45Z

libraries/AP_XRCE_Client/AP_INS.cpp

+
+bool AP_INS_serialize_topic(ucdrBuffer* writer, const AP_INS* topic)
+{
+    (void) ucdr_serialize_int32_t(writer, topic->accel_count);


This casting-to-void is a bit of a red flag for me. If it is warranted then it deserves a comment.

It looks like the examples in the XRCE-DDS also do this by some poor convention even though there is a perfectly good return code to consume and pass up to the application. After stripping down this implementation, I'll make sure the error codes get passed up.

Quick question - Is C++17, specifically [[nodiscard]] allowed here?

I can submit a PR upstream - this serialization function is actually adapted from the generated code from MicroXRCEDDSGen. I agree, we should not discard write issues.

Submit PR upstream to not void cast

Add submodule for fastddgen

Remove manually written serialization instead with autogenerated files using waf

eProsima/Micro-XRCE-DDS-Gen#45
Created issue and submitted PR.

This is now part of our generator. Resolved.

peterbarker · 2021-06-17T04:55:42Z

libraries/AP_XRCE_Client/AP_INS.h

+ * @brief This struct represents the structure AP_INS defined by the user in the IDL file.
+ * @ingroup AP_INS
+ */
+typedef struct AP_INS


Any reason you need a typedef?

peterbarker · 2021-06-17T04:59:45Z

libraries/AP_XRCE_Client/AP_XRCE_Client.cpp

+    if (this->use_serial) {
+        this->fd=open("/dev/ttyUSB1", O_RDWR | O_NOCTTY | O_NONBLOCK);


Suggested change

if (this->use_serial) {

this->fd=open("/dev/ttyUSB1", O_RDWR | O_NOCTTY | O_NONBLOCK);

if (use_serial) {

fd = open("/dev/ttyUSB1", O_RDWR | O_NOCTTY | O_NONBLOCK);

(as @jmachuca77 says, use the serial manager instead; you can still use a real serial device easily enough)

It's being used now.

peterbarker · 2021-06-17T05:00:32Z

libraries/AP_XRCE_Client/AP_XRCE_Client.cpp

+    else {
+        if(!uxr_init_udp_transport(&this->udp_transport,UXR_IPv4,ip,port)) {
+            return false;
+        }
+        uxr_init_session(&this->session, &this->udp_transport.comm, 0xAAAABBBB);


We do not want to support UDP with this at this time - simplify the PR by always assuming use_serial

Also... dispense with this-> in member functions.

Done and Done.

peterbarker · 2021-06-17T05:01:47Z

libraries/AP_XRCE_Client/AP_XRCE_Client.cpp

+bool AP_XRCE_Client::create()
+{
+
+    if(_csem->take(5)){


taking-with-timeout is almost always something you do not want to do.

Also, usually you want to be doing WITH_SEMAPHORE(_csem)

Done (already)

peterbarker · 2021-06-17T05:03:30Z

libraries/AP_XRCE_Client/AP_XRCE_Client.cpp

+    if(_csem->take(5)){
+        if(this->connected)
+        {


Avoid indenting

Suggested change

if(_csem->take(5)){

if(this->connected)

{

if(!_csem->take(5)){

return;

}

if(!this->connected)

return;

}

Everything is formatted with astyle on the PR.

peterbarker · 2021-06-17T05:04:09Z

libraries/AP_XRCE_Client/AP_XRCE_Client.h

+    uxrObjectId dwriter_id ;
+    uint16_t dwriter_req ;


Suggested change

uxrObjectId dwriter_id ;

uint16_t dwriter_req ;

uxrObjectId dwriter_id;

uint16_t dwriter_req;

and everywhere else

Format is resolved with astyle

peterbarker · 2021-06-18T01:38:18Z

On Wed, 16 Jun 2021, Pierre Kancir wrote: You should see this as the mavlink xml, it is the definition to generate the AP_INS.cpp code that is auto generated

We don't commit the generated code in ArduPilot :-)

jmachuca77 · 2021-06-23T00:58:25Z

Adding to the conversation so we can keep track and for easier discussion.
The pr in its current form add the submodule to the project, however it still requires manual installation of the libraries and its looking to the system folders for those libraries.
These are the instructions for building the XRCE client:
https://micro-xrce-dds.docs.eprosima.com/en/latest/installation.html

This needs to be added to the waf build system.

tridge · 2021-07-16T03:53:03Z

it looks like we will need to do config.h.in processing

tridge · 2021-07-16T06:00:51Z

@arshPratap I've added the waf rules for config.h generation here:
https://github.com/tridge/ardupilot/commits/ddsPrototype

amfern · 2021-09-06T19:53:25Z

This is a very good work, i am wondering why you opted out to use XRCE client directly and not use https://github.com/micro-ROS/micro_ros_setup for example like https://github.com/micro-ROS/micro_ros_arduino

eMrazSVK · 2022-09-16T10:49:41Z

Hello, is there any progress regarding implementing direct DDS ROS2 communication with ardupilot? Thank you.

arshPratap · 2022-09-16T11:02:11Z

Hello, is there any progress regarding implementing direct DDS ROS2 communication with ardupilot? Thank you.

Hi @eMrazSVK ! I am so sorry I wasnt able to work on this in the recent time due to my current predicaments, but I would love to resume working on it and collaborating if you are intereseted! You can mail me : [email protected] for further information.In the mean time I would love if you could take a look here : https://github.com/arshPratap/ardupilot_ros2
and https://discuss.ardupilot.org/t/ardupilot-native-ros2-support/73720

eMrazSVK · 2022-09-16T11:05:11Z

Thank you for your quick response @arshPratap.

If I decided to collaborate on this, could you give me just a really rough estimate of how much time would be needed to fully implement this? Are we speaking weeks, months or years?

Thank you once again.

arshPratap · 2022-09-16T11:14:13Z

Thank you for your quick response @arshPratap.

If I decided to collaborate on this, could you give me just a really rough estimate of how much time would be needed to fully implement this? Are we speaking weeks, months or years?

Thank you once again.

I am not sure on the time frame as I believe there are few things currently in this project that I believe need to be taken care of first :

Working on the some of the dependencies to make sure a smooth installation of the project could be done

But I believe that this PR has been idle for a long time and I would love to see this merged as soon as possible. Also the fact that since you are interested to collaborate on this , makes me think we can work on this faster and i think we can have a month's time as a nice deadline on this. @eMrazSVK Let me know what you think...and lets start the work immediately

Ryanf55 · 2023-03-14T05:25:26Z

On Wed, 16 Jun 2021, Pierre Kancir wrote: You should see this as the mavlink xml, it is the definition to generate the AP_INS.cpp code that is auto generated
We don't commit the generated code in ArduPilot :-)

We generate the code now in waf, it shows up in the build directory at build time.

khancyr

partial review only.
Seems ok. build looks constraint.
test build are ok
documentation is readable for dev!

libraries/AP_DDS/AP_DDS_Client.cpp

peterbarker · 2023-03-19T06:17:20Z

libraries/AP_DDS/AP_DDS_Client.cpp

+
+void AP_DDS_Client::update_topic(builtin_interfaces_msg_Time* msg)
+{
+    if (msg != nullptr) {


Early return here rather than indent. Possibly doing an internal error first?

update_topic is called at a very high rate. Is it OK to do internal errors at that high rate?

Now that it's passed by reference, this is no longer an error condition.

peterbarker · 2023-03-19T06:18:29Z

libraries/AP_DDS/AP_DDS_Client.cpp

+
+#include "AP_DDS_Topic_Table.h"
+
+void AP_DDS_Client::update_topic(builtin_interfaces_msg_Time* msg)


Why not take a reference instead?!

Suggested change

void AP_DDS_Client::update_topic(builtin_interfaces_msg_Time* msg)

void AP_DDS_Client::update_topic(builtin_interfaces_msg_Time& msg)

Done in fc514d56571d59e90a906fd36699ae0219e68973

Ryanf55 · 2023-03-21T00:50:19Z

Once CI passes the current build, I'll do a final cleanup on the commit history and tag for merge on build success. All PR feedback is now done. Re-tested fine after final cleanup.

peterbarker · 2023-03-21T00:54:44Z

libraries/AP_DDS/AP_DDS_Client.cpp

+    msg.sec = utc_usec / 1000000ULL;
+    msg.nanosec = (utc_usec % 1000000ULL) * 1000UL;


Suggested change

msg.sec = utc_usec / 1000000ULL;

msg.nanosec = (utc_usec % 1000000ULL) * 1000UL;

msg = {

utc_usec / 1000000ULL,

(utc_usec % 1000000ULL) * 1000UL

};

Just a suggestion that you play with this. It means the structure is fully initialised on return, and gives you some type-checking too.

It sadly loses the context of the first element being seconds and the second element being nanoseconds. Unless you could use C++20's aggregate initialization that supports designated initializers. Maybe there a way to get the best of both worlds 🔍

* Use interal forks Signed-off-by: Ryan Friedman <[email protected]> Co-authored-by: Arsh Pratap <[email protected]> Co-authored-by: Andrew Tridgell <[email protected]> Co-authored-by: Russ Webber <[email protected]>

Signed-off-by: Ryan Friedman <[email protected]> Co-authored-by: Arsh Pratap <[email protected]> Co-authored-by: Andrew Tridgell <[email protected]>

Signed-off-by: Ryan Friedman <[email protected]> Co-authored-by: Rhys Mainwaring <[email protected]> Co-authored-by: Arsh Pratap <[email protected]> Co-authored-by: Andrew Tridgell <[email protected]>

* Use clang to verify no unused files * Add a topic table to prepare for code generating interfaces * Generated IDL's to to a generated directory in build * Use black to format python files * Populate a ROS time maessage with Linux epoch time for ROS time * Add workarounds for PoseStamped and TwistStamped with manual mods to IDL Signed-off-by: Ryan Friedman <[email protected]> Co-authored-by: Rhys Mainwaring <[email protected]> Co-authored-by: Arsh Pratap <[email protected]> Co-authored-by: Andrew Tridgell <[email protected]> Co-authored-by: Russ Webber <[email protected]> Co-authored-by: Peter Barker <[email protected]>

Signed-off-by: Ryan Friedman <[email protected]> Co-authored-by: Arsh Pratap <[email protected]> Co-authored-by: Andrew Tridgell <[email protected]> Co-authored-by: Russ Webber <[email protected]>

* Had to ignore pre-commit hooks for isort and mypy since there are unrelated broken issues in the file Signed-off-by: Ryan Friedman <[email protected]> Co-authored-by: Arsh Pratap <[email protected]> Co-authored-by: Andrew Tridgell <[email protected]>

* Disabled by default Signed-off-by: Ryan Friedman <[email protected]> Co-authored-by: Arsh Pratap <[email protected]> Co-authored-by: Andrew Tridgell <[email protected]>

Signed-off-by: Ryan Friedman <[email protected]>

Signed-off-by: Ryan Friedman <[email protected]> Co-authored-by: Pierre Kancir <[email protected]>

russkel · 2023-03-21T22:27:40Z

🎊 🥳

jmachuca77 added ROS WIP labels Jun 15, 2021

jmachuca77 reviewed Jun 15, 2021

View reviewed changes

ArduCopter/xrce-client.cpp Outdated Show resolved Hide resolved

khancyr reviewed Jun 15, 2021

View reviewed changes

rishabsingh3003 added the GSoC Google Summer of Code label Jun 16, 2021

jmachuca77 requested review from tridge and peterbarker June 16, 2021 14:45

peterbarker requested changes Jun 17, 2021

View reviewed changes

arshPratap force-pushed the ddsPrototype branch 5 times, most recently from af7d461 to ca2728d Compare July 14, 2021 13:49

tridge force-pushed the ddsPrototype branch from ca2728d to b289a13 Compare July 16, 2021 06:11

arshPratap force-pushed the ddsPrototype branch 3 times, most recently from d07b90f to bfce51a Compare August 10, 2021 23:44

arshPratap force-pushed the ddsPrototype branch 3 times, most recently from a8c1c7b to 3c9882f Compare August 19, 2021 14:25

rmackay9 mentioned this pull request May 24, 2022

AP + ROS interface #20830

Closed

andyp1per removed the DevCallEU label Mar 15, 2023

Ryanf55 requested a review from peterbarker March 16, 2023 03:19

Ryanf55 added the DevCallTopic label Mar 20, 2023

khancyr reviewed Mar 20, 2023

View reviewed changes

CraigElder removed the DevCallTopic label Mar 20, 2023

peterbarker approved these changes Mar 20, 2023

View reviewed changes

tridge approved these changes Mar 20, 2023

View reviewed changes

tridge added the MergeOnCIPass label Mar 20, 2023

Ryanf55 removed the MergeOnCIPass label Mar 21, 2023

peterbarker reviewed Mar 21, 2023

View reviewed changes

Ryanf55 and others added 12 commits March 21, 2023 08:00

.gitmodules: Add DDS dependencies

414670a

* Use interal forks Signed-off-by: Ryan Friedman <[email protected]> Co-authored-by: Arsh Pratap <[email protected]> Co-authored-by: Andrew Tridgell <[email protected]> Co-authored-by: Russ Webber <[email protected]>

AP_HAL_ChibiOS: Add poll implementation

c96f64b

Signed-off-by: Ryan Friedman <[email protected]> Co-authored-by: Arsh Pratap <[email protected]> Co-authored-by: Andrew Tridgell <[email protected]>

AP_SerialManager: Add enum for DDS over serial

dc4d9e6

Signed-off-by: Ryan Friedman <[email protected]> Co-authored-by: Rhys Mainwaring <[email protected]> Co-authored-by: Arsh Pratap <[email protected]> Co-authored-by: Andrew Tridgell <[email protected]>

AP_Vehicle: Add DDS initialization and params to the vehicle if enabled

a53d6c9

Signed-off-by: Ryan Friedman <[email protected]> Co-authored-by: Arsh Pratap <[email protected]> Co-authored-by: Andrew Tridgell <[email protected]> Co-authored-by: Russ Webber <[email protected]>

autotest: Add option to enable dds

9dbe5e7

* Disabled by default Signed-off-by: Ryan Friedman <[email protected]> Co-authored-by: Arsh Pratap <[email protected]> Co-authored-by: Andrew Tridgell <[email protected]>

pre-commit: Add python formatter for wscript and XML formatter

e4232b6

Signed-off-by: Ryan Friedman <[email protected]>

waf: Remove unused XRCE_DEPENDENCIES

5123d77

Signed-off-by: Ryan Friedman <[email protected]>

Tools: CI: Add building DDS to a STM32H7 board

f586ad5

Signed-off-by: Ryan Friedman <[email protected]>

Tools: split dds test into SITL and STM

0bc194d

.github: add dds testing workflow

d4e87f3

Signed-off-by: Ryan Friedman <[email protected]> Co-authored-by: Pierre Kancir <[email protected]>

Ryanf55 force-pushed the ddsPrototype branch from fc514d5 to d4e87f3 Compare March 21, 2023 14:07

Ryanf55 added the MergeOnCIPass label Mar 21, 2023

tridge merged commit d05b598 into ArduPilot:master Mar 21, 2023

Ryanf55 mentioned this pull request Jul 18, 2023

AP_DDS: Switch to maybe-unused instead of void casts #24156

Closed

srmainwaring removed the MergeOnCIPass label Feb 27, 2024

		if (this->use_serial) {
		this->fd=open("/dev/ttyUSB1", O_RDWR \| O_NOCTTY \| O_NONBLOCK);


		#include "AP_DDS_Topic_Table.h"

		void AP_DDS_Client::update_topic(builtin_interfaces_msg_Time* msg)

		msg.sec = utc_usec / 1000000ULL;
		msg.nanosec = (utc_usec % 1000000ULL) * 1000UL;

Dds prototype #17779

Dds prototype #17779

Conversation

arshPratap commented Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbarker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ryanf55 Feb 24, 2023 • edited Loading

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

Choose a reason for hiding this comment

Ryanf55 Mar 14, 2023 • edited Loading

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

peterbarker commented Jun 18, 2021 via email

jmachuca77 commented Jun 23, 2021

tridge commented Jul 16, 2021

tridge commented Jul 16, 2021

amfern commented Sep 6, 2021

eMrazSVK commented Sep 16, 2022

arshPratap commented Sep 16, 2022

eMrazSVK commented Sep 16, 2022

arshPratap commented Sep 16, 2022 • edited Loading

Ryanf55 commented Mar 14, 2023

khancyr left a 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

Ryanf55 commented Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Ryanf55 Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

russkel commented Mar 21, 2023

arshPratap commented Jun 15, 2021 •

edited

Loading

Ryanf55 Feb 24, 2023 •

edited

Loading

Ryanf55 Mar 14, 2023 •

edited

Loading

arshPratap commented Sep 16, 2022 •

edited

Loading

Ryanf55 commented Mar 21, 2023 •

edited

Loading

Ryanf55 Mar 21, 2023 •

edited

Loading