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

[wpiunits] Add Feedforward unit types #7064

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

Conversation

narmstro2020
Copy link
Contributor

I'm the kind of person who will use the Units class for these constants and I wanted to test the waters.

Hence why this is a draft. Also not 100% sure how the pregen will go.

Also I've only added one for kV for linear velocity.

Feedback (Haha get it) is always appreciated.

@narmstro2020
Copy link
Contributor Author

/pregen

@narmstro2020
Copy link
Contributor Author

/pregen

@narmstro2020
Copy link
Contributor Author

So what's causing the pregen to fail?

@Gold856
Copy link
Contributor

Gold856 commented Sep 11, 2024

The pregen command wasn't updated to regenerate wpiunits. I'll work on fixing this, but in the meantime, if you can install Python and Jinja (pip install Jinja2), I recommend you do so and just run python wpiunits/generate_units.py and commit the changes.

@narmstro2020
Copy link
Contributor Author

The pregen command wasn't updated to regenerate wpiunits. I'll work on fixing this, but in the meantime, if you can install Python and Jinja (pip install Jinja2), I recommend you do so and just run python wpiunits/generate_units.py and commit the changes.

Thank you. Am I corect in that the generator doesn't generate the Unit classes in first\units. it just creates the files stored in generated?

@Gold856
Copy link
Contributor

Gold856 commented Sep 11, 2024

Yes. The pregen scripts write the generated files to src/generated/main for a given subproject.

@narmstro2020
Copy link
Contributor Author

At this point I've added all of the linear and angular units for the feedforward constants (kv, ka).

I am not married to the names of these units and will rename them when prompted.
Always hard to name things in programming.

Also if it hasn't been said enough @SamCarlberg did a wonderful job on the rewrite.
It is so much nicer to use this API now.

@narmstro2020 narmstro2020 marked this pull request as ready for review September 11, 2024 03:06
@narmstro2020 narmstro2020 requested a review from a team as a code owner September 11, 2024 03:06
@narmstro2020 narmstro2020 changed the title [wpiunits] Add Feed forward classes [wpiunits] Add Feedforward unit types Sep 11, 2024
@SamCarlberg
Copy link
Member

@narmstro2020 What do you see this providing that Per<VoltageUnit, LinearVelocity> (and friends) do not?

@narmstro2020
Copy link
Contributor Author

narmstro2020 commented Sep 12, 2024

@narmstro2020 What do you see this providing that Per<VoltageUnit, LinearVelocity> (and friends) do not?

Really just ease of use from the student perspectice. I think the named units shouldn’t rely on the syntax of generics for the type whenever possible. Makes housing these in constants a bit easier to read.

That being said I’m open to modifications to the name

@SamCarlberg
Copy link
Member

That's fair, though I'm personally ambivalent. A similar set of units for PID constants would also be useful in conjunction with these so users don't have a mix of generic Per<VoltageUnit, DistanceUnit> and sharp VoltagePerXYZ objects

@calcmogul
Copy link
Member

calcmogul commented Sep 12, 2024

Note that not all feedback controllers output voltage. The variety of unit possibilities is why PIDController is unitless.

@narmstro2020
Copy link
Contributor Author

narmstro2020 commented Sep 13, 2024

Note that not all feedback controllers output voltage. The variety of unit possibilities is why PIDController is unitless.

I thought about that which I why I left it out in this PR. I guess I could just make them for the common motor usages, but I will defer to the reviewers as to what would be best.

@narmstro2020
Copy link
Contributor Author

I'll go ahead and add a PID set for motor control voltage/position and motor control voltage/velocity.

@narmstro2020
Copy link
Contributor Author

@calcmogul @SamCarlberg

So I haven't added units for the common PID.
Should I or not bother. I'm okay either way, I just wanted a recommendation before proceeding.

@SamCarlberg
Copy link
Member

@narmstro2020 I'm ambivalent, and we can always add them later

@narmstro2020
Copy link
Contributor Author

@narmstro2020 I'm ambivalent, and we can always add them later

I will consider this PR done in regards to new features then. I'll make changes as requested of course.

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