[commands] Add CommandRobot #6859
Conversation
|
This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR. |
spacey-sooty
force-pushed
the
commandrobot
branch
from
40cd203
to
c467506
Compare
Python already has TimedCommandRobot. Should that be replaced with this? Should that stay and this not be added? Should this be added and that stay? |
spacey-sooty
force-pushed
the
commandrobot
branch
2 times, most recently
from
8bd5018
to
d840979
Compare
spacey-sooty
force-pushed
the
commandrobot
branch
2 times, most recently
from
63269be
to
cde09bd
Compare
spacey-sooty
force-pushed
the
commandrobot
branch
3 times, most recently
from
504f6ab
to
c761646
Compare
|
This would need a corresponding change in robotbuilder |
spacey-sooty
force-pushed
the
commandrobot
branch
from
c761646
to
93bf2b3
Compare
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandRobot.java
Outdated
Show resolved
Hide resolved
spacey-sooty
force-pushed
the
commandrobot
branch
from
93bf2b3
to
d5586d9
Compare
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandRobot.java
Outdated
Show resolved
Hide resolved
wpilibjExamples/src/main/java/edu/wpi/first/wpilibj/templates/xrpcommandbased/Robot.java
Show resolved
Hide resolved
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandRobot.java
Outdated
Show resolved
Hide resolved
spacey-sooty
force-pushed
the
commandrobot
branch
5 times, most recently
from
fa3d279
to
6610438
Compare
spacey-sooty
force-pushed
the
commandrobot
branch
3 times, most recently
from
4181cbf
to
19f46cc
Compare
spacey-sooty
force-pushed
the
commandrobot
branch
from
19f46cc
to
092668c
Compare
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandRobot.java
Show resolved
Hide resolved
| m_autonomousCommand = m_autoChooser.getSelected(); | ||
| if (m_autonomousCommand != null) { | ||
| m_scheduler.schedule(m_autonomousCommand); | ||
| } |
There was a problem hiding this comment.
Should use the Autonomous Start trigger for this
|
@Oblarg can you review? |
|
There seem to be merge conflicts? |
There was a problem hiding this comment.
I'm not sure I'm fully onboard with the design here.
For instance, the abstraction here over the sendablechooser feels leaky, similar to the control subsystems. It doesn't save much code, and bites teams doing other stuff (such as the EventLoop/Trigger-based auto idea I've seen thrown around). It being an "invisible" protected field that teams are supposed to know exist and modify is suspect to me.
The mode-specific fields are locked away, yet there isn't infrastructure to replace their use cases. Not much benefit over just not overriding them in team code, as long as there's no actual content in the final implementation.
I think cutting the empty overrides from the templates/examples and merging RobotContainer into Robot would bring most of the value here without adding a new base class (which is a somewhat orthogonal enhancement, but needs to be further fleshed out imo).
This feels like an empty yet leaking abstraction; the main convenience here is that the scheduler is called automatically.
Going for a multi-EventLoop base class might be an idea worth pursuing (though others will have to convey their thoughts on that), but that's more of an EventRobot than CommandRobot.
Going Trigger-based would likely be easier and closer to paradigms that are already starting to circulate.
I'll assume there have been discussions about this. Since I'm nearly not involved, feel free to ignore my review if others have been involved and this is the design that was agreed upon.
This is probably my biggest concern. The reason I added it was as a way to support having an autonomous command by default. Originally I did this with an autonomous command variable but @virtuald suggested it should be easier to integrate with a sendable chooser. When I get around to documentation for this then it will obviously be explained how to use it then. As for using Trigger based autos this is actually something that was discussed quite in depth, there are a few seperate ways that you can achieve this with this model, such as your command factory also creating Triggers then deferring your command factory.
What functionality are teams missing with these functions locked away? Fundamentally the idea of this pr is that if you are using CommandBased correctly you shouldn't need these functions. If your doing something custom like periodic sensor or odometry refresh then you will use addPeriodic for that anyways but other than that you should only need Commands and Triggers.
The idea here isn't convenience, it's meant to point teams towards using Command based correctly.
The idea of this is to encourage people to go Trigger based by making it more difficult for them to not use Triggers. |
Yes there are conflicts with the examples I'll address those. |
| @Override | ||
| public final void teleopInit() { | ||
| if (m_autonomousCommand != null) { | ||
| m_scheduler.cancel(m_autonomousCommand); |
There was a problem hiding this comment.
What’s the reasoning for having the auto command cancelled in teleopInit() rather than autonomousExit()?
There was a problem hiding this comment.
That's how it was before no particular reason
Resolves #6624