-
-
Notifications
You must be signed in to change notification settings - Fork 117
[XPT2046] Add new binding and sample #1406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Readme and versioning Example code copied from FT6xx6x driver
@Workshopshed I've fixed the checklist for you. |
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughTwo new files were added under the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
@dotnet-policy-service agree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
README.md
is excluded by!**/*.md
and included by nonedevices/XPT2046/Point.cs
is excluded by none and included by nonedevices/XPT2046/Properties/AssemblyInfo.cs
is excluded by none and included by nonedevices/XPT2046/README.md
is excluded by!**/*.md
and included by nonedevices/XPT2046/TouchDemo/Program.cs
is excluded by none and included by nonedevices/XPT2046/TouchDemo/TouchDemo.nfproj
is excluded by none and included by nonedevices/XPT2046/TouchDemo/TouchDemo.sln
is excluded by none and included by nonedevices/XPT2046/TouchDemo/packages.config
is excluded by none and included by nonedevices/XPT2046/XPT2046.cs
is excluded by none and included by nonedevices/XPT2046/XPT2046.nfproj
is excluded by none and included by nonedevices/XPT2046/XPT2046.nuspec
is excluded by none and included by nonedevices/XPT2046/XPT2046.sln
is excluded by none and included by nonedevices/XPT2046/packages.config
is excluded by none and included by none
📒 Files selected for processing (2)
devices/XPT2046/category.txt
(1 hunks)devices/XPT2046/version.json
(1 hunks)
🔇 Additional comments (1)
devices/XPT2046/category.txt (1)
1-3
: Re-evaluate the “proximity” classificationThe XPT2046 is strictly a resistive-touch controller; it does not provide proximity-sensing capabilities. Keeping an incorrect category may confuse consumers of the metadata and mis-categorise NuGet search results.
-proximity touch +touchPlease confirm with the repo’s category taxonomy and drop the inaccurate entry (or justify its presence).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a quick review to start as once those adjusted, it will be faster for the rest. Thank you!
Added StyleCop to the project and followed recommendations. Changed PowerMode constants to binary to be consistent with other constants.
Bumped version to 1
All the packaging and publishing is handled by the pipeline. This has to be signed before distribution. Plus only authorized accounts can publish nugets for the nanoFramework org. |
azp /run |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
One warning from the build, guessing it doesn't apply to this specific build? The windows-latest label will migrate from Windows Server 2022 to Windows Server 2025 beginning September 2, 2025. For more information see actions/runner-images#12677 |
Just checking if anything is outstanding for me to fix? |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Many thanks for the feedback, will get those sorted |
Move sample into samples folder Moved namespace Added assembly info for sample Move sample to top level code for simplicity Added package lock files
45a1410
to
5a939ec
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
devices/XPT2046/Point.cs
Outdated
/// <summary> | ||
/// A touch point. | ||
/// </summary> | ||
public struct Point |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for seen this but we do have the nanoFramework.Graphics.Core nuget which implements the points: https://github.com/nanoframework/nanoFramework.Graphics/blob/main/nanoFramework.Graphics.Core/System/Drawing/Point.cs
Please use that one instead of your own struct. That will allow to use it properly with the screens and have more flexibility. So, it's about adding this nuget, removing this file and adding the import in your code. It should just work transparently! As a bonus, you get all the point operations and more!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple of more comments
Build error seems to related to RestorePackagesWithLockFile = true which I don't think is used in VSCode so I've taken that out. Lets see if that fixes it. NU1004: The packages lock file is inconsistent with the project dependencies so restore can't be run in locked mode. Run restore without using restore locked mode to update the lock file. |
Tidy gitignore Rename Point as it's not the same as the point in nanoFramework.Graphics Tidy comments Remove restore with lock as it breaks the pipeline
Restored
Manually delete the lock file so it can be recreated by nuget. |
I did cross check the files and the versions are the same between the lock file and the packages file. Deleted the file. nuget restore -forceEvaluate Identical file is generated. Git tells me there is nothing to commit. Looking at another recent device Adc128D818. There's a difference in the top of the lock file.
So I've manually added that into my file. I'm guessing you are using a slightly different process to generate your lock files. Also spotted a different in the project file. Mine had the Nerdbank.GitVersioning declared at both the beginning and end of the file. So I've made it same as per Adc128D818 example. |
Doesn't seem to make a difference but consistent with other devices
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@Workshopshed you should be good with copy/paste from any of the other bindings or libraries. The patterns used everywhere are always the same. 😉 As for the lock file, I don't have any problems with those... Are you using VS Code or full VS? If it's VS Code, that can explain it... |
Just checked it on the work laptop and it generates a lockfile with .NETnanoFramework. Using nuget version 6.13.2.1 nuget restore XPT2046.sln Nuget at home is version 6.14.0.116 Upgraded work machine (with Visual Studio) recreated the file, still has added NETnanoFramework. So agree seems to be a VS vs VSCode issue. |
Just checking in that everything is now covered. As mentioned, the graphic POINT doesn't meet the requirements. But otherwise I've made all the requested changes. |
Description
A driver for interacting with the XPT2046 resistive touch screen.
Motivation and Context
The SPI protocol used by this driver is quite complex and typical developers just want the X and Y values of the touch.
Created by referring to the datasheet and drivers in other languages.
Based the API on other touch controllers already in the repo such as the CHSC6540 and Ft6xx6x
How Has This Been Tested?
Ran this on an ESP32 based board which has a touch screen built in. Ensured that different touches produced outputs that were proportional to the position.
Screenshots
Types of changes
Checklist:
Summary by CodeRabbit