-
-
Notifications
You must be signed in to change notification settings - Fork 117
Driver DRV8825 for stepper motor was added #1407
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
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (7)
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 WalkthroughThree new files were added under 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 (
|
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: 0
🧹 Nitpick comments (1)
devices/Drv8825/version.json (1)
8-12
: Releasing fromdevelop
may expose unstable codeIncluding
^refs/heads/develop$
inpublicReleaseRefSpec
means every build from the develop branch is treated as a public (non-prerelease) package. Most repos keepdevelop
for integration and reservemain
/version tags for release artifacts, letting CI append a pre-release label (e.g.-alpha
). Consider either:
- Removing
develop
from the list, or- Adding a
release
: false / prerelease scheme for develop to prevent accidental public pushes.Example minimal change:
- "^refs/heads/develop$", + "^refs/heads/develop$" // remove or turn into prerelease patternUp to project policy, but worth a second look.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (16)
devices/Drv8825/Direction.cs
is excluded by none and included by nonedevices/Drv8825/Drv8825.cs
is excluded by none and included by nonedevices/Drv8825/Drv8825.nfproj
is excluded by none and included by nonedevices/Drv8825/Drv8825.nuspec
is excluded by none and included by nonedevices/Drv8825/Drv8825.sln
is excluded by none and included by nonedevices/Drv8825/Drv8825_circuit_bb.jpeg
is excluded by!**/*.jpeg
and included by nonedevices/Drv8825/MicrostepsController.cs
is excluded by none and included by nonedevices/Drv8825/Properties/AssemblyInfo.cs
is excluded by none and included by nonedevices/Drv8825/README.md
is excluded by!**/*.md
and included by nonedevices/Drv8825/Settings.StyleCop
is excluded by none and included by nonedevices/Drv8825/StepSize.cs
is excluded by none and included by nonedevices/Drv8825/packages.config
is excluded by none and included by nonedevices/Drv8825/samples/Drv8825.samples.nfproj
is excluded by none and included by nonedevices/Drv8825/samples/Program.cs
is excluded by none and included by nonedevices/Drv8825/samples/Properties/AssemblyInfo.cs
is excluded by none and included by nonedevices/Drv8825/samples/packages.config
is excluded by none and included by none
📒 Files selected for processing (3)
devices/Drv8825/category.txt
(1 hunks)devices/Drv8825/packages.lock.json
(1 hunks)devices/Drv8825/version.json
(1 hunks)
🔇 Additional comments (5)
devices/Drv8825/category.txt (1)
1-1
: Category looks correctThe device is appropriately categorized as "motor". No further action required.
devices/Drv8825/packages.lock.json (2)
29-40
: Consider marking build-only tools as PrivateAssets to avoid bloating the deployment image
Nerdbank.GitVersioning
andStyleCop.MSBuild
are build/analysis-time tools. When they appear as direct package references the nanoFramework project will carry them into the lock file, but unlessPrivateAssets="all"
(ordevelopmentDependency="true"
) is set in the<PackageReference>
the assemblies are copied to the target and increase flash/RAM usage on MCU targets for no benefit.Please double-check the corresponding
csproj
:<PackageReference Include="Nerdbank.GitVersioning" Version="3.7.115" PrivateAssets="all" /> <PackageReference Include="StyleCop.MSBuild" Version="6.2.0" PrivateAssets="all" />If those attributes are already present you can ignore the warning; otherwise consider adding them and regenerating the lock file.
1-49
: Lock file LGTM – exact pinning matches nanoFramework guidanceAll runtime packages are pinned to exact patch versions with content hashes, ensuring deterministic builds — exactly what nanoFramework recommends. No anomalies spotted.
devices/Drv8825/version.json (2)
3-3
: Patch component is implicit – confirm that this is intentional
"version": "1.1"
resolves to1.1.0
under NBGV.
If you intend to ship the very first patch (-preview or ‑beta later) this is fine; if you expected1.1.x
to be auto-incremented you need to bump the third component manually or rely onversionHeight
.
1-16
: Versioning file otherwise looks consistentSchema reference, numeric-identifier padding and cloud-build variable propagation are all in line with the repo’s other device libraries.
azp /run |
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 comments, looks good overall!
@Ellerbach Thank you for your review! I made all the suggested changes manually and pushed them in one commit, because if I accepted each suggestion separately, each of them would result in a new commit, and I don't think that's a good idea. Anyway, how do you do it in nanoFramework? I'll keep that in mind for future reference. |
When you are in the "file changed" tab on GitHub, you can batch accept all the changes, or add changes to the batch and commit once only! |
I tried to do this, but "Add suggestion to batch" button was not available even in the files tab. I don't know why. But using this method was my first thought. |
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.
Looks great, few more elements and then I think we'll be good to go!
Description
Motivation and Context
DRV8825 is a stepper motor driver that is made as compatible as possible with A4988, but there are some nuances. I tried to use the A4988 library to work with my DRV8825 driver, but the driver did not work correctly. After little investigation, I realized that despite the same connectors, this driver is more capricious to the control signal. Thus, I wrote a library to control the stepper motor through the DRV8825 driver.
How Has This Been Tested?
When I written a new library, I tested it several times through a sample project with my ESP32 WROOM(rev 3), DRV8825 and Nema 17 (stepping motor) to make sure that the motor does not miss steps.
Screenshots
Types of changes
Checklist:
Summary by CodeRabbit