Address more review comments

Author: PeterJohnson

design-docs/opmodes.md

@@ -150,16 +150,8 @@ The `OpModeRobot` class is the base class for the user's `Robot` class.  It also
 
 ```java
 public abstract class OpModeRobot {
-  public void disabledStart() {
-    // this code is called when the robot enters disabled state (including at startup)
-  }
-
-  public void disabledPeriodic() {
-    // this code is called periodically while the robot is disabled
-  }
-
-  public void disabledEnd() {
-    // this code is called when the robot exits disabled state, prior to a opmode starting
+  public void nonePeriodic() {
+    // this code is called when no opmode is selected
   }
 
   // these functions allow users to add opmodes without annotations
@@ -177,22 +169,24 @@ The full lifecycle of a opmode is as follows:
 - Operator selects opmode on DS -> opmode object is constructed
 - If different opmode is selected -> `opModeClose()` is called (which typically just calls `close()`), object is released to GC
 - While opmode is selected and robot is disabled, `disabledPeriodic()` is called
-- When the robot is enabled in the selected opmode, `opmodeRun()` is called; the result of this is different for different opmode base classes:
+- When the robot is enabled in the selected opmode, `opModeRun()` is called; the result of this is different for different opmode base classes:
   - `start()` is called once (for `PeriodicOpMode`)
   - `run()` is called (for `LinearOpMode`), or `periodic()` is called periodically (for `PeriodicOpMode`)
 - When the robot is disabled, `opModeStop()` is called (which results in `end()` being called for `PeriodicOpMode`), followed by `opModeClose()` (which results in `close()` being called for both `PeriodicOpMode` and `LinearOpMode`), object is released to GC
 
 Following `opModeClose()` being called, a *new* opmode object is constructed based on the DS teleop/auto/test/match selector and selected opmode.  In teleop/auto/test, the drop-down selection will be the same as before the previous enable, so the same opmode class is constructed again.  In match (or when FMS-connected), only the selected auto opmode object is initially constructed; once auto completes, the selected teleop opmode object is constructed.  Thus only zero or one opmode objects will ever be "alive" at any given time.
 
 For consistency in operation, the library will ensure that `disabledPeriodic()` is always called at least once before `opModeRun()` is called.
-
+- When the robot is enabled in the selected opmode, `opModeRun()` is called; the result of this is different for different opmode base classes:
 User implementations of opmode classes may have either a no-parameter constructor or a constructor that accepts the user's `Robot` class type.  If available, the library will call the latter and pass the user's `Robot` object to it when constructing the class.
 
 The library will use escalating steps to attempt to terminate a opmode if `opModeRun()` does not return within a reasonable timeframe after `opModeStop()` is called, up to and including termination of the robot executable process (which will result in an automatic restart of it at the system level).  User code (particularly in `LinearOpMode` derived classes) should check for `isRunning()` returning false and return as quickly as possible to allow the opmode to terminate gracefully.
 
 ```java
 public interface OpMode {
   // this function is called periodically while the opmode is selected on the DS (robot is disabled)
+  // Note: it may be called once when the robot is enabled to ensure it's always called once before
+  // opModeRun() is called
   default void disabledPeriodic() {}
 
   // this function is called when the opmode starts (robot is enabled)
@@ -225,7 +219,7 @@ public abstract class LinearOpMode implements OpMode {
   public abstract void run() throws InterruptedException:
 
   public boolean isRunning() {
-    // returns true until opModeEnd() is called
+    // returns true until opModeStop() is called
   }
 
   // implements OpMode interface
@@ -235,7 +229,7 @@ public abstract class LinearOpMode implements OpMode {
   }
 
   @Override
-  public final void opModeEnd() {
+  public final void opModeStop() {
     // pseudo-code
     isRunning = false;
   }
@@ -282,10 +276,6 @@ public abstract class PeriodicOpMode implements OpMode {
   // returns the start time of the current loop in microseconds
   public final long getLoopStartTime() {...}
 
-  public boolean isRunning() {
-    // returns true until opModeEnd() is called
-  }
-
   // implements OpMode interface
   @Override
   public final void opModeRun(long opModeId) {
@@ -487,6 +477,7 @@ Adding the following function enables maintaining the available opmode options l
 
 `HAL_OpModeOption` is a structure describing each option:
 - `long id` - unique ID identifying the opmode (robot mode, name) pair.  This encodes the robot mode in the upper bits, indicating which robot mode the opmode should be visible for (auto/teleop/test)
+- `string name`
 - `string group`
 - `string description`
 - `int textColor` - optional, used to set the text color for the option in the DS GUI

glass/src/libnt/native/cpp/NTFMS.cpp

@@ -10,8 +10,17 @@
 
 #include <fmt/format.h>
 
-#include "wpi/util/SmallVector.hpp"
-#include "wpi/util/timestamp.h"
+#include "wpi/util/Endian.hpp"
+
+// FIXME: use dynamic struct decoding
+// Duplicated here from DriverStationTypes.h to avoid HAL dependency
+#define HAL_CONTROLWORD_OPMODE_HASH_MASK 0x00FFFFFFFFFFFFFFLL
+#define HAL_CONTROLWORD_ROBOT_MODE_MASK 0x0300000000000000LL
+#define HAL_CONTROLWORD_ROBOT_MODE_SHIFT 56
+#define HAL_CONTROLWORD_ENABLED_MASK 0x0400000000000000LL
+#define HAL_CONTROLWORD_ESTOP_MASK 0x0800000000000000LL
+#define HAL_CONTROLWORD_FMS_ATTACHED_MASK 0x1000000000000000LL
+#define HAL_CONTROLWORD_DS_ATTACHED_MASK 0x2000000000000000LL
 
 using namespace wpi::glass;
 
@@ -54,16 +63,24 @@ void NTFMSModel::Update() {
     m_allianceStationId.SetValue(v.value - 1 + 3 * (isRed ? 0 : 1), v.time);
   }
   for (auto&& v : m_controlWord.ReadQueue()) {
-    if (v.value.size() != sizeof(uint8_t)) {
+    if (v.value.size() != sizeof(uint64_t)) {
       continue;
     }
-    uint8_t controlWord = v.value[0];
+    uint64_t controlWord = wpi::util::support::endian::read64le(v.value.data());
     // See wpi::Struct<HAL_ControlWord> definition
-    m_enabled.SetValue(((controlWord & 0x01) != 0) ? 1 : 0, v.time);
-    m_robotMode.SetValue((controlWord & 0x06) >> 1, v.time);
-    m_estop.SetValue(((controlWord & 0x08) != 0) ? 1 : 0, v.time);
-    m_fmsAttached.SetValue(((controlWord & 0x10) != 0) ? 1 : 0, v.time);
-    m_dsAttached.SetValue(((controlWord & 0x20) != 0) ? 1 : 0, v.time);
+    m_enabled.SetValue(
+        ((controlWord & HAL_CONTROLWORD_ENABLED_MASK) != 0) ? 1 : 0, v.time);
+    m_robotMode.SetValue((controlWord & HAL_CONTROLWORD_ROBOT_MODE_MASK) >>
+                             HAL_CONTROLWORD_ROBOT_MODE_SHIFT,
+                         v.time);
+    m_estop.SetValue(((controlWord & HAL_CONTROLWORD_ESTOP_MASK) != 0) ? 1 : 0,
+                     v.time);
+    m_fmsAttached.SetValue(
+        ((controlWord & HAL_CONTROLWORD_FMS_ATTACHED_MASK) != 0) ? 1 : 0,
+        v.time);
+    m_dsAttached.SetValue(
+        ((controlWord & HAL_CONTROLWORD_DS_ATTACHED_MASK) != 0) ? 1 : 0,
+        v.time);
   }
   for (auto&& v : m_gameSpecificMessage.ReadQueue()) {
     m_gameSpecificMessageData.SetValue(std::move(v.value), v.time);

hal/src/main/java/org/wpilib/hardware/hal/ControlWord.java

@@ -69,24 +69,6 @@ public void update(ControlWord word) {
     m_robotMode = word.m_robotMode;
   }
 
-  /**
-   * Gets the Enabled flag.
-   *
-   * @return the Enabled flag
-   */
-  public boolean isEnabled() {
-    return (m_word & ENABLED_MASK) != 0;
-  }
-
-  /**
-   * Gets the robot mode.
-   *
-   * @return the robot mode
-   */
-  public RobotMode getRobotMode() {
-    return m_robotMode;
-  }
-
   /**
    * Gets the opmode ID.
    *
@@ -113,6 +95,24 @@ public void setOpModeId(long id) {
     m_robotMode = RobotMode.fromInt((int) ((m_word & ROBOT_MODE_MASK) >> ROBOT_MODE_SHIFT));
   }
 
+  /**
+   * Gets the robot mode.
+   *
+   * @return the robot mode
+   */
+  public RobotMode getRobotMode() {
+    return m_robotMode;
+  }
+
+  /**
+   * Gets the Enabled flag.
+   *
+   * @return the Enabled flag
+   */
+  public boolean isEnabled() {
+    return (m_word & ENABLED_MASK) != 0;
+  }
+
   /**
    * Gets the E-Stop flag.
    *