Add support for 18 bit SPI graphics (ILI9488)#3288
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded an 18-bit windowed display transfer path, routed Changes18-bit display transfer path
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DisplayDriver
participant DisplayInterface
participant SPI Buffer/Hardware
DisplayDriver->>DisplayInterface: SendData18Windowed(data, startX, startY, width, height, stride)
DisplayInterface->>DisplayInterface: Traverse window rows and pixels
DisplayInterface->>DisplayInterface: Expand RGB565 pixels into B, G, R bytes
DisplayInterface->>SPI Buffer/Hardware: InternalSendBytes(..., async=true)
DisplayInterface->>SPI Buffer/Hardware: SwapBuffers()
DisplayInterface->>SPI Buffer/Hardware: Flush remaining bytes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
mikmog
force-pushed
the
add-18-bit-spi-graphics
branch
from
6e8800a to
0335505
Compare
|
@Ellerbach Can you take a look when time suits? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nanoFramework.Graphics/Graphics/Displays/Generic_SPI.cpp`:
- Around line 268-271: The code uses DisplayInterface::FillData16(0,
Attributes.Width * Attributes.Height * 3 / 2) which truncates for odd pixel
counts and loses the final RGB666 byte; switch this branch to a byte-oriented
clear path (call the byte-based method such as FillData8 or the DisplayInterface
byte-fill routine) and send Attributes.Width * Attributes.Height * 3 bytes so
the full 3 bytes per pixel are transmitted for RGB666 panels; update the branch
in Generic_SPI.cpp (the 3-bytes-per-pixel branch) to use the byte-fill API
instead of FillData16.
In `@src/nanoFramework.Graphics/Graphics/Displays/Spi_To_Display.cpp`:
- Around line 341-352: The code currently appends three bytes (b,g,r) into
byteBuffer then checks capacity, which can overflow when bytesWritten is near
SPI_MAX_TRANSFER_SIZE; update the logic in the method using
byteBuffer/bytesWritten (the pixel write loop) to check if bytesWritten + 3 >
SPI_MAX_TRANSFER_SIZE and call InternalSendBytes((CLR_UINT8*)currentBuffer,
bytesWritten, true) followed by SwapBuffers() and resetting byteBuffer and
bytesWritten before writing the next pixel bytes, so the staging buffer is
flushed preemptively rather than after the three-byte write.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f550caf5-1d32-4448-bd9e-ad994aa1a70d
📒 Files selected for processing (3)
src/nanoFramework.Graphics/Graphics/Displays/DisplayInterface.hsrc/nanoFramework.Graphics/Graphics/Displays/Generic_SPI.cppsrc/nanoFramework.Graphics/Graphics/Displays/Spi_To_Display.cpp
|
@mikmog, you still need the managed nuget library for this Generic SPI for ILI9488 to work. |
|
Yeah. The plan was if there is any hope for this PR moving forward I'll also add the PR for managed side. |
If you have the PR for the nuget then I can test it. |
Added PR for Ili9488 nanoframework/nanoFramework.Graphics#180 |
|
@mikmog thanks for both PR. See the comment from Coderabbit about the bits alignment. |
There was a problem hiding this comment.
Pull request overview
Adds 18-bit (RGB666 over SPI) transfer support so ILI9488-class displays can be driven correctly from the graphics stack.
Changes:
- Introduces a new
DisplayInterface::SendData18Windowed()path that expands RGB565 pixels to 3-byte transfers. - Updates the Generic SPI display driver to choose 16-bit vs 18-bit transfer/clear behavior based on
Attributes.BitsPerPixel. - Extends
DisplayInterfaceAPI surface with the new 18-bit windowed send method.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/nanoFramework.Graphics/Graphics/Displays/Spi_To_Display.cpp |
Adds 18-bit windowed pixel send routine built on the existing SPI transfer buffers. |
src/nanoFramework.Graphics/Graphics/Displays/Generic_SPI.cpp |
Switches Clear/BitBlt to use 16-bit or 18-bit transfer logic depending on BitsPerPixel. |
src/nanoFramework.Graphics/Graphics/Displays/DisplayInterface.h |
Declares the new SendData18Windowed API. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nanoFramework.Graphics/Graphics/Displays/Spi_To_Display.cpp`:
- Around line 341-353: The code currently writes r/g/b into byteBuffer using
byteBuffer[bytesWritten++] before checking capacity, causing a one-byte overrun
when bytesWritten is near SPI_MAX_TRANSFER_SIZE; move the capacity check so it
runs before writing the three bytes: ensure you test if (bytesWritten + 3 >
SPI_MAX_TRANSFER_SIZE) and call InternalSendBytes((CLR_UINT8*)currentBuffer,
bytesWritten, true); SwapBuffers(); byteBuffer = (CLR_UINT8*)currentBuffer;
bytesWritten = 0; before performing the three writes to byteBuffer, preserving
use of bytesWritten, SPI_MAX_TRANSFER_SIZE, InternalSendBytes and SwapBuffers
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48e008da-86f5-4d63-ac48-3f7f3771041c
📒 Files selected for processing (3)
src/nanoFramework.Graphics/Graphics/Displays/DisplayInterface.hsrc/nanoFramework.Graphics/Graphics/Displays/Generic_SPI.cppsrc/nanoFramework.Graphics/Graphics/Displays/Spi_To_Display.cpp
|
@mikmog any progress on this? It's sooo close... |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/nanoFramework.Graphics/Graphics/Displays/Generic_SPI.cpp (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAvoid introducing a UTF-8 BOM on the header line.
Same as in
Spi_To_Display.cpp: the leading BOM before the//copyright header should be removed so the file starts with the required header text and stays consistent with the codebase encoding. Re-save as UTF-8 without BOM.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nanoFramework.Graphics/Graphics/Displays/Generic_SPI.cpp` at line 1, Remove the leading UTF-8 BOM from the file so the header starts with the expected comment text instead of an invisible prefix. Re-save the file as UTF-8 without BOM and verify the first line in Generic_SPI.cpp matches the same header style used by Spi_To_Display.cpp.Source: Coding guidelines
src/nanoFramework.Graphics/Graphics/Displays/Spi_To_Display.cpp (2)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAvoid introducing a UTF-8 BOM on the header line.
The change prepends a UTF-8 BOM before the
//copyright header. This makes the file no longer begin with the required copyright header text, is inconsistent with the rest of the sources, and can trip up some toolchains/diffs. Please re-save the file as UTF-8 without BOM.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nanoFramework.Graphics/Graphics/Displays/Spi_To_Display.cpp` at line 1, The file currently starts with a UTF-8 BOM before the copyright header, so resave Spi_To_Display.cpp as UTF-8 without BOM to keep the leading // header as the first bytes in the file; make sure the file content begins directly with the existing header text and no hidden BOM is introduced.Source: Coding guidelines
344-364: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftGuard async writes before swapping buffers.
src/nanoFramework.Graphics/Graphics/Displays/Spi_To_Display.cpp:344-364queues the SPI transfer and immediately flips to the other staging buffer with no completion barrier. The same pattern also exists inFlushData(), so both paths need to wait for the previous transfer to finish before either buffer is reused.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nanoFramework.Graphics/Graphics/Displays/Spi_To_Display.cpp` around lines 344 - 364, The SPI write path in Spi_To_Display::FlushData and the related buffer-flush logic swaps staging buffers immediately after InternalSendBytes, so the buffer can be reused before the async transfer completes. Add a completion barrier or wait for the pending transfer to finish before calling SwapBuffers or reassigning byteBuffer/currentBuffer, and apply the same fix in both the inline flush path and FlushData so neither path can overwrite an in-flight buffer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/nanoFramework.Graphics/Graphics/Displays/Generic_SPI.cpp`:
- Line 1: Remove the leading UTF-8 BOM from the file so the header starts with
the expected comment text instead of an invisible prefix. Re-save the file as
UTF-8 without BOM and verify the first line in Generic_SPI.cpp matches the same
header style used by Spi_To_Display.cpp.
In `@src/nanoFramework.Graphics/Graphics/Displays/Spi_To_Display.cpp`:
- Line 1: The file currently starts with a UTF-8 BOM before the copyright
header, so resave Spi_To_Display.cpp as UTF-8 without BOM to keep the leading //
header as the first bytes in the file; make sure the file content begins
directly with the existing header text and no hidden BOM is introduced.
- Around line 344-364: The SPI write path in Spi_To_Display::FlushData and the
related buffer-flush logic swaps staging buffers immediately after
InternalSendBytes, so the buffer can be reused before the async transfer
completes. Add a completion barrier or wait for the pending transfer to finish
before calling SwapBuffers or reassigning byteBuffer/currentBuffer, and apply
the same fix in both the inline flush path and FlushData so neither path can
overwrite an in-flight buffer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6bca0aa-4c73-4715-beab-50c5e06ecc6f
📒 Files selected for processing (2)
src/nanoFramework.Graphics/Graphics/Displays/Generic_SPI.cppsrc/nanoFramework.Graphics/Graphics/Displays/Spi_To_Display.cpp
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nanoFramework.Graphics/Graphics/Displays/Spi_To_Display.cpp (1)
310-357: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMissing full-stride fast path for full-width windows.
SendData16Windowed(Lines 291-294) optimizes the common case wherewidth == strideby sending the whole buffer in one call, avoiding the per-line loop overhead.SendData18Windowedalways iterates per-line and per-pixel even when the window spans the full stride, adding unnecessary loop/branch overhead to an already higher-bandwidth (3 bytes/pixel) path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nanoFramework.Graphics/Graphics/Displays/Spi_To_Display.cpp` around lines 310 - 357, `SendData18Windowed` currently always walks the window line-by-line and pixel-by-pixel, even when `width == stride`, unlike the optimized full-width path used in `SendData16Windowed`. Add a full-stride fast path in `SendData18Windowed` that detects the common full-width case and sends the buffer more directly, while keeping the existing windowed per-line logic as the fallback. Use the `SendData18Windowed` and `SendData16Windowed` implementations as the reference points when updating the branching and transfer flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/nanoFramework.Graphics/Graphics/Displays/Spi_To_Display.cpp`:
- Around line 310-357: `SendData18Windowed` currently always walks the window
line-by-line and pixel-by-pixel, even when `width == stride`, unlike the
optimized full-width path used in `SendData16Windowed`. Add a full-stride fast
path in `SendData18Windowed` that detects the common full-width case and sends
the buffer more directly, while keeping the existing windowed per-line logic as
the fallback. Use the `SendData18Windowed` and `SendData16Windowed`
implementations as the reference points when updating the branching and transfer
flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: f6e174a5-979c-40cf-8c19-e08ba77a6604
📒 Files selected for processing (3)
src/nanoFramework.Graphics/Graphics/Displays/DisplayInterface.hsrc/nanoFramework.Graphics/Graphics/Displays/Generic_SPI.cppsrc/nanoFramework.Graphics/Graphics/Displays/Spi_To_Display.cpp
Description
Add support for the Ili9488 display.
Motivation and Context
Inspired by PR #2976
Second attempt at adding 18-bit support for the Ili9488 display.
Managed driver in separate PR if applicable.
How Has This Been Tested?
On an ESP32_S3 Feather and a Ili9488 RGB display.
Performance wise it's ok and colors renders as expected.
Screenshots
Types of changes
Checklist