Add support for 18 bit SPI graphics (ILI9488)#3288
Add support for 18 bit SPI graphics (ILI9488)#3288mikmog wants to merge 5 commits intonanoframework:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 58 minutes and 27 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded support for windowed 18-bit transfers by introducing Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Driver as DisplayDriver
participant Interface as DisplayInterface
participant SPI as SPI Buffer/Hardware
App->>Driver: BitBlt(data, startX, startY, width, height)
Driver->>Driver: if Attributes.BitsPerPixel > 16
alt >16 bpp
Driver->>Interface: SendData18Windowed(data, startX, startY, width, height, stride)
Interface->>Interface: for each row:
Interface->>Interface: calculate startOfLine = data + row*stride + startX
loop per-pixel
Interface->>Interface: extract R/G/B from RGB565 and expand to 8-bit bytes
Interface->>SPI: append 3 bytes (B,G,R) to currentBuffer
alt buffer would exceed SPI_MAX_TRANSFER_SIZE
Interface->>SPI: InternalSendBytes(currentBuffer, async=true)
Interface->>SPI: SwapBuffers()
end
end
Interface->>SPI: flush remaining bytes
Interface->>SPI: SwapBuffers()
else <=16 bpp
Driver->>Interface: SendData16Windowed(..., doByteSwap)
end
Interface-->>Driver: Transfer complete
Driver-->>App: BitBlt finished
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
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