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

C++ command-based examples use both Command* and CommandPtr for GetAutonomousCommand #5921

Closed
rzblue opened this issue Nov 13, 2023 · 5 comments
Labels
component: command-based WPILib Command Based Library

Comments

@rzblue
Copy link
Member

rzblue commented Nov 13, 2023

The examples show GetAutonomousCommand returning both CommandPtr and Command* in various places.

In some examples, GetAutonomousCommand constructs and returns a Command* constructed with the new operator, leaking memory.
In others, it returns pointers to commands constructed inside RobotContainer (which obviously can't be accomplished with CommandPtr due to ownership constraints- functions that return CommandPtr must construct a new command each time)
)

Additionally, SendableChooser must be used with Command*.

It seems like there's not one way that will work for all cases.

I'm not sure what the best solution is here. Perhaps we should keep the examples as-is, and better document the differences between CommandPtr and Command*?

@rzblue rzblue added the component: command-based WPILib Command Based Library label Nov 13, 2023
@Starlight220
Copy link
Member

In some examples, GetAutonomousCommand constructs and returns a Command* constructed with the new operator, leaking memory.

This is a leftover from before CommandPtr was a thing (and possibly even from old commands).

it returns pointers to commands constructed inside RobotContainer (which obviously can't be accomplished with CommandPtr due to ownership constraints- functions that return CommandPtr must construct a new command each time)

CommandPtr or Command*? Returning Command* from a CommandPtr owned elsewhere with the correct lifetime (such as RobotContainer) is ok. As long as ppl don't start composing it.

Additionally, SendableChooser must be used with Command*.

That's been brought up before; there are multiple options:

  • Choose between Command*, own the commands externally in RobotContainer
  • Choose between function<CommandPtr()>.
  • Choose between strings/ints/enums/etc, feed them into SelectCommand. (Maybe add a class for doing this?)

better document the differences between CommandPtr and Command*?

I think we have some docs in the C++ Commands page, but improving it and/or putting it somewhere more obvious is never a bad idea.
Having concepts might improve this, might make it more confusing due to a bunch of type names.

@Starlight220
Copy link
Member

Starlight220 commented Dec 2, 2023

To clarify, the action items I see from this issue are:

Using concepts for command types should also be done, but I see that as deriving more from #5789 than from this issue.

@ncorrea210
Copy link
Contributor

ncorrea210 commented Dec 3, 2023

  • Document differences between CommandPtr (owned) and Command* (non-owned).

Is there any plan as to where this may be documented? Is it going to be one long comment on every single GetAutonomousCommand() or will there be a link to a new page(or an addition to an old one) on the FRC Docs? Or something else entirely, though I am not sure of many other options.

@Starlight220
Copy link
Member

Starlight220 commented Dec 3, 2023

The C++ Commands Technical Discussion page on frc-docs, though it might need a refactor/split/rework (it currently has a lot of historic information that is relevant to command classes and less so to CommandPtr).

@Starlight220
Copy link
Member

I extracted the remaining documentation part to wpilibsuite/frc-docs#2438; closing.

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

No branches or pull requests

3 participants