From 3800b352b0876260ce3c367193da83832a455711 Mon Sep 17 00:00:00 2001 From: Tomaz Zlindra Date: Wed, 29 Jul 2026 09:20:27 -0700 Subject: [PATCH 1/3] Settings menu: drop SD logging, make the PID settings runtime-tunable The menu had three pages; it now has two. SD LOGGING is gone. Nothing received it: there is no SD controller task, SD_CONTROLLER_TASK_ENABLE is 0 and the queue is NULL in main.c, so every publish path was already compiled out. Removed the page, the FSM state and flag, SessionController::PublishSdLoggingChange, the display screen id and its message field, and the layout branches and tests. The sd_controller queue and TaskMonitor slot stay -- those are the future task's seat at the table, not part of the menu option. PID LOGGING is renamed PID CONTROL, which is what it does: it gates arming the loop in-session, not logging. PID_ENABLE and PID_DESIRED_RPM become runtime sysconfig parameters (ids 34 and 35, appended -- no renumbering, so no protocol bump). The FSM no longer keeps copies: its getters read the store and its handlers write it, so the encoder and the host edit one value rather than two that drift. The compile-time PID_CONTROLLER_TASK_ENABLE stays alongside them and means something different -- whether the task exists at all, versus whether the SessionController offers it -- and config.h now spells that out. Three PID defects fixed, all pre-existing rather than regressions from #119: - The first sample after every enable slammed the brake to full. Reset() zeroes _prevTimestamp while the next sample carries a live microsecond counter, so GetTimeDelta returned time-since-boot and the integral took a term the size of the timestamp range on pass one. K_I defaults to 1.0f, so this fired every time. Guarded with _havePreviousSample: the first sample sets the baseline and drives nothing. A flag rather than seeding the timestamp, because a sample predating the reset hit the same bug through GetTimeDelta's wrap branch. - The loop stayed armed after a session ended, so the task ran on against a finished session until every pass logged a queue-full warning -- and with _prevPIDEnabled stuck true the next session found no enable edge, never pointed the BPM at the PID output, and showed PIDE over a brake the controller no longer reached. ShowSessionScreen disarms on entry; PublishSessionTransition publishes the disable on the way out, which is the only place that can, since Run()'s PID step sits below the in-session gate. - Clamp() had been dead since #119 removed the output mixing. Setpoint changes now reach a running loop too: PublishPidEnableChange becomes PublishPidInstruction and republishes when either half moves. It only ever fired on an enable edge, which was enough while the menu was the sole editor and unreachable mid-session -- but the host can move the setpoint over USB now. Verified: ARM Debug build clean with no warnings, 105/105 firmware host tests, 266/266 Dyno.Core tests, both message_gen drift checks in sync. Co-Authored-By: Claude Opus 5 --- firmware/Core/Inc/Config/config.h | 21 ++++ firmware/Core/Inc/Config/debug.h | 3 +- firmware/Core/Inc/Config/sysconfig_table.inc | 2 + .../Inc/MessagePassing/messages_private.h | 4 +- .../Core/Inc/MessagePassing/messages_public.h | 4 +- firmware/Core/Inc/Tasks/PID/PID.hpp | 5 + .../SessionController/FiniteStateMachine.hpp | 29 +++--- .../SessionController/SessionController.hpp | 5 +- .../Tasks/Display/ILI9341/ili9341_layout.c | 9 +- .../Core/Src/Tasks/Display/Lumex/README.md | 18 ++-- .../Src/Tasks/Display/Lumex/lumex_layout.c | 9 +- firmware/Core/Src/Tasks/PID/PID.cpp | 32 +++++-- firmware/Core/Src/Tasks/PID/README.md | 40 ++++++++ .../SessionController/FiniteStateMachine.cpp | 95 ++++++++----------- .../Src/Tasks/SessionController/README.md | 16 ++-- .../SessionController/SessionController.cpp | 52 ++++++---- firmware/tests/ili9341_layout_tests.cpp | 15 ++- firmware/tests/lumex_layout_tests.cpp | 21 +--- .../message_gen/schema/messages_private.yaml | 4 +- .../message_gen/schema/messages_public.yaml | 16 ++++ src/Dyno.Core/Messages/Generated/Messages.cs | 4 +- .../SysConfig/Generated/SysConfigCatalog.cs | 23 +++++ 22 files changed, 264 insertions(+), 163 deletions(-) diff --git a/firmware/Core/Inc/Config/config.h b/firmware/Core/Inc/Config/config.h index cb1042c..2d0ff2d 100644 --- a/firmware/Core/Inc/Config/config.h +++ b/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 diff --git a/firmware/Core/Inc/Config/debug.h b/firmware/Core/Inc/Config/debug.h index 1e91d57..29deb61 100644 --- a/firmware/Core/Inc/Config/debug.h +++ b/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 diff --git a/firmware/Core/Inc/Config/sysconfig_table.inc b/firmware/Core/Inc/Config/sysconfig_table.inc index 4bec7d0..19bbdbb 100644 --- a/firmware/Core/Inc/Config/sysconfig_table.inc +++ b/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), diff --git a/firmware/Core/Inc/MessagePassing/messages_private.h b/firmware/Core/Inc/MessagePassing/messages_private.h index 8968f72..6a2583c 100644 --- a/firmware/Core/Inc/MessagePassing/messages_private.h +++ b/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) diff --git a/firmware/Core/Inc/MessagePassing/messages_public.h b/firmware/Core/Inc/MessagePassing/messages_public.h index c630921..d315697 100644 --- a/firmware/Core/Inc/MessagePassing/messages_public.h +++ b/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"); diff --git a/firmware/Core/Inc/Tasks/PID/PID.hpp b/firmware/Core/Inc/Tasks/PID/PID.hpp index c8161d1..cbe4a15 100644 --- a/firmware/Core/Inc/Tasks/PID/PID.hpp +++ b/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; diff --git a/firmware/Core/Inc/Tasks/SessionController/FiniteStateMachine.hpp b/firmware/Core/Inc/Tasks/SessionController/FiniteStateMachine.hpp index 4d7066c..744e4c0 100644 --- a/firmware/Core/Inc/Tasks/SessionController/FiniteStateMachine.hpp +++ b/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 @@ -101,7 +99,6 @@ class FSM // What the SessionController acts on State GetState() const; - bool GetSDLoggingEnabledStatus() const; bool GetPIDEnabledModeStatus() const; bool GetPIDOptionToggleableEnabledStatus() const; @@ -109,13 +106,12 @@ class FSM 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 +148,13 @@ 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. // Session state. Whether a session is running is _state.mainState and nothing else -- // see GetInSessionStatus. diff --git a/firmware/Core/Inc/Tasks/SessionController/SessionController.hpp b/firmware/Core/Inc/Tasks/SessionController/SessionController.hpp index 316acbf..d5c1b9c 100644 --- a/firmware/Core/Inc/Tasks/SessionController/SessionController.hpp +++ b/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; diff --git a/firmware/Core/Src/Tasks/Display/ILI9341/ili9341_layout.c b/firmware/Core/Src/Tasks/Display/ILI9341/ili9341_layout.c index e223749..a276b22 100644 --- a/firmware/Core/Src/Tasks/Display/ILI9341/ili9341_layout.c +++ b/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; diff --git a/firmware/Core/Src/Tasks/Display/Lumex/README.md b/firmware/Core/Src/Tasks/Display/Lumex/README.md index 1c39189..208d597 100644 --- a/firmware/Core/Src/Tasks/Display/Lumex/README.md +++ b/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 diff --git a/firmware/Core/Src/Tasks/Display/Lumex/lumex_layout.c b/firmware/Core/Src/Tasks/Display/Lumex/lumex_layout.c index 2b0723a..48ea2ce 100644 --- a/firmware/Core/Src/Tasks/Display/Lumex/lumex_layout.c +++ b/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; diff --git a/firmware/Core/Src/Tasks/PID/PID.cpp b/firmware/Core/Src/Tasks/PID/PID.cpp index a16536d..0cc6185 100644 --- a/firmware/Core/Src/Tasks/PID/PID.cpp +++ b/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(0)), _desiredAngularVelocity(static_cast(0)), _prevError(static_cast(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(_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(_desiredAngularVelocity) - _curAngularVelocity; derivative = (_error - _prevError) / static_cast(timeDelta); integral += _error * static_cast(timeDelta); @@ -155,6 +167,10 @@ void PIDController::Reset() _error = static_cast(0); _prevError = static_cast(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) diff --git a/firmware/Core/Src/Tasks/PID/README.md b/firmware/Core/Src/Tasks/PID/README.md index a30467a..960f7df 100644 --- a/firmware/Core/Src/Tasks/PID/README.md +++ b/firmware/Core/Src/Tasks/PID/README.md @@ -29,17 +29,57 @@ and feeds it to [[BPM]]. Enabled/disabled by [[SessionController]]. 2. **Enabled:** read encoder velocity → compute P/I/D terms → brake duty cycle → BPM queue; ACK SessionController. 3. **Disabled:** empty the command queue, block until the next instruction. +An enable instruction calls `Reset()`, which clears `_havePreviousSample`; the first sample after +it establishes the baseline and drives nothing. See below for why. + ## Errors / warnings - `WARNING_PID_CONTROLLER_MESSAGE_QUEUE_FULL` ## Key constants (config.h) - `K_P`, `K_I`, `K_D`, `PID_MAX_OUTPUT`, `BRAKE_GAIN`, `THROTTLE_GAIN`, `PID_TASK_OSDELAY`, `PID_INITIAL_STATUS` +- `PID_ENABLE`, `PID_DESIRED_RPM` — the two runtime (sysconfig) settings, editable from the + board's settings menu *and* by the host over USB. See below. + +## The two enables +- **`PID_CONTROLLER_TASK_ENABLE`** (`debug.h`, compile time) — whether this task exists. Off, the + thread suspends at entry; nothing at runtime brings it back. +- **`PID_ENABLE` / `SYSCFG_PID_ENABLE`** (`config.h`, runtime) — whether [[SessionController]] + *offers* the loop. Off, the task is alive but never armed and the encoder drives the brake by + hand. This is the `PID CONTROL` menu page. + +Both must be on for the loop to drive anything. `config.h` carries the long-form note. + +## Three fixed bugs, recorded so they are not reintroduced + +- **The first sample after an enable slammed the brake to full.** `Reset()` zeroes + `_prevTimestamp`, but the sample that follows carries a live microsecond counter, so + `GetTimeDelta()` returned *time since boot* — up to 4.29e9. `integral += _error * timeDelta` + therefore took a term the size of the whole timestamp range on the very first pass, the output + saturated, and `BPM::SetDutyCycle` clamped it to the maximum duty cycle, which is where the + brake stayed while the integral unwound. `K_I` defaults to `1.0f`, so this fired on every + arming. Fixed with `_havePreviousSample`: the first sample sets the baseline and produces no + output. A sample that *predates* the reset hit the same bug through `GetTimeDelta`'s wrap + branch, which is why the fix is a flag rather than seeding `_prevTimestamp` from the clock. +- **The loop stayed armed after a session ended.** Nothing cleared the FSM's `_pidEnabled`, and + [[SessionController]] publishes PID instructions below its in-session gate — so the task was + never told to stop. It went on computing against a finished session until every pass logged + `WARNING_PID_CONTROLLER_MESSAGE_QUEUE_FULL`, and since `_prevPIDEnabled` stayed true the *next* + session found no enable edge to publish: the BPM was never pointed at the PID output, so the + panel read `PIDE` over a brake the controller no longer reached. Now `ShowSessionScreen()` + disarms on entry and `PublishSessionTransition(false)` publishes the disable on the way out. +- **A setpoint change never reached a running loop.** See [[SessionController]]'s + `PublishPidInstruction`. ## Notes - Brake-only. The unfinished second output (throttle) and its mixing sketch were removed along with the manual throttle control in the [[SessionController]]; nothing was ever wired to receive either. `THROTTLE_GAIN`, `HORIZONTAL_BIAS`, `VERTICAL_BIAS` and `PID_MAX_OUTPUT` remain in the config schema for whoever revives it, but no code reads them. +- **The output is not bounded by this task.** `SendBrakeDutyCycle` passes the raw gain-scaled + sum, and `BPM::SetDutyCycle` clamps it to the configured duty-cycle envelope. That is the only + limit — there is no anti-windup here beyond the reset on enable, so a sustained error still + grows the integral without bound. Left as-is because the actuator clamp makes it safe, not + because it is good control. - State machine diagram: `pid_brake_controller.puml`. ## Related diff --git a/firmware/Core/Src/Tasks/SessionController/FiniteStateMachine.cpp b/firmware/Core/Src/Tasks/SessionController/FiniteStateMachine.cpp index 9908d42..ccb45dd 100644 --- a/firmware/Core/Src/Tasks/SessionController/FiniteStateMachine.cpp +++ b/firmware/Core/Src/Tasks/SessionController/FiniteStateMachine.cpp @@ -11,9 +11,6 @@ FSM::FSM(osMessageQueueId_t sessionControllerToDisplayHandle) : State::SettingsState::INIT_STATE, State::DesiredRpmUnitsState::INIT_STATE, }, - _sdLoggingEnabled(false), - _pidOptionToggleableEnabled(false), - _desiredRpm(5000), _pidEnabled(false), _desiredManualBpmDutyCycle(0), _rpm(0.0f), @@ -87,20 +84,14 @@ void FSM::HandleRotaryEncoderInSettings(bool positiveTick) { switch (_state.settingsState) { - // The three pages form a ring; a tick steps one page along it in the tick's direction. - case State::SettingsState::SD_LOGGING_OPTION_DISPLAYED: - if (positiveTick) ShowPidEnablePage(); - else ShowDesiredRpmPage(); - break; - + // The two pages form a ring, so a tick lands on the other one whichever way it turned. + // Direction starts mattering again the moment a third page is added. case State::SettingsState::PID_ENABLE_DISPLAYED: - if (positiveTick) ShowDesiredRpmPage(); - else ShowSdLoggingPage(); + ShowDesiredRpmPage(); break; case State::SettingsState::PID_DESIRED_RPM_DISPLAYED: - if (positiveTick) ShowSdLoggingPage(); - else ShowPidEnablePage(); + ShowPidEnablePage(); break; // Inside the editor a tick changes the digit under the cursor rather than the page. @@ -109,8 +100,7 @@ void FSM::HandleRotaryEncoderInSettings(bool positiveTick) ShowDesiredRpmEditor(); break; - // Unreachable -- the toggle settings have no edit screen (see State::SettingsState). - case State::SettingsState::SD_LOGGING_OPTION_EDIT: + // Unreachable -- the toggle setting has no edit screen (see State::SettingsState). case State::SettingsState::PID_ENABLE_EDIT: default: break; @@ -140,7 +130,6 @@ void FSM::HandleButtonBackInSettings() switch (_state.settingsState) { // From any settings page, BACK leaves the menu. - case State::SettingsState::SD_LOGGING_OPTION_DISPLAYED: case State::SettingsState::PID_ENABLE_DISPLAYED: case State::SettingsState::PID_DESIRED_RPM_DISPLAYED: ShowIdleScreen(); @@ -152,8 +141,7 @@ void FSM::HandleButtonBackInSettings() else ShowDesiredRpmEditor(); break; - // Unreachable -- the toggle settings have no edit screen (see State::SettingsState). - case State::SettingsState::SD_LOGGING_OPTION_EDIT: + // Unreachable -- the toggle setting has no edit screen (see State::SettingsState). case State::SettingsState::PID_ENABLE_EDIT: default: break; @@ -165,7 +153,7 @@ void FSM::HandleButtonSelectInput(void) switch (_state.mainState) { case State::MainDynoState::IDLE: - ShowSdLoggingPage(); // opens the settings menu on its first page + ShowPidEnablePage(); // opens the settings menu on its first page break; case State::MainDynoState::SETTINGS_MENU: @@ -176,7 +164,7 @@ void FSM::HandleButtonSelectInput(void) // SELECT arms and disarms the PID loop, and only when the menu option allows it. // With the option off there is nothing to switch: the brake is the only actuator // the encoder drives. - if (_pidOptionToggleableEnabled) _pidEnabled = !_pidEnabled; + if (GetPIDOptionToggleableEnabledStatus()) _pidEnabled = !_pidEnabled; break; } } @@ -186,13 +174,9 @@ void FSM::HandleButtonSelectInSettings() switch (_state.settingsState) { // A toggle is applied and redrawn on the page itself; there is no edit screen to enter. - case State::SettingsState::SD_LOGGING_OPTION_DISPLAYED: - _sdLoggingEnabled = !_sdLoggingEnabled; - ShowSdLoggingPage(); - break; - case State::SettingsState::PID_ENABLE_DISPLAYED: - _pidOptionToggleableEnabled = !_pidOptionToggleableEnabled; + sysconfig_set_raw(SYSCFG_PID_ENABLE, + GetPIDOptionToggleableEnabledStatus() ? 0u : 1u); ShowPidEnablePage(); break; @@ -206,8 +190,7 @@ void FSM::HandleButtonSelectInSettings() else ShowDesiredRpmEditor(); break; - // Unreachable -- the toggle settings have no edit screen (see State::SettingsState). - case State::SettingsState::SD_LOGGING_OPTION_EDIT: + // Unreachable -- the toggle setting has no edit screen (see State::SettingsState). case State::SettingsState::PID_ENABLE_EDIT: default: break; @@ -311,8 +294,18 @@ int FSM::DesiredRpmDigitIncrement() const void FSM::AdjustDesiredRpm(bool positiveTick) { const int increment = DesiredRpmDigitIncrement(); + const int candidate = static_cast(GetDesiredRpm()) + + (positiveTick ? increment : -increment); - _desiredRpm = std::max(0, _desiredRpm + (positiveTick ? increment : -increment)); + // A candidate outside the accepted range is simply not applied, so the value stops at + // either end rather than wrapping -- which is what the old std::max(0, ...) did at the + // bottom. The range itself is deliberately not repeated here: sysconfig_set_raw rejects + // anything outside the store's own bounds for this id, so the editor and a host write + // are held to the same limits without a second copy of them to drift. The negative case + // is caught before the cast, which would otherwise turn -1 into 4294967295. + if (candidate < 0) return; + + sysconfig_set_raw(SYSCFG_PID_DESIRED_RPM, static_cast(candidate)); } // Moves the digit cursor by one place. Returns true when it wrapped past an end of the number, @@ -339,14 +332,6 @@ void FSM::ShowIdleScreen() PostDisplayState(); } -void FSM::ShowSdLoggingPage() -{ - _state.mainState = State::MainDynoState::SETTINGS_MENU; - _state.settingsState = State::SettingsState::SD_LOGGING_OPTION_DISPLAYED; - - PostDisplayState(); -} - void FSM::ShowPidEnablePage() { _state.mainState = State::MainDynoState::SETTINGS_MENU; @@ -392,6 +377,12 @@ void FSM::ShowSessionScreen() // first encoder tick moves into the envelope. _desiredManualBpmDutyCycle = 0.0f; + // Every session starts disarmed, for the same reason the brake starts at 0: arming is a + // decision made during a run, and this one had not been made yet. It used to persist -- + // nothing ever cleared it -- so a session that ended with the loop armed left it armed, and + // the next one came up already driving the brake from the controller without a SELECT. + _pidEnabled = false; + PostDisplayState(); } @@ -451,15 +442,13 @@ display_screen_id FSM::CurrentScreen() const case State::MainDynoState::SETTINGS_MENU: switch (_state.settingsState) { - case State::SettingsState::PID_ENABLE_DISPLAYED: - return DISPLAY_SCREEN_PID_ENABLE; case State::SettingsState::PID_DESIRED_RPM_DISPLAYED: return DISPLAY_SCREEN_DESIRED_RPM; case State::SettingsState::PID_DESIRED_RPM_EDIT: return DISPLAY_SCREEN_DESIRED_RPM_EDIT; - // SD_LOGGING_OPTION_DISPLAYED, plus the two edit states nothing ever enters. + // PID_ENABLE_DISPLAYED, plus the edit state nothing ever enters. default: - return DISPLAY_SCREEN_SD_LOGGING; + return DISPLAY_SCREEN_PID_ENABLE; } case State::MainDynoState::IDLE: @@ -477,11 +466,10 @@ void FSM::PostDisplayState() msg.rpm = _rpm; msg.force = _force; msg.bpm_duty_cycle = _desiredManualBpmDutyCycle; - msg.desired_rpm = static_cast(_desiredRpm); + msg.desired_rpm = GetDesiredRpm(); msg.cursor_digit = static_cast(_state.desiredRpmUnitsState); msg.pid_enabled = _pidEnabled; - msg.pid_option_toggleable = _pidOptionToggleableEnabled; - msg.sd_logging_enabled = _sdLoggingEnabled; + msg.pid_option_toggleable = GetPIDOptionToggleableEnabledStatus(); msg.angular_acceleration = _angularAcceleration; msg.peak_force = _peakForce; @@ -503,20 +491,17 @@ State FSM::GetState() const return _state; } -bool FSM::GetSDLoggingEnabledStatus() const -{ - return _sdLoggingEnabled; -} - // The PID loop runs only when the menu option allows it and it has been switched on in-session. bool FSM::GetPIDEnabledModeStatus() const { - return _pidOptionToggleableEnabled && _pidEnabled; + return GetPIDOptionToggleableEnabledStatus() && _pidEnabled; } +// Read straight out of the sysconfig store on every call rather than cached: the host writes +// this id over USB as well as the menu writing it, and a copy here is what would go stale. bool FSM::GetPIDOptionToggleableEnabledStatus() const { - return _pidOptionToggleableEnabled; + return sysconfig_get_u32(SYSCFG_PID_ENABLE) != 0u; } bool FSM::GetInSessionStatus() const @@ -529,12 +514,14 @@ float FSM::GetDesiredBpmDutyCycle() const return _desiredManualBpmDutyCycle; } -float FSM::GetDesiredRpm() const +uint32_t FSM::GetDesiredRpm() const { - return _desiredRpm; + return sysconfig_get_u32(SYSCFG_PID_DESIRED_RPM); } +// RPM is what the panel and the host talk in; rad/s is what the encoder measures and what the +// PID task compares against, so the conversion happens once, here, on the way out. float FSM::GetDesiredAngularVelocity() const { - return _desiredRpm * 2 * M_PI / 60; + return static_cast(GetDesiredRpm()) * 2.0f * static_cast(M_PI) / 60.0f; } diff --git a/firmware/Core/Src/Tasks/SessionController/README.md b/firmware/Core/Src/Tasks/SessionController/README.md index 5cd89c4..8d46117 100644 --- a/firmware/Core/Src/Tasks/SessionController/README.md +++ b/firmware/Core/Src/Tasks/SessionController/README.md @@ -37,21 +37,23 @@ Each step below is a method of the same name; all are edge-triggered against the so a steady state produces no queue traffic. `PublishStartupState()` runs once before the loop. 1. `_fsm.HandleUserInputs()` — process pending button/encoder events. -2. `PublishSdLoggingChange()` — notify the SD queue only when the setting moves. -3. `PublishSessionTransition()` on a session start/stop edge (`GetInSessionStatus`): tell [[USB]] +2. `PublishSessionTransition()` on a session start/stop edge (`GetInSessionStatus`): tell [[USB]] whether a session is running (it streams sensor data only then), reset the display, and stop [[BPM]] (`STOP_PWM`) on the way out. Sensor sampling itself is enabled once at startup and never gated, so a session starts against sensors that are already warm. There is **no USB-logging option**: USB streaming follows the session, and nothing can turn it - off. (SD logging remains a togglable setting in the menu.) + off. There is no SD-logging option either — that menu page was removed, because no SD task + exists to receive it (`SD_CONTROLLER_TASK_ENABLE` is 0 and the queue is `NULL`). Outside a session the iteration ends here — nothing below may drive an actuator. -4. `PublishPidEnableChange()` — send `session_controller_to_pid_controller` (enable + desired ω); - `AwaitPidAck()` then waits for `pid_controller_ack` before pointing the BPM at the PID output. -5. `DriveManualBrake()` — PID option off only: brake duty cycle to the BPM queue (`START_PWM`), +3. `PublishPidInstruction()` — send `session_controller_to_pid_controller` (enable + desired ω) + when **either** moves; `AwaitPidAck()` then waits for `pid_controller_ack` before pointing the + BPM at the PID output. The setpoint is republished because it is a runtime sysconfig parameter + the host can move mid-session, not only a menu value fixed before the run. +4. `DriveManualBrake()` — PID option off only: brake duty cycle to the BPM queue (`START_PWM`), clamped by the FSM to the same envelope `BPM::SetDutyCycle` enforces. -6. `UpdateMeasurementDisplay()` — drain to the newest `forcesensor_output_data` + +5. `UpdateMeasurementDisplay()` — drain to the newest `forcesensor_output_data` + `optical_encoder_output_data`, then push angular velocity and force to the LCD, each only when its value changed. An iteration with no new samples keeps the last reading. diff --git a/firmware/Core/Src/Tasks/SessionController/SessionController.cpp b/firmware/Core/Src/Tasks/SessionController/SessionController.cpp index 42b5798..0f9700b 100644 --- a/firmware/Core/Src/Tasks/SessionController/SessionController.cpp +++ b/firmware/Core/Src/Tasks/SessionController/SessionController.cpp @@ -17,8 +17,8 @@ SessionController::SessionController(session_controller_os_task_queues* task_que _optical_encoder_buffer_reader(optical_encoder_circular_buffer, &optical_encoder_circular_buffer_index_writer, OPTICAL_ENCODER_CIRCULAR_BUFFER_SIZE), _fsm(task_queues->display), _task_queues(task_queues), - _prevSDLoggingEnabled(false), _prevPIDEnabled(false), + _prevDesiredAngularVelocity(0.0f), _prevInSession(false), _pidAckReceived(false), _prevBpmDutyCycle(0.0f), @@ -111,18 +111,6 @@ void SessionController::PublishStartupState() #endif } -void SessionController::PublishSdLoggingChange() -{ - const bool sdLoggingEnabled = _fsm.GetSDLoggingEnabledStatus(); - - if (sdLoggingEnabled == _prevSDLoggingEnabled) return; - - #if SD_CONTROLLER_TASK_ENABLE - osMessageQueuePut(_task_queues->sd_controller, &sdLoggingEnabled, 0, osWaitForever); - #endif - _prevSDLoggingEnabled = sdLoggingEnabled; -} - // A session just started or stopped. Sensor sampling is not gated (it is enabled once at // startup and left on), so what changes here is what leaves the board, what the board drives, // and -- critically -- that the BPM stops on the way out. The brake must never be actuated @@ -151,20 +139,48 @@ void SessionController::PublishSessionTransition(bool inSession) bpmSettings.new_duty_cycle_percent = 0.0f; osMessageQueuePut(_task_queues->bpm_controller, &bpmSettings, 0, osWaitForever); + + // The loop stops with the session, and this is the only place that can say so: Run() + // publishes PID instructions below its in-session gate, so once the session is over + // that step never executes again. Left armed, the task went on computing against a + // session that had ended and filling its output queue until every pass logged a + // queue-full warning -- and because _prevPIDEnabled stayed true, the next session + // found no enable edge to publish, so the BPM was never pointed at the PID output and + // the screen read PIDE over a brake the controller no longer reached. + PublishPidInstruction(false); } } -void SessionController::PublishPidEnableChange(bool pidEnabled) +// One instruction carries both halves of what the PID task needs -- whether to run, and what +// to aim at -- so this republishes when either moves. +// +// The setpoint half is new: it used to be sent only on an enable edge, which was enough while +// the only way to change it was the menu, and the menu is unreachable during a session. It is +// a runtime sysconfig parameter now, so the host can move it over USB mid-run, and a task +// still chasing the previous figure would leave the app showing one setpoint while the brake +// worked towards another. +// +// A setpoint change is only worth sending while the loop is enabled: disabled, the task is +// blocked waiting for an instruction and the next enable will carry the current value anyway. +// The task resets its integrator whenever it takes an instruction while enabled, which is the +// behaviour a setpoint change wants in any case. +void SessionController::PublishPidInstruction(bool pidEnabled) { - if (pidEnabled == _prevPIDEnabled) return; + const float desiredAngularVelocity = _fsm.GetDesiredAngularVelocity(); + + const bool enableMoved = (pidEnabled != _prevPIDEnabled); + const bool setpointMoved = pidEnabled && (desiredAngularVelocity != _prevDesiredAngularVelocity); + + if (!enableMoved && !setpointMoved) return; session_controller_to_pid_controller pid_msg; pid_msg.enable_status = pidEnabled; - pid_msg.desired_angular_velocity = _fsm.GetDesiredAngularVelocity(); + pid_msg.desired_angular_velocity = desiredAngularVelocity; _pidAckReceived = false; osMessageQueuePut(_task_queues->pid_controller, &pid_msg, 0, osWaitForever); _prevPIDEnabled = pidEnabled; + _prevDesiredAngularVelocity = desiredAngularVelocity; } // The PID task acknowledges an enable/disable, and only once it has may the BPM be pointed at @@ -295,8 +311,6 @@ void SessionController::Run() _fsm.HandleUserInputs(); DrainHostCommands(); - PublishSdLoggingChange(); - const bool inSession = _fsm.GetInSessionStatus(); if (inSession != _prevInSession) { @@ -315,7 +329,7 @@ void SessionController::Run() const bool pidEnabled = _fsm.GetPIDEnabledModeStatus(); const bool pidOptionEnabled = _fsm.GetPIDOptionToggleableEnabledStatus(); - PublishPidEnableChange(pidEnabled); + PublishPidInstruction(pidEnabled); AwaitPidAck(pidEnabled, pidOptionEnabled); if (!pidOptionEnabled) diff --git a/firmware/tests/ili9341_layout_tests.cpp b/firmware/tests/ili9341_layout_tests.cpp index 906af08..30376c8 100644 --- a/firmware/tests/ili9341_layout_tests.cpp +++ b/firmware/tests/ili9341_layout_tests.cpp @@ -40,9 +40,9 @@ ili9341_frame Layout(const session_controller_to_display &state, } const display_screen_id kAllScreens[] = { - DISPLAY_SCREEN_IDLE, DISPLAY_SCREEN_SD_LOGGING, - DISPLAY_SCREEN_PID_ENABLE, DISPLAY_SCREEN_DESIRED_RPM, - DISPLAY_SCREEN_DESIRED_RPM_EDIT, DISPLAY_SCREEN_SESSION, + DISPLAY_SCREEN_IDLE, DISPLAY_SCREEN_PID_ENABLE, + DISPLAY_SCREEN_DESIRED_RPM, DISPLAY_SCREEN_DESIRED_RPM_EDIT, + DISPLAY_SCREEN_SESSION, }; uint16_t FieldRight(const ili9341_field &field) @@ -130,7 +130,6 @@ TEST(Ili9341Layout, AScreensFieldListIsPositionallyStable) busy.desired_rpm = 98765; busy.bpm_duty_cycle = 0.87f; busy.pid_enabled = true; - busy.sd_logging_enabled = true; busy.cursor_digit = DISPLAY_RPM_DIGIT_ONE; for (display_screen_id screen : kAllScreens) @@ -204,15 +203,15 @@ TEST(Ili9341Layout, ColourChangeAloneCountsAsAChange) // --------------------------------------------------------------------------- content -TEST(Ili9341Layout, TogglePagesUseEqualWidthLabels) +TEST(Ili9341Layout, TogglePageUsesEqualWidthLabels) { // "ENABLED " is padded to eight so it covers "DISABLED" exactly. - session_controller_to_display state = State(DISPLAY_SCREEN_SD_LOGGING); + session_controller_to_display state = State(DISPLAY_SCREEN_PID_ENABLE); - state.sd_logging_enabled = true; + state.pid_option_toggleable = true; const ili9341_frame enabled = Layout(state); - state.sd_logging_enabled = false; + state.pid_option_toggleable = false; const ili9341_frame disabled = Layout(state); EXPECT_EQ(std::string(enabled.fields[1].text), "ENABLED "); diff --git a/firmware/tests/lumex_layout_tests.cpp b/firmware/tests/lumex_layout_tests.cpp index 6aee459..bd7ba27 100644 --- a/firmware/tests/lumex_layout_tests.cpp +++ b/firmware/tests/lumex_layout_tests.cpp @@ -49,9 +49,9 @@ TEST(LumexLayout, EveryScreenFillsEveryCell) // No cell is left uninitialised: the driver diffs whole frames, so a stray NUL would be a // difference it tried to write to the panel. const display_screen_id screens[] = { - DISPLAY_SCREEN_IDLE, DISPLAY_SCREEN_SD_LOGGING, - DISPLAY_SCREEN_PID_ENABLE, DISPLAY_SCREEN_DESIRED_RPM, - DISPLAY_SCREEN_DESIRED_RPM_EDIT, DISPLAY_SCREEN_SESSION, + DISPLAY_SCREEN_IDLE, DISPLAY_SCREEN_PID_ENABLE, + DISPLAY_SCREEN_DESIRED_RPM, DISPLAY_SCREEN_DESIRED_RPM_EDIT, + DISPLAY_SCREEN_SESSION, }; for (display_screen_id screen : screens) @@ -97,19 +97,6 @@ TEST(LumexLayout, IdleScreen) // --------------------------------------------------------------------------- settings pages -TEST(LumexLayout, SdLoggingPageShowsItsOwnFlag) -{ - session_controller_to_display state = State(DISPLAY_SCREEN_SD_LOGGING); - - state.sd_logging_enabled = false; - // 0123456789012345 - EXPECT_EQ(Row(Render(state), 0), " SD LOGGING "); - EXPECT_EQ(Row(Render(state), 1), " DISABLED "); - - state.sd_logging_enabled = true; - EXPECT_EQ(Row(Render(state), 1), " ENABLED "); -} - TEST(LumexLayout, PidEnablePageShowsTheToggleableFlagNotTheLiveOne) { // The page is about whether the option may be armed at all, so it reads @@ -119,7 +106,7 @@ TEST(LumexLayout, PidEnablePageShowsTheToggleableFlagNotTheLiveOne) state.pid_option_toggleable = false; // 0123456789012345 - EXPECT_EQ(Row(Render(state), 0), " PID LOGGING "); + EXPECT_EQ(Row(Render(state), 0), " PID CONTROL "); EXPECT_EQ(Row(Render(state), 1), " DISABLED "); state.pid_option_toggleable = true; diff --git a/firmware/tools/message_gen/schema/messages_private.yaml b/firmware/tools/message_gen/schema/messages_private.yaml index 7b78b45..079f1c1 100644 --- a/firmware/tools/message_gen/schema/messages_private.yaml +++ b/firmware/tools/message_gen/schema/messages_private.yaml @@ -32,7 +32,6 @@ sections: meaningless on the character LCD -- so the seam is here instead, at what the values mean. values: - { name: DISPLAY_SCREEN_IDLE, value: "0", comment: "Attract screen; SELECT opens the settings menu" } - - { name: DISPLAY_SCREEN_SD_LOGGING, comment: "Settings: SD logging on/off" } - { name: DISPLAY_SCREEN_PID_ENABLE, comment: "Settings: whether the PID option may be toggled in-session" } - { name: DISPLAY_SCREEN_DESIRED_RPM, comment: "Settings: the desired-RPM setpoint" } - { name: DISPLAY_SCREEN_DESIRED_RPM_EDIT, comment: "Settings: the same setpoint with the digit cursor showing" } @@ -73,8 +72,7 @@ sections: - { type: uint32_t, name: desired_rpm, comment: "The PID setpoint being displayed or edited" } - { type: display_rpm_digit, name: cursor_digit, comment: "Digit the encoder edits (DESIRED_RPM_EDIT only)" } - { type: bool, name: pid_enabled, comment: "Whether the PID loop is armed for this session" } - - { type: bool, name: pid_option_toggleable, comment: "Whether the menu allows arming it; also selects the in-session drive-mode field" } - - { type: bool, name: sd_logging_enabled, comment: "Whether SD logging is switched on" } + - { type: bool, name: pid_option_toggleable, comment: "SYSCFG_PID_ENABLE: whether the menu allows arming it; also selects the in-session drive-mode field" } # Extended session detail. Carried for every panel but only shown by one: a 2x16 character # grid has no room for it, so LumexLCD ignores these and the ILI9341 draws them. See the # Show* methods on the DisplayDriver concept. diff --git a/firmware/tools/message_gen/schema/messages_public.yaml b/firmware/tools/message_gen/schema/messages_public.yaml index fb449a2..97fca9c 100644 --- a/firmware/tools/message_gen/schema/messages_public.yaml +++ b/firmware/tools/message_gen/schema/messages_public.yaml @@ -661,6 +661,22 @@ sections: # and a macro would only suggest that rebuilding with it changed something. Default 0 -- # off is the only safe way for a board to come up, since anything it streams is invented. - { name: USB_MOCK_MESSAGES, type: enum, default: 0, category: "USB", unit: "", description: "Replace the sensor stream with synthetic counter data (and report a session as running, since the host shows sensor data only during one). For exercising the link with no sensors attached. Leave disabled for any real run: nothing it streams is a measurement.", options: [ {value: 0, label: "Disabled"}, {value: 1, label: "Enabled (fake data)"} ] } + # The two settings the on-board settings menu edits. They are sysconfig parameters rather + # than SessionController members so that the encoder and the host write the *same* value -- + # the menu page and the host's editor are two views of one setting, and neither can show a + # figure the other has quietly overwritten. The FSM keeps no copy: it reads the store when + # it draws the page and writes it when a button moves the value. + # + # PID_ENABLE is the runtime half of a pair; debug.h's PID_CONTROLLER_TASK_ENABLE is the + # compile-time half, and config.h explains why both exist. This one only decides whether + # the SessionController offers the loop -- with the task compiled out, writing it does + # nothing, which is the intended reading of "the machinery is not in this build". + - { name: PID_ENABLE, type: enum, category: "PID Controller", unit: "", description: "Whether the session controller offers the PID loop. Enabled, SELECT arms and disarms it during a session and the brake follows the controller; disabled, the rotary encoder sets the brake duty cycle by hand. Separate from the compile-time PID_CONTROLLER_TASK_ENABLE, which decides whether the task runs at all.", options: [ {value: 0, label: "Disabled (manual brake)"}, {value: 1, label: "Enabled (PID brake)"} ] } + # A shaft speed in RPM is a uint16_t, so 0..65535 -- the width of the type, like every + # other integer here, not a judgement about how fast the rig can spin. It comfortably + # clears the on-board editor's five digits as well. The firmware converts to rad/s on the + # way to the PID task; RPM is the unit here because it is the one the panel and host show. + - { name: PID_DESIRED_RPM, type: uint32, min: 0, max: 65535, category: "PID Controller", unit: "RPM", description: "Shaft-speed setpoint the PID loop drives the brake towards. Editable on the board from the PID DES RPM menu page, or from here." } # GEAR_RATIO is deliberately NOT here: it is a compile-time config.h setting (editable via # the app's compile-time overrides, applied on the next build/flash), and it is not streamed # either -- the host reads it from its own copy of the firmware config for the geared readouts. diff --git a/src/Dyno.Core/Messages/Generated/Messages.cs b/src/Dyno.Core/Messages/Generated/Messages.cs index 618f405..31748d8 100644 --- a/src/Dyno.Core/Messages/Generated/Messages.cs +++ b/src/Dyno.Core/Messages/Generated/Messages.cs @@ -23,7 +23,7 @@ public static class MessageConstants public const uint USB_FRAME_CRC_POLY = 0x1021u; // 0x1021u public const uint USB_RX_MAX_PAYLOAD = 128u; // 128u public const uint USB_PROTOCOL_VERSION = 8u; // 8u - public const uint SYSCFG_PARAM_COUNT = 34u; // 34u -- one past the highest sysconfig_param_t id; sizes the firmware store + public const uint SYSCFG_PARAM_COUNT = 36u; // 36u -- one past the highest sysconfig_param_t id; sizes the firmware store } // A task error/warning is reported as a single 32-bit code: @@ -376,6 +376,8 @@ public enum sysconfig_param_t : ushort 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 } // Body of USB_CMD_SET_SYSCONFIG (after the usb_cmd_header_t). raw_value carries the diff --git a/src/Dyno.Core/SysConfig/Generated/SysConfigCatalog.cs b/src/Dyno.Core/SysConfig/Generated/SysConfigCatalog.cs index 80ad724..4c4c4a8 100644 --- a/src/Dyno.Core/SysConfig/Generated/SysConfigCatalog.cs +++ b/src/Dyno.Core/SysConfig/Generated/SysConfigCatalog.cs @@ -407,6 +407,29 @@ public static class SysConfigCatalog Max: 1.0, Options: new SysConfigEnumOption[] { new(0u, "Disabled"), new(1u, "Enabled (fake data)") } ), + new( + sysconfig_param_t.SYSCFG_PID_ENABLE, + "PID_ENABLE", + "PID Controller", + "", + "Whether the session controller offers the PID loop. Enabled, SELECT arms and disarms it during a session and the brake follows the controller; disabled, the rotary encoder sets the brake duty cycle by hand. Separate from the compile-time PID_CONTROLLER_TASK_ENABLE, which decides whether the task runs at all.", + IsFloat: false, + Default: 0.0, + Min: 0.0, + Max: 1.0, + Options: new SysConfigEnumOption[] { new(0u, "Disabled (manual brake)"), new(1u, "Enabled (PID brake)") } + ), + new( + sysconfig_param_t.SYSCFG_PID_DESIRED_RPM, + "PID_DESIRED_RPM", + "PID Controller", + "RPM", + "Shaft-speed setpoint the PID loop drives the brake towards. Editable on the board from the PID DES RPM menu page, or from here.", + IsFloat: false, + Default: 5000.0, + Min: 0.0, + Max: 65535.0 + ), }; /// Looks up a parameter's definition by wire id. From d86610f26e56d640869b0cbd1ab897e9db4d0968 Mon Sep 17 00:00:00 2001 From: Tomaz Zlindra Date: Wed, 29 Jul 2026 19:49:54 -0700 Subject: [PATCH 2/3] SessionController: redraw the menu when the host edits a PID setting Writing SYSCFG_PID_ENABLE or SYSCFG_PID_DESIRED_RPM from the app left the LCD showing the old value while sitting on the settings menu. The store is plain RAM and USB_CMD_SET_SYSCONFIG is applied by the USB task itself, deliberately with no queue and no task wake-up -- so nothing told the FSM the value had moved. Every other route to the panel is an FSM event that reposts on its way through, which is why an encoder tick always redraws; a host write has no such event. Making these two settings host-writable in the previous commit is what opened the gap, since until then the menu was the only writer. RefreshHostEditedSettings polls for it instead: PostDisplayState now records what it sent for the two, and the SessionController calls the check once per pass, above the in-session gate because the settings pages only exist outside a session. It compares two words and posts nothing when they match, so a steady state still produces no queue traffic. Verified: ARM Debug build clean, 105/105 firmware tests, 266/266 Dyno.Core tests, schemas in sync. Co-Authored-By: Claude Opus 5 --- .../SessionController/FiniteStateMachine.hpp | 18 +++++++++++++ .../SessionController/FiniteStateMachine.cpp | 26 +++++++++++++++++++ .../SessionController/SessionController.cpp | 5 ++++ 3 files changed, 49 insertions(+) diff --git a/firmware/Core/Inc/Tasks/SessionController/FiniteStateMachine.hpp b/firmware/Core/Inc/Tasks/SessionController/FiniteStateMachine.hpp index 744e4c0..6ca7bdf 100644 --- a/firmware/Core/Inc/Tasks/SessionController/FiniteStateMachine.hpp +++ b/firmware/Core/Inc/Tasks/SessionController/FiniteStateMachine.hpp @@ -97,6 +97,17 @@ 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 (it is plain RAM -- no queue, no wake-up), so + // nothing tells this class the value moved. + // + // Every other path to the panel is an event this FSM handles, and each of those reposts on + // its way through, which is why a menu page drawn by the encoder is always current. A host + // write has no such event, so the SessionController calls this once per pass and it polls + // instead. Cheap: it compares two words and posts nothing when they match. + void RefreshHostEditedSettings(); + // What the SessionController acts on State GetState() const; bool GetPIDEnabledModeStatus() const; @@ -155,6 +166,13 @@ class FSM // 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 RefreshHostEditedSettings 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. diff --git a/firmware/Core/Src/Tasks/SessionController/FiniteStateMachine.cpp b/firmware/Core/Src/Tasks/SessionController/FiniteStateMachine.cpp index ccb45dd..3982194 100644 --- a/firmware/Core/Src/Tasks/SessionController/FiniteStateMachine.cpp +++ b/firmware/Core/Src/Tasks/SessionController/FiniteStateMachine.cpp @@ -11,6 +11,10 @@ FSM::FSM(osMessageQueueId_t sessionControllerToDisplayHandle) : State::SettingsState::INIT_STATE, State::DesiredRpmUnitsState::INIT_STATE, }, + // Overwritten by the ShowIdleScreen() below before anything can read them; initialised + // anyway so the "what is on the panel" pair is never garbage. + _postedPidOptionEnabled(false), + _postedDesiredRpm(0), _pidEnabled(false), _desiredManualBpmDutyCycle(0), _rpm(0.0f), @@ -256,6 +260,22 @@ void FSM::AdjustBrakeDutyCycle(bool positiveTick) std::clamp(_desiredManualBpmDutyCycle + increment, minDutyCycle, maxDutyCycle); } +// Polled by the SessionController, because a host sysconfig write arrives with no event +// attached -- see the declaration. Posts the whole screen state, exactly as an encoder tick +// would: which page is showing and what belongs on it are already worked out by +// PostDisplayState/CurrentScreen, and the display driver diffs frames, so a repost that +// happens to change nothing visible costs one queue message and no panel traffic. +void FSM::RefreshHostEditedSettings() +{ + if (_postedPidOptionEnabled == GetPIDOptionToggleableEnabledStatus() + && _postedDesiredRpm == GetDesiredRpm()) + { + return; + } + + PostDisplayState(); +} + // The host's equivalent of turning the brake knob. Deliberately routed through the same state // the encoder writes, so everything downstream -- the clamp, the BPM post, the on-screen // readout -- behaves identically whether the request came from the rig or from the PC. @@ -470,6 +490,12 @@ void FSM::PostDisplayState() msg.cursor_digit = static_cast(_state.desiredRpmUnitsState); msg.pid_enabled = _pidEnabled; msg.pid_option_toggleable = GetPIDOptionToggleableEnabledStatus(); + + // Remember what went out for the two the host can also write, so a change made behind this + // FSM's back can be spotted -- see RefreshHostEditedSettings. + _postedPidOptionEnabled = msg.pid_option_toggleable; + _postedDesiredRpm = msg.desired_rpm; + msg.angular_acceleration = _angularAcceleration; msg.peak_force = _peakForce; diff --git a/firmware/Core/Src/Tasks/SessionController/SessionController.cpp b/firmware/Core/Src/Tasks/SessionController/SessionController.cpp index 0f9700b..5ca1483 100644 --- a/firmware/Core/Src/Tasks/SessionController/SessionController.cpp +++ b/firmware/Core/Src/Tasks/SessionController/SessionController.cpp @@ -311,6 +311,11 @@ void SessionController::Run() _fsm.HandleUserInputs(); DrainHostCommands(); + // Above the in-session gate on purpose: the settings pages are only reachable outside a + // session, and they are the screens a host sysconfig write can leave stale. In a session + // the steps below repost every pass anyway, so this finds nothing to do. + _fsm.RefreshHostEditedSettings(); + const bool inSession = _fsm.GetInSessionStatus(); if (inSession != _prevInSession) { From c03ea5922eb1b0f02c68ee10c065ea81cab7cacc Mon Sep 17 00:00:00 2001 From: Tomaz Zlindra Date: Wed, 29 Jul 2026 19:56:25 -0700 Subject: [PATCH 3/3] SessionController: retry a dropped display post, per the reconcile pattern Follows the force sensor's ReconcileConfig discipline properly. Its ApplyIfChanged leaves _applied stale when an I2C write fails, so the next reconcile retries that register; the display reconcile was missing the equivalent. PostDisplayState posts with timeout 0 -- a full queue drops the frame rather than stalling the SessionController -- but it recorded the "what is on the panel" pair regardless of whether the put took. A dropped frame therefore marked the value as shown when it had never reached the panel, and since the reconcile is a diff against that pair, it would never look at it again: the stale reading stuck until some unrelated event reposted. Now the pair is only updated once osMessageQueuePut returns osOK. Renamed RefreshHostEditedSettings to ReconcileHostEditedSettings to match the name the rest of the board uses for this. No bounded-wait wakeup is needed on this side, unlike the force sensor's FORCESENSOR_COMMAND_POLL_OSDELAY: that exists because its task would otherwise sit in osWaitForever on the enable queue while idle, whereas SessionController::Run ends every iteration on osDelay and so is already awake to reconcile 100 times a second. Verified: ARM Debug build clean, 105/105 firmware tests. Co-Authored-By: Claude Opus 5 --- .../SessionController/FiniteStateMachine.hpp | 24 ++++++++++++------- .../SessionController/FiniteStateMachine.cpp | 22 ++++++++++------- .../SessionController/SessionController.cpp | 2 +- 3 files changed, 30 insertions(+), 18 deletions(-) diff --git a/firmware/Core/Inc/Tasks/SessionController/FiniteStateMachine.hpp b/firmware/Core/Inc/Tasks/SessionController/FiniteStateMachine.hpp index 6ca7bdf..229f3a3 100644 --- a/firmware/Core/Inc/Tasks/SessionController/FiniteStateMachine.hpp +++ b/firmware/Core/Inc/Tasks/SessionController/FiniteStateMachine.hpp @@ -98,15 +98,21 @@ class FSM 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 (it is plain RAM -- no queue, no wake-up), so - // nothing tells this class the value moved. + // 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. // - // Every other path to the panel is an event this FSM handles, and each of those reposts on - // its way through, which is why a menu page drawn by the encoder is always current. A host - // write has no such event, so the SessionController calls this once per pass and it polls - // instead. Cheap: it compares two words and posts nothing when they match. - void RefreshHostEditedSettings(); + // 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; @@ -168,7 +174,7 @@ class FSM // 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 RefreshHostEditedSettings can tell + // 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; diff --git a/firmware/Core/Src/Tasks/SessionController/FiniteStateMachine.cpp b/firmware/Core/Src/Tasks/SessionController/FiniteStateMachine.cpp index 3982194..e5f30b3 100644 --- a/firmware/Core/Src/Tasks/SessionController/FiniteStateMachine.cpp +++ b/firmware/Core/Src/Tasks/SessionController/FiniteStateMachine.cpp @@ -265,7 +265,7 @@ void FSM::AdjustBrakeDutyCycle(bool positiveTick) // would: which page is showing and what belongs on it are already worked out by // PostDisplayState/CurrentScreen, and the display driver diffs frames, so a repost that // happens to change nothing visible costs one queue message and no panel traffic. -void FSM::RefreshHostEditedSettings() +void FSM::ReconcileHostEditedSettings() { if (_postedPidOptionEnabled == GetPIDOptionToggleableEnabledStatus() && _postedDesiredRpm == GetDesiredRpm()) @@ -490,12 +490,6 @@ void FSM::PostDisplayState() msg.cursor_digit = static_cast(_state.desiredRpmUnitsState); msg.pid_enabled = _pidEnabled; msg.pid_option_toggleable = GetPIDOptionToggleableEnabledStatus(); - - // Remember what went out for the two the host can also write, so a change made behind this - // FSM's back can be spotted -- see RefreshHostEditedSettings. - _postedPidOptionEnabled = msg.pid_option_toggleable; - _postedDesiredRpm = msg.desired_rpm; - msg.angular_acceleration = _angularAcceleration; msg.peak_force = _peakForce; @@ -506,7 +500,19 @@ void FSM::PostDisplayState() ? (get_timestamp() - _sessionStartTimestamp) / scale : 0; - osMessageQueuePut(_toDisplayHandle, &msg, 0, 0); + // Timeout 0: a full queue drops this frame rather than stalling the SessionController. Which + // is why the "what is on the panel" pair below is only updated when the put actually took -- + // a dropped frame never reached the panel, so recording it as shown would tell + // ReconcileHostEditedSettings there is nothing to redraw and the stale value would stick until + // the next unrelated event. Left stale, the next pass retries. Same discipline as the force + // sensor's ApplyIfChanged, which leaves _applied stale when an I2C write fails. + if (osMessageQueuePut(_toDisplayHandle, &msg, 0, 0) != osOK) + { + return; + } + + _postedPidOptionEnabled = msg.pid_option_toggleable; + _postedDesiredRpm = msg.desired_rpm; } diff --git a/firmware/Core/Src/Tasks/SessionController/SessionController.cpp b/firmware/Core/Src/Tasks/SessionController/SessionController.cpp index 5ca1483..ccd3bbe 100644 --- a/firmware/Core/Src/Tasks/SessionController/SessionController.cpp +++ b/firmware/Core/Src/Tasks/SessionController/SessionController.cpp @@ -314,7 +314,7 @@ void SessionController::Run() // Above the in-session gate on purpose: the settings pages are only reachable outside a // session, and they are the screens a host sysconfig write can leave stale. In a session // the steps below repost every pass anyway, so this finds nothing to do. - _fsm.RefreshHostEditedSettings(); + _fsm.ReconcileHostEditedSettings(); const bool inSession = _fsm.GetInSessionStatus(); if (inSession != _prevInSession)