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

[sysid] Show warning tooltips next to bad feedforward gains instead of throwing #6251

Merged
merged 3 commits into from
Jan 20, 2024

Conversation

HarryXChen3
Copy link
Contributor

@HarryXChen3 HarryXChen3 commented Jan 18, 2024

Refactors SysId error handling to not completely cancel analysis when bad feedforward gains are encountered but instead show a warning next to the specific gains that are invalid (with reasoning) - closes #6198. Also still needs to address #6248. #6248 should instead be addressed in a separate PR.

@HarryXChen3
Copy link
Contributor Author

Looks like its mad about how I initialize some structs. I thought that maybe I could just leave the arguments I didn't need out to let them be default initialized, but it doesn't seem like its happy about that. Maybe that's only a thing in C++23? (is wpilib on C++11?)

@calcmogul
Copy link
Member

calcmogul commented Jan 18, 2024

WPILib uses C++20. You're probably looking for form (3) or (4) on https://en.cppreference.com/w/cpp/language/aggregate_initialization.

@HarryXChen3
Copy link
Contributor Author

WPILib uses C++20. You're probably looking for form (3) or (4) on https://en.cppreference.com/w/cpp/language/aggregate_initialization.

Is the way im initializing structs with partially left out values to be "default" initialized only a thing in a newer version? My local setup must be also on an incorrect version then, since both intellisense and local builds are happy.

@calcmogul
Copy link
Member

calcmogul commented Jan 18, 2024

The syntax you wrote isn't a thing on any version. You have to write it like this instead:

      thing = {.field = newValue};

@HarryXChen3
Copy link
Contributor Author

The syntax you wrote isn't a thing on any version.

Huh. I wonder why it still works 🤔. I'll make sure to address it on the next commit, though.

@HarryXChen3
Copy link
Contributor Author

[ FAILED ] ElevatorSimulationTest.Teleop (97 ms)

I think this should be unrelated to my changes - is this a flaky test?

@calcmogul
Copy link
Member

Yes. Ignore it.

@calcmogul calcmogul changed the title [sysid] OLS quality check and handling of bad feedforward gains refactor [sysid] Show warning next to bad feedforward gains instead of throwing Jan 19, 2024
@calcmogul calcmogul marked this pull request as ready for review January 19, 2024 00:29
@calcmogul calcmogul requested a review from a team as a code owner January 19, 2024 00:29
@calcmogul calcmogul changed the title [sysid] Show warning next to bad feedforward gains instead of throwing [sysid] Show warning tooltips next to bad feedforward gains instead of throwing Jan 19, 2024
@PeterJohnson PeterJohnson merged commit 1330235 into wpilibsuite:main Jan 20, 2024
25 checks passed
@HarryXChen3 HarryXChen3 deleted the sysid-ols-quality-check branch January 20, 2024 05:07
This pull request was closed.
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.

Can't view SysId logs unless analysis succeeds
3 participants