ILI9431 Support - #120
Merged
Merged
Conversation
The firmware set CMAKE_C_STANDARD but never CMAKE_CXX_STANDARD, so the C++ standard was whatever the toolchain happened to default to -- gnu++17 for the pinned ARM GCC 13.3.rel1, and liable to move under us on a toolchain bump. Pin it, and pin it at 20: the display work that follows checks both LCD drivers against a concept, which needs C++20. firmware/tests/CMakeLists.txt already builds at 20, so this also stops the two from drifting apart. Nothing else changes. Debug and Release both build clean, compile_commands.json confirms -std=gnu++20 (C stays at gnu11), and the 60 host tests still pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The SessionController FSM formatted its own text and sent the Lumex panel
(row, column, string) triples. That protocol is the panel: 16x2 literals with
hand-counted padding, field widths chosen to land in exact cells, and column
offsets baked into the state machine. A second display cannot share it -- the
intersection of a 16x2 character LCD and a 320x240 TFT caps the TFT at 16x2,
and their union is meaningless on the LCD.
So move the seam to what the values mean. session_controller_to_display carries
a screen id plus every value any screen shows, and laying that out is the
driver's job. The FSM keeps its state machine and loses only its formatting;
SessionController is untouched apart from a queue rename, because the four
methods it calls -- DisplayRpm, DisplayForce, DisplayPIDEnabled,
DisplayManualBPMDutyCycle -- were already saying what rather than how.
This is also what makes a TFT viable later. Sending a rendered string leaves a
driver unable to tell which quantity moved, so it can only repaint everything;
at 320x240x16bpp that is ~50-100 ms over SPI. Sending state lets a driver diff
and repaint one field in ~1-2 ms. The Lumex driver already does exactly that,
writing only the runs of cells that differ.
Clearing is now derived rather than passed: the driver clears when the screen id
changes. That is the old rule exactly -- every Show*Screen cleared, and the one
redraw that deliberately did not (a tick inside the RPM editor) is also the one
that does not change screen. ShowDesiredRpmEditor loses its bool for that reason.
The layout lives in lumex_layout.c as a pure function -- state in, a full 2x16
frame out, no HAL or RTOS -- so tests/lumex_layout_tests.cpp pins all six screens
cell-for-cell on the host. Expectations were transcribed by hand from the
literals the FSM used to write.
Two things found on the way and deliberately preserved rather than fixed here:
- The session screen's force field is six characters at columns 2-7 while the
label literal carries "0.00" at columns 6-9, so columns 8-9 keep a stale "00"
and 12.34 N reads as "12.3400". Pinned by a test that says so.
- What the screen labels "rpm" is the optical encoder's angular_velocity,
which is rad/s. The struct field is named honestly; the rendering is not
changed.
The queue is CubeMX-owned, so the rename goes in the .ioc too -- otherwise the
next regen restores the old name and emits a type that no longer exists.
Debug and Release both build; the 3 generated headers are in sync with their
schemas; 74 host tests pass, 14 of them new. Unverified on hardware: the panel
walk-through is still to do.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two display bugs, both pre-existing and both found while moving the layout out of the FSM. They were left alone there so that commit stayed behaviour-preserving. The session screen's force field is six characters at columns 2-7, but the row's label literal carried a "0.00" of its own at columns 6-9. Nothing ever rewrote columns 8-9, so two digits of the label stayed on screen underneath every reading: 12.34 N displayed as "12.3400". The literals now carry labels and units only, with the units placed just past where each field ends, so a value and its unit cannot overlap however wide the reading gets. The other is a unit error. encoder_angular_velocity() returns rad/s -- its header says so -- and SessionController passed it straight to DisplayRpm, which printed it under an "rpm" label. A shaft at 3000 RPM read as 314. The conversion now happens once, in encoder_rpm() next to the measurement it converts, and the FSM applies it on the way in; the message field is named rpm because that is now what it holds. DisplayRpm is renamed DisplayAngularVelocity, which is what its caller was always passing it. Putting the conversion in encoder_math.c rather than in a display driver keeps it in one place no matter how many panels end up showing the number, and it is host-tested there alongside the arithmetic it belongs to. 78 host tests pass, 7 new. Debug and Release build; generated headers in sync. Still unverified on hardware -- and note this one does change what the panel shows, so the walk-through is checking new output, not identical output. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds an ILI9341 320x240 SPI TFT alongside the Lumex 16x2, selected in Config/debug.h and flashed. Both read the same queue and the same session_controller_to_display message, so the SessionController and its FSM are byte-identical either way; only the driver linked in changes. Both configurations are built in CI terms here -- Debug and Release, each way. Not the Adafruit library, and not a submodule. That would be three submodules (ILI9341 -> GFX -> BusIO), and the chain is Adafruit_ILI9341 : Adafruit_SPITFT : Adafruit_GFX : Print -- twenty virtual functions on a codebase that builds -fno-rtti -fno-exceptions to avoid exactly that. Adafruit_SPITFT.cpp is also 2621 lines of per-MCU #ifdef over Arduino's digitalWrite/SPIClass with no STM32 branch, so using it means permanently forking someone else's dispatch tree. What is actually panel-specific is ~30 lines: the init/gamma table, the address-window command, the MADCTL rotations. Those are vendored with Adafruit's BSD notice, along with the 5x7 GFX font (1280 bytes of pure data), and the transport is written against HAL_SPI_Transmit. Same call the ADS1115 driver already made. The common interface is a C++20 concept rather than a base class: the panel is fixed at link time, so DisplayDriver checks Init/Clear/Render at compile time and inlines through it, with a static_assert in each driver. It exposes no drawing primitives on purpose -- the intersection of a character grid and a TFT caps the TFT at 16x2, the union is meaningless on the LCD -- so everything the TFT can do that the Lumex cannot lives inside its Render() and never surfaces. Both drivers now share one queue-drain loop. Blocking SPI, not DMA. The display task is osPriorityBelowNormal, so a polling wait is preempted by anything that matters. DMA would also be real work rather than a flag here: DMA1/DMA2 cannot reach DTCM on the H7, and the linker script puts .data, .bss, the FreeRTOS heap and every task stack there, so it would need a scratch buffer in a new section in the (currently unused) AXI SRAM. Four hardware faults fixed in the .ioc, all of which would have left a dead panel, and none reachable by editing main.c since CubeMX owns it: - SPI1 DataSize was SPI_DATASIZE_4BIT (CubeMX default, never set) -> 8-bit - SPI1 prescaler 2 off a 200 MHz kernel clock = a 100 MHz SCK -> 16, 12.5 MHz - ILI_SPI1_LCD_CS was driven low at init, leaving chip select asserted - ILI_LCD_RST was driven low at init, holding the panel in reset forever The display task's stack also goes 512 -> 1024 bytes; the old figure predates any driver with pixel buffers. TASK_OFFSET_LUMEX_LCD becomes TASK_OFFSET_DISPLAY and the error enum follows, which regenerates the C# mirrors too -- deferred out of the previous commit so that one stayed firmware-only. No hand-written C# referenced either name. 93 host tests pass, 15 new. The ILI layout tests check what the driver actually depends on rather than a pixel golden: that fields stay on the panel at their widest values, that a screen's field list is positionally stable whatever the values (which is what makes the index-wise diff valid), and that fields sharing a slot are equal width so a redraw erases the previous value. Generated headers, C# mirrors and CubeMX output all pass their drift checks. Unverified on hardware: the panel has not been driven. Bring-up order is in Drivers/ILI9341/README.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The blink task has no LED of its own -- it toggles ILI_SPI2_SD_CS (PH7), the microSD slot's chip select on the ILI9341 module, picked as a convenient scope point back when nothing else used the pin. Once that module is fitted the pin is not free, and driving a chip select on a timer is not something anyone would notice from the symptoms. Both flags being 1 is now a compile error rather than a runtime surprise. The check reads ILI9341_LCD_TASK_ENABLE, which is defined above it, and is guarded on definedness as well: an undefined macro is 0 to the preprocessor, so moving either #define below this point would otherwise switch the check off silently -- the same class of accident being guarded against. All four combinations exercised: Lumex+LED off, Lumex+LED on and ILI+LED off all build; ILI+LED on fails with the message naming the pin. Defaults are unchanged (Lumex, blink task off), so nothing that built before stops building. The real fix is a pin of its own for the blink task, which is a board question. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The panel reads upside down on the rig: it is mounted 180 degrees from the controller's default landscape, so ILI9341_ROTATION_LANDSCAPE put the origin in the wrong corner. Use LANDSCAPE_FLIP. That is MADCTL 0x28 -> 0xE8. The difference is MX|MY, a mirror in both axes and nothing else: MV stays set so the panel is still 320x240, and BGR is untouched so colours are unaffected. The layout needs no changes for the same reason. Which way up the panel sits is a property of the enclosure rather than of the driver, so it is now ILI9341_DISPLAY_ROTATION in config.h beside the Lumex's grid dimensions -- remounting the panel should not mean editing driver code. Everything else about the bring-up was already right: the panel initialised, took the gamma sequence, drew the font and got its colours correct, which exercises reset, chip select, D/C and the SPI settings. Both drivers build Debug and Release; 93 host tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pressing the brake button bricked the board: no LEDs, no buttons, only a reset.
That is vApplicationStackOverflowHook, which does taskDISABLE_INTERRUPTS() then
spins -- with interrupts off nothing runs, so the symptom is a dead device rather
than a reported fault.
The overflowing task was the display. Entering a session is the only way to reach
the session screen, and the session screen was the only screen that formatted a
float: snprintf("%6.2f") for the force reading, the sole floating-point
conversion in the whole firmware. newlib's float formatter needs several hundred
bytes of stack, on top of the 448 the ILI render path already used, against a
1 KB task stack. Every other screen formats integers only, which is why nothing
else triggered it.
Two fixes, either of which would have been enough, both worth having:
- display_format_fixed2() replaces the "%6.2f" in both layouts. It rounds into
hundredths once and formats integers from there, so the display path no longer
reaches a routine whose stack cost cannot be read off -fstack-usage output.
- ILI9341Display::Render's ili9341_frame local becomes a member. The object is
already a static in ili9341_lcd_main, so the frame moves to .bss: Render goes
from 256 bytes of stack to 32.
The ILI session path is now RunDisplayTask 48 + Render 32 + ili9341_layout 48 +
layout_session 64 + display_format_fixed2 80 = 272 bytes, all measurable, against
448 plus an unbounded formatter before. The Lumex path benefits the same way and
had the same latent bug with less margin.
Six tests pin the new formatter against what %6.2f produced, including the
sign-when-the-whole-part-is-zero case (-0.50 must not render as 0.50) that
truncating division would otherwise lose.
-u _printf_float stays in the link for now; nothing calls %f any more, so it is
removable, but that is a global setting and a separate decision.
99 host tests pass; both drivers build Debug and Release. TaskMonitor reports
per-task stack high-water marks over USB, which is how to confirm the headroom on
real hardware.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This reverts commit 45c917f.
The actual cause of the board dying on brake press, and it was mine.
RunDisplayTask did `if (!display.Render(state)) return;`. Returning from a
FreeRTOS task function lands in prvTaskExitError() (port.c:217), which fails a
configASSERT, calls portDISABLE_INTERRUPTS() and spins forever. With interrupts
masked the button EXTI never fires again -- and since the brake LED is driven
inside that ISR (input_manager_interrupts.c:35), it freezes mid-press. That is
exactly the reported symptom: whole rig dead, LED stuck, only a reset recovers.
So one failed SPI write killed the entire dynamometer. A display is the least
important thing on this board and had the most fatal failure mode on it.
Why the session screen and nothing else: it is the busiest render -- most fields,
largest glyphs -- and it happens precisely as the USB, PID, BPM and sensor tasks
spin up. HAL_SPI_Transmit's timeout is wall-clock and keeps counting while the
caller is preempted, so the lowest-priority task on the board can blow a 100 ms
timeout on scheduling latency alone, with nothing wrong on the bus. That is now
1000 ms, far past any real transfer (96 bytes is ~61 us), so it fires on a wedged
bus rather than on a busy scheduler.
Three changes:
- RunDisplayTask is [[noreturn]] and swallows the render result. A failure is
already recorded in the task error buffer and reaches the host over USB; the
loop carries on.
- Both drivers clear _hasRendered when a write fails, so the next pass clears
and repaints in full. Without that the shadow copy disagrees with the panel
and the field diff would skip cells that were never actually painted.
- Both _main functions suspend instead of falling off the end.
The previous commit is reverted. It theorised a stack overflow in the float
formatter, which I inferred rather than confirmed and which turned out to be
wrong -- and it made things worse on the bench.
Both drivers build Debug and Release; 93 host tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… Lumex Adds three in-session readouts that only the ILI9341 has room for: angular acceleration, peak force this session, and elapsed time. They join the DisplayDriver concept as ShowAngularAcceleration / ShowPeakForce / ShowSessionElapsed, which LumexLCD implements as one-line no-ops that discard the argument. This is deliberately the "union" interface argued against when the seam was designed: the concept now carries methods one panel cannot honour. The trade is worth making here because the alternative -- keeping the interface to the intersection -- means the TFT can never show more than a 2x16 character grid can, which is most of the reason for fitting it. The stubs are inline and empty, so the Lumex build emits no code for them at all: they do not appear in its -fstack-usage output. What it costs is honesty in the header, and the comments say plainly that the asymmetry is intended. They sit on the concept rather than only on ILI9341Display so the shared task loop can call them without knowing which panel it drives, and so a driver that silently stopped implementing one is a compile error rather than a link error. Adding a fourth readout is one real implementation and one (void) line. Data plumbing: angular acceleration was already measured by the optical encoder and thrown away at the display. Peak force is tracked by magnitude, since the rig is loaded whichever way the cell is driven. Elapsed time comes from the same microsecond counter the samples are stamped with. Both per-session figures reset on entry to a session rather than exit, so the screen keeps the last run's numbers until a new one starts. The detail fields clamp rather than overflow their formats. The driver diffs field i against field i and repaints only the movers, so a reading that outgrew its width would shift its neighbours and leave their old pixels behind -- a test pins the widths against absurd inputs for exactly that reason. ili9341_frame grew to 12 fields and with it past what belongs on a 1 KB task stack, so Render's working frame is now a member of the (static) driver object. 98 host tests pass, 5 new. Both drivers build Debug and Release; generated headers and C# mirrors in sync. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Home page showed BPM duty cycle as a read-only figure, and there was no way for the host to set it: the only two commands in the protocol were USB_CMD_ACK and USB_CMD_SET_SYSCONFIG, and duty cycle is not a sysconfig parameter. Swapping the TextBlock for a TextBox alone would have produced a box that displays telemetry, fights the user as BPM samples overwrite it several times a second, and sends nothing. So this is both halves. Firmware: a new routed command, SESSION_CMD_SET_BRAKE_DUTY_CYCLE, addressed to TASK_OFFSET_SESSION_CONTROLLER. The USB task already had the routing -- a task_offset -> queue switch whose comment invited exactly this -- so it needed a queue and a case rather than new machinery. It is applied through FSM::SetHostBrakeDutyCycle, which writes the same state a rotary-encoder tick does. That is deliberate: the clamp to MIN/MAX_DUTY_CYCLE_PERCENT, the post to the BPM task and the on-screen readout are then identical whether the request came from the rig or the PC, rather than being a second path that has to remember the same rules. The brake is an actuator, so the command is refused unless a session is running, answering USB_RSP_NOT_SUPPORTED rather than a silent OK -- the app should not show a duty cycle the brake never went to. That preserves the existing invariant that the brake is never actuated outside a session, however the request arrives. Unlike a sysconfig write the command is not retried: re-sending one whose ack was lost could drive the brake to a figure the user has since moved away from. App: the readout becomes a text box, in percent to match what it replaced. It is disabled unless connected and in-session, sends on Enter or focus loss rather than per keystroke, takes 1% per scroll notch (sent immediately -- a notch is a complete intent, unlike a half-typed number), and reds its border when the device refuses. Telemetry stops writing to the box while it has focus; without that the caret would jump to the end between keystrokes. USB_PROTOCOL_VERSION 7 -> 8: a new opcode is wire-visible, and the handshake refuses a mismatch, so an old app and new firmware will say so instead of misbehaving. Flash the board and rebuild the app together. CommandOpcodes learned the new enum so the event log names it. Its tripwire test -- which fails when a task gains commands the host cannot name -- did its job. 258 C# tests and 98 firmware host tests pass; both display drivers build Debug and Release; generated headers, C# mirrors and CubeMX output all in sync. Unverified on hardware. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y's stack telemetry The BPM broke the moment the ILI9341 was switched on, and the branch history makes that precise: every commit before 70e95b8 built with LUMEX_LCD_TASK_ENABLE 1, so SPI1 was idle and the TFT had never run on hardware. 70e95b8 flipped the panel over and raised the SPI pins to GPIO_SPEED_FREQ_VERY_HIGH in the same change. None of the three fixes below depend on which of those two did it. ShowSessionScreen() was re-entrant. HandleButtonBrakeInput calls it on every press edge, and BTN_BRAKE is GPIO_MODE_IT_RISING_FALLING with report_press = true, so a bouncing contact -- or an edge coupled in from somewhere else -- re-entered it mid-session. It is destructive: it zeroes _desiredManualBpmDutyCycle, wipes _peakForce and restarts the session clock, so the brake dropped to 0% under the user's hand. It is the only path in the firmware that reaches 0% without leaving the session. Only a real IDLE -> IN_SESSION transition may reset those now. SPI1's pins go back to GPIO_SPEED_FREQ_MEDIUM. VERY_HIGH is the fastest edge rate the part can produce and buys nothing at 12.5 MHz, where MEDIUM is already comfortable; on flying leads beside unfiltered EXTI inputs it is a cost with no benefit. Changed in the .ioc as well as the generated MSP, so a CubeMX regen stays a no-op. Three #if LUMEX_LCD_TASK_ENABLE guards went dead when that macro became 0. Two were null checks. The third gated TaskMonitor's per-task report for TASK_OFFSET_DISPLAY, so the display task's stack high-water mark silently stopped being streamed -- which is exactly the telemetry needed to judge whether that task is close to overflowing, and it went away in the same change that started running the panel that puts it under pressure. All three now test a new DISPLAY_TASK_ENABLE, which is what code outside the two drivers actually means, and SessionController.hpp's !defined tripwire covers it so it cannot silently evaluate to 0 again. CheckTaskQueuesValid also gained the two queues 7f70c55 added but did not validate: a null usb_command made the SessionController swallow every host command instead of reporting the bad wiring. Both panels build Debug and Release; 100 firmware host tests pass. Unverified on hardware -- the first thing to read off the board is that restored stack high-water mark. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…L_Delay out of the task
Three changes that all land on the same files, so they go together rather than as a rename
commit that would not build without the fix inside it.
Tasks/LCD and Tasks/Display read as two modules. They are one task: one task_offset, one
queue, one entry point chosen at compile time, and the dependency already ran one way --
LumexLCD.cpp included Tasks/Display/DisplayDriver.hpp, never the reverse. LCD was simply
where the files were when the Lumex was the only panel. Everything now lives under
Tasks/Display with the shared code at the top and a subdirectory per panel:
DisplayDriver.hpp the concept every panel satisfies + the shared task loop
display_common.{h,c} helpers neither panel owns
Lumex/ the 16x2 character driver and its layout
ILI9341/ the 320x240 TFT driver and its layout
Neither panel subdirectory includes the other. The firmware CMakeLists needed nothing --
APP_SOURCES is a GLOB_RECURSE -- so only the two explicit paths in tests/CMakeLists.txt and
the include lines moved. The two module READMEs merge into one.
display_format_fixed2 comes back. 45c917f removed newlib's float formatter from the display
path after it overflowed the task's 1 KB stack and bricked the board on the first brake
press; dacdd1f reverted that wholesale, and de0b341 then added a second "%6.2f" for peak
force. Measured on a Release build, the session render path went from ~570 bytes to ~384:
RunDisplayTask 48 + Render 32 + ili9341_layout 88 + display_format_fixed2 64
versus 48 + 32 + 96 + ~396 for _svfiprintf_r -> _printf_float -> _dtoa_r -> _malloc_r
The size matters less than what left with it: _dtoa_r allocates, so the old cost could not
be read off -fstack-usage at all. That stack's neighbour in the heap is the
SessionController's -- they are created back to back -- and its outermost frame holds the
FSM, so an overflow too small to trip the 16-byte canary lands on the commanded brake duty
cycle. The six tests pinning the formatter against printf come back with it.
The ILI9341 driver takes an injected delay callback instead of calling HAL_Delay. HAL_Delay
spins rather than yields, so Init() burnt ~325 ms of CPU at the display task's priority, and
it cannot return at all inside a FreeRTOS critical section -- HAL's tick is a TIM at
TICK_INT_PRIORITY 15, which is masked there. A void(*)(uint32_t) defaulting to HAL_Delay
keeps Drivers/ILI9341 free of cmsis_os2.h, which is what makes it host-testable; the task
passes a one-line osDelay wrapper. A static_assert on configTICK_RATE_HZ turns a change of
tick rate into a build error rather than a panel that quietly misses its init timings.
LumexLCD::ClearDisplay's lone HAL_Delay(20) becomes osDelay(20); the rest of that driver was
already correct.
Both panels build Debug and Release; 106 firmware host tests pass; both codegen checkers
report everything still in sync. Unverified on hardware.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The box jumped while you used it. It was bound to a property telemetry also wrote: a BpmSample lands several times a second, so a moment after every scroll notch the device's own reading overwrote what you had just dialled in, and the value snapped back. The focus gate added with the box was never going to hold. It only covered typing, and scrolling does not focus a control -- so the wheel, the one input that produces a change per notch, was exactly the case it did not cover. Widening the gate would not have fixed the cause either, which is that one property was being asked to be two things: what you have asked the brake for, and what the brake is doing. Those are different numbers and they legitimately differ -- while a command is in flight, and permanently when the firmware clamps to the MIN/MAX_DUTY_CYCLE_PERCENT envelope. So they are two things now. The box is a setpoint that telemetry never touches; the measured figure sits beside it as "(now 45.0%)". Seeding happens at exactly two moments -- when a session starts, and when a link drops -- rather than continuously, because a setpoint that re-synced to the measurement would drag whatever you had dialled in back to wherever the brake happened to be. That also removes the IsEditingDutyCycle flag and its GotFocus handler entirely: nothing writes the box behind the user any more, so there is nothing to referee. Escape now reverts to the brake's actual figure rather than waiting for a telemetry sample to put it back, which is what it was really relying on. Showing both is worth it on its own. The old readout could not tell you whether a duty cycle you asked for had been clamped; the pair does. App only -- no firmware change. 258 C# tests pass. Unverified on hardware: the thing to check is that a scroll notch now stays put. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sets both panel enables to 0, so no display task runs and nothing drives SPI1.
Everything else on the branch stays exactly as it is: the host command route, the
session-detail message, the app's duty-cycle control, the SPI1 peripheral setup.
Why this and not a debounce: the search for the brake fault came back with the
encoder path byte-identical to main. AdjustBrakeDutyCycle, the BPM and PID tasks,
every ISR, input_manager_interrupts.c, and the encoder and brake pin
configuration all diff clean. The FreeRTOS heap is ~13.6 KB of 46 KB. The one
functional difference left is that a display task drives SPI1 at 12.5 MHz on
PD7/PG9/PG10/PG11 while the encoder is being turned -- and the fault appears only
when the encoder is turned, never when the app sets the same value over USB,
which rules out everything downstream of the setpoint since both paths share it.
So this changes one variable rather than adding logic on top of a guess.
Verified the isolation rather than assuming it: no ILI9341:: or LumexLCD:: symbol
survives in the image, and HAL_SPI_Transmit is not linked in at all, so no code
path can reach SPI1. Only the two timer handle variables in main.c remain, which
are data.
Reading it:
- brake behaves -> the display's SPI activity is implicated, and the next
question is which part (clock rate, edge rate, wiring)
- brake still misbehaves -> the display is exonerated and the cause is elsewhere
Supporting changes, both of which outlive the experiment: "at most one panel" is
now a legal configuration rather than "exactly one", and the display task parks
instead of falling through to the Lumex when neither is enabled. The
SessionController no longer treats a display as a hard dependency -- it never
needed one, it posts to a queue nobody has to drain.
All three configurations build Debug and Release (ILI, Lumex, none); 100 host
tests pass. Put ILI9341_LCD_TASK_ENABLE back to 1 to undo.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…loating pin ROT_EN_B (PI8) was configured GPIO_NOPULL. It is the only user input on the board without a pull resistor: ROT_EN_A, ROT_EN_SW, BTN_SELECT, BTN_BACK and ADS1115_ALERT are all PULLUP, and BTN_BRAKE is PULLDOWN because it is active high. ROT_EN_A is PULLUP with GPIO_MODE_IT_FALLING, so the encoder switches its contacts to ground -- which means B needs a pull-up as much as A does. Without one it floats whenever the B contact is open, and register_rotary_encoder_input() samples exactly that pin, at every A edge, to decide which way the knob turned. So the direction bit was noise. A high-impedance input is an antenna and does not need a neighbouring trace to pick something up, which is why the encoder and SPI traces not crossing did not rule this out. With SPI1 idle the pin held its last level long enough to usually read right, which is why main and the display-off build were both fine. With the panel driving SPI1 at 12.5 MHz it read randomly, so ticks came back with random direction and the duty cycle random-walked instead of climbing -- and since it is clamped at MIN_DUTY_CYCLE_PERCENT (0.0), the walk had a floor to collect at. That is the "drops to 0%" symptom, and it explains why only the encoder was affected while the app's command path, which never touches PI8, worked throughout. The panel is back on: this is the build the fault should be judged against. Also keeps two things from the diagnostic commit, which are worth their own keeping: at most one panel is a legal configuration (so the display can be turned off again without editing dispatch), and the display task parks instead of falling through to the Lumex when neither is enabled. Both panels build Debug and Release; 100 host tests pass. The .ioc is the source of the change, so a CubeMX regen is a no-op. Unverified on hardware -- the test is the one that has been failing: turn the encoder during a session with the panel running. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two mitigations together rather than one per flash cycle, because iterating a guess at a time has already cost several. Answering the question directly first: there is no configuration that is both "same as main" and "runs the TFT". On main SPI1 is never used -- the Lumex is a GPIO bit-banger -- so those pins simply never toggle, which is why main's LOW edge rate and prescaler-2 on SPI1 mean nothing there. Turning the panel on is the difference, and the display-off build proved it is the trigger. The previous commit's pull-up was not the fix; the board already has external pull-ups. It is left in place because a stronger pull on a line demonstrably picking up noise costs nothing, but it should not be read as the cause. Quieter bus: SPI1 goes 12.5 -> 6.25 MHz and its pins from MEDIUM back to LOW, which is what they were before this branch touched them. Half the clock and the slowest edge rate the part offers, so whatever is coupling has less to couple. A full repaint goes ~98 -> ~197 ms, which is only visible on a screen change; a single field is ~4 ms and the readouts are all single fields. Harder input: the encoder was decoded with no filtering at all -- an EXTI on ROT_EN_A reading ROT_EN_B's level for direction. One disturbed read of B reverses the tick, and a reversed tick is worse than a lost one, because the setpoint then random-walks instead of lagging; clamped at 0.0 it collects at the bottom, which is the reported symptom. Now an A edge within 1 ms of the last accepted one is ignored, and B is read three times with a majority vote. Both counters are exposed as globals -- rotary_encoder_rejected_edges and rotary_encoder_split_direction_reads. If the encoder behaves now, those two say whether it is because the noise stopped or because it is being filtered out, and a split-direction count that climbs while the knob is still is a coupling measurement rather than an inference. If this still is not enough, the proper fix is to stop decoding the encoder in software: TIM1/2/3/4/8 have a hardware encoder interface with a digital input filter, which is what that filter exists for. That is a real change, so it is not being made on a guess. Both panels build Debug and Release; 100 host tests pass. .ioc is the source for the SPI change, so a regen is a no-op. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both display READMEs rewritten to answer the question they did not: how does anything actually get on the screen. Drivers/ILI9341/README.md now documents the wire. The panel has no idea what text is -- it is a framebuffer with a cursor -- and nothing said so. Added: what each of the six signals does and that ILI_LCD_DC is the entire framing mechanism, since there are no addresses or headers on this bus; the bus settings and why NSS_SOFT matters (CS can stay low across a command and the pixels that follow); the transaction shape as a CS window with D/C toggling inside it; two worked byte-level examples, a FillRect with its real CASET/PASET/RAMWR arguments and a character cell; RGB565 with its bit layout and big-endian byte order; and the auto-advancing cursor, which is why the rule is blit rectangles and never pixels. Also written down: that there is no read-modify-write, so everything drawn is opaque and that is what forces fixed-width padded fields one layer up; the glyph format, with 'A' drawn out as its five column bytes; that `size` is pixel replication rather than a font, so size 5 is the same 35 dots five times blockier; that nothing reflows or shrinks, only clips; the init table's self-describing format including the high-bit-means-delay wrinkle; the four MADCTL rotations and which bit does what. Core/Src/Tasks/Display/README.md now opens with the full path from a force reading to pixels, as a diagram with the six stages and what each one discards, then explains each. The parts that were previously only in commit messages are now where someone will find them: why the queue put uses timeout 0, why RunDisplayTask is [[noreturn]] and what returning from a task function actually does, why field equality includes colour, why a failed render clears _hasRendered, and the session screen's field order as a table -- the index-wise diff depends on that order and it was written nowhere. Every number checked against the code rather than carried over: the ten field positions match ili9341_layout.c, and the timings are recomputed at the current 6.25 MHz (full frame ~197 ms, one size-5 field ~18 ms, one size-3 character ~1.1 ms). The stale "~61 us at 12.5 MHz" in the SPI timeout comment is corrected to ~123 us at 6.25 MHz; that clock changed two commits ago and the comment did not. Docs and one comment only. Builds Debug; 100 host tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LumexLCD was two classes wearing one hat. Its public surface was task-level
(Init/Clear/Render, satisfying DisplayDriver) and its private surface was pure
HD44780 protocol (SendByte, WriteCommand, SetCursor, DisplayString, ToggleBlink).
That is exactly the ILI9341Display-over-ILI9341 boundary, just never drawn, so
the codebase taught two patterns for one job.
Drivers/Lumex/LumexPanel now holds the protocol and nothing above it: board
wiring handed in as a struct, the instruction set named rather than written as
magic numbers, and no knowledge of screens, diffing or the RTOS.
Tasks/Display/Lumex/LumexLCD keeps the layout, the changed-cell diff and the task
around it. Neither file changed behaviour.
TIM13 is gone entirely -- peripheral, NVIC line, IRQ handler, MSP init, the
global handle, the main.c dispatch, the ISR, the volatile flag and the .ioc
entry. It was never earning its place:
StartTimer(40);
while (!timerCallbackFlag); // spun for the full 40 us anyway
The timer's only job was to drop E and set the flag the task was already
spinning on, so it cost a whole peripheral and saved no CPU. The same 40 us now
comes from the free-running microsecond timestamp counter that every sample is
already stamped from.
Not osDelay, which was the first suggestion and does not fit: at a 1 kHz tick its
floor is 1 ms, and the wait is 40 us. That is not a pulse-width requirement to be
rounded up -- with no R/W pin on this board the busy flag can never be read, so
the enable pulse doubles as the ~37 us instruction-execution wait, which is why
SetCursor and the character that follows it need nothing between them. Rounding
40 us to 1 ms would stretch a full 32-cell repaint from ~2.6 ms to ~64 ms. So the
microsecond wait busy-waits and the millisecond ones (power-on, CLEAR) use
osDelay, exactly as the ILI9341 driver's injected delay does.
The busy-wait is bounded as well as timed, because get_timestamp() reads a
counter SessionController starts and SESSION_CONTROLLER_TASK_ENABLE 0 is a legal
configuration -- a purely time-based loop would then never exit and would wedge
the display task. LumexLCD::Init starts the counter itself, and the bound turns a
failure there into a mistimed panel rather than a hung one.
Drivers/Lumex/README.md documents the panel the way the ILI9341 one documents
its: the signals and that RS is the whole framing mechanism, the write cycle with
its falling-edge latch, all eight instructions and their option bits, the four
codes this driver actually sends, DDRAM's non-contiguous rows (row 1 starts at
0x40, so a cell is not a linear offset), a worked byte-level example, why
FUNCTION_SET is sent three times, and the timing. It opens by contrasting the two
panels, since the interesting thing about this one is that it contains a
character generator and the ILI9341 does not.
Both panels build Debug and Release; 100 host tests pass; CubeMX drift check
passes, so the .ioc surgery -- which needed the Mcu.IP/Mcu.Pin index lists
renumbered and their counts decremented -- came back clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Display README had grown to cover three things at once: the shared path, the Lumex, and the ILI9341. Someone working on one panel had to read past the other, and the two "rendering" sections at the bottom were where the panel-specific detail had collected because there was nowhere better to put it. Now it mirrors the code, which since the last commit has one directory per panel: Tasks/Display/README.md what is true of both Tasks/Display/Lumex/README.md the 16x2 character grid Tasks/Display/ILI9341/README.md the 320x240 TFT The top level keeps the end-to-end path (stages 1-3 in full, 4-6 as a pointer to whichever panel is fitted), the contract and why there is no common drawing API, the DisplayDriver concept, panel selection, the two shared helpers, and units. It also names the rule both panels obey and neither README should have to restate: neither supports read-modify-write, so a field is erased only by being repainted, and every layout rule in both subdirectories descends from that. Each panel README picks up stages 4-6 for itself. Lumex/ gains the six screens drawn out, the run-of-changed-cells diff, the clear-on-screen-change rule and where it came from, the two fixed layout bugs recorded so they are not reintroduced, and what it does with the three readouts it has no room for. ILI9341/ gains the field model, the positional-stability and fixed-width properties the diff depends on, the session screen's field table, why the driver object is a function-local static, and the timings that make diffing necessary rather than optional. Nothing is dropped and nothing is duplicated: each fact is in exactly one of the four files, with links where it is needed from more than one. Four display READMEs is more than it sounds like -- two describe rendering and two describe hardware protocols, and they were already separate concerns before this. Core/README.md's module index lists all of them. Every [[wiki link]] across the six files was checked against the module names actually defined; all resolve. Docs only. 100 host tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The counter is read by every task that stamps a sample or times a wait, and owned by none of them. It was started by whichever Init() ran first, which made startup depend on a scheduling race -- and the display task lost that race. HAL_TIM_Base_Start returns HAL_ERROR whenever the handle is not in READY state, and a timer someone has already started is BUSY. It answers "already running" and "failed to start" with the same value. SessionController runs at osPriorityHigh against the display's osPriorityBelowNormal, so it always got there first; LumexLCD::Init took the HAL_ERROR at face value, reported ERROR_DISPLAY_INIT_FAILURE and suspended its own task. A blank panel, driven by a timer that was working perfectly. The board said so, too: the error arrived stamped "@ 220514" -- 220 ms of successfully counted time, read from the counter it was reporting as failed to start. So it starts once in main()'s USER CODE BEGIN 2, next to sysconfig_init(), before any task exists. Both Init()s drop their call. The return is ignored on purpose: if TIM2 is configured, MX_TIM2_Init has already called Error_Handler on anything that could fail, so by that line the handle is READY; and if STM32_PERIPHERAL_TIM2_ENABLE is 0 the timer is deliberately absent, where halting the board would defeat the point of the switch. start_timestamp_timer stays idempotent regardless -- it checks TIM2->CR1.CEN and only calls the HAL when the counter is stopped, asking the hardware whether it is running rather than asking the HAL whether this caller is the one who started it. A handle that was never initialised still fails honestly: CEN stays clear and HAL_ERROR comes back. Both functions in the header are now static inline rather than plain inline. A bare `inline` in C provides only an inline definition -- the compiler need not emit an external one -- so at -O0 a C caller links against a symbol nothing defines. It went unnoticed while every caller was C++, where inline has vague linkage, and surfaced the moment main.c called one: Debug failed to link while Release inlined it and passed. LumexLCD::Init now reports ERROR_DISPLAY_INIT_FAILURE when the panel itself fails, matching ILI9341Display. It previously returned false silently, so the one error code the display owns went from meaning the wrong thing to meaning nothing. Lumex Debug and Release, ILI9341 Debug, and 106 host tests all pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lude each other
Four things, all about the compile-time settings page.
Sections. The page takes a category from the comment block heading a group in
the header, so the display settings had never really had one: with no banner
line the parser falls back to the first line of the prose above the define, and
"The Lumex panel's character grid. The display message no longer carries
strings..." was a section name on screen. Banners now name three:
Display debug.h the two panel switches
Display: Lumex 16x2 config.h LUMEX_LCD_ROWS / _COLUMNS
Display: ILI9341 320x240 TFT config.h ILI9341_DISPLAY_ROTATION
Three cards rather than one because the page groups by (file, category), and
these span two headers by nature -- the switches are enables and the rest are
quantities. The switches also pick up trailing comments, which is the only way a
setting gets a description when its block is a banner, so the Display card reads
as two named panels instead of two bare macro names.
DISPLAY_TASK_ENABLE is gone. It was a derived (A || B) sitting in a file the app
offers as a list of switches to override, where nothing good could come of
editing it. Its four sites spell the disjunction out. It did exist for a reason
-- gating on one panel's enable silently compiles a feature out when the other
is selected, which is how the display task's stack high-water mark fell off the
USB stream on the ILI9341 move -- so that warning now sits in the Display
section comment, where someone adding a fifth site will read it.
Mutual exclusion. Turning one panel on turns the other off, rather than staging
a build the firmware's #error would reject minutes later. Turning one off
touches nothing, which is what keeps "both off" reachable: that is a real
configuration, the one where the display task parks and nothing drives SPI1, and
it is how the panel gets ruled in or out of a fault elsewhere on the board. So
it is deliberately not a radio group.
The rule lives in Dyno.Core as ConfigExclusiveGroups -- it is firmware knowledge
rather than view state, and it is worth testing. The view model wires it once
both headers are parsed, since a group may span files. Saved values are left
exactly as loaded: normalising a stored combination on open would silently
rewrite the user's settings, and the header check still catches one that arrived
by some other route. The firmware's #error remains the authority; this only
decides what a click does.
The committed default is now the ILI9341.
Also folded in: csharpier had drifted on MainWindowViewModel.cs and
CommandOpcodeTests.cs from earlier commits on this branch, which the CI
formatting gate would have failed, and the duty-cycle StackPanel in HomeView
gets the multi-line attribute style the rest of that file uses.
All three panel configurations build -- ILI9341, Lumex, and both off, that last
one exercising the new disjunction's false branch. 266 C# tests pass, seven of
them new: four on the exclusion rule and three pinning the section grouping and
the switch descriptions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…h one The box showed what you had asked for and a muted "(now 45.0%)" beside it showed what the brake was doing. Two numbers for one quantity, and the second one there only because the first could not be trusted to follow the board. Now the box follows the board itself, so a change made at the rig's rotary encoder -- or the PID driving the brake -- lands in it, and the second readout is gone. Telemetry stops writing it in exactly the two cases where the box is not the board's to write: Uncommitted edits. Any change the app did not make itself marks the box as the user's, and it is left alone until Enter, focus-out or Escape. Tracked with a guard flag around the app's own writes rather than by focus, so Enter hands the box back at once even though the caret is still in it. Commands in flight. After a send, readings are ignored until the board reports the commanded figure or a second passes. This is what the "(now ...)" split was working around: a BPM sample lands several times a second, so without it every scroll notch would be overwritten by the reading that follows it. That was the jumping the box had before, and the previous fix was to stop telemetry writing the box at all -- which is what made a second readout necessary. The timeout is the exit that matters for clamping. Ask for 95% against a firmware MAX_DUTY_CYCLE_PERCENT of 80 and the board never reports 95, so matching the commanded value would never end the wait. A second later the box drops to what the brake is actually at. That is a visible change in behaviour: the box used to hold 95 indefinitely with the truth only in the text beside it, and now a clamp looks like the number moving a beat after it is committed. Not covered by tests -- this is in Dyno.App and the only test project is Dyno.Core.Tests. Solution builds clean and 266 tests still pass, but the mirror-versus-hold behaviour wants confirming on the rig. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.