RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.24k stars 1.25k forks source link

Periodic events don't work at negative times #13439

Open sherm1 opened 4 years ago

sherm1 commented 4 years ago

Soliciting thoughts on this:

In PR #13061 @RussTedrake enabled our integrators to work even with time t < 0 (though still moving forward only). However, periodic events are defined to issue only for times tₙ = offset + n * period with offset, period, and n all ≥ 0. This has the effect that periodic events are disabled until t ≥ offset. Russ says he was expecting that periodic events would still issue with negative times: "I always thought of it more like a phase shift, not an absolute starting time."

With time allowed to be negative it would make more sense to relax the restriction n ≥ 0 and treat offset just as a phase shift and not as a start time. OTOH that would be a behavioral change that would break any code that was using offset as a start time.

Several possible fixes come to mind:

Any thoughts about this? Would you be sad to lose the start time functionality?

cc @edrumwri

jwnimmer-tri commented 4 years ago

Surveying the uses of periodic events in Anzu, I see these use cases for periodic events in evidence:

Type Offset Dual-sided Purpose
publish 0 yes telemetry / logging
discrete update 0 yes dynamics / control
unrestricted update positive constant no a kind of requirements monitor that is disabled during a warm-up period

In the "dual-sided" yes / no column here, I mean "Should the event should fire for times prior to the offset?".

There is only one instance of the last row. If periodic events become dual-sided by default, we could easily update that one instance to have if (time < offset) { return; } as the first line of the calc callback. (We would probably also set the phase to zero; having the warm-up guard aligned to the event times is not strictly required, so the next multiple of period after the offset is equally good.)

jwnimmer-tri commented 3 years ago

\CC @dmcconachie-tri and I were chatting today, and it came up that this might be a useful improvement for a certain use case.

The general idea is that we wanted to generate traces of state and/or outputs (via publish events into a logger) over some 10-second period of interest for offline analysis. However, we also want to allow some settling time for the state to reach near-zero velocities, in case our initial conditions were not stable.

We could simulate and log for 20 seconds and then snip out the prefix of the log we didn't want, probably also rewriting the timestamps to range from [0, 10] again instead of [10, 20]. For the ease of offline analysis, we want to have uniform timestamps for the trajectory of interest, even if the amount of settling time is different when conditions change.

One cute way we could implement this is to begin the simulation at t = -max_settling_time, but only start logging at t >= 0. That means we'd want our discrete updates to fire dual-sided to allow the systems to settle, but the loggers we're using would only start at t >= 0.

Because events (especially MbP steps!) don't fire at negative times, we can't actually do this trick today, but it might be nice to have this at hand in the future.

sherm1 commented 3 years ago

I think this would have been a better definition for periodic events -- missing those MbP steps during negative time is a real drag! There would be a little pain for a while after changing the behavior but it would be minor (unless someone didn't know to do it!).