SITL: Fix SIM_SPEEDUP -1 #30229
SITL: Fix SIM_SPEEDUP -1 #30229
Conversation
|
Thanks Tom - I've found that using the default on Mac crashes SITL. I always set |
| @@ -1057,7 +1057,7 @@ bool AP_Logger_File::io_thread_alive() const | |||
| // disk. Unfortunately these hardware devices do not obey our | |||
| // SITL speedup options, so we allow for it here. | |||
| SITL::SIM *sitl = AP::sitl(); | |||
| if (sitl != nullptr) { | |||
| if (sitl != nullptr && sitl->speedup > 0) { | |||
There was a problem hiding this comment.
Shouldn't we be using get_speedup() here?
Tim pointed out that he uses speedup on the command-line; if he does that then the number used here would be incorrect?
There was a problem hiding this comment.
Yes, using get_speedup() would be better but I didn't see any easy way for AP_Logger to reach down into a SIM::AIRCRAFT class.
| @@ -228,7 +228,7 @@ const AP_Param::GroupInfo SIM::var_info[] = { | |||
| // @Description: Runs the simulation at multiples of normal speed. Do not use if realtime physics, like RealFlight, is being used | |||
| // @Range: 1 10 | |||
| // @User: Advanced | |||
| AP_GROUPINFO("SPEEDUP", 52, SIM, speedup, -1), | |||
| AP_GROUPINFO("SPEEDUP", 52, SIM, speedup, 1), | |||
There was a problem hiding this comment.
This -1 is used elsewhere in the code as a flag value, eg.
if (!is_equal(last_speedup, float(sitl->speedup)) && sitl->speedup > 0) {
maybe better to just use the method in the logger library and leave this thing sleep...
| AP_GROUPINFO("SPEEDUP", 52, SIM, speedup, 1), | |
| AP_GROUPINFO("SPEEDUP", 52, SIM, speedup, -1), |
There was a problem hiding this comment.
yeah.. I dunno. I saw that code there, it's literally the only other place looking at the param. Seems convoluted for a run-once scheme. I think using a negative number is just bad practice here. For example, this -1 value would only here here on the first run after a --wipe but not the second run.. so what is it accomplishing?
| if (sitl != nullptr && sitl->speedup > 0) { | ||
| timeout_ms *= sitl->speedup; |
There was a problem hiding this comment.
| if (sitl != nullptr && sitl->speedup > 0) { | |
| timeout_ms *= sitl->speedup; | |
| if (sitl != nullptr) { | |
| timeout_ms *= sitl->get_speedup(); |
|
OK, correct fix is probably to move all the time handling code ( We should probably merge this this-century, however. @timtuxworth are you able to test this PR? |
|
I talked to @peterbarker about this and we both agree the better solution is to go through get_speedup() which I tried to use but we can't reqch that from our normal libraries so it requires larger infrastructure changes that start to become out of scope so this is good as-is for now until we get those larger infrastructure changes at a later time. Just waiting on a test confirmation from @timtuxworth to merge. |
magicrub
force-pushed
the
pr/sitl_speedup_default
branch
from
2db3215 to
71e6777
Compare
|
Might take me a couple of days, I'm on the road, but I'll test it as soon as I get back to Edmonton likely Monday/Tuesday. |
|
This works. Tested in SITL on Mac M1 Pro. Without this fix SITL will crash unless you pass --speedup 1 on the command line (which sim_vehicle.py does by default, which is a workaround to this problem). Now running a SITL instance of ardupilot on a Mac doesn't crash. |
|
Merged, thanks! |
magicrub
force-pushed
the
pr/sitl_speedup_default
branch
from
71e6777 to
1f2b68b
Compare
|
@timtuxworth Thanks for testing! |
If you start a sim without setting param SIM_SPEEDUP then it will default to -1. That can't be good. Luckily, what's actually used in the SITL code is target_speedup that can be fetched via get_speedup() within the Airplane class and that set_speedup() at boot does a sanity check on the param. However, AP_Logger looks directly at the param value and sets a timeout depending on that.
When it's negative it hangs the logio thread.
This PR does two things:
change SIM_SPEEDUP default from -1 to +1
sanity check sitl->speedup in logger. It's the only place in the code that uses it