allwpilib icon indicating copy to clipboard operation
allwpilib copied to clipboard

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

Open BrownGenius opened this issue 1 year ago • 5 comments

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 avatar Mar 20 '24 14:03 BrownGenius

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

calcmogul avatar Mar 20 '24 17:03 calcmogul

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?

BrownGenius avatar Mar 20 '24 18:03 BrownGenius

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.

KangarooKoala avatar Mar 21 '24 21:03 KangarooKoala

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.

calcmogul avatar Mar 21 '24 21:03 calcmogul

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.

BrownGenius avatar Mar 22 '24 15:03 BrownGenius