Large scale rendering refactor - #757
Draft
JanPetterMG wants to merge 3 commits into
Draft
Conversation
… and tuned performance.
JanPetterMG
added
firmware
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors firmware rendering by introducing faster frame operations (row/column/shape fills), simplifying mode drawing logic, and restructuring display mapping/orientation handling to support more efficient refresh.
Changes:
- Reworked
DisplayServicerendering pipeline (pixel mapping, flush planes, new drawing/fill primitives, updated frame accessors). - Updated many modes/extensions to use the new
DisplayServiceAPIs (e.g.,fillFrame,fillRow,fillColumn, ellipse/rectangle helpers). - Updated streaming mode protocol handling to route by port and parse payload offsets per protocol.
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| firmware/src/services/ModesService.cpp | Updates mode-switch UI rendering and power-on behavior using new display primitives. |
| firmware/src/services/DisplayService.cpp | Major rendering refactor: new flush logic, orientation mapping, new drawing/fill APIs, reordered callbacks. |
| firmware/src/services/DeviceService.cpp | Adjusts shutdown clear behavior and global instance initialization style. |
| firmware/src/modes/WeatherMode.cpp | Switches to fillFrame(0U) for clearing before drawing. |
| firmware/src/modes/WaveformMode.cpp | Uses fillFrame/fillRow instead of per-pixel loops. |
| firmware/src/modes/TickerMode.cpp | Switches to fillFrame(0U) for clearing before drawing. |
| firmware/src/modes/StreamMode.cpp | Routes UDP packet parsing by protocol/port and updates frame setting logic. |
| firmware/src/modes/StarsMode.cpp | Optimizes star placement using linear indices and new pixel APIs. |
| firmware/src/modes/SnakeMode.cpp | Updates clearing/brightness calls for new pixel API signatures. |
| firmware/src/modes/ScanMode.cpp | Uses fillColumn to reduce per-pixel drawing complexity. |
| firmware/src/modes/RingMode.cpp | Switches to fillFrame(0U) for clearing before drawing. |
| firmware/src/modes/RainMode.cpp | Switches to fillFrame(0U) for clearing. |
| firmware/src/modes/PixelSequenceMode.cpp | Precomputes pixel order mapping and uses index-based pixel API. |
| firmware/src/modes/PingPongMode.cpp | Uses new line helpers for paddles and explicit brightness for pixels. |
| firmware/src/modes/NoiseMode.cpp | Switches to index-based pixels and explicit brightness updates. |
| firmware/src/modes/LinesMode.cpp | Uses fillColumn for faster line animation rendering. |
| firmware/src/modes/HomeThermometerMode.cpp | Uses fillFrame(0U) and tightens integer constants/casts. |
| firmware/src/modes/GlitterMode.cpp | Uses index-based pixel API for random glitter placement. |
| firmware/src/modes/FliesMode.cpp | Simplifies fly storage to a pair and uses new display calls. |
| firmware/src/modes/FireworkMode.cpp | Splits ellipse drawing into outline/solid helpers and updates calls accordingly. |
| firmware/src/modes/EqualizerMode.cpp | Refactors bar structure/looping and uses rectangle/line helpers. |
| firmware/src/modes/CountdownMode.cpp | Switches to fillFrame(0U) for clearing before drawing. |
| firmware/src/modes/ClockMode.cpp | Switches to fillFrame(0U) for clearing before drawing. |
| firmware/src/modes/CircleMode.cpp | Uses new ellipse outline helper. |
| firmware/src/modes/BrightMode.cpp | Uses fillFrame(UINT8_MAX) for full-bright fill. |
| firmware/src/modes/BreakoutClockMode.cpp | Uses fillRows/line helpers for faster block/paddle drawing. |
| firmware/src/modes/BlinkMode.cpp | Simplifies blink toggling using fillFrame. |
| firmware/src/modes/BlindsMode.cpp | Refactors blinds rendering with fillFrame/fillRow and state handling. |
| firmware/src/modes/BinaryEpochMode.cpp | Uses rectangle fill helper instead of per-pixel loops. |
| firmware/src/modes/ArrowMode.cpp | Uses fillFrame(0U) and tightens boundary condition checks. |
| firmware/src/handlers/ClockHandler.cpp | Replaces manual clearing loops with fillRows. |
| firmware/src/extensions/SignalExtension.cpp | Switches to fillFrame(0U) when displaying signals. |
| firmware/src/extensions/OtaExtension.cpp | Ensures display is powered before drawing OTA UI and uses fillFrame(0U). |
| firmware/src/extensions/MessageExtension.cpp | Switches to fillFrame(0U) for message rendering. |
| firmware/include/services/DisplayService.h | Updates public display API surface and adds pixel mapping structures/static pixels. |
| firmware/include/modes/StreamMode.h | Splits stream handlers per protocol and adds <span>. |
| firmware/include/modes/StarsMode.h | Refactors star state from x/y to linear index. |
| firmware/include/modes/ScanMode.h | Renames scan state from column to x. |
| firmware/include/modes/PixelSequenceMode.h | Changes pixel map storage and adds configure() override. |
| firmware/include/modes/NoiseMode.h | Refactors dot state from x/y to linear index. |
| firmware/include/modes/FliesMode.h | Simplifies fly storage and cleans up pending initialization. |
| firmware/include/modes/EqualizerMode.h | Simplifies bar representation to a pair. |
| firmware/include/modes/BlinkMode.h | Adds lit state and modernizes initializers. |
| extra/Python/ModeGenerator.py | Updates generated modes to use Display.fillFrame(0U). |
Suppressed comments (7)
firmware/src/services/DisplayService.cpp:101
indicesis declared asstd::pair<uint8_t,uint8_t>but it stores values frompixelsMapped(aPixelMap). This is a type mismatch that will fail to compile.
firmware/src/services/DisplayService.cpp:116- This loop treats
indices[idx]as astd::pairand uses.first/.second, butindicesshould storePixelMap(bit/byte). As written, it won’t compile onceindicesis corrected.
firmware/src/services/DisplayService.cpp:189 PixelMaphasbit/bytefields, butmapPixelassigns to.first/.second, which will not compile.
firmware/src/modes/StreamMode.cpp:98- This
static_castto a fixed-extent mutablestd::spanis ill-formed and risks UB. Construct thestd::span<const uint8_t, N>directly from the packet pointer and expected payload size.
firmware/src/modes/StreamMode.cpp:103 - This
static_castto a fixed-extent mutablestd::spanis ill-formed and risks UB. Construct thestd::span<const uint8_t, N>directly from the packet pointer and expected payload size.
firmware/src/modes/StreamMode.cpp:112 - This
static_castto a fixed-extent mutablestd::spanis ill-formed and risks UB. Construct thestd::span<const uint8_t, N>directly from the packet pointer and expected payload size.
firmware/src/modes/StreamMode.cpp:94 data.front()is undefined behavior if an empty UDP packet is received. Guard the access so length 0 safely results intime == false.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+344
to
+346
| const uint8_t margin{ | ||
| max<uint8_t>(1U, static_cast<int>(GRID_ROWS - height) / static_cast<int>(lines.size() + 1U))}; | ||
| uint8_t y{max<uint8_t>(0U, (GRID_ROWS - height - ((lines.size() - 1U) * margin)) / 2U)}; |
Comment on lines
+87
to
+88
| const std::pair<uint8_t, uint8_t> &mapping{pixelsMapped[logical]}; | ||
| planes[0U][mapping.second] |= static_cast<uint8_t>(mapping.first); |
Comment on lines
+53
to
+57
| struct PixelMap | ||
| { | ||
| uint8_t bit; | ||
| uint8_t byte; | ||
| }; |
Comment on lines
+445
to
+450
| const size_t maxX{static_cast<size_t>(minX + columns)}; | ||
| for (size_t y{static_cast<size_t>(minY + 1U)}; y < maxY; ++y) | ||
| { | ||
| frame[minX + (y * GRID_COLUMNS)] = _brightness; | ||
| frame[maxX + (y * GRID_COLUMNS)] = _brightness; | ||
| } |
Comment on lines
+331
to
+332
| for (size_t idx{static_cast<size_t>(x + (yMin * GRID_COLUMNS))}; idx < x + (yMax * GRID_COLUMNS); | ||
| idx += GRID_COLUMNS) |
Comment on lines
+86
to
+87
| Display.setFrame(static_cast<std::span<uint8_t, GRID_COLUMNS * GRID_ROWS>>( | ||
| std::span(packet.data(), packet.length()).subspan(18U))); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 45 changed files in this pull request and generated 4 comments.
Suppressed comments (6)
firmware/src/services/ModesService.cpp:347
lines.size() - 1Ucan underflow whenlinesis empty (e.g., if the firstTextHandlerheight is>= GRID_ROWS, the loop breaks before pushing any lines). That will make theycalculation wrap and can lead to drawing with an invalid starting Y position.
firmware/src/modes/StreamMode.cpp:97data.front()is used without guarding against zero-length packets, and the same dynamic-to-fixedstd::spancast issue appears here twice. This can be simplified by computing the payload offset and constructing a fixed-extent span frompacket.data() + offsetwhen the length matches.
firmware/src/modes/StreamMode.cpp:113- Same dynamic-extent to fixed-extent
std::spancast issue as inonArtNet(): construct the fixed-extent span directly from pointer+count.
firmware/src/services/DisplayService.cpp:60 configure()callssetOrientation(Orientation::deg0)on every boot when the orientation is default, which persists to NVS and can cause unnecessary flash wear. You can initializepixelsMapped(and setrenderso the splash flushes) without writing the orientation back to NVS.
firmware/include/services/DisplayService.h:79mapPixeltakesuint8_t logical, but the code has an explicit#elsebranch for displays withGRID_COLUMNS * GRID_ROWS > 256. In that configuration,setOrientation()iterateslogicalbeyond 255, and passing it tomapPixel(uint8_t, …)will wrap and write the mapping into the wrong slots (leaving most ofpixelsMappeduninitialized).
void mapPixel(uint8_t logical, uint8_t physical);
firmware/src/modes/StreamMode.cpp:41
begin()starts listening before validatingport. If NVS contains an unexpected value, thedefault:path returns without registering a handler, leaving the UDP socket open but effectively unmanaged.
| pending = true; | ||
| } | ||
|
|
||
| void DisplayService::mapPixel(uint8_t logical, uint8_t physical) |
Comment on lines
+84
to
+88
| if (packet.length() == 18U + GRID_COLUMNS * GRID_ROWS) | ||
| { | ||
| Display.setFrame(static_cast<std::span<const uint8_t, GRID_COLUMNS * GRID_ROWS>>( | ||
| std::span(packet.data(), packet.length()).subspan(18U))); | ||
| } |
Comment on lines
+304
to
307
| uint8_t DisplayService::getPixel(uint8_t x, uint8_t y) const | ||
| { | ||
| for (size_t idx{0U}; idx < frame.size(); ++idx) | ||
| { | ||
| _frame[idx] = frame[pixels[idx]]; | ||
| } | ||
| return frame[static_cast<size_t>(x + (y * GRID_COLUMNS))]; | ||
| } |
|
|
||
| void fillColumn(uint8_t x, uint8_t _brightness); | ||
|
|
||
| void fillFrame(uint8_t _brightness); |
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
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.
Refactor rendering logic to enhance performance and reduce complexity across various modes.
Key changes
Impact
These changes optimize rendering performance, potentially improving responsiveness and reducing resource usage. The refactor also lays the groundwork for future enhancements and easier maintenance.