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

Ported the ProfiledPIDSubsystem from the wpilib java source to Python #49

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

cwstryker
Copy link
Contributor

Used ChatGPT to get 80% of the job done. This is the ProfiledPIDSubsystem only. No new tests were added. I verified functionality using a separate robot simulation project.

Copy link
Member

@virtuald virtuald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly good, thanks

commands2/profiledpidsubsystem.py Show resolved Hide resolved
commands2/profiledpidsubsystem.py Outdated Show resolved Hide resolved
commands2/profiledpidsubsystem.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@cwstryker cwstryker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I fixed all of the issues.

@cwstryker
Copy link
Contributor Author

I see that some of the checks are failing. I think I understand the cause of the issue. I will work on fixing it tomorrow.

@cwstryker
Copy link
Contributor Author

I fixed the bug causing the Python 3.8 and 3.9 tests to fail.

@virtuald
Copy link
Member

Thanks, everything looks good.

There are multiple ProfiledPIDControllers (one for dimensionless, and one for radians), and even though they're effectively the same thing there are some C++ classes that require one or the other. If you have any experience with python's typing generics, any ideas you have to make this work with both types of ProfiledPIDControllers would be welcomed.

I posted some thoughts on a related issue at #40, but I'm still mulling it over.

If you don't have any ideas or expertise in this, that's fine. I'll contemplate and merge/adjust this once #46 is finished, which hopefully will be soon.

@cwstryker
Copy link
Contributor Author

I am working on a modification to the ProfiledPIDSubsystem that is also compatible with ProfiledPIDControllerRadians objects. Since there are (currently) only two different kinds of ProfiledPIDControllers, I am just using a Union (from the typing module) to include both types of controller object. While working on the changes, mypy indicated that ProfiledPIDController.setGoal() and ProfiledPIDControllerRadians.setGoal() take different argument types. The later only accepts a float argument, where the former accepts either a TrapezoidProfile.State or a float. I think I have a fix for that, but I want to create some test code before I push up a new commit.

@virtuald
Copy link
Member

I'm going to merge this without typing for now. I don't really like the union approach, and I'm too tired to think through the generic/typevar approach. We can add types back at a later date.

@virtuald virtuald force-pushed the cwstryker/add-profiledpidsubsystem branch from f6ec750 to 3a9e0bc Compare January 24, 2024 07:54
@virtuald virtuald merged commit 421f7f6 into robotpy:main Jan 24, 2024
18 checks passed
@cwstryker cwstryker deleted the cwstryker/add-profiledpidsubsystem branch January 25, 2024 03:26
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.

2 participants