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..229f3a3 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 @@ -99,9 +97,25 @@ 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; @@ -109,13 +123,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 +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. 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..e5f30b3 100644 --- a/firmware/Core/Src/Tasks/SessionController/FiniteStateMachine.cpp +++ b/firmware/Core/Src/Tasks/SessionController/FiniteStateMachine.cpp @@ -11,9 +11,10 @@ FSM::FSM(osMessageQueueId_t sessionControllerToDisplayHandle) : State::SettingsState::INIT_STATE, State::DesiredRpmUnitsState::INIT_STATE, }, - _sdLoggingEnabled(false), - _pidOptionToggleableEnabled(false), - _desiredRpm(5000), + // 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), @@ -87,20 +88,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 +104,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 +134,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 +145,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 +157,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 +168,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 +178,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 +194,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; @@ -273,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::ReconcileHostEditedSettings() +{ + 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. @@ -311,8 +314,18 @@ int FSM::DesiredRpmDigitIncrement() const void FSM::AdjustDesiredRpm(bool positiveTick) { const int increment = DesiredRpmDigitIncrement(); + const int candidate = static_cast(GetDesiredRpm()) + + (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; - _desiredRpm = std::max(0, _desiredRpm + (positiveTick ? increment : -increment)); + 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 +352,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 +397,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 +462,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 +486,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; @@ -492,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; } @@ -503,20 +523,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 +546,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..ccd3bbe 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,7 +311,10 @@ void SessionController::Run() _fsm.HandleUserInputs(); DrainHostCommands(); - PublishSdLoggingChange(); + // 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.ReconcileHostEditedSettings(); const bool inSession = _fsm.GetInSessionStatus(); if (inSession != _prevInSession) @@ -315,7 +334,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.