Adding M5Stack CoreS3#430
Merged
Merged
Conversation
nfbot
added
Type: enhancement
|
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 the nanoFramework.M5CoreS3 library, CoreS3 display support, a CoreS3 test app, and matching project, packaging, CI, solution, and README updates. ChangesM5Stack CoreS3 Board Support Package
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 `@nanoFramework.M5CoreS3.nuspec`:
- Around line 20-34: The dependencies section in the nuspec file is missing
transitive packages and contains outdated versions. Add the missing dependencies
nanoFramework.System.Buffers.Binary.BinaryPrimitives and
nanoFramework.Iot.Device.Common.NumberHelper to the dependencies element, and
update all existing dependency versions to match the versions used in the actual
build (for example, update nanoFramework.CoreLibrary from 1.17.11 to 1.17.12,
nanoFramework.Iot.Device.Axp2101 from 1.0.3 to 1.0.33, and verify all other
versions against the assembly's actual references). This ensures consumers
restore the same dependency graph that was tested.
In `@nanoFramework.M5CoreS3/CoreS3Ili9342.cs`:
- Around line 99-106: The PowerModeNormal property incorrectly uses
ILI9342Command.PowerState (0x10) which enters sleep mode instead of resuming
normal operation. In the PowerModeNormal byte array definition, replace
(byte)ILI9342Command.PowerState with (byte)ILI9342Command.SleepOut to correctly
exit sleep mode and resume display operation, consistent with the SleepOut
command used during initialization.
In `@nanoFramework.M5CoreS3/M5CoreS3.cs`:
- Around line 196-208: The Magnetometer property getter creates a second
I2cDevice instance at Bmi270AccelerometerGyroscope.SecondaryI2cAddress, which is
the same address already opened by the AccelerometerGyroscope property, leading
to poor practice and potential I2C bus contention. Either refactor the code to
reuse the existing I2cDevice from AccelerometerGyroscope instead of creating a
new one in the Magnetometer getter, or add a clear comment documenting why
maintaining two separate I2cDevice instances for the same hardware address is
intentional. Additionally, the unused variable assignment `var imu =
AccelerometerGyroscope;` on line 202 appears to exist only for lazy
initialization triggering—either remove it or replace it with a more explicit
call to AccelerometerGyroscope property and add a comment explaining the lazy
initialization intent.
In `@nanoFramework.M5CoreS3/packages.config`:
- Around line 3-36: The packages.lock.json lockfile is out of sync with
packages.config and is missing entries for nanoFramework.System.Device.Adc and
nanoFramework.System.IO.Ports packages. Run the nuget restore command to
regenerate the packages.lock.json file with the complete package graph that
includes all packages currently defined in packages.config. This will ensure all
dependencies are properly tracked and locked in the lockfile.
In `@nanoFramework.M5CoreS3/packages.lock.json`:
- Around line 1-199: The packages.lock.json file is missing two declared
dependencies: nanoFramework.System.Device.Adc (1.1.24) and
nanoFramework.System.IO.Ports (1.1.132). Regenerate the packages.lock.json
lockfile by running a NuGet restore command to ensure all declared packages from
packages.config are included in the lock file, which is required for
RestoreLockedMode to work correctly in CI builds.
In `@nanoFramework.M5CoreS3/Screen.cs`:
- Around line 88-95: The Brightness property setter has a guard condition that
uses `_isEnabled || _isInitialized`, but since `_isInitialized` remains true
after construction, this condition is effectively always true. This causes the
SetScreenBacklight method to be called even when the display is disabled, which
re-enables the backlight and violates the disabled state contract. Replace the
guard condition in the set accessor of the Brightness property to check only
`_isEnabled` instead of using the OR operator with `_isInitialized`.
In `@README.md`:
- Around line 69-76: Add a clarification note to the CoreS3 section of the
README after the nanoff command example to explain that the ESP32_S3_OCTAL
target is a generic ESP32-S3 profile rather than a CoreS3-specific target.
Include a brief statement such as "CoreS3 uses the generic ESP32-S3 target as no
dedicated CoreS3 target is currently available in nanoFramework" to help users
understand why a generic target is used instead of a board-specific one,
ensuring consistency with how other boards are documented in the same section.
In `@Tests/M5CoreS3TestApp/packages.config`:
- Around line 3-31: Update all NuGet package versions in the packages.config
file to their latest available versions that satisfy the project requirements.
After updating the package versions for all dependencies (including
nanoFramework packages, System packages, UnitsNet packages, and
Nerdbank.GitVersioning), regenerate the corresponding packages.lock.json file by
restoring NuGet packages, which will ensure the lock file matches the updated
package specifications and the CI check_nuget_latest validation will pass.
In `@Tests/M5CoreS3TestApp/packages.lock.json`:
- Around line 4-161: The packages.lock.json file is missing two direct
dependencies that are declared in packages.config:
nanoFramework.System.Device.Adc and nanoFramework.System.IO.Ports. Add these two
packages to the direct dependencies section within the
.NETnanoFramework,Version=v1.0 object in packages.lock.json, following the same
format as the existing entries (with type, requested version range, resolved
version, and contentHash properties). You can find the correct version and
contentHash values by checking packages.config or by running a package restore
to generate them.
In `@Tests/M5CoreS3TestApp/Program.cs`:
- Around line 168-191: The Glyph method is allocating a new string array for
every character lookup, causing unnecessary garbage collection in the hot render
loop. Create a static readonly dictionary that maps each character to its
corresponding glyph pattern string array, initialize this dictionary once with
all the glyph patterns from the switch statement, and then modify the Glyph
method to simply look up and return the cached pattern from the dictionary
instead of creating new arrays with each call.
In `@Tests/M5CoreS3TestApp/Properties/AssemblyInfo.cs`:
- Around line 4-9: The AssemblyTitle and AssemblyProduct attributes in
AssemblyInfo.cs contain template values "CSharp.BlankApplication" instead of the
actual application identity. Replace both AssemblyTitle and AssemblyProduct
values with "M5CoreS3TestApp" to reflect the actual test app name. Additionally,
replace the empty string values in AssemblyDescription, AssemblyConfiguration,
AssemblyCompany, and AssemblyCopyright with appropriate metadata values for this
test application to ensure test binaries and logs display correct information.
🪄 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: a4e5084f-48da-40ed-be84-fdc72c214383
📒 Files selected for processing (16)
README.mdTests/M5CoreS3TestApp/M5CoreS3TestApp.nfprojTests/M5CoreS3TestApp/Program.csTests/M5CoreS3TestApp/Properties/AssemblyInfo.csTests/M5CoreS3TestApp/packages.configTests/M5CoreS3TestApp/packages.lock.jsonazure-pipelines.ymlnanoFramework.M5CoreS3.nuspecnanoFramework.M5CoreS3/CoreS3Ili9342.csnanoFramework.M5CoreS3/M5CoreS3.csnanoFramework.M5CoreS3/Properties/AssemblyInfo.csnanoFramework.M5CoreS3/Screen.csnanoFramework.M5CoreS3/nanoFramework.M5CoreS3.nfprojnanoFramework.M5CoreS3/packages.confignanoFramework.M5CoreS3/packages.lock.jsonnanoFramework.M5Stack.sln
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@nanoFramework.M5CoreS3.nuspec`:
- Around line 20-38: The <dependencies> section in the nuspec file is missing 15
direct dependencies that are declared in packages.lock.json as direct
dependencies of the assembly. Since RestoreLockedMode is enabled, the lock file
is the source of truth and consumers will fail at runtime if these packages are
not declared. Add all 15 missing dependencies
(nanoFramework.Iot.Device.Common.NumberHelper, nanoFramework.ResourceManager,
nanoFramework.Runtime.Events, nanoFramework.Runtime.Native,
nanoFramework.System.Buffers.Binary.BinaryPrimitives,
nanoFramework.System.Collections, nanoFramework.System.Device.Gpio,
nanoFramework.System.IO.Streams, nanoFramework.System.Math,
nanoFramework.System.Numerics, nanoFramework.System.Text,
nanoFramework.UnitsNet.ElectricCurrent,
nanoFramework.UnitsNet.ElectricPotential, nanoFramework.UnitsNet.Power, and
nanoFramework.UnitsNet.Temperature) as new dependency elements within the
<dependencies> section, using the exact versions from packages.lock.json for
each.
🪄 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: 59454089-188a-4e48-8775-c46157b93aa2
📒 Files selected for processing (5)
Tests/M5CoreS3TestApp/packages.lock.jsonnanoFramework.M5CoreS3.nuspecnanoFramework.M5CoreS3/nanoFramework.M5CoreS3.nfprojnanoFramework.M5CoreS3/packages.confignanoFramework.M5CoreS3/packages.lock.json
💤 Files with no reviewable changes (2)
- nanoFramework.M5CoreS3/packages.config
- nanoFramework.M5CoreS3/nanoFramework.M5CoreS3.nfproj
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Tests/M5CoreS3TestApp/Program.cs`:
- Around line 25-44: Startup memory diagnostics are rendering blanks because the
glyph lookup used by the startup screen is missing coverage for several
characters. Update the glyph mapping used by Glyph(char) to include the
diagnostics text characters, especially digits 0-9 and the letters/symbols M, K,
Q, and :. Add the missing glyph definitions alongside the existing glyph arrays
in Program.cs so the startup telemetry values and labels render correctly.
🪄 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: fbf32c5d-b0b6-4329-b2ab-cf6ce23302d8
📒 Files selected for processing (4)
Tests/M5CoreS3TestApp/Program.csTests/M5CoreS3TestApp/Properties/AssemblyInfo.csnanoFramework.M5CoreS3/CoreS3Ili9342.csnanoFramework.M5CoreS3/Screen.cs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nanoFramework.M5CoreS3.nuspec (1)
22-36: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSync the nuspec dependencies with
packages.lock.json.This
<dependencies>block is still missing direct packages that the assembly is built against, so consumers will restore an incomplete graph. Please regenerate or reconcile this section from the lockfile.🤖 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 `@nanoFramework.M5CoreS3.nuspec` around lines 22 - 36, The nanoFramework.M5CoreS3.nuspec dependency list is out of sync with packages.lock.json and is missing direct build dependencies. Update the <dependencies> block so it matches the lockfile’s direct package graph, using the existing dependency entries in the nuspec as the place to reconcile and regenerate the full set.
🤖 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 `@Tests/M5CoreS3TestApp/M5CoreS3TestApp.nfproj`:
- Around line 54-56: In the M5CoreS3TestApp.nfproj file, the HintPath for the
Reference Include named nanoFramework.Graphics.Core is pointing to an incorrect
package directory. The HintPath currently references
nanoFramework.Graphics.1.2.52 but should reference
nanoFramework.Graphics.Core.1.2.52 to match the correct package structure used
in other test projects (AtomLiteTestApp, AtomMatrixTestApp, FireTestApp,
M5Core2TestApp, M5CoreTestApp). Update the HintPath value to include "Core" in
the package directory name so it aligns with the reference name and the
conventions used elsewhere in the repository.
---
Duplicate comments:
In `@nanoFramework.M5CoreS3.nuspec`:
- Around line 22-36: The nanoFramework.M5CoreS3.nuspec dependency list is out of
sync with packages.lock.json and is missing direct build dependencies. Update
the <dependencies> block so it matches the lockfile’s direct package graph,
using the existing dependency entries in the nuspec as the place to reconcile
and regenerate the full set.
🪄 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: dd852ceb-23cc-4365-966a-81fef3f8cef8
📒 Files selected for processing (7)
Tests/M5CoreS3TestApp/M5CoreS3TestApp.nfprojTests/M5CoreS3TestApp/packages.configTests/M5CoreS3TestApp/packages.lock.jsonnanoFramework.M5CoreS3.nuspecnanoFramework.M5CoreS3/nanoFramework.M5CoreS3.nfprojnanoFramework.M5CoreS3/packages.confignanoFramework.M5CoreS3/packages.lock.json
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Tests/M5CoreS3TestApp/M5CoreS3TestApp.nfproj (1)
54-56: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix
nanoFramework.Graphics.CoreHintPath package directory mismatch.At Line 55,
nanoFramework.Graphics.Corepoints to thenanoFramework.Graphics.1.2.52folder instead ofnanoFramework.Graphics.Core.1.2.52, which can break reference resolution.♻️ Proposed fix
- <HintPath>..\..\packages\nanoFramework.Graphics.1.2.52\lib\nanoFramework.Graphics.Core.dll</HintPath> + <HintPath>..\..\packages\nanoFramework.Graphics.Core.1.2.52\lib\nanoFramework.Graphics.Core.dll</HintPath>#!/bin/bash # Verify current Graphics.Core reference path in this project rg -n 'Reference Include="nanoFramework.Graphics.Core"|nanoFramework\.Graphics\.Core\.dll|nanoFramework\.Graphics\.Core\.' Tests/M5CoreS3TestApp/M5CoreS3TestApp.nfproj -n -C2 # Compare with nearby test projects' Graphics.Core package folder convention fd -e nfproj "TestApp" Tests | xargs rg -n 'Reference Include="nanoFramework.Graphics.Core"|HintPath>.*nanoFramework\.Graphics(\.Core)?\.[0-9].*Graphics\.Core\.dll' -C1🤖 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 `@Tests/M5CoreS3TestApp/M5CoreS3TestApp.nfproj` around lines 54 - 56, The nanoFramework.Graphics.Core reference in M5CoreS3TestApp.nfproj uses the wrong package folder name, so update the HintPath for the nanoFramework.Graphics.Core Reference to point to the nanoFramework.Graphics.Core.1.2.52 directory instead of nanoFramework.Graphics.1.2.52. Keep the existing Reference Include value and only correct the package path so assembly resolution works consistently with other Graphics.Core references.
🤖 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.
Duplicate comments:
In `@Tests/M5CoreS3TestApp/M5CoreS3TestApp.nfproj`:
- Around line 54-56: The nanoFramework.Graphics.Core reference in
M5CoreS3TestApp.nfproj uses the wrong package folder name, so update the
HintPath for the nanoFramework.Graphics.Core Reference to point to the
nanoFramework.Graphics.Core.1.2.52 directory instead of
nanoFramework.Graphics.1.2.52. Keep the existing Reference Include value and
only correct the package path so assembly resolution works consistently with
other Graphics.Core references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b5e3209-276c-4b9d-9d18-419a822e89c7
📒 Files selected for processing (6)
Tests/M5CoreS3TestApp/M5CoreS3TestApp.nfprojTests/M5CoreS3TestApp/packages.configTests/M5CoreS3TestApp/packages.lock.jsonnanoFramework.M5CoreS3/nanoFramework.M5CoreS3.nfprojnanoFramework.M5CoreS3/packages.confignanoFramework.M5CoreS3/packages.lock.json
|
josesimoes
approved these changes
Jun 24, 2026
josesimoes
left a comment
josesimoes
left a comment
Member
There was a problem hiding this comment.
LGTM!! Another one bites the dust 😉
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
3 participants
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.
Description
Adding M5Stack CoreS3
Motivation and Context
How Has This Been Tested?
on a real device
Screenshots
Types of changes
Checklist: