Issue 84 - Simplify engine and executor

This change rewrites how the `Strongback.java` class is implemented to correct a number of errors and keep the engine as simple as possible. All of the existing public methods are still available, though quite a few have been deprecated and now may no longer do anything. For example, the different timer modes and clocks only complicated how to configure Strongback, yet really didn’t offer any advantages and may have made the code worse by making the execution period less consistent. In short, robot code can continue to use these deprecated methods, but the methods may have no effect and could be removed from the robot’s codebase. Note that the deprecated methods will be removed in the next major release of Strongback for 2017.

This change also corrects and simplifies how the executor thread operates. Now, Strongback measures the time required to run all executables (e.g., the switch reactor, command scheduler, data recorder, event recorder, etc.), and uses this time to determine if the executor did not complete in the specified amount of time. This should improve the accuracy of the error messages about taking too long to execute each cycle. And Strongback only uses the “busy” timer mode and the system clock, which simplified the executor implementation.

Finally, Strongback now supports priorities for its executables:

* High priority executables will run every cycle of the executor;
* Medium priority executables will run every other cycle of the executor; and
* Low priority executables will run every 4th cycle of the executor.

Currently, the command scheduler (through with all commands are run) will operate at HIGH priority, while the data recorder (if used) and the switch reactor will operate at the MEDIUM priority. The event recorder still records the time at which events occurred, but events are written out at LOW priority.

Author: rhauch

build.properties

@@ -1,4 +1,4 @@
-strongback.version=1.1.7
+strongback.version=1.2.0
 #
 # The build will download a specific version of the WPILib given by the following URL
 # and install it into the 'libs/wpilib' folder. To use a different version of WPILib,

build.xml

@@ -88,7 +88,6 @@
 
             <doctitle><![CDATA[<h1>Strongback Java Library</h1>]]></doctitle>
             <bottom><![CDATA[<i>Copyright &#169; ${current.year} Strongback and individual contributors. All Rights Reserved.</i>]]></bottom>
-            <tag name="todo" scope="all" description="To do:"/>
             <group title="Strongback" packages="org.strongback"/>
             <group title="Components" packages="org.strongback.components*:org.strongback.hardware*"/>
             <group title="Drives" packages="org.strongback.drive"/>

strongback-tests/src/org/strongback/ExecutableTimerTest.java

@@ -20,19 +20,15 @@
 
 import java.util.concurrent.TimeUnit;
 
-import junit.framework.AssertionFailedError;
-
 import org.junit.AfterClass;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.strongback.Logger.Level;
-import org.strongback.Strongback.Configurator.TimerMode;
+
+import junit.framework.AssertionFailedError;
 
 /**
  * This test attempts to measure the timing of the {@link Strongback#executor() Strongback Executor} instance at various periods
- * using the system clock and various {@link TimerMode}s. On OS X 10.10.3, the {@link TimerMode#BUSY} is clearly the most
- * accurate down to 1 millisecond, whereas {@link TimerMode#PARK} and {@link TimerMode#SLEEP} are relatively inaccurate and
- * imprecise for intervals even as high as 20ms.
  * <p>
  * The code committed into Git are small execution rates and sample sizes to minimize the time required to run the tests. As
  * such, the results are probably not terribly meaningful. To obtain more meaningful results on your own platform, simply adjust
@@ -44,28 +40,22 @@ public class ExecutableTimerTest {
 
     @BeforeClass
     public static void beforeAll() {
-        Strongback.configure().useSystemLogger(Level.ERROR).recordNoData().recordNoEvents().initialize();
-        Strongback.start();
+        Strongback.configure().setLogLevel(Level.ERROR).recordNoData().recordNoEvents().initialize();
     }
 
     @AfterClass
     public static void afterAll() {
-        Strongback.shutdown();
-    }
-
-    @Test
-    public void shouldMeasureAndPrintTimingHistogramUsingBusyMode() throws InterruptedException {
-        runTimer(TimerMode.BUSY, 4, 2000);
+        Strongback.stop();
     }
 
     @Test
-    public void shouldMeasureAndPrintTimingHistogramUsingParkMode() throws InterruptedException {
-        runTimer(TimerMode.PARK, 5, 2000);
+    public void shouldMeasureAndPrintTimingHistogramFor4MillisecondPeriod() throws InterruptedException {
+        runTimer(4, 2000);
     }
 
     @Test
-    public void shouldMeasureAndPrintTimingHistogramUsingSleepMode() throws InterruptedException {
-        runTimer(TimerMode.SLEEP, 6, 2000);
+    public void shouldMeasureAndPrintTimingHistogramFor10MillisecondPeriod() throws InterruptedException {
+        runTimer(10, 2000);
     }
 
     /**
@@ -75,14 +65,14 @@ public void shouldMeasureAndPrintTimingHistogramUsingSleepMode() throws Interrup
      * @param millisecondExecutionPeriod the execution period in milliseconds
      * @param sampleTimeInMilliseconds the sample time in milliseconds
      */
-    protected void runTimer(TimerMode mode, int millisecondExecutionPeriod, int sampleTimeInMilliseconds) {
+    protected void runTimer(int millisecondExecutionPeriod, int sampleTimeInMilliseconds) {
         try {
+            Strongback.stop();
             Strongback.configure()
-                      .useExecutionTimerMode(mode)
-                      .useExecutionPeriod(millisecondExecutionPeriod, TimeUnit.MILLISECONDS)
-                      .initialize();
+                      .useExecutionPeriod(millisecondExecutionPeriod, TimeUnit.MILLISECONDS);
+            Strongback.start();
             assertThat(ExecutableTimer.measureTimingAndPrint(Strongback.executor(),
-                                                             mode.toString() + " for " + millisecondExecutionPeriod + " ms",
+                                                             " for " + millisecondExecutionPeriod + " ms",
                                                              sampleTimeInMilliseconds / millisecondExecutionPeriod)
                                       .await(10, TimeUnit.SECONDS)
                                       .isComplete());

strongback-tests/src/org/strongback/StrongbackTest.java

@@ -0,0 +1,89 @@
+/*
+ * Strongback
+ * Copyright 2015, Strongback and individual contributors by the @authors tag.
+ * See the COPYRIGHT.txt in the distribution for a full listing of individual
+ * contributors.
+ *
+ * Licensed under the MIT License; you may not use this file except in
+ * compliance with the License. You may obtain a copy of the License at
+ * http://opensource.org/licenses/MIT
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.strongback;
+
+import static org.fest.assertions.Assertions.assertThat;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.strongback.Logger.Level;
+import org.strongback.components.Clock;
+
+/**
+ * Tests that check the functionality of the {@link Strongback.Engine}.
+ */
+public class StrongbackTest {
+
+    private static final SystemLogger LOGGER = new SystemLogger();
+
+    private Clock clock;
+    private Strongback.Engine engine;
+
+    @Before
+    public void beforeEach() {
+        LOGGER.enable(Level.INFO);
+        clock = Clock.system();
+        engine = new Strongback.Engine(clock, LOGGER);
+    }
+
+    @After
+    public void afterEach() {
+        try {
+            if (engine != null && engine.isRunning()) {
+                engine.stop();
+            }
+        } finally {
+            engine = null;
+        }
+    }
+
+    @Test
+    public void shouldNotBeRunningWhenCreated() {
+        assertThat(engine.isRunning()).isFalse();
+    }
+
+    @Test
+    public void shouldStartWithDefaultConfiguration() {
+        engine.logConfiguration();
+        assertThat(engine.isRunning()).isFalse();
+        assertThat(engine.start()).isTrue();
+        assertThat(engine.isRunning()).isTrue();
+    }
+
+    @Test
+    public void shouldAllowChangingExecutionPeriodWhenNotRunning() {
+        assertThat(engine.isRunning()).isFalse();
+        assertThat(engine.getExecutionPeriod()).isEqualTo(20);
+        engine.setExecutionPeriod(5);
+        assertThat(engine.getExecutionPeriod()).isEqualTo(5);
+        assertThat(engine.start()).isTrue();
+        engine.logConfiguration();
+    }
+
+    @Test
+    public void shouldNotAllowChangingExecutionPeriodWhenRunning() {
+        assertThat(engine.isRunning()).isFalse();
+        assertThat(engine.start()).isTrue();
+        assertThat(engine.getExecutionPeriod()).isEqualTo(20);
+        LOGGER.enable(Level.OFF);
+        assertThat(engine.setExecutionPeriod(5)).isFalse();
+        LOGGER.enable(Level.INFO);
+        assertThat(engine.getExecutionPeriod()).isEqualTo(20);
+    }
+
+}

strongback/src/org/strongback/Executables.java

@@ -16,41 +16,89 @@
 
 package org.strongback;
 
-import java.util.Iterator;
+import java.util.List;
 import java.util.concurrent.CopyOnWriteArrayList;
 
 import org.strongback.annotation.ThreadSafe;
 
 /**
  * A simple threadsafe list of {@link Executable} instances.
+ *
  * @author Randall Hauch
  */
 @ThreadSafe
-final class Executables implements Executor, Iterable<Executable> {
+final class Executables implements Executor {
 
-    private final CopyOnWriteArrayList<Executable> executables = new CopyOnWriteArrayList<>();
+    private final CopyOnWriteArrayList<Executable> highPriority = new CopyOnWriteArrayList<>();
+    private final CopyOnWriteArrayList<Executable> mediumPriority = new CopyOnWriteArrayList<>();
+    private final CopyOnWriteArrayList<Executable> lowPriority = new CopyOnWriteArrayList<>();
 
     Executables() {
     }
 
     @Override
-    public boolean register(Executable r) {
-        return executables.addIfAbsent(r);
+    public boolean register(Executable r, Priority priority) {
+        if (priority != null) {
+            unregister(r);
+            switch (priority) {
+                case HIGH:
+                    mediumPriority.remove(r);
+                    lowPriority.remove(r);
+                    return highPriority.addIfAbsent(r);
+                case MEDIUM:
+                    highPriority.remove(r);
+                    lowPriority.remove(r);
+                    return mediumPriority.addIfAbsent(r);
+                case LOW:
+                    highPriority.remove(r);
+                    mediumPriority.remove(r);
+                    return lowPriority.addIfAbsent(r);
+            }
+        }
+        return false;
     }
 
     @Override
     public boolean unregister(Executable r) {
-        return executables.remove(r);
+        if (r != null) {
+            // Remove from all
+            boolean removed = highPriority.remove(r);
+            removed = mediumPriority.remove(r) || removed;
+            removed = lowPriority.remove(r) || removed;
+            return removed;
+        }
+        return false;
     }
 
     @Override
     public void unregisterAll() {
-        executables.clear();
+        highPriority.clear();
+        mediumPriority.clear();
+        lowPriority.clear();
     }
 
-    @Override
-    public Iterator<Executable> iterator() {
-        return executables.iterator();
+    public List<Executable> lowPriorityExecutables() {
+        return lowPriority;
+    }
+
+    public List<Executable> mediumPriorityExecutables() {
+        return mediumPriority;
+    }
+
+    public List<Executable> highPriorityExecutables() {
+        return highPriority;
+    }
+
+    protected Executable[] lowPriorityExecutablesAsArrays() {
+        return lowPriority.toArray(new Executable[0]); // will be reallocated with correct size
+    }
+
+    protected Executable[] mediumPriorityExecutablesAsArrays() {
+        return mediumPriority.toArray(new Executable[0]); // will be reallocated with correct size
+    }
+
+    protected Executable[] highPriorityExecutablesAsArrays() {
+        return highPriority.toArray(new Executable[0]); // will be reallocated with correct size
     }
 
 }

strongback/src/org/strongback/Executor.java

@@ -24,21 +24,45 @@
 @ThreadSafe
 public interface Executor {
 
+    public static enum Priority {
+        HIGH, MEDIUM, LOW;
+    }
+
+    /**
+     * Register a high priority {@link Executable} task so that it is called repeatedly on Strongback's executor thread.
+     *
+     * @param task the executable task
+     * @return {@code true} if the executable task was registered for the first time as a high priority, or {@code false} if
+     *         {@code task} was null or was already registered with this executor at high priority
+     * @deprecated Use {@link #register(Executable, Priority)} instead
+     */
+    @Deprecated
+    default boolean register(Executable task) {
+        return register(task, Priority.HIGH);
+    }
+
     /**
-     * Register an {@link Executable} task to be called repeatedly.
+     * Register an {@link Executable} task with the given priority so that it is called repeatedly on Strongback's executor
+     * thread. If the given task is already registered with a different priority, this method reassigns it to the desired
+     * priority; if the given task is already registered with the desired priority, this method does nothing.
+     * <p>
+     * This executor runs high priority tasks every cycle, medium priority tasks slightly every other cycles, and low priority
+     * tasks every 4 cycles. All {@link Executable} tasks are called on the first cycle.
      *
-     * @param r the executable task
-     * @return true if the executable task was registered, or false if it was null or was already registered with this executor
+     * @param task the executable task
+     * @param priority the priority of the executable; may not be null
+     * @return {@code true} if the executable task was registered for the first time at the given priority, or {@code false} if
+     *         {@code task} was null or was already registered with this executor at the given priority
      */
-    public boolean register(Executable r);
+    public boolean register(Executable task, Priority priority);
 
     /**
      * Unregister an {@link Executable} task to no longer be called.
      *
-     * @param r the executable task
+     * @param task the executable task
      * @return true if the executable task was unregistered, or false if it was null or not registered with this executor
      */
-    public boolean unregister(Executable r);
+    public boolean unregister(Executable task);
 
     /**
      * Unregister all {@link Executable} tasks.