wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.05k stars 611 forks source link

Allow wait command to receive a supplier #6239

Open TechplexEngineer opened 7 months ago

TechplexEngineer commented 7 months ago

Is your feature request related to a problem? Please describe. When testing our shooter we wanted to have a network tables value dictate the length of the delay between two actions (turning on one motor then another). We found that since the wait command only takes the delay as a constructor parameter at initialization.

Describe the solution you'd like Have an alternate constructor which takes a double supplier for the delay seconds.

Describe alternatives you've considered Recreating the sequential command group each time the button is pressed seems wasteful of memory and quite frankly we couldn't figure out how to do it.

Additional context We have a modified version of the WaitCommand class provided here as an example. We will follow this up with a pull request.

// Copyright (c) FIRST and other WPILib contributors.
// Open Source Software; you can modify and/or share it under the terms of
// the WPILib BSD license file in the root directory of this project.

package edu.wpi.first.wpilibj2.command;

import java.util.function.DoubleSupplier;

import edu.wpi.first.networktables.DoubleSubscriber;
import edu.wpi.first.util.sendable.SendableBuilder;
import edu.wpi.first.util.sendable.SendableRegistry;
import edu.wpi.first.wpilibj.Timer;

/**
 * A command that does nothing but takes a specified amount of time to finish.
 *
 * <p>This class is provided by the NewCommands VendorDep
 */
public class WaitCommand extends Command {
  /** The timer used for waiting. */
  protected Timer m_timer = new Timer();

  // private final double m_duration;
  private final DoubleSupplier m_delaySeconds;

  /**
   * Creates a new WaitCommand. This command will do nothing, and end after the specified duration.
   *
   * @param seconds the time to wait, in seconds
   */
  @SuppressWarnings("this-escape")
  public WaitCommand(double seconds) {
    // m_duration = seconds;
    m_delaySeconds = () -> seconds;
    SendableRegistry.setName(this, getName() + ": " + seconds + " seconds");
  }

  public WaitCommand(DoubleSupplier delaySeconds) {
    m_delaySeconds = delaySeconds;
    SendableRegistry.setName(this, getName() + ": DYNAMIC seconds");
  }

  @Override
  public void initialize() {
    m_timer.restart();
  }

  @Override
  public void end(boolean interrupted) {
    m_timer.stop();
  }

  @Override
  public boolean isFinished() {
    return m_timer.hasElapsed(m_delaySeconds.getAsDouble());
  }

  @Override
  public boolean runsWhenDisabled() {
    return true;
  }

  @Override
  public void initSendable(SendableBuilder builder) {
    super.initSendable(builder);
    builder.addDoubleProperty("duration", () -> m_delaySeconds.getAsDouble(), null);
  }
}
rzblue commented 7 months ago

As an alternative, you can also use DeferredCommand around a wait command:

Commands.defer(() -> Commands.waitSeconds(getWaitTime()), requirements)

I think this is a good addition, though.

Re: memory concerns Creating new commands shouldn't be too much of a concern; As long as you're not keeping them around they will be garbage collected. As long as you're not allocating constantly it shouldn't be an issue.

Starlight220 commented 7 months ago

Shouldn't the supplier be polled at schedule/init (like DeferredCommand and multiple others) and not constantly?

Also, given DeferredCommand, I'm wondering whether we should be directing to it rather than adding deferred overloads for everything.