[epilogue] Allow configuring logging period and logging period offset

[spacey-sooty]

Resolves #6883

[spacey-sooty]

/format

[spacey-sooty]

@SamCarlberg Can you review?

[KangarooKoala]

I might be wrong, but I think the original issue said that for the offset, null should be the default value, which is interpreted as "half of whatever the logging period is" (since that isn't guaranteed to be 20ms). It'd probably also make sense to make the logging period have a default value of null that is interpreted as "whatever the robot period is" (since that isn't guaranteed to be 20ms).

[spacey-sooty]

The reason I avoided making it null by default was that we would need to do an extra check on if the value is null before applying the default value. By doing this we make it a no cost operation for most cases and then have then leave it up to the user to configure it if they are running at a different speed (which if they are doing are probably already an advanced user).

[SamCarlberg]

The reason I avoided making it null by default was that we would need to do an extra check on if the value is null before applying the default value. By doing this we make it a no cost operation for most cases

This is a premature optimization; null checks are already very fast

and then have then leave it up to the user to configure it if they are running at a different speed (which if they are doing are probably already an advanced user).

The issue only specified a configurable offset to keep logging at the same frequency as the main robot loop. Making the default value null means that we can log based on whatever user-configured loop frequency was specified in the robot constructor without forcing users to set it explicitly for epilogue. A configurable period on top of that is certainly strong, but means there'd be a second place to specify a loop period for users not on a 50hz frequency:

public Robot() {
  super(0.01); // 100Hz update rate

  Epilogue.configure(config -> {
    // No configuration: logging at 50hz intervals and coincide with every second main loop iteration
  });

  Epilogue.configure(config -> {
    // Both of these are required to be set in order to keep logging in sync and with the default behavior
    config.loggingPeriod = Milliseconds.of(10);
    config.loggingPeriodOffset = Milliseconds.of(5);
  });
}

versus:

public Robot() {
  super(0.01);

  Epilogue.configure(config -> {
    // No configuration: logs at 100Hz, offset by 5ms from the main loop
  });

  Epilogue.configure(config -> {
    // Configure the offset: still log at 100Hz, but synchronized with the main robot loop
    // This comes at the cost of higher CPU load
    config.loggingPeriodOffset = Milliseconds.zero();
  });
}

If users really want custom logging periods, they can add a periodic callback themselves without using bind. This is essentially what the bind method does anyway, just without tracking logging timings (which is also trivial to add)

public Robot() {
  super(0.01);

  addPeriodic(
    () -> {
      Epilogue.robotLogger.tryUpdate(
        Epilogue.getConfig().dataLogger.getSubLogger(Epilogue.getConfig().root),
        this,
        Epilogue.getConfig().errorHandler
      )
    },
    loggingPeriod,
    loggingPeriodOffset
  );
}

(As an aside, generating a logOnce or logNow method in the Epilogue class would be helpful to simplify direct usage for people not using bind)

[spacey-sooty]

The issue only specified a configurable offset to keep logging at the same frequency as the main robot loop.

I am aware but adding a loggingPeriod seemed like a good addition to me as well.

[SamCarlberg]

Wow, spotbugs is dumber than I remembered it being. You actually do need to disable the spotbugs warnings on the two fields since it doesn't detect usages in examples

[spacey-sooty]

Wow, spotbugs is dumber than I remembered it being. You actually do need to disable the spotbugs warnings on the two fields since it doesn't detect usages in examples

Example number infinity of why spotbugs is stupid and this warning should be suppressed 🙂

[calcmogul]

We can suppress it locally via styleguide/spotbugs-exclude.xml. Suppressing it globally isn't ideal because it has caught valid issues before.