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

ROS2 - ardupilot_sitl launch file to include a "out" parameter for Mavproxy #25724

Conversation

tizianofiorenzani
Copy link
Contributor

The ROS2 ardupilot_sitl launch file is currently setting the --out argument to mavproxy as default to localhost:14550. This PR allows to change the default output, in case for example we launch the node on a docker container and we want to connect to a Ground Station running on the host.

How to test:
We can follow the example provided in https://ardupilot.org/dev/docs/ros2-sitl.html and launch the sitl with:

ros2 launch ardupilot_sitl sitl_dds_udp.launch.py transport:=udp4 refs:=$(ros2 pkg prefix ardupilot_sitl)/share/ardupilot_sitl/config/dds_xrce_profile.xml synthetic_clock:=True wipe:=False model:=quad speedup:=1 slave:=0 instance:=0 defaults:=$(ros2 pkg prefix ardupilot_sitl)/share/ardupilot_sitl/config/default_params/copter.parm,$(ros2 pkg prefix ardupilot_sitl)/share/ardupilot_sitl/config/default_params/dds_udp.parm sim_address:=127.0.0.1 master:=tcp:127.0.0.1:5760 sitl:=127.0.0.1:5501

This should output something like:
image
which shows that, by default, the output is still localhost:14550

We can then add the option out:=udp:<any_ip_address>:<any_port>, and verify that the output to the launch file becomes:
image

We can then use either mavproxy or any GUI to connect to the UDP stream. In my case, I went from my Ubuntu Virtual Machine to my Windows host where I run QGCS:
image

@Ryanf55 Ryanf55 self-assigned this Dec 7, 2023
@Ryanf55 Ryanf55 self-requested a review December 7, 2023 23:42
@Ryanf55 Ryanf55 added the ROS label Dec 7, 2023
@Ryanf55
Copy link
Collaborator

Ryanf55 commented Dec 8, 2023

@srmainwaring - Do you know the context for why this was commented out earlier? I like this change.

@srmainwaring
Copy link
Contributor

srmainwaring commented Dec 8, 2023

Do you know the context for why this was commented out earlier?

It's most likely an oversight from the original development. mavproxy.py will accept multiple --out parameters, however ROS 2 launch scripts do not. IIRC I fixed the single parameter --out parameter during development, did not find a solution for handling multiple --outs, and forgot to leave the one available that could be set.

@tizianofiorenzani thanks for your contribution. It's good to have this as available as an argument so will add my approval as well.


# Create action.
mavproxy_process = ExecuteProcess(
cmd=[
[
f"{command} ",
"--out ",
"127.0.0.1:14550 ",
f"--out {out} ",
Copy link
Contributor

Choose a reason for hiding this comment

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

should use a list as argument so you can add as much as you want .
That will be handly to remove the second --out too.

And it probably need default value to "127.0.0.1:14550 " in case nothing is passed to retain the current behavior

Copy link
Contributor

Choose a reason for hiding this comment

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

And it probably need default value to "127.0.0.1:14550 " in case nothing is passed to retain the current behavior

That's already handled by the generate_launch_arguments method which supplies a default for a single --out param.

should use a list as argument so you can add as much as you want .

Yep. Could revisit the current launch methods in a follow up PR and apply this consistently. Will prob need to handle both list and single value as fallback to support existing usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the list of --out, in general you may want to pass more than one argument at a time. And yes, the localhost:14550 is default, so you don't need to add it. One option could be to have localhost anyway and add an external --out argument in case it was different

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Commit list is bad.

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Feb 10, 2024

Commit list is bad.

When are we gonna get an automated github action for this

@srmainwaring
Copy link
Contributor

@Ryanf55 can you please rebase and remove the merge commit.

@Ryanf55 Ryanf55 force-pushed the ros2-launch-sitl-with-out-option branch from e79ae35 to da6c506 Compare February 12, 2024 05:18
@Ryanf55
Copy link
Collaborator

Ryanf55 commented Feb 12, 2024

@Ryanf55 can you please rebase and remove the merge commit.

Done.

@peterbarker peterbarker merged commit c864047 into ArduPilot:master Feb 13, 2024
92 checks passed
@peterbarker
Copy link
Contributor

Merged, thanks

@rmackay9
Copy link
Contributor

This is included in Copter-4.5.7-beta1 as part of PR #27919

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 4.5.7-beta1
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants