Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new managed Ili9488 graphics driver (480×320) to nanoFramework.UI.GraphicDrivers plus project files, packaging, solution integration, and a CI packaging step. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 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 |
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
| // associated with an assembly. | ||
| [assembly: AssemblyTitle("nanoFramework.Graphics.Ili9488")] | ||
| [assembly: AssemblyCompany("nanoFramework Contributors")] | ||
| [assembly: AssemblyProduct("nanoFramework.System.Text")] |
There was a problem hiding this comment.
Incorrect AssemblyProduct value - copy-paste error.
The AssemblyProduct attribute references an unrelated product. It should match this assembly's product name.
Proposed fix
-[assembly: AssemblyProduct("nanoFramework.System.Text")]
+[assembly: AssemblyProduct("nanoFramework.Graphics.Ili9488")]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [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 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).
|
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.
Description
Add driver for the Ili9488 display.
Motivation and Context
Adds managed driver for the Ili9488 display.
Default color mode is RGB but can be overridden to BGR in driver
How Has This Been Tested?
On an ESP32_S3 Feather and a Ili9488 RGB display.
Screenshots
Types of changes
Checklist: