-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
mavsdk tests: use tester sleep_for function #24266
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Autsch, good find. Looks very reasonable to me how you changed it. Have a look at the format CI failures though.
the previously used std::this_thread::sleep_for is with respect to host system time which is different from autopilot time if: - speed factor != 1 - something runs slower than realtime regardless of speed factor - debugging or otherwise interrupting PX4 control code tester.sleep_for (which already existed) correctly sleeps w.r.t. px4/simulation time.
@@ -62,8 +62,7 @@ TEST_CASE("Control Allocation - Remove one motor", "[controlallocation]") | |||
tester.check_tracks_mission(5.f); | |||
tester.store_home(); | |||
tester.enable_actuator_output_status(); | |||
std::this_thread::sleep_for(std::chrono::seconds( | |||
1)); // This is necessary for the takeoff altitude to be applied properly | |||
tester.sleep_for(std::chrono::seconds(1)); // Necessary for the takeoff altitude to be applied properly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this find. I am still not sure if this is a proper solution. The reason is i am not sure why this sleep is needed at all. I agree that the std::this_thread::sleep_for is dumb and can screw up timings on a faster than real time scenario. But from the comment, if they want to make sure that the altitude takeoff is applied, it should sent a command to set the altitude and then get feedback that it is applied and then continue not sleep for an arbitrary amount (also then scaling with the speed factor might not work as then the comm is not done yet and you continue before PX4 receives the command).
But checking the tester.set_takeoff_altitude(flight_altitude); function it actually does check if the parameter is properly set. So can we test if the sleep is needed at all? Because the comment then oes not make sense to me.
Thanks @mbjd, doing everything with respect to simulation time was definitely needed. |
Some more thoughts on this. Feel free to merge this if you see fit, however as @KonradRudin points out it would probably be wiser to not rely on "blind" sleep statements in any tests, and rather wait for the specific needed condition. There are some additional timing fixes in #24237, speeding up the polling frequency in proportion to the speed factor, which fixes another bunch of timing issues arising at large speed factor. IMO though polling is generally a suboptimal solution. Ideally I imagine some sort of interrupt/callback thing: After each lockstep loop we send a callback to the tester, so it knows to check all relevant conditions. This would ensure nothing would ever be missed regardless of simulation speed, and would achieve that with the minimum possible number of checks. |
Solved Problem
Some mavsdk tests currently have hardcoded sleep statements using
std::this_thread::sleep_for
. This leads to test behaviour depending heavily on the speed with which the simulation runs -- if we run at a large speed factor, a (real-time) sleep of only a few seconds may already trigger all kinds of timeouts in PX4.Solution
We instead use the function
AutopilotTester::sleep_for
which sleeps w.r.t. PX4 simulation time, not host system time. This should lead to more reliable test behaviour across different speed factors.Alternatives
Open to all suggestions & reasons why this might not be such a great idea.
Context
Should we set up some CI warning that reminds anyone putting real-time sleeps in tests of this footgun?