Add Ili9488 display driver#180
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new managed Ili9488 graphics driver at 480×320 and wires it into the project, NuGet packaging, Azure pipeline, and solution configuration. ChangesIli9488 driver addition
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels
🚥 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 |
coderabbitai
Bot
added
Area: Config-and-Build
and removed
Type: enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ManagedDrivers/Ili9488/Ili9488.cs`:
- Around line 74-77: The XML doc comment for the static property Width contains
a typo ("weight" instead of "width"); update the <summary> text that precedes
the public static uint Width { get; } = 480; so it reads "Default width. Use to
override the one you'll pass in the screen and add it to the driver." (i.e.,
replace "weight" with "width") to accurately describe the Width property.
In `@ManagedDrivers/Ili9488/Properties/AssemblyInfo.cs`:
- Line 9: The AssemblyProduct attribute in AssemblyInfo.cs is incorrect
(copy-paste); update the AssemblyProduct attribute value so it matches this
assembly's product name by replacing the current "nanoFramework.System.Text"
string in the [assembly: AssemblyProduct(...)] attribute with the correct
product name for the ManagedDrivers.Ili9488 assembly (edit the AssemblyProduct
attribute in AssemblyInfo.cs to the proper product identifier).
In `@nanoFramework.Graphics.Ili9488.nuspec`:
- Around line 41-45: The nuspec <file> entries currently point to
ManagedDrivers\Ili9342\bin\Release\... which is wrong for the Ili9488 package;
update each <file
src="ManagedDrivers\Ili9342\bin\Release\nanoFramework.Graphics.Ili9488.*"> entry
in nanoFramework.Graphics.Ili9488.nuspec to use
ManagedDrivers\Ili9488\bin\Release\... (for the .dll, .pdb, .pdbx, .pe and .xml
entries) so the package pulls artifacts from the Ili9488 build output directory.
🪄 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: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f336b83e-c39f-437d-a4b2-64028e637ab4
📒 Files selected for processing (8)
ManagedDrivers/Ili9488/Ili9488.csManagedDrivers/Ili9488/Properties/AssemblyInfo.csManagedDrivers/Ili9488/nanoFramework.Graphics.Ili9488.nfprojManagedDrivers/Ili9488/packages.configManagedDrivers/Ili9488/packages.lock.jsonazure-pipelines.ymlnanoFramework.Graphics.Ili9488.nuspecnanoFramework.Graphics.sln
|
Blocked by nanoframework/nf-interpreter#3288 |
coderabbitai
Bot
added
Type: enhancement
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ManagedDrivers/Ili9488/Properties/AssemblyInfo.cs (1)
9-9:⚠️ Potential issue | 🟡 MinorFix incorrect assembly product metadata.
Line 9 still points to an unrelated product name and should match this assembly/package identity (
nanoFramework.Graphics.Ili9488).Proposed fix
-[assembly: AssemblyProduct("nanoFramework.System.Text")] +[assembly: AssemblyProduct("nanoFramework.Graphics.Ili9488")]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ManagedDrivers/Ili9488/Properties/AssemblyInfo.cs` at line 9, The AssemblyProduct attribute value is incorrect: update the AssemblyProduct assembly attribute (the [assembly: AssemblyProduct(...)] entry) so it reflects the correct package identity; replace "nanoFramework.System.Text" with "nanoFramework.Graphics.Ili9488" in the AssemblyInfo.cs AssemblyProduct attribute to align the assembly metadata with the package.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ManagedDrivers/Ili9488/Ili9488.cs`:
- Around line 103-104: The Memory_Access_Control payloads are hard-coding MADCTL
RGB (0x00) and never set MADCTL_BGR, so the driver cannot honor a BGR override;
update all uses of GraphicDriverCommandType.Command with
ILI9488_CMD.Memory_Access_Control (and the orientation setup code around lines
using the same command) to select the MADCTL byte based on a configuration flag
(e.g., a BgrOverride or ColorOrder property) and set the
MADCTL_MX/MADCTL_MY/MADCTL_MV bits as before while OR-ing in the MADCTL_BGR bit
when the flag is true; ensure every orientation payload path that currently uses
0x40 or similar combines the orientation mask with the conditional BGR bit so
both default RGB and BGR override are reachable.
---
Duplicate comments:
In `@ManagedDrivers/Ili9488/Properties/AssemblyInfo.cs`:
- Line 9: The AssemblyProduct attribute value is incorrect: update the
AssemblyProduct assembly attribute (the [assembly: AssemblyProduct(...)] entry)
so it reflects the correct package identity; replace "nanoFramework.System.Text"
with "nanoFramework.Graphics.Ili9488" in the AssemblyInfo.cs AssemblyProduct
attribute to align the assembly metadata with the package.
🪄 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: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: abb7bdf3-32ad-4157-b042-8f066510dae1
📒 Files selected for processing (8)
ManagedDrivers/Ili9488/Ili9488.csManagedDrivers/Ili9488/Properties/AssemblyInfo.csManagedDrivers/Ili9488/nanoFramework.Graphics.Ili9488.nfprojManagedDrivers/Ili9488/packages.configManagedDrivers/Ili9488/packages.lock.jsonazure-pipelines.ymlnanoFramework.Graphics.Ili9488.nuspecnanoFramework.Graphics.sln
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
||
| private enum ILI9488_Orientation | ||
| { | ||
| MADCTL_MH = 0x04, // sets the Horizontal Refresh, 0=Left-Right and 1=Right-Left |
There was a problem hiding this comment.
Better move all these as proper IntelliSense comments
There was a problem hiding this comment.
This is more or less a copy paste of Ili9342 driver. Tried to keep them as identical as possible. Prepared for future refactoring. Note the enum is private and defined like this in ~all of the drivers.
Given the similarities of the Ili9xxx drivers they can probably live in the same nuget and share the enums for commands and orientation. But that's out of scope in current pr.
| @@ -0,0 +1,56 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
Missing here the nbgv props and targets. Also in the reference in the packages.lock file.
I can see these are missing on the other drivers too. I'm submitting a PR with a fix for those.
There was a problem hiding this comment.
If you are editing feel free to include this pr, Otherwise I'll do it
josesimoes
left a comment
josesimoes
left a comment
There was a problem hiding this comment.
You're on the right track!
Minor stuff only.
And everything OK with the nuspec.
Co-authored-by: José Simões <jose.simoes@eclo.solutions>
coderabbitai
Bot
added
Area: Config-and-Build
and removed
Type: enhancement
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ManagedDrivers/Ili9488/Properties/AssemblyInfo.cs (1)
9-9:⚠️ Potential issue | 🟡 MinorFix
AssemblyProductmetadata mismatch at Line 9.
AssemblyProductstill points to an unrelated product (nanoFramework.System.Text), which is inconsistent with this assembly/package identity (nanoFramework.Graphics.Ili9488).Proposed fix
-[assembly: AssemblyProduct("nanoFramework.System.Text")] +[assembly: AssemblyProduct("nanoFramework.Graphics.Ili9488")]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ManagedDrivers/Ili9488/Properties/AssemblyInfo.cs` at line 9, The AssemblyProduct attribute value is incorrect: update the assembly metadata by changing the AssemblyProduct attribute (the line containing [assembly: AssemblyProduct("nanoFramework.System.Text")]) to the correct product name for this assembly (e.g., "nanoFramework.Graphics.Ili9488") so the metadata matches the package/assembly identity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ManagedDrivers/Ili9488/Properties/AssemblyInfo.cs`:
- Line 9: The AssemblyProduct attribute value is incorrect: update the assembly
metadata by changing the AssemblyProduct attribute (the line containing
[assembly: AssemblyProduct("nanoFramework.System.Text")]) to the correct product
name for this assembly (e.g., "nanoFramework.Graphics.Ili9488") so the metadata
matches the package/assembly identity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea30b083-f666-4161-a7b6-0b00842a6221
📒 Files selected for processing (1)
ManagedDrivers/Ili9488/Properties/AssemblyInfo.cs
coderabbitai
Bot
added
Type: enhancement
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ManagedDrivers/Ili9488/Ili9488.cs (1)
119-152:⚠️ Potential issue | 🟠 MajorAdd an actual BGR override path for MADCTL composition.
Line 121, Line 131, Line 139, and Line 147 hard-code
MADCTL_RGB, soMADCTL_BGR(Line 70) is never reachable. This contradicts the PR behavior claim (RGB default with BGR override).Proposed fix
public static class Ili9488 { private static GraphicDriver _driver; + private static ILI9488_Orientation _colorOrder = ILI9488_Orientation.MADCTL_RGB; + + public static void SetColorOrderBgr(bool useBgr) + { + _colorOrder = useBgr ? ILI9488_Orientation.MADCTL_BGR : ILI9488_Orientation.MADCTL_RGB; + _driver = null; + } @@ OrientationLandscape = new byte[] { (byte)GraphicDriverCommandType.Command, 2, (byte)ILI9488_CMD.Memory_Access_Control, (byte)( ILI9488_Orientation.MADCTL_MX | ILI9488_Orientation.MADCTL_MY | ILI9488_Orientation.MADCTL_MV | - ILI9488_Orientation.MADCTL_RGB + _colorOrder ), }, @@ ILI9488_Orientation.MADCTL_MV | - ILI9488_Orientation.MADCTL_RGB + _colorOrder ), }, @@ ILI9488_Orientation.MADCTL_MX | - ILI9488_Orientation.MADCTL_RGB + _colorOrder ), }, @@ ILI9488_Orientation.MADCTL_MY | - ILI9488_Orientation.MADCTL_RGB + _colorOrder ), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ManagedDrivers/Ili9488/Ili9488.cs` around lines 119 - 152, The MADCTL bytes in OrientationLandscape, OrientationLandscape180, OrientationPortrait, and OrientationPortrait180 are hard-coded with MADCTL_RGB so MADCTL_BGR can never be selected; change these initializers to compute the MADCTL byte using a single composed value (e.g., var madctl = ILI9488_Orientation.MADCTL_MX | ... ) and include ILI9488_Orientation.MADCTL_BGR instead of MADCTL_RGB when the BGR override flag is set (use an existing config/flag or add a static/useBgr boolean) so the arrays use the correct MADCTL constant at runtime; update the array constructions in Ili9488 (the Orientation* byte[] initializers) to reference that composed madctl value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ManagedDrivers/Ili9488/Ili9488.cs`:
- Around line 119-152: The MADCTL bytes in OrientationLandscape,
OrientationLandscape180, OrientationPortrait, and OrientationPortrait180 are
hard-coded with MADCTL_RGB so MADCTL_BGR can never be selected; change these
initializers to compute the MADCTL byte using a single composed value (e.g., var
madctl = ILI9488_Orientation.MADCTL_MX | ... ) and include
ILI9488_Orientation.MADCTL_BGR instead of MADCTL_RGB when the BGR override flag
is set (use an existing config/flag or add a static/useBgr boolean) so the
arrays use the correct MADCTL constant at runtime; update the array
constructions in Ili9488 (the Orientation* byte[] initializers) to reference
that composed madctl value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8fbfafbb-e69d-480c-baf7-1c53032632a8
📒 Files selected for processing (1)
ManagedDrivers/Ili9488/Ili9488.cs
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Ellerbach
left a comment
Ellerbach
left a comment
There was a problem hiding this comment.
it looks good to me! And thanks for the native addition for the 18 bits. Check the comments there.
There was a problem hiding this comment.
Pull request overview
Adds a new managed (generic GraphicDriver-based) display driver package for the ILI9488 panel to the nanoFramework.Graphics ecosystem, including project/packaging and CI integration.
Changes:
- Adds
ManagedDrivers/Ili9488driver implementation (Ili9488.cs) plus a new nanoFramework project (.nfproj) and NuGet packaging assets. - Registers the new driver project and nuspec in
nanoFramework.Graphics.sln. - Adds a new package build step to
azure-pipelines.ymlfornanoFramework.Graphics.Ili9488.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| nanoFramework.Graphics.sln | Adds the ILI9488 managed driver project and nuspec to the solution and build configurations. |
| nanoFramework.Graphics.Ili9488.nuspec | Introduces a new NuGet spec to ship the ILI9488 managed driver alongside the Graphics assemblies. |
| ManagedDrivers/Ili9488/Ili9488.cs | Implements the managed ILI9488 initialization/orientation command sequences via GraphicDriver. |
| ManagedDrivers/Ili9488/nanoFramework.Graphics.Ili9488.nfproj | Adds a new nanoFramework library project for building the driver assembly. |
| ManagedDrivers/Ili9488/Properties/AssemblyInfo.cs | Adds assembly metadata for the new driver assembly. |
| ManagedDrivers/Ili9488/packages.config | Declares nanoFramework.CoreLibrary dependency for the new driver project. |
| ManagedDrivers/Ili9488/packages.lock.json | Locks the restored dependency version/hash for reproducible restores. |
| azure-pipelines.yml | Adds CI packaging/build entry for the new nanoFramework.Graphics.Ili9488 package. |
| (byte)GraphicDriverCommandType.Command, 3, (byte)ILI9488_CMD.Power_Control_1, 0x17, 0x15, | ||
| (byte)GraphicDriverCommandType.Command, 2, (byte)ILI9488_CMD.Power_Control_2, 0x41, | ||
| (byte)GraphicDriverCommandType.Command, 4, (byte)ILI9488_CMD.VCOM_Control_1, 0x00, 0x12, 0x80, | ||
| (byte)GraphicDriverCommandType.Command, 2, (byte)ILI9488_CMD.Pixel_Format_Set, 0x66, // 18 bit SPI |
There was a problem hiding this comment.
The initialization config sets Pixel_Format_Set to 0x66 (18-bit) and the driver later reports BitsPerPixel=18. Most existing managed drivers configure 16-bit RGB565 (0x55), and DisplayControl.BitsPerPixel is documented as “Currently 16 bits in RGB565 format” (nanoFramework.Graphics/Primitive/DisplayControl.cs). Unless the native generic driver explicitly supports 18-bit transfers (including pixel packing/conversion), this mismatch can break rendering (misaligned pixel data). Consider switching to 16-bit mode (0x55) + BitsPerPixel=16, or clearly documenting/ensuring the required 18-bit pixel pipeline end-to-end.
| (byte)GraphicDriverCommandType.Command, 2, (byte)ILI9488_CMD.Pixel_Format_Set, 0x66, // 18 bit SPI | |
| (byte)GraphicDriverCommandType.Command, 2, (byte)ILI9488_CMD.Pixel_Format_Set, 0x55, // 16 bit RGB565 SPI |
| OrientationLandscape = new byte[] | ||
| { | ||
| (byte)GraphicDriverCommandType.Command, 2, (byte)ILI9488_CMD.Memory_Access_Control, | ||
| (byte)( | ||
| ILI9488_Orientation.MADCTL_MX | | ||
| ILI9488_Orientation.MADCTL_MY | | ||
| ILI9488_Orientation.MADCTL_MV | | ||
| ILI9488_Orientation.MADCTL_RGB | ||
| ), | ||
| }, | ||
| OrientationLandscape180 = new byte[] | ||
| { | ||
| (byte)GraphicDriverCommandType.Command, 2, (byte)ILI9488_CMD.Memory_Access_Control, | ||
| (byte)( | ||
| ILI9488_Orientation.MADCTL_MV | | ||
| ILI9488_Orientation.MADCTL_RGB | ||
| ), | ||
| }, | ||
| OrientationPortrait = new byte[] | ||
| { | ||
| (byte)GraphicDriverCommandType.Command, 2, (byte)ILI9488_CMD.Memory_Access_Control, | ||
| (byte)( | ||
| ILI9488_Orientation.MADCTL_MX | | ||
| ILI9488_Orientation.MADCTL_RGB | ||
| ), | ||
| }, | ||
| OrientationPortrait180 = new byte[] | ||
| { | ||
| (byte)GraphicDriverCommandType.Command, 2, (byte)ILI9488_CMD.Memory_Access_Control, | ||
| (byte)( | ||
| ILI9488_Orientation.MADCTL_MY | | ||
| ILI9488_Orientation.MADCTL_RGB | ||
| ), | ||
| }, |
There was a problem hiding this comment.
All orientation command sequences hardcode MADCTL_RGB. The PR description mentions the default is RGB but that it “can be overridden to BGR”; with the current implementation, consumers can only do that by manually editing the returned byte arrays, and the MADCTL enum is private so they can’t reuse the named constant. Consider exposing an explicit/public way to get an alternate BGR-configured GraphicDriver (or a switch/parameter) so the advertised override is straightforward and less error-prone.
There was a problem hiding this comment.
I've removed "can be overridden to BGR" in PR description. This is now streamlined with other drivers.
Suggest a refactoring in a different PR to allow defining drivers and reusing enums and constants
|
@mikmog any plans to get back to this and wrap it up? Missing some review comments to address... |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ManagedDrivers/Ili9488/Ili9488.cs (1)
145-178: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBGR override still unimplemented — contradicts PR description.
All four orientation payloads (
OrientationLandscape,OrientationLandscape180,OrientationPortrait,OrientationPortrait180) unconditionally OR inILI9488_Orientation.MADCTL_RGB.MADCTL_BGRis defined but never referenced, so there is no way to actually select BGR mode, despite the PR description advertising "support for overriding to BGR in the driver." This was already flagged in a previous review round and remains unaddressed.🎨 Proposed fix (expose a color-order override)
public static class Ili9488 { private static GraphicDriver _driver; + private static ILI9488_Orientation _colorOrder = ILI9488_Orientation.MADCTL_RGB; + + /// <summary> + /// Overrides the default RGB pixel order with BGR. + /// </summary> + public static void SetColorOrderBgr(bool useBgr) + { + _colorOrder = useBgr ? ILI9488_Orientation.MADCTL_BGR : ILI9488_Orientation.MADCTL_RGB; + _driver = null; + }And replace each
ILI9488_Orientation.MADCTL_RGBin the orientation arrays (lines 152, 160, 168, 176) with_colorOrder.🤖 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 `@ManagedDrivers/Ili9488/Ili9488.cs` around lines 145 - 178, The four orientation payloads in Ili9488 currently hardcode ILI9488_Orientation.MADCTL_RGB, so the driver cannot actually switch to BGR mode as described. Introduce a reusable color-order field or property in Ili9488 and have the orientation setup use that value instead of always OR’ing RGB. Update OrientationLandscape, OrientationLandscape180, OrientationPortrait, and OrientationPortrait180 to reference the shared color-order setting so MADCTL_BGR can be selected.
🤖 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.
Inline comments:
In `@ManagedDrivers/Ili9488/nanoFramework.Graphics.Ili9488.nfproj`:
- Line 3: The Nerdbank.GitVersioning imports in
nanoFramework.Graphics.Ili9488.nfproj are using mismatched Exists conditions, so
the props/targets can be skipped even when the package is present. Update the
Import conditions for the Nerdbank.GitVersioning.props and
Nerdbank.GitVersioning.targets entries to use the same ..\..\packages\... path
as their Project values, so both imports resolve consistently.
---
Duplicate comments:
In `@ManagedDrivers/Ili9488/Ili9488.cs`:
- Around line 145-178: The four orientation payloads in Ili9488 currently
hardcode ILI9488_Orientation.MADCTL_RGB, so the driver cannot actually switch to
BGR mode as described. Introduce a reusable color-order field or property in
Ili9488 and have the orientation setup use that value instead of always OR’ing
RGB. Update OrientationLandscape, OrientationLandscape180, OrientationPortrait,
and OrientationPortrait180 to reference the shared color-order setting so
MADCTL_BGR can be selected.
🪄 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: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ce119ee-9e45-4b47-a2f5-3ea835e02ee0
📒 Files selected for processing (5)
ManagedDrivers/Ili9488/Ili9488.csManagedDrivers/Ili9488/Properties/AssemblyInfo.csManagedDrivers/Ili9488/nanoFramework.Graphics.Ili9488.nfprojManagedDrivers/Ili9488/packages.configManagedDrivers/Ili9488/packages.lock.json
Description
Add driver for the Ili9488 display.
Motivation and Context
Adds managed driver for the Ili9488 display.
How Has This Been Tested?
On an ESP32_S3 Feather and a Ili9488 RGB display.
Screenshots
Types of changes
Checklist: