Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow wait command to receive a supplier #6239

Closed
TechplexEngineer opened this issue Jan 16, 2024 · 3 comments
Closed

Allow wait command to receive a supplier #6239

TechplexEngineer opened this issue Jan 16, 2024 · 3 comments
Labels
component: command-based WPILib Command Based Library type: fix Iterations on existing features or infrastructure.

Comments

@TechplexEngineer
Copy link
Contributor

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
Copy link
Member

rzblue commented Jan 16, 2024

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
Copy link
Member

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.

spacey-sooty added a commit to spacey-sooty/allwpilib that referenced this issue Jan 22, 2024
@calcmogul calcmogul added component: command-based WPILib Command Based Library type: fix Iterations on existing features or infrastructure. labels Mar 8, 2024
@Starlight220
Copy link
Member

Closing as wontfix.

DeferredCommand can solve this issue:

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

This isn't a common enough use case to warrant syntax sugar, imo.

The memory worries shouldn't be a problem, and with the 2027 controller won't be a consideration at all.

@Starlight220 Starlight220 closed this as not planned Won't fix, can't repro, duplicate, stale Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: command-based WPILib Command Based Library type: fix Iterations on existing features or infrastructure.
Projects
None yet
4 participants