ILI9431 Support

firmware/CMakeLists.txt

@@ -12,6 +12,12 @@ set(CMAKE_C_STANDARD 11)
 set(CMAKE_C_STANDARD_REQUIRED ON)
 set(CMAKE_C_EXTENSIONS ON)
 
+# Pinned rather than left to the compiler default, which was gnu++17 and would drift with
+# the toolchain. C++20 is needed for the concept that both display drivers are checked
+# against; firmware/tests/CMakeLists.txt is already on 20.
+set(CMAKE_CXX_STANDARD 20)
+set(CMAKE_CXX_STANDARD_REQUIRED ON)
+
 
 # Define the build type
 if(NOT CMAKE_BUILD_TYPE)
@@ -59,15 +65,22 @@ file(GLOB_RECURSE APP_SOURCES CONFIGURE_DEPENDS
 target_sources(${CMAKE_PROJECT_NAME} PRIVATE
     ${CMAKE_SOURCE_DIR}/Core/Src/interrupts.c
     ${CMAKE_SOURCE_DIR}/Drivers/ADS1115/ADS1115.cpp
+    ${CMAKE_SOURCE_DIR}/Drivers/Lumex/LumexPanel.cpp
+    ${CMAKE_SOURCE_DIR}/Drivers/ILI9341/ILI9341.cpp
+    ${CMAKE_SOURCE_DIR}/Drivers/ILI9341/ILI9341_font.c
     ${APP_SOURCES}
 )
 
 # Add include paths
 target_include_directories(${CMAKE_PROJECT_NAME} PRIVATE
-    # Application headers and the on-board ADS1115 force-sensor driver headers.
+    # Application headers, plus the on-board device drivers: the ADS1115 force sensor and the
+    # ILI9341 display. Both display drivers are always compiled; Config/debug.h picks which one
+    # the display task actually runs, and --gc-sections drops the other.
     ${CMAKE_SOURCE_DIR}/Core/Inc
     ${CMAKE_SOURCE_DIR}/Middlewares/CircularBuffer/Inc
     ${CMAKE_SOURCE_DIR}/Drivers/ADS1115
+    ${CMAKE_SOURCE_DIR}/Drivers/ILI9341
+    ${CMAKE_SOURCE_DIR}/Drivers/Lumex
 )
 
 # Add project symbols (macros)

firmware/Core/Inc/Config/config.h

@@ -2,6 +2,7 @@
 #define INC_CONFIG_CONFIG_H_
 
 #include "ADS1115_main.h"
+#include "ILI9341_main.h"
 
 // Tunable quantities below (gains, task delays, thresholds) are *boot defaults*: they
 // seed the runtime sysconfig store (Config/sysconfig.h), which the host can rewrite live
@@ -31,6 +32,22 @@
 // User Input Config (like buttons)
 #define USER_INPUT_CIRCULAR_BUFFER_SIZE 100u
 
+// Rotary encoder input conditioning.
+//
+// The encoder is decoded the cheap way: an EXTI on ROT_EN_A, reading ROT_EN_B's level to get
+// the direction. That has no filtering of any kind, so a bounced contact gives extra ticks and
+// a disturbed read of B gives a tick in the wrong direction -- and a wrong direction is worse
+// than a missed tick, because the brake setpoint then random-walks instead of merely lagging.
+//
+// Two guards, both cheap enough for interrupt context:
+//   DEBOUNCE_US  -- ignore an A edge that lands within this long of the last accepted one.
+//                   A hand-turned encoder produces edges milliseconds apart; contact bounce
+//                   and coupled noise are microseconds. 0 disables.
+//   DIRECTION_SAMPLES -- read B this many times and take the majority, so a single disturbed
+//                   sample cannot decide which way the knob went. Must be odd.
+#define ROTARY_ENCODER_DEBOUNCE_US 1000u
+#define ROTARY_ENCODER_DIRECTION_SAMPLES 3u
+
 // Session Controller Config
 // 10ms = 100 Hz torque/power. Task delays below are tuned as a set: at the old rates the four
 // streams totalled ~38 kB/s, which saturated the USB TX path once a session started (rising
@@ -103,9 +120,26 @@
 // the old 5 gave up (and dropped the batch) during ordinary congestion, not just dead hosts.
 #define USB_TX_FLUSH_MAX_RETRIES 20
 
-// LCD config
+// ===== Display =====
+// Applies whichever panel is fitted. Which one that is, is a task enable in debug.h.
 #define LCD_TASK_OSDELAY 20
-#define SESSION_CONTROLLER_TO_LUMEX_LCD_MSG_STRING_SIZE 16 + 1
+
+// ===== Display: Lumex 16x2 =====
+// The Lumex panel's character grid. The display message no longer carries strings -- it
+// carries screen state, and the Lumex driver lays that out into a grid this size -- so these
+// describe the panel itself rather than a queue payload, which is what the old
+// SESSION_CONTROLLER_TO_LUMEX_LCD_MSG_STRING_SIZE was really doing.
+#define LUMEX_LCD_ROWS 2
+#define LUMEX_LCD_COLUMNS 16
+
+// ===== Display: ILI9341 320x240 TFT =====
+// Which way up the panel is fitted. Both LANDSCAPE and LANDSCAPE_FLIP are 320x240, so this
+// changes nothing but the origin corner -- the layout is unaffected either way.
+//
+// FLIP because the panel is mounted 180 degrees from the controller's default landscape:
+// LANDSCAPE rendered the screens upside down on the rig. This is a property of the enclosure,
+// not of the driver, so it lives here rather than in ILI9341Display::Init().
+#define ILI9341_DISPLAY_ROTATION ILI9341_ROTATION_LANDSCAPE_FLIP
 
 // LED config
 #define LED_TASK_OSDELAY 500

firmware/Core/Inc/Config/debug.h

@@ -8,7 +8,6 @@
 // ===== TIMERS =====
 #define STM32_PERIPHERAL_TIM1_ENABLE      1
 #define STM32_PERIPHERAL_TIM2_ENABLE      1
-#define STM32_PERIPHERAL_TIM13_ENABLE     1
 #define STM32_PERIPHERAL_TIM14_ENABLE     1
 #define STM32_PERIPHERAL_TIM16_ENABLE     1
 
@@ -48,8 +47,28 @@
 // BPM Controller Task
 #define BPM_CONTROLLER_TASK_ENABLE 1
 
-// Lumex LCD Task
-#define LUMEX_LCD_TASK_ENABLE 1
+// ===== Display =====
+// At most one panel, chosen here and flashed. Both consume the same
+// session_controller_to_display message, so the SessionController and its FSM are identical
+// either way -- only the driver linked in changes. There is no runtime switch because there is
+// no runtime question: a board has one panel soldered to it.
+//
+// Neither enabled is legal, and useful: the display task parks and nothing drives SPI1, which is
+// how the panel gets ruled in or out of a fault elsewhere on the board.
+//
+// Anything outside the two drivers that needs to know a display task exists -- the null checks on
+// the display queue and thread id, the task monitor's stack-usage report -- must test BOTH of
+// these, never one. Gating on a single panel's enable silently compiles the feature out when the
+// other panel is selected: that is how the display task's stack high-water mark fell off the USB
+// stream when this board moved to the ILI9341. There used to be a DISPLAY_TASK_ENABLE macro
+// spelling that disjunction once, but a derived value has no business in a file the desktop app
+// offers as a list of switches to override, so the disjunction is written out where it is used.
+#define LUMEX_LCD_TASK_ENABLE   0   // Lumex 16x2 character LCD, bit-banged over GPIO
+#define ILI9341_LCD_TASK_ENABLE 1   // ILI9341 320x240 SPI TFT
+
+#if (LUMEX_LCD_TASK_ENABLE + ILI9341_LCD_TASK_ENABLE) > 1
+#error "At most one display driver may be enabled: set at most one of LUMEX_LCD_TASK_ENABLE / ILI9341_LCD_TASK_ENABLE to 1."
+#endif
 
 // USB Controller task settings
 // The mock-message stream used to live here as DEBUG_USB_CONTROLLER_MOCK_MESSAGES. It is now the
@@ -59,8 +78,23 @@
 #define USB_CONTROLLER_TASK_ENABLE 1
 
 // Led Blink Task
+//
+// It has no LED of its own: it blinks by toggling ILI_SPI2_SD_CS (PH7), which is the microSD
+// slot's chip select on the ILI9341 module. That was picked as a convenient scope point back
+// when nothing else used the pin. It is not a free pin once that module is fitted, so the
+// check below refuses the combination rather than leaving someone to find it with a scope.
 #define LED_BLINK_TASK_ENABLE 0
 
+// Guarded on definedness too: an undefined macro is 0 to the preprocessor, so moving either
+// #define below this point would silently switch the check off rather than break the build.
+#if !defined(ILI9341_LCD_TASK_ENABLE)
+#error "ILI9341_LCD_TASK_ENABLE must be defined above LED_BLINK_TASK_ENABLE -- the pin-conflict check below reads it."
+#endif
+
+#if LED_BLINK_TASK_ENABLE && ILI9341_LCD_TASK_ENABLE
+#error "LED_BLINK_TASK_ENABLE toggles ILI_SPI2_SD_CS (PH7), a chip select on the ILI9341 module. Disable one of LED_BLINK_TASK_ENABLE / ILI9341_LCD_TASK_ENABLE, or point the blink task at a pin of its own."
+#endif
+
 // Task Monitoring Task
 #define TASK_MONITOR_TASK_ENABLE 1
 

firmware/Core/Inc/MessagePassing/messages_private.h

@@ -27,25 +27,59 @@
 extern "C" {
 #endif
 
-// Opcodes for controlling the Lumex LCD display from the session controller
+// Which screen the session controller's FSM is showing. The message below carries screen
+// state, not draw commands: the FSM says what it is displaying and each display driver
+// renders that however its panel allows. A 16x2 character LCD and a 320x240 TFT have no
+// useful common drawing API -- the intersection caps the TFT at 16x2, the union is
+// meaningless on the character LCD -- so the seam is here instead, at what the values mean.
 typedef enum : uint32_t
 {
-    CLEAR_DISPLAY = 0,   // Clear the entire display
-    WRITE_TO_DISPLAY = 1   // Write a string to a specific location on the display
-} session_controller_to_lumex_lcd_opcode;
-
-DYNO_STATIC_ASSERT(sizeof(session_controller_to_lumex_lcd_opcode) == 4, "Size of session_controller_to_lumex_lcd_opcode must be 4 bytes");
-
-// Message sent from the session controller to the Lumex LCD
+    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
+    DISPLAY_SCREEN_SESSION   // Live readout while a session runs
+} display_screen_id;
+
+DYNO_STATIC_ASSERT(sizeof(display_screen_id) == 4, "Size of display_screen_id must be 4 bytes");
+
+// Which decimal digit of the desired RPM the encoder is editing. Mirrors
+// State::DesiredRpmUnitsState in FiniteStateMachine.hpp. Sent as the cursor position
+// rather than as the step size it implies, so a driver can mark the digit itself
+// instead of only printing the increment.
+typedef enum : uint32_t
+{
+    DISPLAY_RPM_DIGIT_TEN_THOUSAND = 0,
+    DISPLAY_RPM_DIGIT_THOUSAND,
+    DISPLAY_RPM_DIGIT_HUNDRED,
+    DISPLAY_RPM_DIGIT_TEN,
+    DISPLAY_RPM_DIGIT_ONE
+} display_rpm_digit;
+
+DYNO_STATIC_ASSERT(sizeof(display_rpm_digit) == 4, "Size of display_rpm_digit must be 4 bytes");
+
+// Everything any screen shows, sent whole on every update. Drivers diff it against the
+// last one they rendered and repaint only what moved -- which is what makes a 320x240
+// panel viable at all, since a full frame over SPI costs ~50-100 ms but a single field
+// costs ~1-2 ms. The older protocol sent ("  1234", row 0, col 3) and left the driver
+// unable to tell which quantity had changed.
 typedef struct {
-    session_controller_to_lumex_lcd_opcode op;   // Operation to perform on the display
-    uint32_t row;   // Row number on the LCD
-    uint32_t column;   // Column number on the LCD
-    size_t size;
-    char display_string[SESSION_CONTROLLER_TO_LUMEX_LCD_MSG_STRING_SIZE];   // String to write (if WRITE_TO_DISPLAY)
-} session_controller_to_lumex_lcd;
-
-DYNO_STATIC_ASSERT(sizeof(session_controller_to_lumex_lcd) >= offsetof(session_controller_to_lumex_lcd, display_string) + sizeof(((session_controller_to_lumex_lcd *)0)->display_string), "Size of session_controller_to_lumex_lcd must be correct");
+    display_screen_id screen;   // Which screen to render
+    float rpm;   // Measured shaft speed in RPM, already converted from the encoder's rad/s
+    float force;   // Measured force in N
+    float bpm_duty_cycle;   // Commanded brake duty cycle, 0 - 1
+    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
+    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)
+} session_controller_to_display;
+
+DYNO_STATIC_ASSERT(sizeof(session_controller_to_display) <= 48, "session_controller_to_display is queued 25 deep -- keep it small");
 
 // Opcodes for controlling the BPM (Pulse Width Modulation) module from the session controller
 typedef enum : uint32_t

firmware/Core/Inc/MessagePassing/messages_public.h

@@ -56,7 +56,7 @@ typedef enum : uint32_t
     TASK_OFFSET_FORCE_SENSOR_ADS1115 = 6u  << TASK_OFFSET_SHIFT,
     TASK_OFFSET_BPM_CONTROLLER = 7u  << TASK_OFFSET_SHIFT,
     TASK_OFFSET_PID_CONTROLLER = 8u  << TASK_OFFSET_SHIFT,
-    TASK_OFFSET_LUMEX_LCD = 9u  << TASK_OFFSET_SHIFT
+    TASK_OFFSET_DISPLAY = 9u  << TASK_OFFSET_SHIFT
 } task_offset_t;
 
 typedef struct __attribute__((packed)) {
@@ -94,10 +94,12 @@ DYNO_STATIC_ASSERT(sizeof(bpm_task_error_ids) == 4, "Size of bpm_task_error_ids
 
 typedef enum : uint32_t
 {
-    ERROR_LUMEX_LCD_TIMER_START_FAILURE = 0
-} lumex_lcd_task_error_ids;
+    ERROR_LUMEX_LCD_TIMER_START_FAILURE = 0,
+    ERROR_DISPLAY_INIT_FAILURE,
+    ERROR_DISPLAY_SPI_TRANSMIT_FAILURE
+} display_task_error_ids;
 
-DYNO_STATIC_ASSERT(sizeof(lumex_lcd_task_error_ids) == 4, "Size of lumex_lcd_task_error_ids must be 4 bytes");
+DYNO_STATIC_ASSERT(sizeof(display_task_error_ids) == 4, "Size of display_task_error_ids must be 4 bytes");
 
 typedef enum : uint32_t
 {
@@ -248,7 +250,7 @@ DYNO_STATIC_ASSERT(sizeof(usb_msg_header_t) == 12, "Size of usb_msg_header_t mus
 // v6 host would decode the trailer as an unknown STATUS record and log it a few hundred
 // times a second.
 
-#define USB_PROTOCOL_VERSION 7u
+#define USB_PROTOCOL_VERSION 8u
 
 // Shared CRC so firmware and host compute identical checksums over a frame body.
 
@@ -314,6 +316,20 @@ typedef enum : uint16_t
     USB_CMD_SET_SYSCONFIG = 1   // body = sysconfig_set_param_body; writes one runtime parameter into the sysconfig store. Applied by the USB task itself (the store is plain RAM), so the OK is still a full-path ack
 } usb_controller_command_t;
 
+// Session-controller-local commands: frames addressed to TASK_OFFSET_SESSION_CONTROLLER.
+typedef enum : uint16_t
+{
+    SESSION_CMD_SET_BRAKE_DUTY_CYCLE = 0   // body = session_set_brake_duty_body; sets the commanded brake duty cycle, exactly as a rotary-encoder tick on the rig would. Honoured only while a session is running and clamped to the MIN/MAX_DUTY_CYCLE_PERCENT envelope -- the brake is never actuated outside a session, however the request arrives. Answered USB_RSP_OK when applied and USB_RSP_NOT_SUPPORTED when no session is running
+} session_controller_command_t;
+
+// Body of SESSION_CMD_SET_BRAKE_DUTY_CYCLE. Fraction, not percent: 0.0 - 1.0, matching
+// MIN/MAX_DUTY_CYCLE_PERCENT and what the BPM task takes.
+typedef struct __attribute__((packed)) {
+    float duty_cycle;
+} session_set_brake_duty_body;
+
+DYNO_STATIC_ASSERT(sizeof(session_set_brake_duty_body) == 4, "Size of session_set_brake_duty_body must be 4 bytes");
+
 // Device-ready announcement (STM32 -> PC): emitted as USB_MSG_EVENT with task_offset
 // TASK_OFFSET_USB_CONTROLLER and repeated (~every 200ms) until the host answers with
 // USB_CMD_ACK. Carries the firmware's USB_PROTOCOL_VERSION so the host can confirm the

firmware/Core/Inc/Tasks/Display/DisplayDriver.hpp

@@ -0,0 +1,94 @@
+#ifndef INC_TASKS_DISPLAY_DISPLAYDRIVER_HPP_
+#define INC_TASKS_DISPLAY_DISPLAYDRIVER_HPP_
+
+// What every display driver has to be, and the task loop they share.
+//
+// A concept rather than a base class. Virtual dispatch would cost a vtable pointer per object
+// and an indirect call per draw for a choice that is fixed at link time -- exactly one driver
+// is compiled in -- so this checks the same contract at compile time and inlines through it.
+//
+// It also deliberately exposes no drawing primitives. A 16x2 character LCD and a 320x240 TFT
+// have wildly different capabilities, and any common *drawing* API would either cap the TFT or
+// be meaningless on the LCD. Render() takes the whole screen state and each driver does
+// whatever its panel can with it, so a richer panel needs nothing added here.
+
+#include <concepts>
+#include <cstring>
+
+#include "cmsis_os2.h"
+
+#include "Config/sysconfig.h"
+#include "MessagePassing/messages_private.h"
+
+template <typename T>
+concept DisplayDriver = requires(T driver, const session_controller_to_display& state)
+{
+    // Brings the panel up. False means the task suspends rather than spinning on dead hardware.
+    { driver.Init() } -> std::same_as<bool>;
+
+    // Blanks the panel and forgets what was on it, so the next Render repaints in full.
+    { driver.Clear() } -> std::same_as<bool>;
+
+    // Paints one screen state. Called on every message; drivers are expected to diff against
+    // what they last drew and repaint only what moved.
+    { driver.Render(state) } -> std::same_as<bool>;
+
+    // --- Extended session detail.
+    //
+    // Everything above is the common ground: values every panel can show. These are not.
+    // They are extra readouts for the in-session screen that need room a 2x16 character grid
+    // does not have, so LumexLCD implements them as one-line no-ops that discard the argument
+    // and the ILI9341 draws them.
+    //
+    // They sit here rather than only on ILI9341Display so the display task can call them
+    // without knowing which panel it has, and so a driver that quietly stopped implementing
+    // one is a compile error. The asymmetry is deliberate and is the cost of letting the TFT
+    // grow without dragging the character panel along: adding a fourth readout means one real
+    // implementation and one `(void)` line.
+    //
+    // Called before Render(), so a driver may simply record them and lay them out there.
+    { driver.ShowAngularAcceleration(float{}) } -> std::same_as<bool>;
+    { driver.ShowPeakForce(float{}) } -> std::same_as<bool>;
+    { driver.ShowSessionElapsed(uint32_t{}) } -> std::same_as<bool>;
+};
+
+// The queue-drain loop, identical for every panel.
+//
+// Drains to the newest message before drawing: each one is the whole of what should be on
+// screen, so the ones behind it are already stale and rendering them in turn would only paint
+// values the user is never going to see. That matters more the slower the panel is.
+//
+// Never returns, and that is load-bearing rather than stylistic. A FreeRTOS task function that
+// returns lands in prvTaskExitError(), which fails a configASSERT, calls
+// portDISABLE_INTERRUPTS() and spins -- so the whole rig dies, buttons and brake included, with
+// no LED and no fault report. A display is the least important thing on this board and must not
+// be able to do that: a failed write is reported through the error buffer and the loop carries
+// on. The driver repaints in full on its next pass, so a half-drawn screen corrects itself.
+// [[noreturn]] is the guard: adding a `return` here is what caused the fault above, and the
+// compiler now says so rather than leaving it to be found on the bench.
+template <DisplayDriver Display>
+[[noreturn]] void RunDisplayTask(Display& display, osMessageQueueId_t queue)
+{
+    session_controller_to_display state;
+    memset(&state, 0, sizeof(state));
+
+    for (;;)
+    {
+        if (osMessageQueueGet(queue, &state, 0, osWaitForever) == osOK)
+        {
+            while (osMessageQueueGet(queue, &state, 0, 0) == osOK);
+
+            // Detail first: a driver that renders these records them here and lays them out
+            // in Render. On the character panel all three are no-ops the optimiser deletes.
+            (void)display.ShowAngularAcceleration(state.angular_acceleration);
+            (void)display.ShowPeakForce(state.peak_force);
+            (void)display.ShowSessionElapsed(state.session_seconds);
+
+            (void)display.Render(state);
+        }
+
+        osDelay(sysconfig_get_u32(SYSCFG_LCD_TASK_OSDELAY));
+    }
+}
+
+#endif /* INC_TASKS_DISPLAY_DISPLAYDRIVER_HPP_ */