diff --git a/commandsv3/src/main/java/org/wpilib/command3/Scheduler.java b/commandsv3/src/main/java/org/wpilib/command3/Scheduler.java index 35e361569f..67495c3888 100644 --- a/commandsv3/src/main/java/org/wpilib/command3/Scheduler.java +++ b/commandsv3/src/main/java/org/wpilib/command3/Scheduler.java @@ -69,6 +69,8 @@ import org.wpilib.util.protobuf.ProtobufSerializable; *
    *
  1. Cancel any commands bound to scopes that have gone inactive, such as having been scheduled * in an opmode that's no longer selected on the driverstation. + *
  2. Cancel any triggers that were created in scopes that have gone inactive, such as being + * constructed in an opmode that's no longer selected on the driverstation. *
  3. Call {@link #sideload(Consumer) periodic sideload functions}. *
  4. Poll all registered triggers to queue and cancel commands. *
  5. Queue default commands for any mechanisms without a running command. The queued commands @@ -110,6 +112,8 @@ public final class Scheduler implements ProtobufSerializable { */ private final Collection m_activeBindings = new ArrayList<>(); + private final Collection m_boundTriggers = new ArrayList<>(); + /** The set of commands scheduled since the start of the previous run. */ private final SequencedSet m_queuedToRun = new LinkedHashSet<>(); @@ -545,6 +549,8 @@ public final class Scheduler implements ProtobufSerializable { *
      *
    1. Cancel any commands bound to scopes that have gone inactive, such as having been * scheduled in an opmode that's no longer selected on the driverstation + *
    2. Cancel any triggers that were created in scopes that have gone inactive, such as being + * constructed in an opmode that's no longer selected on the driverstation *
    3. Run sideloaded functions from {@link #sideload(Consumer)} and {@link * #addPeriodic(Runnable)} *
    4. Update trigger bindings to queue and cancel bound commands @@ -563,6 +569,11 @@ public final class Scheduler implements ProtobufSerializable { // Cancel any commands with stale binding scopes cancelStaleBindings(); + // Unbind any triggers with stale creation scopes. + // This allows triggers that can never be used again to be garbage collected to reduce + // memory usage and avoid potential OOMs from poorly written user code. + unbindStaleTriggers(); + // Sideloads may change some state that affects triggers. Run them first. runPeriodicSideloads(); @@ -594,6 +605,25 @@ public final class Scheduler implements ProtobufSerializable { } } + private void unbindStaleTriggers() { + for (var iterator = m_boundTriggers.iterator(); iterator.hasNext(); ) { + var trigger = iterator.next(); + if (!trigger.isScopeActive()) { + trigger.unbind(); + iterator.remove(); + } + } + } + + /** + * Adds a bound trigger to this scheduler. The trigger will be unbound from the event loop when + * its creation scope becomes inactive and may be eligible for garbage collection. + */ + // package-private for Trigger to call when constructed + void addBoundTrigger(Trigger trigger) { + m_boundTriggers.add(trigger); + } + private void promoteScheduledCommands() { // Clear any commands that conflict with the scheduled set for (var queuedState : m_queuedToRun) { diff --git a/commandsv3/src/main/java/org/wpilib/command3/Trigger.java b/commandsv3/src/main/java/org/wpilib/command3/Trigger.java index 32baf60303..4833df806b 100644 --- a/commandsv3/src/main/java/org/wpilib/command3/Trigger.java +++ b/commandsv3/src/main/java/org/wpilib/command3/Trigger.java @@ -45,16 +45,23 @@ 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> 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 + private boolean m_bound = true; + private final BindingScope m_creationScope; /** * 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. */ @@ -69,9 +76,10 @@ public class Trigger implements BooleanSupplier { * @param condition the condition represented by this trigger */ public Trigger(Scheduler scheduler, BooleanSupplier condition) { - m_scheduler = requireNonNullParam(scheduler, "scheduler", "Trigger"); - m_loop = scheduler.getDefaultEventLoop(); - m_condition = requireNonNullParam(condition, "condition", "Trigger"); + this( + requireNonNullParam(scheduler, "scheduler", "Trigger"), + scheduler.getDefaultEventLoop(), + requireNonNullParam(condition, "condition", "Trigger")); } /** @@ -93,10 +101,15 @@ public class Trigger implements BooleanSupplier { * @param loop The event loop to poll the trigger. * @param condition the condition represented by this trigger */ + @SuppressWarnings("this-escape") public Trigger(Scheduler scheduler, EventLoop loop, BooleanSupplier condition) { m_scheduler = requireNonNullParam(scheduler, "scheduler", "Trigger"); m_loop = requireNonNullParam(loop, "loop", "Trigger"); m_condition = requireNonNullParam(condition, "condition", "Trigger"); + m_creationScope = BindingScope.createNarrowestScope(m_scheduler); + + m_scheduler.addBoundTrigger(this); + m_loop.bind(m_eventLoopCallback); } /** @@ -177,9 +190,20 @@ public class Trigger implements BooleanSupplier { 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. + * + *

      This method will always return {@code false} before being polled by an event loop. + * + * @return The state of the trigger. + */ @Override public boolean getAsBoolean() { - return m_condition.getAsBoolean(); + return m_cachedSignal == Signal.HIGH; } /** @@ -189,8 +213,7 @@ public class Trigger implements BooleanSupplier { * @return A trigger which is active when both component triggers are active. */ public Trigger and(BooleanSupplier trigger) { - return new Trigger( - m_scheduler, m_loop, () -> m_condition.getAsBoolean() && trigger.getAsBoolean()); + return new Trigger(m_scheduler, m_loop, () -> getAsBoolean() && trigger.getAsBoolean()); } /** @@ -200,8 +223,7 @@ public class Trigger implements BooleanSupplier { * @return A trigger which is active when either component trigger is active. */ public Trigger or(BooleanSupplier trigger) { - return new Trigger( - m_scheduler, m_loop, () -> m_condition.getAsBoolean() || trigger.getAsBoolean()); + return new Trigger(m_scheduler, m_loop, () -> getAsBoolean() || trigger.getAsBoolean()); } /** @@ -211,7 +233,7 @@ public class Trigger implements BooleanSupplier { * @return the negated trigger */ public Trigger negate() { - return new Trigger(m_scheduler, m_loop, () -> !m_condition.getAsBoolean()); + return new Trigger(m_scheduler, m_loop, () -> !getAsBoolean()); } /** @@ -235,7 +257,37 @@ public class Trigger implements BooleanSupplier { */ public Trigger debounce(Time duration, Debouncer.DebounceType type) { var debouncer = new Debouncer(duration.in(Seconds), type); - return new Trigger(m_scheduler, m_loop, () -> debouncer.calculate(m_condition.getAsBoolean())); + return new Trigger(m_scheduler, m_loop, () -> debouncer.calculate(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() { @@ -244,14 +296,15 @@ public class Trigger implements BooleanSupplier { // 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); @@ -259,15 +312,13 @@ public class Trigger implements BooleanSupplier { 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() { @@ -289,7 +340,7 @@ public class Trigger implements BooleanSupplier { continue; } - // The scope is no long active. Remove the binding and immediately cancel its command. + // The scope is no longer active. Remove the binding and immediately cancel its command. iterator.remove(); m_scheduler.cancel(binding.command()); } @@ -336,6 +387,59 @@ public class Trigger implements BooleanSupplier { }); } + /** + * Package-private for testing. Reads the signal of the trigger as it was after 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 before the most + * recent call to {@link #poll()}. May be null. + * + * @return The previous signal. + */ + Signal getPreviousSignal() { + return m_previousSignal; + } + + /** Checks if the creation scope is currently active. */ + // package-private for the scheduler to access + boolean isScopeActive() { + return m_creationScope.active(); + } + + /** + * Unbinds this trigger from the event loop and clears all command bindings; any bound commands + * that are currently running will be canceled. The trigger may be garbage collected if no other + * references exist in user code. Binding a command to a trigger via {@link #onTrue(Command)} or + * similar will re-bind the trigger to the event loop. + * + *

      Note: because triggers are only updated when they're bound to an event loop, calling {@code + * #unbind()} will result in {@link #getAsBoolean()} continuing to return the same value until the + * trigger is re-bound. + * + *

      This method is automatically called by the associated {@link Scheduler} when the trigger's + * creation scope becomes inactive: a trigger created inside a command will be unbound when that + * command completes, and may be eligible for garbage collection; and a trigger created while an + * opmode is running will be unbound when that opmode ends (and also may be eligible for garbage + * collection). + */ + public void unbind() { + // Ensure all bound commands are canceled + m_bindings.forEach( + (_, bindings) -> { + bindings.forEach(binding -> m_scheduler.cancel(binding.command())); + }); + m_bindings.clear(); + m_loop.unbind(m_eventLoopCallback); // note: ConcurrentModificationException if called in poll() + m_bound = false; + } + // 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 @@ -344,9 +448,11 @@ public class Trigger implements BooleanSupplier { .computeIfAbsent(bindingType, _k -> new ArrayList<>()) .add(new Binding(scope, bindingType, command, new Throwable().getStackTrace())); - if (!m_isBoundToEventLoop) { + if (!m_bound) { + // Ensure we're bound to the event loop. + // Otherwise, the command binding will never fire. m_loop.bind(m_eventLoopCallback); - m_isBoundToEventLoop = true; + m_bound = true; } } diff --git a/commandsv3/src/test/java/org/wpilib/command3/TriggerTest.java b/commandsv3/src/test/java/org/wpilib/command3/TriggerTest.java index 366284e575..d5ed5329cd 100644 --- a/commandsv3/src/test/java/org/wpilib/command3/TriggerTest.java +++ b/commandsv3/src/test/java/org/wpilib/command3/TriggerTest.java @@ -4,12 +4,14 @@ 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; import java.util.List; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.BooleanSupplier; import org.junit.jupiter.api.Test; class TriggerTest extends CommandTestBase { @@ -276,4 +278,369 @@ class TriggerTest extends CommandTestBase { assertEquals(List.of(), m_scheduler.getRunningCommands().stream().map(Command::name).toList()); assertFalse(triggeredCommandRan.get(), "Command was unexpectedly triggered"); } + + @Test + void risingEdge() { + var signal = new AtomicBoolean(false); + var baseTrigger = new Trigger(m_scheduler, signal::get); + var risingEdgeTrigger = baseTrigger.risingEdge(); + + 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 signal = new AtomicBoolean(false); + var baseTrigger = new Trigger(m_scheduler, signal::get); + var fallingEdgeTrigger = baseTrigger.fallingEdge(); + + 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")); + } + + @Test + void ensureBoundBindsDependencies() { + var a = new AtomicBoolean(false); + var b = new AtomicBoolean(false); + + var baseA = new Trigger(m_scheduler, a::get); + var baseB = new Trigger(m_scheduler, b::get); + + // Compose a trigger that depends on an intermediate, unbound risingEdge() trigger + var composed = baseA.and(baseB.risingEdge()); + + var command = Command.noRequirements(Coroutine::park).named("Cmd"); + // Bind only the composed trigger; ensureBound() must bind dependencies first so polling order + // updates base triggers before evaluating the composed condition. + composed.onTrue(command); + + // First run initializes all signals to LOW + m_scheduler.run(); + assertFalse( + m_scheduler.isRunning(command), "Command should not run on first initialization run"); + + // Cause both conditions to be true in the same cycle: A is true, and B has a rising edge + a.set(true); + b.set(true); + m_scheduler.run(); + + assertTrue( + m_scheduler.isRunning(command), + "Top-level composed trigger did not fire when dependency rising edge occurred"); + } + + @Test + void ensureBoundDeeplyNestedDependencies() { + var a = new AtomicBoolean(false); + var b = new AtomicBoolean(false); + var c = new AtomicBoolean(false); + + var baseA = new Trigger(m_scheduler, a::get); + var baseB = new Trigger(m_scheduler, b::get); + var baseC = new Trigger(m_scheduler, c::get); + + // Two levels of nesting: baseA AND (baseB.risingEdge() AND baseC.risingEdge()) + final var nested = baseA.and(baseB.risingEdge().and(baseC.risingEdge())); + + // Initialize signals + m_scheduler.run(); + + // Trigger both rising edges and set A high in the same cycle + a.set(true); + b.set(true); + c.set(true); + m_scheduler.run(); + + assertTrue( + nested.getAsBoolean(), + "Deeply nested composed trigger did not fire; dependencies may not have been bound first"); + } + + @Test + void composedAnd() { + var signalA = new AtomicBoolean(false); + var signalB = new AtomicBoolean(false); + var triggerA = new Trigger(m_scheduler, flickering(signalA)); + var triggerB = new Trigger(m_scheduler, flickering(signalB)); + + var andTrigger = triggerA.and(triggerB); + + m_scheduler.run(); + assertFalse(andTrigger.getAsBoolean()); + + signalA.set(true); + m_scheduler.run(); + assertFalse(andTrigger.getAsBoolean()); + + signalA.set(true); + signalB.set(true); + m_scheduler.run(); + assertTrue(andTrigger.getAsBoolean()); + + signalA.set(false); + m_scheduler.run(); + assertFalse(andTrigger.getAsBoolean()); + } + + @Test + void composedAndWithSupplier() { + var signalA = new AtomicBoolean(false); + var signalB = new AtomicBoolean(false); + var triggerA = new Trigger(m_scheduler, flickering(signalA)); + + var andTrigger = triggerA.and(flickering(signalB)); + + m_scheduler.run(); + assertFalse(andTrigger.getAsBoolean()); + + signalA.set(true); + signalB.set(true); + m_scheduler.run(); + assertTrue(andTrigger.getAsBoolean()); + + signalB.set(false); + m_scheduler.run(); + assertFalse(andTrigger.getAsBoolean()); + } + + @Test + void composedOr() { + var signalA = new AtomicBoolean(false); + var signalB = new AtomicBoolean(false); + var triggerA = new Trigger(m_scheduler, flickering(signalA)); + var triggerB = new Trigger(m_scheduler, flickering(signalB)); + + var orTrigger = triggerA.or(triggerB); + + m_scheduler.run(); + assertFalse(orTrigger.getAsBoolean()); + + signalA.set(true); + m_scheduler.run(); + assertTrue(orTrigger.getAsBoolean()); + + signalA.set(false); + m_scheduler.run(); + assertFalse(orTrigger.getAsBoolean()); + + signalB.set(true); + m_scheduler.run(); + assertTrue(orTrigger.getAsBoolean()); + } + + @Test + void composedOrWithSupplier() { + var signalA = new AtomicBoolean(false); + var signalB = new AtomicBoolean(false); + var triggerA = new Trigger(m_scheduler, flickering(signalA)); + + var orTrigger = triggerA.or(flickering(signalB)); + + m_scheduler.run(); + assertFalse(orTrigger.getAsBoolean()); + + signalB.set(true); + m_scheduler.run(); + assertTrue(orTrigger.getAsBoolean()); + } + + @Test + void composedNegate() { + var signal = new AtomicBoolean(false); + var trigger = new Trigger(m_scheduler, flickering(signal)); + + var negated = trigger.negate(); + + m_scheduler.run(); + assertTrue(negated.getAsBoolean()); + + signal.set(true); + m_scheduler.run(); + assertFalse(negated.getAsBoolean()); + } + + @Test + void selfComposition() { + var signal = new AtomicBoolean(false); + var trigger = new Trigger(m_scheduler, flickering(signal)); + + var selfAnd = trigger.and(trigger); + var selfOr = trigger.or(trigger); + + m_scheduler.run(); + assertFalse(selfAnd.getAsBoolean()); + assertFalse(selfOr.getAsBoolean()); + + signal.set(true); + m_scheduler.run(); + assertTrue(selfAnd.getAsBoolean()); + assertTrue(selfOr.getAsBoolean()); + } + + @Test + void complexComposition() { + var signalA = new AtomicBoolean(false); + var signalB = new AtomicBoolean(false); + var signalC = new AtomicBoolean(false); + var triggerA = new Trigger(m_scheduler, flickering(signalA)); + var triggerB = new Trigger(m_scheduler, flickering(signalB)); + var triggerC = new Trigger(m_scheduler, flickering(signalC)); + + // (A and B) or (not C) + var composed = triggerA.and(triggerB).or(triggerC.negate()); + + // Initially A=F, B=F, C=F. (F and F) or (not F) -> F or T -> T + m_scheduler.run(); + assertTrue(composed.getAsBoolean()); + + // A=T, B=T, C=T. (T and T) or (not T) -> T or F -> T + signalA.set(true); + signalB.set(true); + signalC.set(true); + m_scheduler.run(); + assertTrue(composed.getAsBoolean()); + + // A=F, B=T, C=T. (F and T) or (not T) -> F or F -> F + signalA.set(false); + signalC.set(true); // Ensure C is high for next run if it flickered + m_scheduler.run(); + assertFalse(composed.getAsBoolean()); + } + + @Test + void triggerUnbindsWhenCommandScopeInactive() { + var triggerSignal = new AtomicBoolean(false); + var commandRan = new AtomicBoolean(false); + var innerCommand = Command.noRequirements(_ -> commandRan.set(true)).named("Inner"); + + var outerCommand = + Command.noRequirements( + co -> { + var trigger = new Trigger(m_scheduler, triggerSignal::get); + trigger.onTrue(innerCommand); + co.park(); + }) + .named("Outer"); + + m_scheduler.schedule(outerCommand); + m_scheduler.run(); + assertTrue(m_scheduler.isRunning(outerCommand)); + + triggerSignal.set(true); + m_scheduler.run(); + assertTrue(commandRan.get()); + + // Cancel outer command, trigger should now be out of scope + m_scheduler.cancel(outerCommand); + m_scheduler.run(); + assertFalse(m_scheduler.isRunning(outerCommand)); + + // The trigger should have unbound itself during the last run() call. + } + + private BooleanSupplier flickering(AtomicBoolean signal) { + return () -> { + boolean val = signal.get(); + if (val) { + signal.set(false); + } + return val; + }; + } } diff --git a/design-docs/commands-v3.md b/design-docs/commands-v3.md index 1dc642413a..8ba0da32a4 100644 --- a/design-docs/commands-v3.md +++ b/design-docs/commands-v3.md @@ -187,7 +187,8 @@ and manually scheduled commands. A command scheduled in a certain scope, a trigger binding created in a certain scope, or a default command set in a certain scope will only exist _within that scope_. Changing the default command for a mechanism within a custom opmode will only be applied while that opmode is active; when the opmode exits, the default command specified in the robot -constructor (or mechanism constructor) will be reapplied. +constructor (or mechanism constructor) will be reapplied. Triggers created in a scope will be unbound from the event +loop and be allowed to be garbage collected if no existing references exist. ```java class Robot extends OpModeRobot { @@ -420,12 +421,13 @@ and `Command` (a scope for a specific running command). ### Scheduler `run()` Cycle 1. Cancel commands bound to inactive scopes -2. Run periodic sideload functions -3. Poll the event loop for triggers (which may queue or cancel commands) -4. Schedule default commands for the next iteration to pick up and start running -5. Promote scheduled commands to running +2. Cancel triggers bound to inactive scopes +3. Run periodic sideload functions +4. Poll the event loop for triggers (which may queue or cancel commands) +5. Schedule default commands for the next iteration to pick up and start running +6. Promote scheduled commands to running 1. Cancels any running commands that conflict with scheduled ones -6. Iterate over running commands +7. Iterate over running commands 1. Mount 2. Run until yield point is reached or an error is raised 3. Unmount diff --git a/styleguide/checkstyle.xml b/styleguide/checkstyle.xml index 2ca946b294..452fc69fdf 100644 --- a/styleguide/checkstyle.xml +++ b/styleguide/checkstyle.xml @@ -176,7 +176,7 @@ module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN" + value="^(_|_?[a-zA-Z]|_?[a-z][a-zA-Z0-9]+)$" /> diff --git a/wpilibj/src/main/java/org/wpilib/event/EventLoop.java b/wpilibj/src/main/java/org/wpilib/event/EventLoop.java index b87f1ae390..a92ebb4488 100644 --- a/wpilibj/src/main/java/org/wpilib/event/EventLoop.java +++ b/wpilibj/src/main/java/org/wpilib/event/EventLoop.java @@ -30,6 +30,19 @@ public final class EventLoop { m_bindings.add(action); } + /** + * Unbind an action from running when the loop is polled. Has no effect if the given action is not + * already bound. + * + * @param action the action to unbind. + */ + public void unbind(Runnable action) { + if (m_running) { + throw new ConcurrentModificationException("Cannot unbind EventLoop while it is running"); + } + m_bindings.remove(action); + } + /** Poll all bindings. */ @SuppressWarnings("PMD.UnusedAssignment") public void poll() {