-
Notifications
You must be signed in to change notification settings - Fork 611
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
[wpilibcExamples] Update examples to CommandPtr #5988
Conversation
/format |
wpilibcExamples/src/main/cpp/examples/ArmBot/cpp/RobotContainer.cpp
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.
Thanks for doing this!
Some comments:
- General (ArmBot, ArmBotOffboard, DriveDistanceOffboard, GyroDriveCommands, others):
CommandPtr(nullptr)
shouldn't be valid;Robot.m_autonomousCommand
should default to an empty optional (= "no command") rather thancmd::None()
(= "command doing nothing").cmd::None()
might be fitting to be returned fromRobotContainer.GetAutoCommand()
. - FrisbeeBot: an excellent example of RobotContainer-owned CommandPtrs -- is there a reason to change it?
- GearsBot: an excellent example of RobotContainer-owned command class objects -- is there a reason to change it?
- HatchBotInlined/Traditional: even more than FrisbeeBot/GearsBot, this example showcases choosing between RobotContainer-owned CommandPtrs/command class objects, with detailed documentation about the ownership semantics used. These examples literally are some of the few that excellently solve the problem described by the issue. Why change them?
- MecanumControllerCommand: Other than the comment-and-a-bit there, excellent!
- RamseteCommand: see
optional
debate in the first bulletpoint. - SelectCommand: as in FrisbeeBot, showcases a good idiom of RobotContainer-owned CommandPtr. Maybe enhance the example to show integration with SendableChooser? (=basically what you wrote in the HatchBot examples).
- SwerveControllerCommand: same as MecanumControllerCommand: other than one small comment, excellent!
Also, well done on removing the unused SendableChoosers!
wpilibcExamples/src/main/cpp/examples/MecanumControllerCommand/cpp/RobotContainer.cpp
Show resolved
Hide resolved
wpilibcExamples/src/main/cpp/examples/MecanumControllerCommand/cpp/RobotContainer.cpp
Outdated
Show resolved
Hide resolved
wpilibcExamples/src/main/cpp/examples/SwerveControllerCommand/cpp/RobotContainer.cpp
Show resolved
Hide resolved
It's hard to fully prevent constructing a CommandPtr with nullptr value, since you can pass in a unique_ptr with nullptr value. We could prevent just passing nullptr (e.g. |
We can and should test on construction that the passed-in unique_ptr is valid. Does |
Yes, because nullptr implicitly converts to unique_ptr. We can check for an invalid unique_ptr on construction and throw? (in addition to deleting the direct-nullptr constructor, which is even better because it is a compile-time error). |
I think this CommandPtr issue is beyond the scope of this pull request. I recommend either a draft pull request or a issue for this topic. I mainly say this because I am not 100% sure how to implment this, whether it be something like a static_assert, or some kind of if constexpr expression, or neither. |
@Starlight220, about the gearsbot and frisbeebot, are you saying that they should not be changed to CommandPtr? |
Correct |
I submitted #5991 for the CommandPtr issue. |
@Starlight220 I believe I have made all the requested changes. |
Just needed to add some quick formatting things to SelectCommand, should all be ready now. |
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.
The Robot.m_autoCommand
optional
should be .reset()
where it's previously nullptr
ed (teleopInit)
wpilibcExamples/src/main/cpp/examples/SelectCommand/cpp/RobotContainer.cpp
Outdated
Show resolved
Hide resolved
@Starlight220 the requested changes have been completed. |
Fixes memory leaks and uses of Command* instead of CommandPtr in GetAutonomousCommand talked about in #5921.