Adding Ardupilot SITL to MAVSDK CI #1669

ykhedar · 2022-01-15T13:35:49Z

Following discussion on Issue #728 for adding Ardupilot support in MAVSDK (related issues #1562 and #1568), this pull request adds Ardupilot SITL (Copter, Rover) along with necessary changes needed to run it successfully. Currently onnly 3/34 Integrations tests pass. Some notes about changes made:

Added Dockerfiles each for Copter and Rover Firmware versions 4.1.2
Edited the script for building and pushing the new docker files.
Edited run-sitl-tests.sh, start_px4_sitl.sh and stop_px4_sitl.sh to include ardupilot related sitl codes. May need to change the file names in the future. e.g. start_px4_sitl.sh -> start_sitl.sh.
Added code block for running ardupilot sitl into the github actions file.

Requesting Review and Suggestions,
ykhedar

P.S. Congrats on the v1.0 release of MAVSDK :)

JonasVautherin

Nice! It requires the APM containers to be pushed on the mavsdk docker registry, right? I see that the CI is failing to pull them at the moment.

tools/run-sitl-tests.sh

ykhedar · 2022-01-15T16:05:52Z

Nice! It requires the APM containers to be pushed on the mavsdk docker registry, right? I see that the CI is failing to pull them at the moment.

exactly. I did all the tests on my account with all images pushed to my docker hub repo. After everything was working, i quashed all the commits into one so as to keep the pull request clean.

julianoes

Looks quite clean, thank you @ykhedar. Do the tests actually succeed?

julianoes · 2022-01-17T06:08:23Z

tools/start_px4_sitl.sh

@@ -25,61 +25,119 @@ if [[ $AUTOSTART_SITL != 1 ]]; then
    exit 0
 fi

-if [[ -n "$PX4_FIRMWARE_DIR" ]]; then
-    px4_firmware_dir=$PX4_FIRMWARE_DIR
+# PX4 specific SITL Run Code


I agree we should change the name of these scripts.

julianoes

Looks quite clean, thank you @ykhedar. Do the tests actually succeed?

julianoes · 2022-01-17T06:33:03Z

I have built and pushed the docker images.

ykhedar · 2022-01-17T10:01:59Z

Looks quite clean, thank you @ykhedar. Do the tests actually succeed?

No. The tests are going on forever on the CI so I stopped the workflow. When I checked the logs, this was due to bad telemetry health condition inside a while loop in one of the tests for VTOL. Here is the link to the code:

action_transition_multicopter_fixedwing

Should we add maybe a break condition in this test instead of letting the while loop run forever? Maybe do the same for all such while loops in all the tests? Just make the test fail if the while condition is not met after 60 seconds or 120 seconds?

JonasVautherin · 2022-01-17T10:14:50Z

Maybe do the same for all such while loops in all the tests?

Generally I think it would make sense if the tests did not hang forever 👍.

This said, for the health, is it failing because of a difference with Ardupilot? Can we fix health_all_ok() easily?

julianoes · 2022-01-17T12:47:14Z

We should limit the tests to 45mins, like the PX4 SITL ones.

And if you want to go through all of them, feel free, but I think it's quite a bit of work.

Do the tests run through for you locally?

ykhedar · 2022-01-17T23:10:00Z

We should limit the tests to 45mins, like the PX4 SITL ones.

And if you want to go through all of them, feel free, but I think it's quite a bit of work.

Do the tests run through for you locally?

How does PX4 SITL implement the limit of 45 minutes? Cant seem to find the setting for this.

On my local machine i saw 3 tests pass (i swear :D) which now seems to be incorrect. My bad :( .

Should I add a simple timer using std::chrono in the while loop to wait for 30 seconds or so before the loop exits with a failed test? Or do you suggest something better?

JonasVautherin · 2022-01-17T23:14:10Z

How does PX4 SITL implement the limit of 45 minutes? Cant seem to find the setting for this.

I think @julianoes was referring to this.

julianoes · 2022-01-18T05:33:03Z

@ykhedar it would be best to use this function for checks with timeout:

MAVSDK/src/integration_tests/integration_test_helper.h

Lines 69 to 84 in 6ad7dbd

    
           template<typename Rep, typename Period> 
        
           bool poll_condition_with_timeout( 
        
               std::function<bool()> fun, std::chrono::duration<Rep, Period> duration) 
        
           { 
        
               // We need millisecond resolution for sleeping. 
        
               const std::chrono::milliseconds duration_ms(duration); 
        
               unsigned iteration = 0; 
        
               while (!fun()) { 
        
                   std::this_thread::sleep_for(duration_ms / 100); 
        
                   if (iteration++ >= 100) { 
        
                       return false; 
        
                   } 
        
               } 
        
               return true; 
        
           }

julianoes

Nice!

ykhedar · 2022-01-18T20:47:42Z

Nice!

Just made some changes to the commits (squashed them into one) and your review was made stale by git. Also fetched upstream and merged the new commits into ykhedar/MAVSDK.

Apparently 2 out of 34 tests are passing.

…test.

…cking echo statement from start_sitl_px4.sh

ykhedar · 2022-02-02T19:54:19Z

Thanks @JonasVautherin for your review. Do we need to get all the Ardupilot related Integration tests to succeed before merging this pull request?

JonasVautherin · 2022-02-02T21:14:27Z

We already use --gtest_filter in the script running the test. What about adding PX4 in the tests that only pass with PX4, and filter them out in the Ardupilot runs? Enabling a test for Ardupilot would then just require us to remove "PX4" from the test name, so they could be incrementally activated. What do you think?

ykhedar · 2022-02-03T08:30:28Z

Sounds like a good idea to me. I would do the necessary changes. Something like this:

TEST_F(SitlTest, ActionGoto) -> TEST_F(SitlTest, PX4ActionGoto) ?

ykhedar · 2022-02-03T10:53:42Z

Okay, I did the changes but it seems now some checks related to macOS-framework and ios are failing for some reason and i cannot think of why. Any ideas? :)

JonasVautherin · 2022-02-04T09:08:06Z

Grmbl I don't get why the ios build sometimes does that 😞

julianoes

Thanks a lot for your work @ykhedar!

ykhedar requested review from julianoes and JonasVautherin January 15, 2022 13:35

JonasVautherin reviewed Jan 15, 2022

View reviewed changes

tools/run-sitl-tests.sh Outdated Show resolved Hide resolved

julianoes reviewed Jan 17, 2022

View reviewed changes

julianoes previously approved these changes Jan 18, 2022

View reviewed changes

ykhedar dismissed julianoes’s stale review via 5b7205c January 18, 2022 19:28

ykhedar force-pushed the main branch 3 times, most recently from 2dc875b to 408ef24 Compare January 18, 2022 19:57

ykhedar and others added 6 commits February 1, 2022 08:10

Added support for Ardupilot SITL for MAVSDK integration test suite.

47d0f41

Delete sitl.yml

bf0e829

deleted line being called before env assigned.

325b6b4

added poll_condition_with_timeout for all health check conditions in …

ee5df22

…test.

1. added 45 minutes timeout for APM SITL github action 2. removed blo…

adce63b

…cking echo statement from start_sitl_px4.sh

Update proto and fix style

0067dc6

JonasVautherin force-pushed the main branch from 736f22c to 0067dc6 Compare February 1, 2022 07:11

JonasVautherin previously approved these changes Feb 1, 2022

View reviewed changes

renamed integration tests with PX4 suffix.

7723b15

ykhedar dismissed JonasVautherin’s stale review via 7723b15 February 3, 2022 09:09

ykhedar force-pushed the main branch from e3fab3b to dfa1732 Compare February 3, 2022 10:28

added only previously passing tests to Ardupilot.

80821cd

JonasVautherin force-pushed the main branch from dfa1732 to 80821cd Compare February 15, 2022 18:16

JonasVautherin approved these changes Feb 15, 2022

View reviewed changes

julianoes approved these changes Feb 15, 2022

View reviewed changes

julianoes merged commit 837bfbd into mavlink:main Feb 15, 2022

julianoes added the enhancement label Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Ardupilot SITL to MAVSDK CI #1669

Adding Ardupilot SITL to MAVSDK CI #1669

ykhedar commented Jan 15, 2022

JonasVautherin left a comment

ykhedar commented Jan 15, 2022

julianoes left a comment

julianoes Jan 17, 2022

julianoes left a comment

julianoes commented Jan 17, 2022

ykhedar commented Jan 17, 2022

JonasVautherin commented Jan 17, 2022

julianoes commented Jan 17, 2022

ykhedar commented Jan 17, 2022

JonasVautherin commented Jan 17, 2022

julianoes commented Jan 18, 2022

julianoes left a comment

ykhedar commented Jan 18, 2022

ykhedar commented Feb 2, 2022

JonasVautherin commented Feb 2, 2022

ykhedar commented Feb 3, 2022

ykhedar commented Feb 3, 2022

JonasVautherin commented Feb 4, 2022

julianoes left a comment

Adding Ardupilot SITL to MAVSDK CI #1669

Adding Ardupilot SITL to MAVSDK CI #1669

Conversation

ykhedar commented Jan 15, 2022

JonasVautherin left a comment

Choose a reason for hiding this comment

ykhedar commented Jan 15, 2022

julianoes left a comment

Choose a reason for hiding this comment

julianoes Jan 17, 2022

Choose a reason for hiding this comment

julianoes left a comment

Choose a reason for hiding this comment

julianoes commented Jan 17, 2022

ykhedar commented Jan 17, 2022

JonasVautherin commented Jan 17, 2022

julianoes commented Jan 17, 2022

ykhedar commented Jan 17, 2022

JonasVautherin commented Jan 17, 2022

julianoes commented Jan 18, 2022

julianoes left a comment

Choose a reason for hiding this comment

ykhedar commented Jan 18, 2022

ykhedar commented Feb 2, 2022

JonasVautherin commented Feb 2, 2022

ykhedar commented Feb 3, 2022

ykhedar commented Feb 3, 2022

JonasVautherin commented Feb 4, 2022

julianoes left a comment

Choose a reason for hiding this comment