I’m currently working on a ground-up rewrite of the command-based libraries, as they are antiquated and in dire need of renovation.
I figured I’d create a thread here asking what features people might be interested in seeing (apart from, obviously, thorough documentation). What’s been written already can be found here:
I definitely like that you’ve been refactoring things into interfaces. That in itself, I think, will lead to cleaner and more thorough documentation. This year, I used a similar design for our code. Basically, everything was based off an action allowing everything to be updated and ended. None of this actually depends on WPILib, but maybe this will give you some ideas. https://github.com/retrodaredevil/action-lib
I’m definitely going to be looking at what you did for ideas on how to make my library better. Also, I noticed you’re trying to actually make this part of official WPI stuff. Because of all the breaking changes you’re making, I don’t think they’ll ever accept. You could try to make it into an independent library, though.
I’ve been in contact with WPILib devs throughout the development; a WPILib dev has even volunteered to write the cpp port. They’ve wanted to seriously refactor the Command libraries for ages.
There are a lot of breaking changes, but everything’s in a new namespace so the old code can be safely deprecated and gradually phased out.
That’s pretty awesome then. I just now noticed you’ve been messing with the experimental package and not the current command package.
OK, I have a few suggestions that are mostly just my personal opinion.
I’d really like Commands to be unit testable. The current scheduler can’t be initialized unless you’re doing a simulation or are testing on a roboRIO.
I like immutability. I know that the current Sendable interface has a setName method, which I wish could somehow be phased out so no one is tempted to change the name of something which should never really be changed. I think that the requirements for Subsystems on a SendableCommandBase could be passed through the constructor so they’re never changed as well.
While I like the use of most things as an interface, they act a little too much like abstract classes because of all the default methods. I understand that you could just override everything that you want to change, but I really don’t feel like that’s what default methods were made for.
Now for SendableCommandBase. A lot of the changes you’ve made make the library better, but by implementing Command and Sendable, you make the class do two different things (Just like current Command class). When I create classes, I try to make them do one thing and one thing only. This year, instead of directly implementing Sendable or extending SendableBase, I created my own Sendable classes which took my subsystems as arguments through the constructor. My point being yes, implementing Sendable makes it easier on people who want their command to be a Sendable, but for people that might want to separate that code a little more, it encourages that code to be piled into one class, potentially making it more messy than if people had an option on whether or not they want it to extend Sendable.
Solution: Make SendableCommandBase extend CommandBase or come up with another way so people aren’t forced to inherit a class that also inherits Sendable.
ConditionalCommand is a cool idea, but why not just have something like DynamicCommand which has a Supplier<Comamnd> that will use whatever that returns. It gives the caller more options too because they aren’t just limited to two commands.
Now, for even more personal preference. In my action-lib library, my Actions class just had a bunch of factory methods. This had an advantage when I wanted to change how my classes worked as the factory methods could easily adapt to whatever default arguments constructors now needed. Take my DynamicCommand idea. Yes, you could do new DynamicCommand(myCommandSupplier), but a factory method would be better as there’s really no point in the caller knowing that the Command is a DynamicCommand. Plus, factory method names can sometimes be more descriptive.
Anyway, just my personal opinion on some things. I’m guessing not everything I said above makes everything better, but you can take it or leave it.
Great feedback!
The best solution to this, I think, is just to have GradleRIO offer a “test” task that runs user-specified unit tests in simulation (that is how the unit tests for WPILib itself are run). This may even already exist; I’m not sure - I’ll ask one of the devs later.
I’m trying to avoid constructor clutter as much as possible, because the original API had a huge problem with long lists of overloaded constructors. Certain command implementations that are intended to be able to be used without subclassing (e.g. InstantCommand and the various PIDCommands) do indeed allow users to pass requirements through the constructors, but I’m very hesitant to require this for all commands.
Whether that’s what they were “made for” or not, I think this is a very natural usage that preserves flexibility while still allowing us to package a lot of functionality into the interface. The other option would be to move a lot of the “extras” into some other class and offer them as static methods; I think that results in a slightly clunkier API (especially for the decorators), but it does reduce the interface bloat.
I hear you here, and considered doing precisely this - however, there just isn’t enough code in CommandBase once you remove the Sendable bits to justify it, imo. There’s absolutely nothing stopping you from ignoring the premade commands and writing your own if you want to handle Sendable in your own way - this is one of the major reasons I’ve refactored Command to be an interface, rather than an abstract class; the scheduler only cares about the interface, the base is just there for convenience. The premade classes really ought to be Sendable, though, because it provides teams (especially newer teams) a really convenient way to schedule them through the dashboard.
I’m not sure there’s any particularly good way of allowing teams to “optionally” use the Sendable base in the premade commands, given Java’s lack of support for multiple inheritance.
You should take a look at the “SelectCommand” class.
This can be really handy but I think it would be confusing for new teams, and it would greatly increase the amount of code needing to be written. Additionally, it would very tightly couple the framework to the initial decisions about what types of commands to offer; this is great if you’re really certain that those archetypes will endure but think the underlying implementations may change, but I’m not sure that’s the case here.
I would be interested in a way to describe an auto routine as a decision tree, almost like how process scheduling is typically taught in an Operating Systems class.
Essentially, have a command A that is a pre-requisite for commands B and C. B and C won’t run until A is finished. D depends on B, and E depends on D and C. So A runs first, then B and C run in parallel, then once B finishes, D runs. Once D and C finish, E will run.
This may be possible in the current structure, but it’s not easy. I’m also not sure the best way to structure it, since manually creating a tree in code is always verbose.
I definitely agree with all of that. I do, however dislike that every single method is a default method. I feel that initialize(), execute(), end(), isFinished(), getRequirements() and maybe getName() could all be non-default to indicate that, hey these methods are for you to define. Also, if you’re going to keep getRequirements() as a default method, you could at least return Collections.emtpySet() instead of a new HashSet<>() each time.
Also, in SendableCommandBase, you could wrap the requirements in an unmodifiable set to avoid possible bugs from people accidentally doing something to a Set that isn’t their’s. Again, personal preference there.
I believe that’s possible with ParallelCommandGroup and SequentialCommandGroup. I think the idea is to create the desired autonomous Command in autonomous init (or before then), then do exactly that. Another great thing about how this is designed, is you don’t have to create a whole CommandGroup class, you can easily create a Parallel or Sequential CommandGroup.
I like having the four primary methods defaulted to no-ops because it saves a lot of clutter in commands that don’t use them; I think you’re correct that getRequirements() ought not be defaulted, though.
This is very easy to do by composing the existing CommandGroup classes, and is in fact the manner in which they are intended to be used. There will probably need to be a fair bit of instruction on how to do this in ScreenSteps, though, because composition is not obvious to beginning programmers (even if it is very natural once you “get” it).
Regarding creating more complex sequences than CommandGroup allows, this is a design doc I put together for a finite state machine addition to command-based programming a few years ago that we never got around to implementing. https://github.com/PeterJohnson/design-docs/blob/command-state-machine/command-state-machine.adoc
Side note: I would recommend a similar type of document be written for any major additions/changes (e.g. new APIs). The overall outline of summary, motivation, design, drawbacks/pitfalls, alternatives, unresolved questions, and future work really does help you think through all the aspects of a major change and more clearly communicate them to others.
I have confirmed that you can run your own unit tests in sim with gradlew test, so this is actually already solved. Teams should just be careful to reset the static scheduler state between tests.
I personally consider the syntax of addSequential() and addParallel() to be horrendously confusing. How would I go about adding parallel branching logic? I would love to see something more understandable for this use case.
I also would love to see a more functional syntax. I’ve been really loving how I can use Instant command as a function, but that’s really it currently.
My main use case for the Command system is using it to implement an autonomous routine on top of Times robot, so it’s a bit different than the standard (i.e. I don’t extend Subsystems and instead pass my encapsulated objects as constructor parameters). I also rely on making my Commands configurable, for example I have a Move command that can be reused to move the robot different distances.
Overall, the Command system is in my opinion very unintuitive and hard to understand, in large part due to the syntax and its heavy reliance on “black box” Subsystem and Scheduler logic.
Through composition. Think:
Command fooSequence = new SequentialCommandGroup(foo, bar);
Command bazParallelGroup = new ParallelCommandGroup(fooSequence, baz);
This kind of structure can be extended indefinitely, to accomplish a great variety of different things.
If you want to branch conditionally, you can simply use a ConditionalCommand or a SelectCommand as part of your chain of composed commands.
The new PIDCommands make heavy use of functional interfaces. Past that, it wouldn’t be hard to write your own command that takes lambdas for each of the primary methods, but I’m not sure that’s useful enough to be worth including.
There’s basically no black box logic in subsystem in the rewrite - what you see is what you get, which is a mostly-empty interface. The scheduler code is also now quite straightforward and well-commented, and doesn’t really do anything surprising.
Why can’t the scheduler be something that teams initialize if they need to use it? (So it’s not a singleton and can just be created when needed) To make the scheduler work you already have to manually call its run method so why not just have teams create the instance as well? This change would be a big one, but it would make everything more unit-testable. By doing this you could also have individual subsystems that handled commands on their owns without worrying about some mysterious static scheduler instance that could be mutated or added to in some other part of your code.
I know the changes are big ones, but one of the reasons I don’t like Command based robots right now are because of how tightly coupled Commands are to WPILib. I’m not sure if it’s possible to find middle ground between people like me and new programmers that would benefit from the Command framework being tightly coupled to other WPILib functions.
I definitely agree with this. But I don’t think addSequential or addParallel are too hard to understand. I agree that the scheduler is pretty magical, though.
I’ve been considering doing this; there’s really nothing requiring it to be a singleton at the moment (in fact, the constructor is package-private because I instantiate temporary ones for my unit tests, anyway). There’d still be some lingering global state in the command group allocation list, though, which would be really inconvenient to make non-global (I don’t want to have to inject the list into every single command group).
Out of curiosity, why not split the responsibilities of subsystem allocation and command scheduling to two different classes then?
One thing I don’t think we need is global state. From what I can tell, global state is used to make sure that Commands that are being used in one place, such as a Command Group cannot be used in another Command Group. Assuming each Command no longer has access to the global scheduler instance, each command would have to keep track of if it’s currently being ran. Yes, this makes each implementation of Command more tedious, but would take away worrying about the state of a magical singleton and instead make everything less tightly coupled.
An example of how I would do this is on line 47 and 48 here: https://github.com/retrodaredevil/action-lib/blob/master/src/main/java/me/retrodaredevil/action/SetActionMultiplexer.java
There’s no singleton here; the command allocation list is a static variable in CommandGroupBase, and teams can manually clear it if they really want to. I really don’t think one would want to, though, because running a command in two different places at once is a terrible idea that will almost certainly result in inconsistent command state and undesired behavior - the ability to clear it in the first place is only included to facilitate re-allocating commands into different groups if the earlier groups are known to be no longer used.
Having commands themselves know whether they are in command groups would break encapsulation rather badly and make extensive composition exceedingly difficult. It would also make it pretty much impossible to keep Command an interface.
Because these are essentially the same task. Whatever is doing the scheduling needs to know about which subsystems are currently required, because that thing is responsible for ensuring there are no resource conflicts.
I think you might have misread, though - this isn’t really global state at the moment, it’s contained in scheduler which can easily be made non-singleton without changing much (though it’s questionable how useful that would be, because there’s no really good reason to have more than one scheduler, and it removes the ability to have a bunch of convenience methods in Command). The thing that’s global state is the list of all currently-grouped commands, which lives in CommandGroupBase. This is what prevents users from accidentally running a command in a commandgroup from two places at once (the scheduler itself cannot/should not do this, as it does not see commandgroup internals).
Ah you’re right. I looked more closely and there aren’t that many places were the scheduler instance is heavily depended on. What I was thinking of, instead of Commands knowing what Command Groups they belong to, if a Command is added to a Command Group, that Command group should check certain state of the Command, such as if the Command is actively being ran. If the Command is “active” when it should be (e.g. when a Command Group’s initialize is called), that part of the code can throw an IllegalStateException or AssertionError (or something similar) to indicate that that shouldn’t happen. I guess a downside to that is that it would fail later than it currently could right now. (I think we can agree that throwing an exception sooner when something is wrong is always good)