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

[cmd] TrapezoidProfileCommand: Use period instead of elapsed time in new TrapezoidProfile.calculate API #6454

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

Conversation

BrownGenius
Copy link

@BrownGenius BrownGenius commented Mar 20, 2024

DevilBotz 2876 experienced odd instabilities with the TrapezoidProfileCommand. Sometimes, the position returned by the calculate would go in the opposite direction. This was reproduced in simulation, so not a hardware issue. Digging into the code, we found that the new call to calculate is using the wrong parameter for "time" when calling TrapezoidProfile.calculate(double t, State current, State goal). It was providing the elapsed time from the initial state but since the current state is being used, the period should be used, instead. Bug introduced in #5457

My test case was with changing position as follows:

  1. 0 --> 90
  2. 90 --> 0
  3. 0 --> 45
  4. 45 --> 90 this caused the trapezoid profile to slam down to zero before going to 90

@BrownGenius BrownGenius requested a review from a team as a code owner March 20, 2024 14:44
@calcmogul calcmogul added breaking Introduces a breaking change. component: command-based WPILib Command Based Library help: needs C++ Java exists, needs C++ port labels Mar 20, 2024
@calcmogul calcmogul changed the title TrapezoidProfileCommand: Use period instead of elapsed time in new TrapezoidProfile.calculate API [cmd] TrapezoidProfileCommand: Use period instead of elapsed time in new TrapezoidProfile.calculate API Mar 20, 2024
@calcmogul calcmogul removed the help: needs C++ Java exists, needs C++ port label Mar 20, 2024
Co-authored-by: Tyler Veness <calcmogul@gmail.com>
@calcmogul calcmogul changed the base branch from main to development March 20, 2024 17:27
@calcmogul calcmogul requested a review from a team as a code owner March 20, 2024 17:27
@calcmogul
Copy link
Member

You'll need to rebase your branch onto the development branch since it's a breaking behavior change.

@BrownGenius
Copy link
Author

You'll need to rebase your branch onto the development branch since it's a breaking behavior change.

Ok, I can do that. However, this is fixing a change that actually broke the original behavior and now matches the other places where TrapezoidProfile is used (e.g. TrapezoidProfileSubsystem). Also, what do I run locally to verify lint/compilation?

@KangarooKoala
Copy link
Contributor

The reason to target development is that we don't want to break users in the middle of a season (even if it is a fix that restores expected behavior).

For testing locally, you can use ./gradlew wpilibNewCommands:test or ./gradlew wpilibNewCommands:testDesktopJava to run Java tests of the commands subproject, ./gradlew wpilibNewCommands:testDesktopCpp to run cpp tests of the commands subproject, and ./gradlew wpilibNewCommands:testDesktop to run both. For formatting/linting, you can use ./gradlew wpilibNewCommands:javaFormat wpilibNewCommands:spotbugsMain wpilibNewCommands:spotbugsTest wpilibNewCommands:spotbugsDev for Java and wpiformat for C++. (You'll need to run pip3 install wpiformat) You'll also want to run the clang-tidy check: ./gradlew generateCompileCommands -Ptoolchain-optional-roborio, ./.github/workflows/fix_compile_commands.py build/TargetedCompileCommands/linuxx86-64release/compile_commands.json, ./.github/workflows/fix_compile_commands.py build/TargetedCompileCommands/linuxx86-64debug/compile_commands.json, wpiformat -no-format -tidy-changed -compile-commands=build/TargetedCompileCommands/linuxx86-64release -vv, and wpiformat -no-format -tidy-changed -compile-commands=build/TargetedCompileCommands/linuxx86-64release -vv.

I'd recommend making some custom scripts to run the formatting checks, and you can always refer back to the lint-format workflow file (.github/workflows/lint-format.yml) for formatting instructions.

@calcmogul
Copy link
Member

calcmogul commented Mar 21, 2024

You can use git rebase -i development to retarget your branch, then use git push --force-with-lease to update the remote copy. One confounding factor here though is you used your main branch, so it's going to mess up your history on main (which you can restore after this PR is merged). In the future, use a branch off of main or development instead of main or development itself.

@BrownGenius
Copy link
Author

The reason to target development is that we don't want to break users in the middle of a season (even if it is a fix that restores expected behavior).

Ok, that makes total sense. Other teams may have already implemented workarounds for the existing behavior....so we don't want to affect them during competition.

I have a few other fixes related to motion profiling, specifically TrapezoidProfileSubsystem, that also needs some stablity fixes, soi I'll wait until after comp season before updating this PR fully.

@calcmogul calcmogul changed the base branch from development to main May 24, 2024 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Introduces a breaking change. component: command-based WPILib Command Based Library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants