[wpiunits] Add Feedforward unit types

[narmstro2020]

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]

/pregen

[narmstro2020]

/pregen

[narmstro2020]

So what's causing the pregen to fail?

[Gold856]

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]

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]

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

[narmstro2020]

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.

[SamCarlberg]

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

[narmstro2020]

@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]

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]

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

[narmstro2020]

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]

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

[narmstro2020]

@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]

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

[narmstro2020]

@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.

[SamCarlberg]

Looks good to me

[narmstro2020]

@SamCarlberg.
I'm rethinking this one a bit. With the possibility of Torque Sim classes the sheer explosion of type classes this would necessitate after this PR is merge it could be a bit unwieldy as opposed to using the PerUnit construction setup.

If no one object I'd like to close this PR before merging. We can revisit it later needs be.

In fact I think we should consider removing the feedforward Unit objects in the Units file.

[SamCarlberg]

@narmstro2020 I'm fine with closing this PR. I think the existing predefined feedforward and PID units are okay to stay, though; they're not named specifically to be canonical units to be used for those purposes, so there's no risk of collision or confusion with any future torque-based control units.

[narmstro2020]

@narmstro2020 I'm fine with closing this PR. I think the existing predefined feedforward and PID units are okay to stay, though; they're not named specifically to be canonical units to be used for those purposes, so there's no risk of collision or confusion with any future torque-based control units.

Sounds good. We can also add torque based FF units as well if the need arises. In my torque based sim PR I've avoided using them. I'll still hold off a bit on using them in that PR until I get more general feedback on that one.