-
Notifications
You must be signed in to change notification settings - Fork 613
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
Add IdleCommand to wpilibJ2 command framework #5555
Conversation
Implemented an IdleCommand class and a builder for this class in the Commands class. These changes provide a way to construct commands that do nothing until they're interrupted. This could be useful when creating continuously scheduled commands that are intended to run only once, such as subsystem default commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this needs to be a full class rather than only a factory that delegates to the run
factory?
Also, move the factory to be grouped with the other null/waiting factories.
Honestly that's a fair point, I just assumed it should.. |
…n the Commands class instead of having it as an isolated class. Moved the idle factory method alongside the other idling / waiting factories
Move the factory so it's after the |
If it wasn't clear, I have zero knowledge of cpp stuff... I tried to piece together how it should be written purely by logic and looking at other methods, so I may have done some nonsense |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale behind the requirements
parameter? none
doesn't take one, why should this?
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Commands.java
Outdated
Show resolved
Hide resolved
It's actually on purpose, I wanted to ask about it earlier but forgot, since this is meant to be used mostly in the whole default command thingy if the idle Command doesn't require the subsystem, then the CommandScheaduler would re-schedule it, Wouldnt it? |
Changed the way `idle` command is created from using a new instance of `RunCommand` to a call to `run`, improving code readability and consistency.
BTW all the commit massages are written with the JetBrains AI |
As long as the idle is part of a composition, and a different command in the composition requires the subsystem, then it'll work as desired even if the idle has no requirements. |
/format |
Formatting needs fixing; other than that, looks good to go. |
Refactored the way `idle` command is created in Commands.java and Commands.cpp by removing the requirement parameter.
Applied minor changes to improve the readability and maintenance of the code in `Commands.java`, `Commands.cpp` and `Commands.h`. Aligned the lambda expression and adjusted the indentation of comments for the `idle` method. No functionality change was introduced.
These AI Commit massages are spot on. |
I just noticed I forgot to remove the |
Redundant '@param requirements' in the function documentation of the 'idle' method is removed in `Commands.h` file.
wpilibNewCommands/src/main/native/include/frc2/command/Commands.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming CI passes, looks good.
Implemented an IdleCommand class and a builder for this class in the Commands class. These changes provide a way to construct commands that do nothing until they're interrupted. This could be useful when creating continuously scheduled commands that are intended to run only once, such as subsystem default commands.
Fixes #5548