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

[wpilibcExamples] Update examples to CommandPtr #5988

Merged
merged 15 commits into from
Dec 4, 2023
Merged

[wpilibcExamples] Update examples to CommandPtr #5988

merged 15 commits into from
Dec 4, 2023

Conversation

ncorrea210
Copy link
Contributor

Fixes memory leaks and uses of Command* instead of CommandPtr in GetAutonomousCommand talked about in #5921.

@ncorrea210 ncorrea210 requested review from a team as code owners December 2, 2023 03:13
@ncorrea210 ncorrea210 changed the title [WPILibc] [examples] Updated all examples to CommandPtr [wpilibc] [examples] Updated all examples to CommandPtr Dec 2, 2023
@calcmogul
Copy link
Member

/format

calcmogul
calcmogul previously approved these changes Dec 2, 2023
Copy link
Member

@Starlight220 Starlight220 left a 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 than cmd::None() (= "command doing nothing"). cmd::None() might be fitting to be returned from RobotContainer.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!

@PeterJohnson
Copy link
Member

PeterJohnson commented Dec 2, 2023

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. CommandPtr(nullptr)) from working by adding a deleted constructor (explicit CommandPtr(std::nullptr_t) = delete;), but passing in an empty unique_ptr still will work with that.

@Starlight220
Copy link
Member

Starlight220 commented Dec 2, 2023

We can and should test on construction that the passed-in unique_ptr is valid.

Does CommandPtr(nullptr) compile due to implicit conversions?

@PeterJohnson
Copy link
Member

PeterJohnson commented Dec 2, 2023

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).

@ncorrea210
Copy link
Contributor Author

ncorrea210 commented Dec 2, 2023

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.

@ncorrea210
Copy link
Contributor Author

@Starlight220, about the gearsbot and frisbeebot, are you saying that they should not be changed to CommandPtr?

@Starlight220
Copy link
Member

Correct

@Starlight220
Copy link
Member

I submitted #5991 for the CommandPtr issue.

@ncorrea210
Copy link
Contributor Author

@Starlight220 I believe I have made all the requested changes.

@ncorrea210
Copy link
Contributor Author

Just needed to add some quick formatting things to SelectCommand, should all be ready now.

Copy link
Member

@Starlight220 Starlight220 left a 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 nullptred (teleopInit)

@ncorrea210
Copy link
Contributor Author

@Starlight220 the requested changes have been completed.

@Starlight220 Starlight220 changed the title [wpilibc] [examples] Updated all examples to CommandPtr [wpilibcExamples] Update examples to CommandPtr Dec 4, 2023
@PeterJohnson PeterJohnson merged commit d32c104 into wpilibsuite:main Dec 4, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants