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

Extract common trigger binding logic #79

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

KangarooKoala
Copy link

Also use previous and current

Port of wpilibsuite/allwpilib#7550, which was merged before I could open this PR.

commands2/button/trigger.py Outdated Show resolved Hide resolved
@KangarooKoala
Copy link
Author

Thanks for apotting that! It's been too long since I've played with Python. (Side note- that decorator trick is also really cool! It makes sense now that I've seen it, but I don't think I'd have ever thought of it myself- I probably would've ended up translating the code into a single expression)

@KangarooKoala
Copy link
Author

Weird- The check-mypy failure is the same that's on main*, but the test failures are really weird, since test_swervecontrollercommand is failing from a lack of numpy, but I didn't touch any of that.

*By the way, it looks like check-mypy is failing on the return type mismatch in Command.fork() (which was added in #71 when it hadn't actually been added to WPILib - see wpilibsuite/allwpilib#7083 - and the WPILib PR went in a different direction after the robotpy-commands PR was merged) and on missing joystick.getDirectionDegrees() and joystick.getDirectionRadians(), which were deprecated in 2023 in wpilibsuite/allwpilib#5319 in favor of a unit overload and were removed in wpilibsuite/allwpilib#6569.

My impulse would be to remove Command.fork() and wait until the WPILib PR gets merged since it's quite likely the version of fork() that ends up in WPILib will be different from what's currently in robotpy-commands. However, I'm quite new to robotpy-commands and I don't know if there's other reasons to go a different route.

I'm not familiar with how Python units work (How does the pybind work with the C++ units classes?), but it looks like you can probably implement CommandJoystick.getDirectionDegrees() and CommandJoystick().getDirectionRadians() in terms of self._hid.getDirection(), but I'm not sure.

@virtuald
Copy link
Member

I'm not sure why I merged fork. I agree it should be removed.

Python will pass units to C++ as a float, in the unit that the function parameter is specified as. There is no mechanism to check units, but the name of the unit ends up in the .pyi typestub file for the function.

@KangarooKoala
Copy link
Author

Any ideas on the dist/ci/test failures? (Example log here: https://github.com/robotpy/robotpy-commands-v2/actions/runs/12476284136/job/34841958986?pr=79)
They seem to be failing because numpy is missing, but I don't get how in the world my PR could've affected that.

@KangarooKoala
Copy link
Author

By the way, it looks like check-mypy is failing on the return type mismatch in Command.fork() (which was added in #71 when it hadn't actually been added to WPILib - see wpilibsuite/allwpilib#7083 - and the WPILib PR went in a different direction after the robotpy-commands PR was merged)

Update on this- The upstream PR was just closed, so Command.fork() definitely doesn't need to exist.

@auscompgeek
Copy link
Member

They seem to be failing because numpy is missing

Hmm. I think a recent change to WPILib means wpimath.geometry.Rotation2d's constructor has a dependency on numpy now. That dependency is missing in the robotpy-wpimath package metadata.

@auscompgeek
Copy link
Member

Those bugs should now be fixed if you rebase onto main.

Also use previous and current
Co-authored-by: Vasista Vovveti <[email protected]>
@KangarooKoala
Copy link
Author

My bad, I forgot to pull after applying @TheTripleV's suggested change so when I rebased I accidentally dropped that commit.

@auscompgeek auscompgeek requested a review from TheTripleV January 4, 2025 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants