Modernize event handling in SignalLogger

[sherm1]

Rather than overriding the DoPublish() dispatcher, this provides for per-step, periodic, or forced-event logging with a local handler. Also improves documentation to mention existing flaws in performance and thread safety.

This change was motivated by #10346, where the pedagogical examples are still saddled with this unwanted boilerplate:

simulator.set_publish_every_time_step(false);
simulator.set_publish_at_initialization(false);

That's to avoid getting extra per-step log entries when SignalLogger is set to publish periodically. With the change here per-step, publish, and forced events are mutually exclusive and the above lines are no longer necessary in the examples. They are removed here.

The forced-event-only feature is present for backwards compatibility with existing code (LcmDrivenLoop depends on it); it is not expected to be used much in practice. However, it is now using the recommended local-method handling so is not deprecated and is fine to leave in place.

Category            added  modified  removed  
----------------------------------------------
code                60     13        13       
comments            50     19        3        
blank               20     0         1        
----------------------------------------------
TOTAL               130    32        17       

---

This change is 

[sherm1]

Consider MITLS as a cure for SignalLogger's thread safety troubles.

[sherm1]

+@edrumwri for feature review, please
+@jwnimmer-tri for platform review please, per rotation
cc @RussTedrake for the final discrete example cleanup

(please see PR description for info)

Reviewable status: all discussions resolved, LGTM missing from assignee sherm1, platform LGTM missing

[edrumwri]

Reviewed 8 of 9 files at r1, 1 of 1 files at r2.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee edrumwri, LGTM missing from assignee sherm1, platform LGTM missing

---

systems/primitives/signal_log.h, line 10 at r2 (raw file):

/**
 This class serves as an in-memory cache of time-dependent vector values.

FYI Since you're cleaning up this documentation, please consider adding some class documentation that distinguishes between SignalLog and SignalLogger. From a quick examination of the two classes, it's not obvious when a user should use one vs. the other (it's only by the absence of the ports that I saw that this one doesn't plug into a System, which also makes me question why it's located under systems/primitives). It seems that the only use of SignalLog is in SignalLogger, making me think that this class is a good candidate for hiding.

---

systems/primitives/signal_log.h, line 40 at r2 (raw file):

  }

  /** Reset the logged data. */

FYI Please consider updating the reset(.) comment here like you do in SignalLogger.

---

systems/primitives/signal_logger.h, line 28 at r2 (raw file):

/// trajectory-advancing substep (that is, via a per-step Publish event), with
/// the first sample occuring during Simulator::Initialize(). That means the
/// samples will in general be unevenly spaced in time. You may optionally

FYI recommend "in general" -> "generally"

---

systems/primitives/signal_logger.h, line 30 at r2 (raw file):

/// samples will in general be unevenly spaced in time. You may optionally
/// specify a "publish period" in which case sampling occurs periodically, with
/// the first sample occurring at time 0. Alternatively (not common), you can

Would you elaborate on why someone would want to do that?

---

systems/primitives/signal_logger.h, line 32 at r2 (raw file):

/// the first sample occurring at time 0. Alternatively (not common), you can
/// specify that logging should only occur at "forced publish" events, meaning
/// at explicit calls to System::Publish(). The Simulator's "publish every time

Please put either a TODO or a @see reference to the Simulator method to make it easy to remove this warning text when we finally can remove that option.

---

systems/primitives/signal_logger.h, line 63 at r2 (raw file):

  DRAKE_NO_COPY_NO_MOVE_NO_ASSIGN(SignalLogger)

  // TODO(sherm1) The following is an O(n²) storage expansion scheme. Should use

This comment is a little misleading (to the programmer), since it looks like you're talking about the storage being quadratic, i.e., O(n²) space complexity, where it's amortized O(n²) time complexity, and n is the number of samples.

I'd suggest putting a warning in the documentation about long-running simulations requiring more and more time to update the signal log.

---

systems/primitives/signal_logger.h, line 74 at r2 (raw file):

  /// Sets the publishing period of this system to specify periodic sampling
  /// rather than per-step sampling. This method can only be called once, and

BTW Please consider making it more explicit that per-step sampling is disabled when this method is called.

---

systems/primitives/signal_logger.h, line 81 at r2 (raw file):

  void set_publish_period(double period);

  /// Limits logging to forced publish calls only, that is, explicit calls

BTW Please consider making it explicit that per-step sampling is disabled when this method is called.

---

systems/primitives/signal_logger.h, line 90 at r2 (raw file):

  int num_samples() const { return log_.num_samples(); }

  /// Provides access to the (simulation) times of the logged data.

It's not required that SignalLogger be used with a Simulator, which is what this is currently hinting at. Perhaps "virtual time" or "sample times (drawn from the Context)"?

---

systems/primitives/signal_logger.h, line 101 at r2 (raw file):

  }

  /// Resets the logged data. The logger is empty after this call.

Nit It's unclear what the semantics of "resetting" a log entails. Suggest "Clears", "Deletes", or "Removes" instead. Then you can remove the extraneous statement "The logger is empty after this call."

---

systems/primitives/signal_logger.h, line 112 at r2 (raw file):

  EventStatus WriteToLog(const Context<T>& context) const;

  // This event handler logs only if we're in per-step logging mode.

BTW Suggest moving this comment to immediately above if (!period_).

---

systems/primitives/signal_logger.h, line 119 at r2 (raw file):

  }

  // Empty if we're logging per-step; > 0 for periodic; -1 for forced-only.

Nit This would be more robust using an enumerated type with a separate variable for logging period. Then the logic above using if (!period_) could be altered to the much more readable if (logging_trigger_ == kPerStep).

---

systems/primitives/test/signal_logger_test.cc, line 53 at r2 (raw file):

  // Gets the time stamps when each data point is saved.
  const auto& t = logger->sample_times();
  EXPECT_EQ(t.size(), simulator.get_num_publishes());

FYI Just curious- why did you remove this? The test would seem to be valid (predicated on Simulator publish-on-initialization and -per-step defaults).

---

systems/primitives/test/signal_logger_test.cc, line 85 at r2 (raw file):

}

// Test that set_publish_period() triggers properly even for logging a constant

Nit (Since you're cleaning this up) Strictly speaking, "set_publish_period()" does not do any triggering. It should result in proper triggering though.

[sherm1]

All review comments addressed, ptal.

Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee edrumwri, LGTM missing from assignee sherm1, platform LGTM missing

---

systems/primitives/signal_log.h, line 10 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

FYI Since you're cleaning up this documentation, please consider adding some class documentation that distinguishes between SignalLog and SignalLogger. From a quick examination of the two classes, it's not obvious when a user should use one vs. the other (it's only by the absence of the ports that I saw that this one doesn't plug into a System, which also makes me question why it's located under systems/primitives). It seems that the only use of SignalLog is in SignalLogger, making me think that this class is a good candidate for hiding.

Done. I don't know the history of this class but it is also used by Drake Visualizer for logging. I added some text to point out that it is not a System.

---

systems/primitives/signal_log.h, line 40 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

FYI Please consider updating the reset(.) comment here like you do in SignalLogger.

Done.

---

systems/primitives/signal_logger.h, line 28 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

FYI recommend "in general" -> "generally"

Done.

---

systems/primitives/signal_logger.h, line 30 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

Would you elaborate on why someone would want to do that?

Done, ptal. I don't know what circumstances would make someone want regular sampling and don't want to speculate (I'm not even sure it is ever a good idea). So I'm deeming the reason being that someone "prefers regular sampling", which apparently people often do.

---

systems/primitives/signal_logger.h, line 32 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

Please put either a TODO or a @see reference to the Simulator method to make it easy to remove this warning text when we finally can remove that option.

OK, there is a TODO below. If you meant something more than that, please elaborate.

---

systems/primitives/signal_logger.h, line 63 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

This comment is a little misleading (to the programmer), since it looks like you're talking about the storage being quadratic, i.e., O(n²) space complexity, where it's amortized O(n²) time complexity, and n is the number of samples.

I'd suggest putting a warning in the documentation about long-running simulations requiring more and more time to update the signal log.

Done. (Warning just below now, ptal)

---

systems/primitives/signal_logger.h, line 74 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

BTW Please consider making it more explicit that per-step sampling is disabled when this method is called.

Done.

---

systems/primitives/signal_logger.h, line 81 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

BTW Please consider making it explicit that per-step sampling is disabled when this method is called.

Done.

---

systems/primitives/signal_logger.h, line 90 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

It's not required that SignalLogger be used with a Simulator, which is what this is currently hinting at. Perhaps "virtual time" or "sample times (drawn from the Context)"?

Done.

---

systems/primitives/signal_logger.h, line 101 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

Nit It's unclear what the semantics of "resetting" a log entails. Suggest "Clears", "Deletes", or "Removes" instead. Then you can remove the extraneous statement "The logger is empty after this call."

Done.

---

systems/primitives/signal_logger.h, line 112 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

BTW Suggest moving this comment to immediately above if (!period_).

OK. I prefer that it describe the method's behavior.

---

systems/primitives/signal_logger.h, line 119 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

Nit This would be more robust using an enumerated type with a separate variable for logging period. Then the logic above using if (!period_) could be altered to the much more readable if (logging_trigger_ == kPerStep).

Done -- so much nicer, thanks!

---

systems/primitives/test/signal_logger_test.cc, line 53 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

FYI Just curious- why did you remove this? The test would seem to be valid (predicated on Simulator publish-on-initialization and -per-step defaults).

The simulator also does a bunch of per-step forced publishes by default, which no longer produce any logging. So there are twice as many simulator publishes as there are log entries -- I don't want to enforce that behavior here. In fact I'd be delighted if the Simulator stopped doing forced publishes by default!

---

systems/primitives/test/signal_logger_test.cc, line 85 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

Nit (Since you're cleaning this up) Strictly speaking, "set_publish_period()" does not do any triggering. It should result in proper triggering though.

Done.

[edrumwri]

Mostly

Reviewed 4 of 4 files at r3.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee sherm1, platform LGTM missing

---

systems/primitives/signal_logger.h, line 30 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Done, ptal. I don't know what circumstances would make someone want regular sampling and don't want to speculate (I'm not even sure it is ever a good idea). So I'm deeming the reason being that someone "prefers regular sampling", which apparently people often do.

Sorry, I wasn't clear here. I meant why they'd want to log by force publishing- the doc just says "not common" but that doesn't really tell someone why they'd need to do that.

---

systems/primitives/signal_logger.h, line 32 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

OK, there is a TODO below. If you meant something more than that, please elaborate.

Sorry, another instance where I was not sufficiently specific. I meant a TODO that helps us remove all of this documentation that refers to "publish every time step" or "publish at initialization" when we finally remove those options.

---

systems/primitives/signal_logger.h, line 74 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Done.

Nit extraneous comma now (the second clause is not independent).

---

systems/primitives/test/signal_logger_test.cc, line 53 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

The simulator also does a bunch of per-step forced publishes by default, which no longer produce any logging. So there are twice as many simulator publishes as there are log entries -- I don't want to enforce that behavior here. In fact I'd be delighted if the Simulator stopped doing forced publishes by default!

Got it- thanks.

[jwnimmer-tri]

First pass done.

I still want to take a second pass and make sure I'm straight on the changes to the logging semantics, but so far they seem okay. Stay tuned.

Also FYI even though the SignalLog implementation is O(n2) in the worse case, the amortized runtime is actually O(n) because std::realloc is magical (it is a no-op in most cases, because it's already allocated more memory that the minimum requested, because it has its own chunk sizes). I was unable to create a unit test that showed any meaningful change in runtime with an optimized implementation, at least not on a machine with only 32 GiB of RAM. I think all of the new warnings and TODOs about performance hazards are a bit over the top.

Reviewed 5 of 9 files at r1, 4 of 4 files at r3.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee sherm1, platform LGTM missing

---

systems/primitives/signal_logger.h, line 8 at r3 (raw file):

#include "drake/common/drake_copyable.h"
#include "drake/common/drake_optional.h"

nit Unused?

---

systems/primitives/signal_logger.h, line 51 at r3 (raw file):

  /// Access the (simulation) time of the logged data.

nit Wasn't the fact of "simulation time" mentioned by the prior API an important detail, which is now lost? (That it's simulation time vs wall time vs message timestamp time vs etc.)

---

systems/primitives/signal_logger.h, line 58 at r3 (raw file):

// TODO(sherm1) Remove the above warning. Consider using
// Multi-Instance Thread Local Storage pattern:
// https://www.codeproject.com/Tips/875963/%2FTips%2F875963%2FMulti-Instance-Thread-Local-Storage

nit Please check the pydrake documentation for pydrake.systems.SignalLogger class overview. I could be empty because you've split the Doxygen comment away from the class its describing.

(FYI since there's already an issue filed related to this TODO, I'm not sure that you need to duplicate its content inside the code. The issue is much easier to evolve and discuss than a TODO.)

---

systems/primitives/signal_logger.h, line 66 at r3 (raw file):

  // TODO(sherm1) The following is an O(n²)-time storage expansion scheme.
  // Should use an amortized-O(n) scheme like std::vector does (e.g. double
  // each time).

nit There is already an O(n2) TODO in SignalLog's implementation. Here in SignalLogger, nothing about API contract forces the implementation to be O(n2) -- the SignalLog could initialize to batch_allocation_size and then double every time, and it would still be true that "storage is reallocated in blocks of input*batch" -- it just does more than one block at once; the basic chunk size is still the same (i.e., the storage is always still a multiple of batch size). Consider removing the brittle-duplicative TODO and "don't repeat yourself".

---

systems/primitives/signal_logger.cc, line 3 at r3 (raw file):

#include "drake/systems/primitives/signal_logger.h"

#include <string>

nit Unused?

---

systems/primitives/signal_logger.cc, line 36 at r3 (raw file):

      logging_mode_ = kPeriodic;
      return;
    default:

nit Disprefer default, instead leadding the switch fall though. This way, then compiler will yell if we accidentally omit a clause in the future.

---

systems/primitives/signal_logger.cc, line 54 at r3 (raw file):

      logging_mode_ = kForced;
      return;
    default:

nit Disprefer default, instead leadding the switch fall though. This way, then compiler will yell if we accidentally omit a clause in the future.

---

systems/primitives/signal_logger.cc, line 63 at r3 (raw file):

  log_.AddData(context.get_time(),
               this->EvalVectorInput(context, 0)->get_value());
  return EventStatus::Succeeded();

Do I recall correctly, that there are "Declare" overloads that accept both void-returning and status-returning handlers? If so, shouldn't we (by way of our examples, such as this class) encourage System authors to use the void-returning handler in most cases, out of simplicity? We shouldn't have return success boilerplate all over the code, given that it's a reasonable default assumption. I'll posit that we should only use the status-return signature in cases where we have something non-trivial to report. What do you think?

[edrumwri]

Good to know. std::realloc() also doesn't make any guarantees on implementation, so how do you suggest we guard against that?

Reviewable status: 10 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee sherm1, platform LGTM missing

---

systems/primitives/signal_logger.h, line 51 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Wasn't the fact of "simulation time" mentioned by the prior API an important detail, which is now lost? (That it's simulation time vs wall time vs message timestamp time vs etc.)

It's not simulation time though. It's sample time or (less good) virtual time.

---

systems/primitives/signal_logger.cc, line 63 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Do I recall correctly, that there are "Declare" overloads that accept both void-returning and status-returning handlers? If so, shouldn't we (by way of our examples, such as this class) encourage System authors to use the void-returning handler in most cases, out of simplicity? We shouldn't have return success boilerplate all over the code, given that it's a reasonable default assumption. I'll posit that we should only use the status-return signature in cases where we have something non-trivial to report. What do you think?

I like your suggestion.

[jwnimmer-tri]

... how do you suggest we guard against that?

I see no reason to take any action, other than rewording the comment in signal_log.cc with our new understanding of the situation. In the unlikely event that someone starts using a braindead standard library implementation and they are logging large trajectories, then they might notice a performance problem -- in which case they can submit a fix. I don't see any need to borrow trouble.

Reviewable status: 10 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee sherm1, platform LGTM missing

---

systems/primitives/signal_logger.h, line 51 at r3 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

It's not simulation time though. It's sample time or (less good) virtual time.

The sample_times are a log of the values from context.get_time() for each sample point in data(). You're saying you don't call that "simulation time"? What is your name for context.get_time, if not simulation time?

[edrumwri]

Sounds good.

Reviewable status: 10 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee sherm1, platform LGTM missing

---

systems/primitives/signal_logger.h, line 51 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The sample_times are a log of the values from context.get_time() for each sample point in data(). You're saying you don't call that "simulation time"? What is your name for context.get_time, if not simulation time?

I call that "virtual time" since the passage of time doesn't need to come from a Simulator. In Drake, "Context time" would be the best term IMO.

I don't really think the disambiguation is necessary here- it's reasonable from the context that it's not the other kinds of time that you described.

[jwnimmer-tri]

Reviewable status: 9 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee sherm1, platform LGTM missing

---

systems/primitives/signal_logger.cc, line 63 at r3 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

I like your suggestion.

@sherm1 WDYT?

[sherm1]

Reviewable status: 9 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee sherm1, platform LGTM missing

---

systems/primitives/signal_logger.cc, line 63 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

@sherm1 WDYT?

I'm inclined in the other direction -- it seems better to me always to return status for event handlers except in the simplest pedagogical examples. Normally I prefer that anyone writing an event handler give some thought to error conditions and I think having to return a status is a gentle prompt in that direction. I don't think it is a huge burden either!

BTW we do not have full coverage for the assume-success Declare...Event methods, only for the most common cases as used in pedagogical examples. We would have to add one for DeclareForcedPublishEvent() to get rid of the EventStatus here, and it would take many more overloads to provide a complete set (not difficult, but IMO not worth the trouble).

The reason it makes sense for the simple examples is that people don't generally think of a straightforward difference-equation update as an event handler, so when showing examples of how to implement discrete-time systems it is distracting to have to talk about events. But that is just a special case of a much more general concept, and event handling in general cannot be assumed to succeed.

TL;DR: here where the adults are dealing thoughtfully with events, I think we should return EventStatus.

[edrumwri]

Reviewable status: 9 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee sherm1, platform LGTM missing

---

systems/primitives/signal_logger.cc, line 63 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

I'm inclined in the other direction -- it seems better to me always to return status for event handlers except in the simplest pedagogical examples. Normally I prefer that anyone writing an event handler give some thought to error conditions and I think having to return a status is a gentle prompt in that direction. I don't think it is a huge burden either!

BTW we do not have full coverage for the assume-success Declare...Event methods, only for the most common cases as used in pedagogical examples. We would have to add one for DeclareForcedPublishEvent() to get rid of the EventStatus here, and it would take many more overloads to provide a complete set (not difficult, but IMO not worth the trouble).

The reason it makes sense for the simple examples is that people don't generally think of a straightforward difference-equation update as an event handler, so when showing examples of how to implement discrete-time systems it is distracting to have to talk about events. But that is just a special case of a much more general concept, and event handling in general cannot be assumed to succeed.

TL;DR: here where the adults are dealing thoughtfully with events, I think we should return EventStatus.

Sorry, I forgot that the forced trigger called this one. Per my answer in #10341, I agree that we should leave this one as-is.

[jwnimmer-tri]

Reviewable status: 8 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee sherm1, platform LGTM missing

---

systems/primitives/signal_logger.cc, line 63 at r3 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

Sorry, I forgot that the forced trigger called this one. Per my answer in #10341, I agree that we should leave this one as-is.

I will mark myself satisfied in light of "We need an event handler function pointer that works for both forced and unforced events" -- that this particular class is "off the beaten path" enough to be florid.

(For the record though, I do not agree though that we should encourage the copypasta for "return Succeded();" in most event handlers. I disagree that there is correlation between forcing users to type in copypasta, and the user giving thought to error conditions that disrupt or otherwise terminate the simulation. They will just copypasta it mindlessly, and we'll continue to receive complaints that Drake is verbose and complicated. )

[jwnimmer-tri]

I still want to take a second pass and make sure I'm straight on the changes to the logging semantics, but so far they seem okay. Stay tuned.

I'm satisfied on this front.

Just the eight or so open discussions remain.

Reviewable status: 8 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee sherm1, platform LGTM missing

[sherm1]

All review comments addressed, ptal.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri, LGTM missing from assignee sherm1, platform LGTM missing

---

systems/primitives/signal_logger.h, line 30 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

Sorry, I wasn't clear here. I meant why they'd want to log by force publishing- the doc just says "not common" but that doesn't really tell someone why they'd need to do that.

OK, I don't want to encourage forced publish by trying to make up some reason to use it. Currently only LcmDrivenLoop does so, and it's not clear we want to steer any innocent users along that same direction. An expert who wants this feature will hopefully know why; everyone else can move along.

---

systems/primitives/signal_logger.h, line 32 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

Sorry, another instance where I was not sufficiently specific. I meant a TODO that helps us remove all of this documentation that refers to "publish every time step" or "publish at initialization" when we finally remove those options.

Done.

---

systems/primitives/signal_logger.h, line 74 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

Nit extraneous comma now (the second clause is not independent).

Done.

---

systems/primitives/signal_logger.h, line 8 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Unused?

Done.

---

systems/primitives/signal_logger.h, line 51 at r3 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

I call that "virtual time" since the passage of time doesn't need to come from a Simulator. In Drake, "Context time" would be the best term IMO.

I don't really think the disambiguation is necessary here- it's reasonable from the context that it's not the other kinds of time that you described.

Done. Clarified a little.

---

systems/primitives/signal_logger.h, line 58 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Please check the pydrake documentation for pydrake.systems.SignalLogger class overview. I could be empty because you've split the Doxygen comment away from the class its describing.

(FYI since there's already an issue filed related to this TODO, I'm not sure that you need to duplicate its content inside the code. The issue is much easier to evolve and discuss than a TODO.)

Done (removed TODO).

---

systems/primitives/signal_logger.h, line 66 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit There is already an O(n2) TODO in SignalLog's implementation. Here in SignalLogger, nothing about API contract forces the implementation to be O(n2) -- the SignalLog could initialize to batch_allocation_size and then double every time, and it would still be true that "storage is reallocated in blocks of input*batch" -- it just does more than one block at once; the basic chunk size is still the same (i.e., the storage is always still a multiple of batch size). Consider removing the brittle-duplicative TODO and "don't repeat yourself".

Done.

---

systems/primitives/signal_logger.cc, line 3 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Unused?

Done.

---

systems/primitives/signal_logger.cc, line 36 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Disprefer default, instead leadding the switch fall though. This way, then compiler will yell if we accidentally omit a clause in the future.

Done.

---

systems/primitives/signal_logger.cc, line 54 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Disprefer default, instead leadding the switch fall though. This way, then compiler will yell if we accidentally omit a clause in the future.

Done.

---

systems/primitives/signal_logger.cc, line 63 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I will mark myself satisfied in light of "We need an event handler function pointer that works for both forced and unforced events" -- that this particular class is "off the beaten path" enough to be florid.

(For the record though, I do not agree though that we should encourage the copypasta for "return Succeded();" in most event handlers. I disagree that there is correlation between forcing users to type in copypasta, and the user giving thought to error conditions that disrupt or otherwise terminate the simulation. They will just copypasta it mindlessly, and we'll continue to receive complaints that Drake is verbose and complicated. )

OK.

[jwnimmer-tri]

Reviewed 2 of 2 files at r4.
Reviewable status: 2 unresolved discussions, platform LGTM from [jwnimmer-tri]

[edrumwri]

Reviewable status: complete! all discussions resolved, platform LGTM from [jwnimmer-tri]

---

systems/primitives/signal_logger.h, line 74 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Done.

I don't see where it's done? You've got extraneous comments in the first two sentences of this paragraph.

[sherm1]

Reviewable status: complete! all discussions resolved, platform LGTM from [jwnimmer-tri]

---

systems/primitives/signal_logger.h, line 74 at r2 (raw file):

Previously, edrumwri (Evan Drumwright) wrote…

I don't see where it's done? You've got extraneous comments in the first two sentences of this paragraph.

Done for real this time.