Skip to content

[cmd3] Add rising and falling edge trigger factories #8366

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.

Sign up for GitHub

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

Open
SamCarlberg wants to merge 3 commits into
base: 2027
Choose a base branch
from
+327 −20
Open

[cmd3] Add rising and falling edge trigger factories #8366

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
66ed98a
Add rising and falling edge triggers
SamCarlberg Nov 10, 2025
7b37096
Bind triggers to event loops upon construction
SamCarlberg Nov 19, 2025
8898a28
Bind to event loop on binding; track dependencies recursively
SamCarlberg Nov 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 75 additions & 8 deletions commandsv3/src/main/java/org/wpilib/command3/Trigger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,13 @@ public class Trigger implements BooleanSupplier {
private final BooleanSupplier m_condition;
private final EventLoop m_loop;
private final Scheduler m_scheduler;

/** The value of the signal before the most recent call to {@link #poll()}. May be null. */
private Signal m_previousSignal;

/** The value of the signal from the most recent call to {@link #poll()}. May be null. */
private Signal m_cachedSignal;

private final Map<BindingType, List<Binding>> m_bindings = new EnumMap<>(BindingType.class);
private final Runnable m_eventLoopCallback = this::poll;
private boolean m_isBoundToEventLoop; // used for lazily binding to the event loop
Expand All @@ -54,7 +60,7 @@ public class Trigger implements BooleanSupplier {
* Represents the state of a signal: high or low. Used instead of a boolean for nullity on the
* first run, when the previous signal value is undefined and unknown.
*/
private enum Signal {
enum Signal {
/** The signal is high. */
HIGH,
/** The signal is low. */
Expand Down Expand Up @@ -177,9 +183,21 @@ public Trigger toggleOnFalse(Command command) {
return this;
}

/**
* Gets the boolean state of the trigger. Because triggers are checked when their event loop is
* polled, which by default occurs when {@link Scheduler#run()} is called in user code, but may
* differ for custom event loops, the state is only valid immediately after the event loop is
* polled. If the underlying signal changes without a subsequent event loop poll, the return value
* from {@code getAsBoolean()} may not agree with the result of checking the signal directly.
*
* <p>Triggers are only bound to an event loop when at least one command has been bound to the
* trigger. This method will always return {@code false} before commands have been bound.
Copy link
Contributor

@KangarooKoala KangarooKoala Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe adjust this to reflect that commands being bound to dependencies will also bind the Trigger to an event loop?

*
* @return The state of the trigger.
*/
@Override
public boolean getAsBoolean() {
return m_condition.getAsBoolean();
return m_cachedSignal == Signal.HIGH;
}

/**
Expand Down Expand Up @@ -238,36 +256,65 @@ public Trigger debounce(Time duration, Debouncer.DebounceType type) {
return new Trigger(m_scheduler, m_loop, () -> debouncer.calculate(m_condition.getAsBoolean()));
}

/**
* Creates a trigger that activates on a rising edge of this trigger's signal. The rising edge
* trigger is active in the same cycle that this trigger's condition is {@code true} while its
* condition in the previous cycle was {@code false}. The resulting trigger will only be active
* for that single cycle before going inactive again; therefore, {@link #onTrue(Command)} should
* be used instead of {@link #whileTrue(Command)}, as commands bound using the latter method will
* be immediately canceled after a single scheduler cycle.
*
* @return A rising edge trigger.
*/
public Trigger risingEdge() {
return new Trigger(
m_scheduler, m_loop, () -> m_cachedSignal == Signal.HIGH && m_previousSignal == Signal.LOW);
}

/**
* Creates a trigger that activates on a falling edge of this trigger's signal. The falling edge
* trigger is active in the same cycle that this trigger's condition is {@code false} while its
* condition in the previous cycle was {@code true}. The resulting trigger will only be active for
* that single cycle before going inactive again; therefore, {@link #onTrue(Command)} should be
* used instead of {@link #whileTrue(Command)}, as commands bound using the latter method will be
* immediately canceled after a single scheduler cycle.
*
* @return A falling edge trigger.
*/
public Trigger fallingEdge() {
return new Trigger(
m_scheduler, m_loop, () -> m_cachedSignal == Signal.LOW && m_previousSignal == Signal.HIGH);
}

private void poll() {
// Clear bindings that no longer need to run
// This should always be checked, regardless of signal change, since bindings may be scoped
// and those scopes may become inactive.
clearStaleBindings();

var signal = readSignal();
m_previousSignal = m_cachedSignal;
m_cachedSignal = readSignal();

if (signal == m_previousSignal) {
if (m_cachedSignal == m_previousSignal) {
// No change in the signal. Nothing to do
return;
}

if (signal == Signal.HIGH) {
if (m_cachedSignal == Signal.HIGH) {
// Signal is now high when it wasn't before - a rising edge
scheduleBindings(BindingType.SCHEDULE_ON_RISING_EDGE);
scheduleBindings(BindingType.RUN_WHILE_HIGH);
cancelBindings(BindingType.RUN_WHILE_LOW);
toggleBindings(BindingType.TOGGLE_ON_RISING_EDGE);
}

if (signal == Signal.LOW) {
if (m_cachedSignal == Signal.LOW) {
// Signal is now low when it wasn't before - a falling edge
scheduleBindings(BindingType.SCHEDULE_ON_FALLING_EDGE);
scheduleBindings(BindingType.RUN_WHILE_LOW);
cancelBindings(BindingType.RUN_WHILE_HIGH);
toggleBindings(BindingType.TOGGLE_ON_FALLING_EDGE);
}

m_previousSignal = signal;
}

private Signal readSignal() {
Expand Down Expand Up @@ -336,6 +383,26 @@ private void toggleBindings(BindingType bindingType) {
});
}

/**
* Package-private for testing. Reads the signal of the trigger as it was <i>after</i> the most
* recent call to {@link #poll()}. May be null.
*
* @return The most recent signal.
*/
Signal getCachedSignal() {
return m_cachedSignal;
}

/**
* Package-private for testing. Reads the signal of the trigger as it was <i>before</i> the most
* recent call to {@link #poll()}. May be null.
*
* @return The previous signal.
*/
Signal getPreviousSignal() {
return m_previousSignal;
}

// package-private for testing
void addBinding(BindingScope scope, BindingType bindingType, Command command) {
// Note: we use a throwable here instead of Thread.currentThread().getStackTrace() for easier
Expand Down
128 changes: 128 additions & 0 deletions commandsv3/src/test/java/org/wpilib/command3/TriggerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

package org.wpilib.command3;

import static org.junit.jupiter.api.Assertions.assertAll;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
Expand Down Expand Up @@ -244,4 +245,131 @@ void triggerFromExitingCommandDoesNotFire() {
assertEquals(List.of(), m_scheduler.getRunningCommands().stream().map(Command::name).toList());
assertFalse(triggeredCommandRan.get(), "Command was unexpectedly triggered");
}

@Test
void risingEdge() {
var command = new NullCommand();

var signal = new AtomicBoolean(false);
var baseTrigger = new Trigger(m_scheduler, signal::get);
var risingEdgeTrigger = baseTrigger.risingEdge();

// Ensure bindings. Triggers aren't polled when no bindings have been added.
baseTrigger.addBinding(BindingScope.global(), BindingType.RUN_WHILE_HIGH, command);
risingEdgeTrigger.addBinding(BindingScope.global(), BindingType.RUN_WHILE_HIGH, command);
Copy link
Contributor

@KangarooKoala KangarooKoala Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like a major footgun that base triggers need a binding in order for any derived triggers to work. At least from what I've seen in commandsv2, it's not unheard of for triggers to be composed from temporary objects, and silently always using false is those cases would probably cause a lot of confusion and frustration.
One possibility would be for Triggers to track their base trigger(s), so that when a binding is added to a Trigger, it can first make sure all of its base trigger(s) (and their base(s) and so on) are added to the event loop.
Another, simpler but less user-friendly approach is for getAsBoolean() to give a descriptive error or warning if the trigger has not been added to an event loop.


assertAll(
"Signals start null",
() -> assertEquals(null, baseTrigger.getCachedSignal()),
() -> assertEquals(null, baseTrigger.getPreviousSignal()),
() -> assertEquals(null, risingEdgeTrigger.getCachedSignal()),
() -> assertEquals(null, risingEdgeTrigger.getPreviousSignal()));

m_scheduler.run();
assertAll(
"First run (base signal stays low)",
() -> assertEquals(Trigger.Signal.LOW, baseTrigger.getCachedSignal()),
() -> assertEquals(null, baseTrigger.getPreviousSignal()),
() -> assertEquals(Trigger.Signal.LOW, risingEdgeTrigger.getCachedSignal()),
() -> assertEquals(null, risingEdgeTrigger.getPreviousSignal()));

signal.set(true);
m_scheduler.run();
assertAll(
"Second run (base signal goes high)",
() -> assertEquals(Trigger.Signal.HIGH, baseTrigger.getCachedSignal()),
() -> assertEquals(Trigger.Signal.LOW, baseTrigger.getPreviousSignal()),
() ->
assertEquals(
Trigger.Signal.HIGH,
risingEdgeTrigger.getCachedSignal(),
"Rising edge trigger did not go high"),
() -> assertEquals(Trigger.Signal.LOW, risingEdgeTrigger.getPreviousSignal()));

m_scheduler.run();
assertAll(
"Third run (base signal stays high)",
() -> assertEquals(Trigger.Signal.HIGH, baseTrigger.getCachedSignal()),
() -> assertEquals(Trigger.Signal.HIGH, baseTrigger.getPreviousSignal()),
() ->
assertEquals(
Trigger.Signal.LOW,
risingEdgeTrigger.getCachedSignal(),
"Rising edge trigger did not go low"),
() ->
assertEquals(
Trigger.Signal.HIGH,
risingEdgeTrigger.getPreviousSignal(),
"Rising edge trigger was not previously high"));
}

@Test
void fallingEdge() {
var command = new NullCommand();

var signal = new AtomicBoolean(false);
var baseTrigger = new Trigger(m_scheduler, signal::get);
var fallingEdgeTrigger = baseTrigger.fallingEdge();

// Ensure bindings. Triggers aren't polled when no bindings have been added.
baseTrigger.addBinding(BindingScope.global(), BindingType.RUN_WHILE_HIGH, command);
fallingEdgeTrigger.addBinding(BindingScope.global(), BindingType.RUN_WHILE_HIGH, command);

assertAll(
"Signals start null",
() -> assertEquals(null, baseTrigger.getCachedSignal()),
() -> assertEquals(null, baseTrigger.getPreviousSignal()),
() -> assertEquals(null, fallingEdgeTrigger.getCachedSignal()),
() -> assertEquals(null, fallingEdgeTrigger.getPreviousSignal()));

m_scheduler.run();
assertAll(
"First run (base signal stays low)",
() -> assertEquals(Trigger.Signal.LOW, baseTrigger.getCachedSignal()),
() -> assertEquals(null, baseTrigger.getPreviousSignal()),
() -> assertEquals(Trigger.Signal.LOW, fallingEdgeTrigger.getCachedSignal()),
() -> assertEquals(null, fallingEdgeTrigger.getPreviousSignal()));

signal.set(true);
m_scheduler.run();
assertAll(
"Second run (base signal goes high)",
() -> assertEquals(Trigger.Signal.HIGH, baseTrigger.getCachedSignal()),
() -> assertEquals(Trigger.Signal.LOW, baseTrigger.getPreviousSignal()),
() -> assertEquals(Trigger.Signal.LOW, fallingEdgeTrigger.getCachedSignal()),
() -> assertEquals(Trigger.Signal.LOW, fallingEdgeTrigger.getPreviousSignal()));

signal.set(false);
m_scheduler.run();
assertAll(
"Third run (base signal goes low)",
() -> assertEquals(Trigger.Signal.LOW, baseTrigger.getCachedSignal()),
() -> assertEquals(Trigger.Signal.HIGH, baseTrigger.getPreviousSignal()),
() ->
assertEquals(
Trigger.Signal.HIGH,
fallingEdgeTrigger.getCachedSignal(),
"Falling edge trigger did not go high"),
() ->
assertEquals(
Trigger.Signal.LOW,
fallingEdgeTrigger.getPreviousSignal(),
"Falling edge trigger was not previously low"));

m_scheduler.run();
assertAll(
"Fourth run (base signal stays low)",
() -> assertEquals(Trigger.Signal.LOW, baseTrigger.getCachedSignal()),
() -> assertEquals(Trigger.Signal.LOW, baseTrigger.getPreviousSignal()),
() ->
assertEquals(
Trigger.Signal.LOW,
fallingEdgeTrigger.getCachedSignal(),
"Falling edge trigger did not go low"),
() ->
assertEquals(
Trigger.Signal.HIGH,
fallingEdgeTrigger.getPreviousSignal(),
"Falling edge trigger was not previously high"));
}
}
Loading