Skip to content
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

Fix oscillating velocity for differential drive #1052

Merged
merged 2 commits into from
Feb 6, 2025

Conversation

icros-personal
Copy link
Contributor

Also attempt to add unit tests to exercise this code, but they are Windows-specific at the moment (needs fix!)

@mjansen4857
Copy link
Owner

mjansen4857 commented Feb 6, 2025

I’m pretty sure that module acceleration being able to go negative is important to the trajectory generation. I am not 100% on that though. I’ll have to do some testing with this change.

EDIT: never mind I just realized it’s not. Swerve kinematics won’t give a negative. This change should probably be made to the forward accel pass as well. Plus the c++, python, and GUI versions.

Also, I think adding the ability to actually do PPLib unit tests is better to do in a separate PR.

@github-actions github-actions bot added the GUI Changes to the PathPlanner GUI label Feb 6, 2025
@icros-personal
Copy link
Contributor Author

I've removed the unit test and build logic out and tried to make comparable changes to C++ / Python / GUI code. I haven't tested them, however - I'm not sure how to build them, actually.

Copy link

codecov bot commented Feb 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.53%. Comparing base (1027141) to head (6d2a646).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1052   +/-   ##
=======================================
  Coverage   84.53%   84.53%           
=======================================
  Files          95       95           
  Lines        9730     9730           
=======================================
  Hits         8225     8225           
  Misses       1505     1505           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mjansen4857
Copy link
Owner

Once the CI finishes you should be able to download and try the GUI build. That uses the same algorithm as PPLib so as long as that works I’m not too worried about it. The issue wouldn’t be noticeable in the GUI though since that still just uses swerve kinematics for diff drive.

@mjansen4857
Copy link
Owner

Have you run this locally or something to verify this is the fix for the issue? It sounds right to me but just want to make sure there isn’t more to it.

@mjansen4857
Copy link
Owner

To fix the failing c++ builds you need to change this:
units::math::abs(accelStates[m].speed())

to this:
units::math::abs(accelStates[m].speed)()

@icros-personal
Copy link
Contributor Author

I've run it locally in Java to make sure it works there. I hadn't tried it in any other form. I hadn't previously tried it on the forward pass, but it was positive there so I don't expect it to change.

@icros-personal
Copy link
Contributor Author

I tried downloading the Windows GUI build but couldn't figure out how to persuade it to let me install it while unsigned.
From PowerShell, using Add-AppPackage -AllowUnsigned, it reports: "Add-AppPackage : Deployment failed with HRESULT: 0x80073D2C, The package deployment failed because its publisher is
not in the unsigned namespace."

@mjansen4857
Copy link
Owner

You can’t install the msix version, that can only be uploaded to the Microsoft store. Just download the windows zip.

@icros-personal
Copy link
Contributor Author

You can’t install the msix version, that can only be uploaded to the Microsoft store. Just download the windows zip.

Ah, I see. I missed that artifact. Yes, the GUI seems to work much as before, I don't really see any difference with or without (as you predicted). The Java version is fixed for me. I haven't tried either Python or C++.

@icros-personal icros-personal marked this pull request as ready for review February 6, 2025 05:44
@mjansen4857 mjansen4857 merged commit ce94d1e into mjansen4857:main Feb 6, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Changes to the PathPlanner GUI PathPlannerLib Changes to PathPlannerLib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants