Menu Options in Sysconfig

firmware/Core/Inc/Config/config.h

@@ -108,6 +108,27 @@
 #define PID_INITIAL_STATUS false
 #define PID_TASK_OSDELAY 10
 
+// The two PID enables are deliberately separate, and both have to be on for the loop to drive
+// anything:
+//
+//   PID_CONTROLLER_TASK_ENABLE (debug.h, compile-time) -- whether the task exists and runs at
+//     all. Off, the thread suspends itself at entry and no amount of runtime configuration
+//     brings it back; it is a build of the firmware without a PID controller in it.
+//   PID_ENABLE (here, runtime) -- whether the SessionController *offers* the loop. This is the
+//     "PID CONTROL" menu page and the SYSCFG_PID_ENABLE parameter, which are the same value:
+//     the encoder writes it on the board, the host writes it over USB. Off, the task is alive
+//     but never armed, and the rotary encoder drives the brake by hand instead.
+//
+// So the compile-time one decides whether the machinery is present, and this one decides
+// whether the user is given it. Default off: a board with no host attached comes up in manual
+// brake control, which is the mode that needs no setpoint to be meaningful.
+#define PID_ENABLE 0
+
+// The PID setpoint, in RPM. Also the "PID DES RPM" menu page and SYSCFG_PID_DESIRED_RPM -- one
+// value with two editors, same as PID_ENABLE above. The store accepts 0..65535 (a shaft speed
+// is a uint16_t); the on-board editor walks five digits, so it can reach all of it.
+#define PID_DESIRED_RPM 5000
+
 // USB config
 #define USB_TX_BUFFER_SIZE 512 // Buffer that is being sent to USB peripheral
 // 2ms: drain in smaller, more frequent batches. At 5ms a busy session filled the 512-byte

firmware/Core/Inc/Config/debug.h

@@ -41,7 +41,8 @@
 // SD Controller Task
 #define SD_CONTROLLER_TASK_ENABLE 0
 
-// PID Controller Task
+// PID Controller Task. Whether the task exists at all -- not whether the SessionController
+// arms it, which is the runtime PID_ENABLE / SYSCFG_PID_ENABLE in config.h. See the note there.
 #define PID_CONTROLLER_TASK_ENABLE 1
 
 // BPM Controller Task

firmware/Core/Inc/Config/sysconfig_table.inc

@@ -34,3 +34,5 @@
 [SYSCFG_ADS1115_COMP_LAT] = SYSCFG_U32(ADS1115_COMP_LAT, 0u, 1u),
 [SYSCFG_ADS1115_COMP_QUE] = SYSCFG_U32(ADS1115_COMP_QUE, 0u, 3u),
 [SYSCFG_USB_MOCK_MESSAGES] = SYSCFG_U32(0u, 0u, 1u),
+[SYSCFG_PID_ENABLE] = SYSCFG_U32(PID_ENABLE, 0u, 1u),
+[SYSCFG_PID_DESIRED_RPM] = SYSCFG_U32(PID_DESIRED_RPM, 0u, 65535u),

firmware/Core/Inc/MessagePassing/messages_private.h

@@ -35,7 +35,6 @@ extern "C" {
 typedef enum : uint32_t
 {
     DISPLAY_SCREEN_IDLE = 0,   // Attract screen; SELECT opens the settings menu
-    DISPLAY_SCREEN_SD_LOGGING,   // Settings: SD logging on/off
     DISPLAY_SCREEN_PID_ENABLE,   // Settings: whether the PID option may be toggled in-session
     DISPLAY_SCREEN_DESIRED_RPM,   // Settings: the desired-RPM setpoint
     DISPLAY_SCREEN_DESIRED_RPM_EDIT,   // Settings: the same setpoint with the digit cursor showing
@@ -72,8 +71,7 @@ typedef struct {
     uint32_t desired_rpm;   // The PID setpoint being displayed or edited
     display_rpm_digit cursor_digit;   // Digit the encoder edits (DESIRED_RPM_EDIT only)
     bool pid_enabled;   // Whether the PID loop is armed for this session
-    bool pid_option_toggleable;   // Whether the menu allows arming it; also selects the in-session drive-mode field
-    bool sd_logging_enabled;   // Whether SD logging is switched on
+    bool pid_option_toggleable;   // SYSCFG_PID_ENABLE: whether the menu allows arming it; also selects the in-session drive-mode field
     float angular_acceleration;   // Measured angular acceleration in rad/s^2 (session screen detail)
     float peak_force;   // Largest force magnitude seen this session, in N (session screen detail)
     uint32_t session_seconds;   // Seconds since the session started (session screen detail)

firmware/Core/Inc/MessagePassing/messages_public.h

@@ -436,9 +436,11 @@ typedef enum : uint16_t
     SYSCFG_ADS1115_COMP_LAT = 31,   // enum
     SYSCFG_ADS1115_COMP_QUE = 32,   // enum
     SYSCFG_USB_MOCK_MESSAGES = 33,   // enum
+    SYSCFG_PID_ENABLE = 34,   // enum
+    SYSCFG_PID_DESIRED_RPM = 35,   // uint32, RPM
 } sysconfig_param_t;
 
-#define SYSCFG_PARAM_COUNT 34u              // one past the highest sysconfig_param_t id; sizes the firmware store
+#define SYSCFG_PARAM_COUNT 36u              // one past the highest sysconfig_param_t id; sizes the firmware store
 
 DYNO_STATIC_ASSERT(sizeof(sysconfig_param_t) == 2, "Size of sysconfig_param_t must be 2 bytes");
 

firmware/Core/Inc/Tasks/PID/PID.hpp

@@ -39,6 +39,11 @@ class PIDController
 		uint32_t _curTimestamp;
 		uint32_t _prevTimestamp;
 
+		// Whether _prevTimestamp/_prevError describe a real earlier sample of *this* enable.
+		// False after every Reset(), so the next sample sets the baseline instead of being
+		// differenced against a history that does not exist. See Run().
+		bool _havePreviousSample;
+
 		float _curAngularVelocity;
 
 		float _desiredAngularVelocity;

firmware/Core/Inc/Tasks/SessionController/FiniteStateMachine.hpp

@@ -27,9 +27,9 @@
 //
 // SETTINGS_MENU has its own ring of pages, walked with the rotary encoder:
 //
-//   SD_LOGGING_OPTION_DISPLAYED <-> PID_ENABLE_DISPLAYED <-> PID_DESIRED_RPM_DISPLAYED <-> (wraps)
+//   PID_ENABLE_DISPLAYED <-> PID_DESIRED_RPM_DISPLAYED <-> (wraps)
 //
-// SELECT on the two toggle pages flips the setting and redraws in place. SELECT on the
+// SELECT on the toggle page flips the setting and redraws in place. SELECT on the
 // desired-RPM page opens PID_DESIRED_RPM_EDIT, where a cursor (DesiredRpmUnitsState) picks
 // which decimal digit the encoder changes; walking the cursor off either end leaves the editor.
 struct State
@@ -45,12 +45,10 @@ struct State
     enum class SettingsState
     {
         INIT_STATE = 0,
-        SD_LOGGING_OPTION_DISPLAYED = 0,
-        // Nothing ever enters SD_LOGGING_OPTION_EDIT or PID_ENABLE_EDIT: a toggle is applied on
-        // the display page itself, so those two settings have no edit screen. They are kept only
-        // so the enumerators below hold their values.
-        SD_LOGGING_OPTION_EDIT,
-        PID_ENABLE_DISPLAYED,
+        PID_ENABLE_DISPLAYED = 0,
+        // Nothing ever enters PID_ENABLE_EDIT: a toggle is applied on the display page itself,
+        // so that setting has no edit screen. It is kept only so the enumerators below hold
+        // their values.
         PID_ENABLE_EDIT,
         PID_DESIRED_RPM_DISPLAYED,
         PID_DESIRED_RPM_EDIT
@@ -99,23 +97,38 @@ class FSM
     // the request arrives. The value is clamped to the same envelope the encoder is.
     bool SetHostBrakeDutyCycle(float dutyCycle);
 
+    // Redraws the current screen if one of the two settings it can show has been changed by
+    // somebody other than this FSM. That means the host: USB_CMD_SET_SYSCONFIG is applied by the
+    // USB task straight into the store, deliberately with no queue and no task notification, so
+    // nothing tells this class the value moved. Every other route to the panel is an event this
+    // FSM handles and reposts on its way through, which is why an encoder tick always redraws.
+    //
+    // Same shape as the force sensor's ReconcileConfig (ForceSensor_ADS1115.cpp), which is how
+    // every sysconfig consumer on the board keeps up: hold a shadow of what was last applied,
+    // compare it against the store each pass, act only on a difference -- and, importantly,
+    // leave the shadow stale when the apply fails so the next pass retries. Here the "apply" is
+    // the queue post and PostDisplayState does that bookkeeping.
+    //
+    // Called once per SessionController pass. That task never blocks indefinitely (it ends every
+    // iteration on osDelay), so it needs no equivalent of the force sensor's bounded
+    // FORCESENSOR_COMMAND_POLL_OSDELAY wait to stay awake for this.
+    void ReconcileHostEditedSettings();
+
     // What the SessionController acts on
     State GetState() const;
-    bool GetSDLoggingEnabledStatus() const;
     bool GetPIDEnabledModeStatus() const;
     bool GetPIDOptionToggleableEnabledStatus() const;
 
     bool GetInSessionStatus() const;
 
     float GetDesiredBpmDutyCycle() const;
 
-    float GetDesiredRpm() const;
+    uint32_t GetDesiredRpm() const;
     float GetDesiredAngularVelocity() const;
 
 private:
     // --- Screens. Each sets the state it represents and reposts it.
     void ShowIdleScreen();
-    void ShowSdLoggingPage();
     void ShowPidEnablePage();
     void ShowDesiredRpmPage();
     void ShowDesiredRpmEditor();
@@ -152,10 +165,20 @@ class FSM
 
     State _state;
 
-    // Settings, edited from the menu.
-    bool _sdLoggingEnabled;
-    bool _pidOptionToggleableEnabled;
-    int _desiredRpm;
+    // The two settings this menu edits are NOT members: they live in the sysconfig store as
+    // SYSCFG_PID_ENABLE and SYSCFG_PID_DESIRED_RPM, because the host can write them over USB
+    // too and the two editors have to be editing the same value. A cached copy here would be
+    // the thing that goes stale -- the panel would show what the encoder last set while the
+    // PID ran on what the host last pushed. So the getters below read the store, and the
+    // handlers write it; see the note in Config/config.h for how this pairs with the
+    // compile-time PID_CONTROLLER_TASK_ENABLE.
+    //
+    // These two are the exception that proves it, and they are not copies of the settings: they
+    // record what the last PostDisplayState *carried*, so ReconcileHostEditedSettings can tell
+    // that a host write has left the panel showing something else. Nothing reads them as a
+    // setting -- every read of the settings themselves still goes to the store.
+    bool _postedPidOptionEnabled;
+    uint32_t _postedDesiredRpm;
 
     // Session state. Whether a session is running is _state.mainState and nothing else --
     // see GetInSessionStatus.

firmware/Core/Inc/Tasks/SessionController/SessionController.hpp

@@ -63,9 +63,8 @@ class SessionController
 
         // Each of these is one step of a Run() iteration; all of them are edge-triggered
         // against the _prev* fields below, so a steady state produces no queue traffic.
-        void PublishSdLoggingChange();
         void PublishSessionTransition(bool inSession);
-        void PublishPidEnableChange(bool pidEnabled);
+        void PublishPidInstruction(bool pidEnabled);
         void AwaitPidAck(bool pidEnabled, bool pidOptionEnabled);
         void DriveManualBrake();
         void UpdateMeasurementDisplay();
@@ -79,8 +78,8 @@ class SessionController
         session_controller_os_task_queues* _task_queues;
 
         // Last values posted to the other tasks. A step runs only when its value moves.
-        bool _prevSDLoggingEnabled;
         bool _prevPIDEnabled;
+        float _prevDesiredAngularVelocity;
         bool _prevInSession;
         bool _pidAckReceived;
         float _prevBpmDutyCycle;

firmware/Core/Src/Tasks/Display/ILI9341/ili9341_layout.c

@@ -77,7 +77,7 @@ bool ili9341_field_equal(const ili9341_field *a, const ili9341_field *b)
         && memcmp(a->text, b->text, a->length) == 0;
 }
 
-// The two toggle pages share a value row. Fixed at eight characters so "ENABLED " paints over
+// The toggle page's value row. Fixed at eight characters so "ENABLED " paints over
 // the whole of a previous "DISABLED".
 static void add_enabled_disabled(ili9341_frame *out, bool enabled)
 {
@@ -195,13 +195,8 @@ void ili9341_layout(const session_controller_to_display *state,
             add_centred(out, 150, SIZE_SMALL, COLOUR_LABEL, "PRESS SELECT");
             break;
 
-        case DISPLAY_SCREEN_SD_LOGGING:
-            add_centred(out, 60, SIZE_TITLE, COLOUR_LABEL, "SD LOGGING");
-            add_enabled_disabled(out, state->sd_logging_enabled);
-            break;
-
         case DISPLAY_SCREEN_PID_ENABLE:
-            add_centred(out, 60, SIZE_TITLE, COLOUR_LABEL, "PID LOGGING");
+            add_centred(out, 60, SIZE_TITLE, COLOUR_LABEL, "PID CONTROL");
             add_enabled_disabled(out, state->pid_option_toggleable);
             break;
 

firmware/Core/Src/Tasks/Display/Lumex/README.md

@@ -1,6 +1,6 @@
 ---
 module: Lumex display
-summary: Rendering for the Lumex 16x2 character LCD — the six screens, the cell diff, and what it does with the readouts it cannot show.
+summary: Rendering for the Lumex 16x2 character LCD — the five screens, the cell diff, and what it does with the readouts it cannot show.
 code:
   - Core/Inc/Tasks/Display/Lumex/LumexLCD.hpp
   - Core/Src/Tasks/Display/Lumex/LumexLCD.cpp
@@ -32,22 +32,22 @@ typedef struct { char cells[LUMEX_LCD_ROWS][LUMEX_LCD_COLUMNS]; } lumex_frame;
 
 — with **every one of the 32 cells written on every call**, blanks as spaces. Nothing is left
 over from a previous frame, so the result depends only on `state`. That is what makes the diff
-in [5] valid, and what lets `tests/lumex_layout_tests.cpp` pin all six screens cell-for-cell on
+in [5] valid, and what lets `tests/lumex_layout_tests.cpp` pin all five screens cell-for-cell on
 the build machine.
 
-### The six screens
+### The five screens
 
 Written as whole 16-character rows in the tests, because the bugs worth catching are
 off-by-one column errors that a field-level check steps straight over.
 
 ```
-IDLE                  SD_LOGGING            PID_ENABLE
-      DYNO               SD LOGGING           PID LOGGING
-  PRESS SELECT            DISABLED             DISABLED
+IDLE                  PID_ENABLE            DESIRED_RPM
+      DYNO              PID CONTROL          PID DES RPM
+  PRESS SELECT            DISABLED                5000
 
-DESIRED_RPM           DESIRED_RPM_EDIT      SESSION
-  PID DES RPM           PID DES RPM         n:  1235 rpm
-       5000              5000   100         F: 12.34 N  B 45
+DESIRED_RPM_EDIT      SESSION
+  PID DES RPM         n:  1235 rpm
+  5000   100          F: 12.34 N  B 45
 ```
 
 ### Fixed-width fields

firmware/Core/Src/Tasks/Display/Lumex/lumex_layout.c

@@ -34,7 +34,7 @@ static void put_field(lumex_frame *out, unsigned row, unsigned column, size_t wi
     put(out, row, column, scratch, width);
 }
 
-// The second row shared by both toggle pages.
+// The second row of the toggle page.
 static void render_enabled_disabled(lumex_frame *out, bool enabled)
 {
     if (enabled) PUT_LITERAL(out, 1, 4, "ENABLED");
@@ -91,13 +91,8 @@ void lumex_render(const session_controller_to_display *state, lumex_frame *out)
             PUT_LITERAL(out, 1, 2, "PRESS SELECT");
             break;
 
-        case DISPLAY_SCREEN_SD_LOGGING:
-            PUT_LITERAL(out, 0, 3, "SD LOGGING");
-            render_enabled_disabled(out, state->sd_logging_enabled);
-            break;
-
         case DISPLAY_SCREEN_PID_ENABLE:
-            PUT_LITERAL(out, 0, 2, "PID LOGGING");
+            PUT_LITERAL(out, 0, 2, "PID CONTROL");
             render_enabled_disabled(out, state->pid_option_toggleable);
             break;
 

firmware/Core/Src/Tasks/PID/PID.cpp

@@ -17,6 +17,7 @@ PIDController::PIDController(osMessageQueueId_t sessionControllerToPidController
 			_enabled(initialState),
 			_curTimestamp(0),
 			_prevTimestamp(0),
+			_havePreviousSample(false),
 			_curAngularVelocity(static_cast<float>(0)),
 			_desiredAngularVelocity(static_cast<float>(0)),
 			_prevError(static_cast<float>(0)),
@@ -29,13 +30,6 @@ bool PIDController::Init()
 	return true;
 }
 
-static inline float Clamp(float value, float min, float max)
-{
-    if (value < min) return min;
-    if (value > max) return max;
-    return value;
-}
-
 void PIDController::Run()
 {
     float integral = 0.0f;
@@ -91,11 +85,29 @@ void PIDController::Run()
         _curTimestamp = latestOpticalEncoderData.timestamp;
         _curAngularVelocity = latestOpticalEncoderData.angular_velocity;
 
+        _error = static_cast<float>(_desiredAngularVelocity) - _curAngularVelocity;
+
+        // The first sample after an enable only establishes the baseline; it drives nothing.
+        // There is no interval to integrate or differentiate over yet, and GetTimeDelta cannot
+        // say so -- it would answer with the whole time since boot (Reset leaves _prevTimestamp
+        // at 0 while this sample carries a live microsecond counter), or, if the sample happens
+        // to predate the reset, with a full counter period from the wrap branch. Either put a
+        // term the size of the timestamp range into the integral, which saturated the output
+        // and pinned the brake at BPM::SetDutyCycle's clamp the instant the loop was armed.
+        if (!_havePreviousSample)
+        {
+            _prevTimestamp = _curTimestamp;
+            _prevError = _error;
+            _havePreviousSample = true;
+
+            osDelay(sysconfig_get_u32(SYSCFG_PID_TASK_OSDELAY));
+            continue;
+        }
+
         // Compute time delta safely
         timeDelta = GetTimeDelta();
 
         // --- PID calculations ---
-        _error = static_cast<float>(_desiredAngularVelocity) - _curAngularVelocity;
         derivative = (_error - _prevError) / static_cast<float>(timeDelta);
         integral += _error * static_cast<float>(timeDelta);
 
@@ -155,6 +167,10 @@ void PIDController::Reset()
 
 	_error = static_cast<float>(0);
 	_prevError = static_cast<float>(0);
+
+	// Nothing above is a usable history yet -- see the baseline pass in Run(). Clearing this is
+	// what makes the zeroed timestamps safe to leave as they are.
+	_havePreviousSample = false;
 }
 
 void PIDController::SendBrakeDutyCycle(float new_duty_cycle_percent)