Skip to content

Fix SequentialCommandGroup.isFinished is false after command finishes #7901

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

reecelikesramen
Copy link

  • end() sets current command index to -1
  • isFinished() checks if the current command index equals the command list size
  • fixes this by also checking in isFinished() if the current command index is -1

- end() sets current command index to -1
- isFinished() checks if the current command index equals the command list size
- fixes this by also checking in isFinished if the current command index is -1
@reecelikesramen reecelikesramen requested a review from a team as a code owner April 11, 2025 04:57
Copy link
Contributor

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

@github-actions github-actions bot added the component: command-based WPILib Command Based Library label Apr 11, 2025
reecelikesramen added a commit to reecelikesramen/robotpy-commands-v2 that referenced this pull request Apr 11, 2025
- isFinished is false after command is finished
See allwpilib#7901 [here](wpilibsuite/allwpilib#7901)
@reecelikesramen
Copy link
Author

@Starlight220
Copy link
Member

Starlight220 commented Apr 11, 2025

This would cause isFinished to return true before the command started, too -- which is worse behavior than the current, imo.

Calling isFinished only has defined behavior between initialize and end, and is intended only for internal use by the scheduler (or composition implementations). See #6411 and others.

@reecelikesramen
Copy link
Author

yes f/e. Threw this together quick on GH. Easy enough to track at with a var or something else trivial.

I get if it's intended for internal use, but it is public, and its behavior is inconsistent with other command impls.

Caught this by doing unit tests for some command groups and noticed parallel and others isFinished worked but sequential did not.

@reecelikesramen
Copy link
Author

I can switch to isScheduled for unit tests in most cases. I have similar opinions to OP #6411 that it should be in docs that isFinished is for internal scheduler and composition use and behavior may not be what's expected by name or consistent with other commands.

@ArchdukeTim
Copy link
Contributor

its behavior is inconsistent with other command impls.

It's inconsistent because calling isFinished after calling end() or before calling initialize() is UB.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: command-based WPILib Command Based Library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants