Add support for simulators running on a different host than the PX4 instance #15965

FernandezR · 2020-10-14T17:40:12Z

Please use PX4 Discuss or Slack to align on pull requests if necessary. You can then open draft pull requests to get early feedback.

Describe problem solved by this pull request
Allows for the use of simulators running on a different host than the PX4 instance.

Describe your solution
Added use of PX4_SIM_HOSTNAME and PX4_SIM_HOST_ADDR environment variables to pass simulator hostname or ip address through the posix rcs script to the simulator module.

Describe possible alternatives
Didn't consider any other alternatives as this was the simplest implementation.

Test data / coverage
Tested using a docker compose implementation using a a private Unity Simulation, where simulator and PX4 instances exist in different containers. Also, tested on local machine to ensure continued support of localhost simulator.

Additional context
None

Jaeyoung-Lim

Thanks for the contribution!

I was curious if the testing done also with the UDP connection? and a few minor comments in line

Also the SITL tests are failing. Do you mind rebasing the PR?

Jaeyoung-Lim · 2020-10-15T09:14:28Z

ROMFS/px4fmu_common/init.d-posix/rcS

+    simulator start -c $simulator_tcp_port
+  else
+    echo "PX4 SIM HOST: $PX4_SIM_HOST"
+    simulator start -c $simulator_tcp_port $PX4_SIM_HOST


Wouldn't it make more sense if the host is passed through a flag as it is done with the ports?

I didn't use a flag since currently the flag is only used to specify the protocol. I decided to use the length of the arguments as it was already being used to verify that the port was passed through.

src/modules/simulator/simulator_mavlink.cpp

Jaeyoung-Lim · 2020-10-15T09:21:49Z

src/modules/simulator/simulator.cpp

@@ -95,6 +107,8 @@ static void usage()
 	PX4_INFO("Start simulator:     simulator start");
 	PX4_INFO("Connect using UDP: simulator start -u udp_port");
 	PX4_INFO("Connect using TCP: simulator start -c tcp_port");
+	PX4_INFO("Connect using UDP: simulator start -u udp_port hostname");


Is this use case valid / tested?

The UDP version hasn't been tested, I don't have a simulator at this time that uses a UDP connection.

If it is not tested, would it be possible to disable it?

Yes, I can remove it for now or comment it out. Which is preferred?

Can you also make it so that you support passing an explicit ip address instead of hostname ? In some cases (like windows subsystem for linux WSL2) the host name is the same for the linux and windows machines, but the ip addresses are different. Explicit ip address can also be handy to select a specific ethernet interface which can be handy when there's multiple ways to reach the host (e.g. wifi versus hard wired).

FernandezR · 2020-10-15T12:25:11Z

What commit should I rebase the PR against? I was working off the latest commit in master. The two test that were failing were complaining about not being able to connect to a display.

Jaeyoung-Lim · 2020-10-15T12:37:42Z

The two test that were failing were complaining about not being able to connect to a display.

I don't think this is accurate since it is reliably passing on other PRs - can you hvae a look?

FernandezR · 2020-10-15T13:25:02Z

I added the couple of commits that occurred since I made this PR. I'm waiting to see if all the tests pass now.

FernandezR · 2020-10-15T14:46:44Z

Two tests are still failing but it is not caused by the changes I made.

The MacOS build test is failing due to a failed python import of the toml library.
The SITL Test is failing due to this error: [gzserver ] [Err] [RenderEngine.cc:744] Can't open display: :99

Jaeyoung-Lim · 2020-10-15T14:58:30Z

@FernandezR I am pretty sure that the following error is not causing the SITL test to fail, if you look at other PRs the tests pass even though it displays the same message

[gzserver    ] [Err] [RenderEngine.cc:744] Can't open display: :99

FernandezR · 2020-10-15T15:40:05Z

@Jaeyoung-Lim The specific tests that are not passing are these:

standard_vtol:
- 'Fly forward in position control': failed
- 'Fly forward in altitude control': cancelled

I'm not sure how I'm supposed to fix these tests failing as nothing I changed is impacting them. Looking in the logs the simulator connection is established on localhost as expected.

And I can't do anything to solve the MacOS test that is failing as it has something to do with the CI environment as far as I can tell.

FernandezR · 2020-10-15T16:05:51Z

This seems to be the cause of the failure:

[mavsdk_tests] ../../../test/mavsdk_tests/autopilot_tester.cpp:106: FAILED:
[mavsdk_tests] CHECK( ground_truth_horizontal_position_far_from(_home, min_distance_m) )
[mavsdk_tests] with expansion:
[mavsdk_tests] false

FernandezR · 2020-10-15T16:17:02Z

The latest commit on master seems to be failing the same SITL test

FernandezR · 2020-10-19T16:39:12Z

@Jaeyoung-Lim Do you have any suggestions of how to debug the failing test? I'm not sure why this assertion, CHECK( ground_truth_horizontal_position_far_from(_home, min_distance_m) ),would be failing.

FernandezR · 2020-11-09T23:51:28Z

@Jaeyoung-Lim It looks like all the checks finally passed.

Jaeyoung-Lim

Thanks for looking into the failures, and sorry for the late response.

Looks good to me on my side! Thanks!

dayjaby · 2021-01-22T15:59:25Z

@FernandezR can you align this PR with the changes made in #15443? I like your addition of an environment variable to change the IP.

FernandezR · 2021-01-25T19:43:08Z

@FernandezR can you align this PR with the changes made in #15443? I like your addition of an environment variable to change the IP.

Yes, I will work on aligning with change to support both ip address and hostname for remote simulator.

… address or hostname.

FernandezR · 2021-01-25T21:25:09Z

I updated the request to align to PR #15443

FernandezR · 2021-01-26T14:41:22Z

Failed tests are failing due to this error.

Run ./Tools/upload_log.py -q --description "${GITHUB_WORKFLOW} ${GITHUB_RUN_ID}" --feedback "${GITHUB_WORKFLOW} ${GITHUB_RUN_ID} ${GITHUB_REPOSITORY} ${GITHUB_REF}" --source CI ~/.ros/log/*/*.ulg
Traceback (most recent call last):
  File "./Tools/upload_log.py", line 127, in <module>
    main()
  File "./Tools/upload_log.py", line 115, in main
    with open(file_name, 'rb') as f:
FileNotFoundError: [Errno 2] No such file or directory: '/github/home/.ros/log/*/*.ulg'
Error: Process completed with exit code 1.

I'm not sure why some tests were cancelled.

mrpollo · 2021-02-15T18:46:25Z

@Jaeyoung-Lim this PR looks like it would be super useful, and I think we should take over from @FernandezR, could you please branch out, fix the conflicts, and make a new PR? It would be the best way to go since the author already went out of his way to accommodate all of our requests.

FernandezR · 2021-02-15T19:16:56Z

@Jaeyoung-Lim this PR looks like it would be super useful, and I think we should take over from @FernandezR, could you please branch out, fix the conflicts, and make a new PR? It would be the best way to go since the author already went out of his way to accommodate all of our requests.

@mrpollo I have fixed the conflicts and updated to the latest commit in master. Do you still want another pr? This one was updated with the changes I just made automatically.

Some test keep failing due to a missing ulg file that can't be found by the test. I'm not sure how that file is supposed to be created or why it is missing.

Also, someone updated the number of arguments being passed to the simulator script but it was incorrect during my testing. So the values are back to there original values.

mrpollo · 2021-02-15T19:30:14Z

@FernandezR we don't like merge commits from the main branch, we usually rebase, and to avoid further conflicts with the file you are modifying I have asked @Jaeyoung-Lim to help, he's probably creating a new PR to address the issue. Don't worry you will remain the author of the commits, he's just adding some on top.

lovettchris · 2021-03-30T22:51:28Z

I have merged the work Fernandez did with the other change that already added "-t" support for remote ip addresses. See #17275

FernandezR · 2021-04-07T19:23:29Z

@lovettchris Thanks for merging in my changes. Should we close this pull request now?

lovettchris · 2021-04-07T19:28:51Z

Yes I think I got all your changes in so if you are happy with what is in the master branch, go ahead and close this one.

FernandezR · 2021-04-07T19:40:45Z

Changes merged into master through pull request #17275.

dagar requested a review from Jaeyoung-Lim October 14, 2020 19:53

Jaeyoung-Lim requested a review from julianoes October 15, 2020 09:44

Jaeyoung-Lim reviewed Oct 15, 2020

View reviewed changes

Jaeyoung-Lim mentioned this pull request Nov 23, 2020

Would love to be able to run Simulator TCP over something other than localhost #16252

Closed

Jaeyoung-Lim previously approved these changes Nov 23, 2020

View reviewed changes

FernandezR closed this Jan 25, 2021

Update simulator module to support remote simulators specified via ip…

0308c8b

… address or hostname.

FernandezR reopened this Jan 25, 2021

FernandezR dismissed Jaeyoung-Lim’s stale review via 0308c8b January 25, 2021 21:25

Update with format corrections

d08768b

FernandezR requested a review from Jaeyoung-Lim January 26, 2021 14:39

Update through merge to master branch in main repo

bc84059

lovettchris mentioned this pull request Mar 30, 2021

Add support for simulators running on a different host than the PX4 instance #17275

Merged

FernandezR closed this Apr 7, 2021

kistlin mentioned this pull request May 19, 2021

PX4 in WSL2 + AirSim + QGC #17610

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for simulators running on a different host than the PX4 instance #15965

Add support for simulators running on a different host than the PX4 instance #15965

FernandezR commented Oct 14, 2020 •

edited

Loading

Jaeyoung-Lim left a comment •

edited

Loading

Jaeyoung-Lim Oct 15, 2020

FernandezR Oct 15, 2020

Jaeyoung-Lim Oct 15, 2020

FernandezR Oct 15, 2020

Jaeyoung-Lim Oct 15, 2020

FernandezR Oct 15, 2020

lovettchris Nov 23, 2020

FernandezR commented Oct 15, 2020

Jaeyoung-Lim commented Oct 15, 2020

FernandezR commented Oct 15, 2020

FernandezR commented Oct 15, 2020

Jaeyoung-Lim commented Oct 15, 2020

FernandezR commented Oct 15, 2020

FernandezR commented Oct 15, 2020 •

edited

Loading

FernandezR commented Oct 15, 2020

FernandezR commented Oct 19, 2020

FernandezR commented Nov 9, 2020

Jaeyoung-Lim left a comment

dayjaby commented Jan 22, 2021

FernandezR commented Jan 25, 2021

FernandezR commented Jan 25, 2021

FernandezR commented Jan 26, 2021 •

edited by mrpollo

Loading

mrpollo commented Feb 15, 2021

FernandezR commented Feb 15, 2021 •

edited

Loading

mrpollo commented Feb 15, 2021

lovettchris commented Mar 30, 2021

FernandezR commented Apr 7, 2021

lovettchris commented Apr 7, 2021

FernandezR commented Apr 7, 2021

Add support for simulators running on a different host than the PX4 instance #15965

Add support for simulators running on a different host than the PX4 instance #15965

Conversation

FernandezR commented Oct 14, 2020 • edited Loading

Jaeyoung-Lim left a comment • edited Loading

Choose a reason for hiding this comment

Jaeyoung-Lim Oct 15, 2020

Choose a reason for hiding this comment

FernandezR Oct 15, 2020

Choose a reason for hiding this comment

Jaeyoung-Lim Oct 15, 2020

Choose a reason for hiding this comment

FernandezR Oct 15, 2020

Choose a reason for hiding this comment

Jaeyoung-Lim Oct 15, 2020

Choose a reason for hiding this comment

FernandezR Oct 15, 2020

Choose a reason for hiding this comment

lovettchris Nov 23, 2020

Choose a reason for hiding this comment

FernandezR commented Oct 15, 2020

Jaeyoung-Lim commented Oct 15, 2020

FernandezR commented Oct 15, 2020

FernandezR commented Oct 15, 2020

Jaeyoung-Lim commented Oct 15, 2020

FernandezR commented Oct 15, 2020

FernandezR commented Oct 15, 2020 • edited Loading

FernandezR commented Oct 15, 2020

FernandezR commented Oct 19, 2020

FernandezR commented Nov 9, 2020

Jaeyoung-Lim left a comment

Choose a reason for hiding this comment

dayjaby commented Jan 22, 2021

FernandezR commented Jan 25, 2021

FernandezR commented Jan 25, 2021

FernandezR commented Jan 26, 2021 • edited by mrpollo Loading

mrpollo commented Feb 15, 2021

FernandezR commented Feb 15, 2021 • edited Loading

mrpollo commented Feb 15, 2021

lovettchris commented Mar 30, 2021

FernandezR commented Apr 7, 2021

lovettchris commented Apr 7, 2021

FernandezR commented Apr 7, 2021

FernandezR commented Oct 14, 2020 •

edited

Loading

Jaeyoung-Lim left a comment •

edited

Loading

FernandezR commented Oct 15, 2020 •

edited

Loading

FernandezR commented Jan 26, 2021 •

edited by mrpollo

Loading

FernandezR commented Feb 15, 2021 •

edited

Loading