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

[wpimath] Add ElevatorFeedforward.calculate(currentV, nextV) overload #5715

Merged
merged 44 commits into from
Oct 9, 2023

Conversation

narmstro2020
Copy link
Contributor

The Elevator and Arm FF controllers were missing a method of the form
public double calculate(double currentVelocity, double nextVelocity, double dtSeconds)
The Arm FF controller has one with a positionRadians component at the beginning.

Java version only at this point.

@narmstro2020 narmstro2020 requested a review from a team as a code owner October 2, 2023 02:20
Copy link
Member

@calcmogul calcmogul left a comment

Choose a reason for hiding this comment

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

The arm implementation doesn't provide an exact solution. See #5192 (comment).

@narmstro2020
Copy link
Contributor Author

Would page 106 of https://file.tavsys.net/control/controls-engineering-in-frc.pdf
help in this

@calcmogul
Copy link
Member

calcmogul commented Oct 2, 2023

No, because we want uₖ for some function xₖ₊₁ = f_d(xₖ, uₖ). We only have dx/dt = f(x) + Bu. My comment on the other issue suggests a numerical method since there is no analytical discrete model for an arm.

@narmstro2020
Copy link
Contributor Author

Okay. So the arm needs work. Is the Elevator FF good?

@calcmogul
Copy link
Member

calcmogul commented Oct 2, 2023

I think so, but we should include the derivation in an internal comment. It's also worth noting that if x crosses zero during the timestep, the computed input will be wrong.

@narmstro2020
Copy link
Contributor Author

You mean this derivation
dx/dt = Ax + Bu + c
dx/dt = Ax + B(u + B⁺c)
xₖ₊₁ = eᴬᵀxₖ + A⁻¹(eᴬᵀ - I)B(uₖ + B⁺cₖ)
xₖ₊₁ = A_d xₖ + B_d (uₖ + B⁺cₖ)
xₖ₊₁ = A_d xₖ + B_duₖ + B_d B⁺cₖ

Solve for uₖ.

B_duₖ = xₖ₊₁ − A_d xₖ − B_d B⁺cₖ
uₖ = B_d⁺(xₖ₊₁ − A_d xₖ − B_d B⁺cₖ)
uₖ = B_d⁺(xₖ₊₁ − A_d xₖ) − B⁺cₖ

For an elevator with the model dx/dt = -Kv/Ka x + 1/Ka u - Kg/Ka - Ks/Ka sgn(x),
A = -Kv/Ka, B = 1/Ka, and c = -(Kg/Ka + Ks/Ka).

uₖ = B_d⁺(xₖ₊₁ − A_d xₖ) − Ka(-(Kg/Ka + Ks/Ka))
uₖ = B_d⁺(xₖ₊₁ − A_d xₖ) + Ka(Kg/Ka + Ks/Ka)
uₖ = B_d⁺(xₖ₊₁ − A_d xₖ) + Kg + Ks


I'm not sure what you mean when x crosses the zero. Do you mean when the elevator's position reaches bottom

Also should I remove the ArmFF from the pull request. I'm not quite sure how to do that

@calcmogul
Copy link
Member

calcmogul commented Oct 2, 2023

I mean when the velocity crosses zero, because the friction force changes direction and makes the model nonlinear instead of affine (sgn(x) is a nonlinear function in that regime).

@calcmogul
Copy link
Member

calcmogul commented Oct 2, 2023

The elevator derivation should also include sgn(x) and state the assumption that x doesn't cross zero during the step, making sgn(x) a constant.

You could remove the arm feedforward part from this PR. Also revert the comment formatting changes, because javaformat is complaining about them.

A C++ version will be needed as well, of course.

@narmstro2020
Copy link
Contributor Author

narmstro2020 commented Oct 2, 2023

Ah okay. I see what you mean now.

So could that be broken up into a sum of two movements. Like going from 2 to -2. Chop it up into from 2 to 0 and 0 to -2 and handle the 0 separately

@calcmogul
Copy link
Member

calcmogul commented Oct 2, 2023

Yes. It's a small enough effect in a rare enough situation though that I'd be fine just leaving a comment about it.

@narmstro2020
Copy link
Contributor Author

Okay. I've committed a reversion of the ArmFF.

Still trying to find the issues with the comment formatting in the Elevator.

How do I submit and internal comment with the derivation?

@calcmogul
Copy link
Member

Add a sequence of single-line comments at the top of the function body. Longer derivations would go in https://github.com/wpilibsuite/allwpilib/blob/main/wpimath/algorithms.md.

@narmstro2020
Copy link
Contributor Author

Like this

@calcmogul
Copy link
Member

No, the function body is the contents of the function.

@narmstro2020
Copy link
Contributor Author

Sorry. It's late. I inferred above the method signature

…edforward.java

Co-authored-by: Tyler Veness <calcmogul@gmail.com>
@narmstro2020
Copy link
Contributor Author

Thank you @calcmogul for helping me out with this. First time submitting a pull request since I started working with our team 3 years ago.

@narmstro2020
Copy link
Contributor Author

I'm stuck on the p tag at line 62 of ElevFF. Can't seem to format it correctly

@calcmogul
Copy link
Member

I'll clean up the C++ for you.

Gold856 and others added 16 commits October 8, 2023 19:43
Windows 8 added GetSystemTimePreciseAsFileTime().
GetSystemTimePreciseAsFileTime() is supposed to be available, and
wpiutil already uses it.
Co-authored-by: Tyler Veness <calcmogul@gmail.com>
Co-authored-by: Joseph Eng <91924258+KangarooKoala@users.noreply.github.com>
- Utilize TrySend to properly backpressure network traffic
- Split updates into reasonable sized frames using WS fragmentation
- Use WS pings for network aliveness (requires 4.1 protocol revision)
- Measure RTT only at start of connection, rather than periodically
  (this avoids them being affected by other network traffic)
- Refactor network queue
- Refactor network ping, ping from server as well
- Improve meta topic performance
- Implement unified approach for network value updates (currently client and server use very different approaches) that respects requested subscriber update frequency

This adds a new protocol version (4.1) due to WS bugs in prior versions.
std::remove_if() is destructive--it can assume the removed elements are
not used, but NetworkOutgoingQueue needs them to stay intact to be moved
to a different queue. Use std::stable_partition() instead.
This is useful for aliveness checking by clients that can't send
WebSocket PING messages.
@narmstro2020
Copy link
Contributor Author

narmstro2020 commented Oct 9, 2023

Thank you. I tried. I really tried.

Definitely has been a learning experience. Thank you

calcmogul
calcmogul previously approved these changes Oct 9, 2023
@PeterJohnson PeterJohnson changed the title Add ElevatorFeedforward.calculate(currentV, nextV) overload [wpimath] Add ElevatorFeedforward.calculate(currentV, nextV) overload Oct 9, 2023
@PeterJohnson PeterJohnson merged commit faa1e66 into wpilibsuite:main Oct 9, 2023
24 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants