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

Add Lambda Support to InstantCommand for C++ and Java #1262

Merged
merged 1 commit into from Sep 12, 2018

Conversation

Projects
None yet
3 participants
@msoucy
Contributor

msoucy commented Aug 8, 2018

An ActionCommand is derived from a concept from the RobotDotNet implementation of WPIlib. The core concept is that it "lifts" a language-native callable to become a Command.

It also is influenced by RobotDotNet's SubsystemCommand class, which I plan on making a PR for in the future. The rationale for this is that it allows declarative command creation, like so:

    public Command closeOuter() {                                                                                                                             
        return new ActionCommand("Close Outer Manipulator", this, this::closeOuterManipulator);                         
    }
    private void closeOuterManipulator() {
        // ...
    }

    public Command openOuter() {
        return new ActionCommand("Open Outer Manipulator", this, () -> {
            // Lambdas are also accepted
        });
    }

This allows the same class to be used for "simple" commands as well as ones that need to require a subsystem, in a declarative syntax that allows for simpler development of "instant" commands.

I'm open to elaborate on use cases or questions if desired, as can @bot190

@msoucy msoucy force-pushed the msoucy:actioncommand branch 3 times, most recently from 0db5025 to a2018cb Aug 8, 2018

@msoucy

This comment has been minimized.

Contributor

msoucy commented Aug 8, 2018

The AppVeyor test failure doesn't seem to say anything related to this PR, not sure if I missed something.

@calcmogul

This comment has been minimized.

Member

calcmogul commented Aug 8, 2018

Yea, that looks like just a flaky test in something unrelated.

@calcmogul

This comment has been minimized.

Member

calcmogul commented Aug 8, 2018

Here's some internal discussion on the details of this PR:

Thad House:
Java people what is a Java built in type used for just standard void no
parameter lambdas? Action is not the right keyword for that PR. It was just
named that way because C#s generic delegate type is called action.

Austin Shalit:
No parameter and no return value? A runnable.

Thad House:
Ok.

Austin Shalit:
That is what they use in their source. RunnableCommand is not a great name
either.

Tyler Veness:
In C++ template stuff, we usually call arbitrary things that have an
`operator()` "Callable".

Austin Shalit:
In Java a callable is something else because callable has a return value.

Tyler Veness:
What about `FunctionCommand`?

Austin Shalit:
Function is a thing that takes a value and returns a value.

Tyler Veness:
Wat? Java named that something too? `void DoSomething()` is, by definition, a
function regardless of what Java says.

Austin Shalit:
https://docs.oracle.com/javase/8/docs/api/java/util/function/package-summary.html

Jaci Brunning:
Java takes it functional naming from the mathematical space, where a function
acts on data in some way to produce an output.

Austin Shalit:
@tyler - No.  That is a "method"

Tyler Veness:
Then I don't think we're going to reach a consensus here.

Jaci Brunning:
To put a new suggestion in, WrappedCommand.

Austin Shalit:
This is an "every language is different" problem. Have we identified the use
case for this?

Jaci Brunning:
Or, FunctionalCommand, since in Java they belong in the functional package, and
in C++ they belong in the functional stdlib header.

I can see the usecases. Less boilerplate to trigger something you've already
written elsewhere.

Tyler Veness:
Couldn't we do the same thing just by adding a constructor overload to Command?

Austin Shalit:
How do you make sure the user does not override the Run() method?

Tyler Veness:
Don't we already do something weird like have a Init() and _Init()?

ActionCommand calls the thing in Init(), not Run(). That doesn't seem right.

The extension of this PR would be a class that takes Init(), Run(), and
IsFinished() lambdas in the constructor. Object reusability might be a concern
in that scenario, like reusing the ActionCommand you made.

Austin Shalit:
Yes.

Oh. No... at least not in Java because lambdas are always fresh.

Wait... rereading: reusability in what sense?

Tyler Veness:
You make an ActionCommand and pass a lambda to it. What happens if the user then
adds that instance to two different CommandGroups?

Austin Shalit:
That is bad, because the ActionCommand has state.

Tyler Veness:
If you can't do that, you would have to create two copies of that ActionCommand,
which encourages copy-pasting because of verbosity of setup, which is also bad.

I basically ran into this problem with a lambda state machine framework I wrote
in 2015:
https://github.com/Team3512/Robot-2015/blob/master/src/AutonomousModes/AutoOneCanCenter.cpp.
Composability was basically impossible with this approach without lots of
copy-pasting. That's the style of code ActionCommand is condoning. Also with
that PR in particular, I'm not a fan of typedefing std::function<void()> as
Action. It makes it completely opaque as to what kind of function the user is
supposed to pass in there.

Austin Shalit:
Did you do the ConditionalCommand lambda PR?

Tyler Veness:
Someone else did and I took over the PR when they disappeared.

Austin Shalit:
Did the same problem exist there?

Tyler Veness:
Runnable constructor overloads would work great for things like PIDController
that need a sensor data source or arbitrary feedforward calculation, but that's
because the configuration space doesn't explode like it would for commands.

We'd hit the same copy-paste problem, yes. Lambdas are nice for one-off things,
but if you need composability, it's better to make a full-fledged class. The
risk we're running is that teams won't understand that trade-off and will create
messes.

In other words, PIDController is something you only make once, so specifying a
lambda saves you a lot of time. Commands are things you make a lot of. The
lambda being specified every time makes the amount of typing for reuse O(n).
Making a subclass is O(1). O(n) doesn't scale well for commands.

If we were super clear to users in documentation about that cross-over point, it
would be OK, but no one reads our docs (granted, it's debatable whether this is
a problem we should acknowledge; it's the users doing a dumb here by not reading
the docs for the library component before they use the library component).

Perhaps a solution to the O(n) problem is to have users create factory
functions. That brings things back to O(1) for code reusability.

In my opinion, we need to think through how lambdas are worked into the rest of
WPILib as a whole before we merge this PR. That'll help us come up with a more
holistic design and potentially avoid some API design mistakes.

@msoucy

This comment has been minimized.

Contributor

msoucy commented Aug 9, 2018

A few notes:

  • We're certainly not married to the name - it indeed only came around because we had been experimenting with RobotDotNet. FunctionalCommand seems reasonable to me, personally.
  • The factory methods that you describe are the intended use case. We used them extensively in our 2018 code, which made accessing commands from subsystems very straightforward.
  • The "shared state" shouldn't be a concern at this time, as the implementation of CommandGroup and Command specifically limit commands to have a single parent.
  • Playing off of that, it's possible for any Command derived class to have state that could cause problems if the command's object is reused.
  • The extension of this PR that was mentioned, passing in several lambdas, seems rather obtuse and would probably be better served with anonymous inner classes, and is in fact already supported by the existing framework.
@msoucy

This comment has been minimized.

Contributor

msoucy commented Aug 14, 2018

Hey, what is the status of this? I'd like to get it included if possible, especially because it checks one of the to do items you have, and I'm more than happy to make changes if necessary.

@calcmogul

calcmogul requested changes Aug 14, 2018 edited

I think we're agreed on using FunctionalCommand as the name for this.

I like the idea of doing dependency injection with the subsystems, and I think we should do that more in the other Command classes, but this approach only supports one subsystem. Making users specify additional subsystems externally via Requires() breaks symmetry. Perhaps you could make the constructor signature the name argument, the lambda, then a variadic argument list of subsystems at the end. See SpeedControllerGroup for how to do this.

Also, please use std::function instead of typedefing it to Action.

Just to explore other design options, this is essentially just a wrapper around InstantCommand. Couldn't we just add constructor overloads to InstantCommand so it uses the lambda passed in like #430 does? It would unify things quite nicely.

* @param subsystem The subsystem that this command runs on.
* @param action The action to take when the command is run.
*/
ActionCommand(const wpi::Twine& name, Subsystem* subsystem, Action action);

This comment has been minimized.

@calcmogul

calcmogul Aug 14, 2018

Member

This should take a reference instead of a pointer because it's a constructor.

* @param subsystem The subsystem that this command runs on.
* @param action The action to take when the command is run.
*/
ActionCommand(Subsystem* subsystem, Action action);

This comment has been minimized.

@calcmogul

calcmogul Aug 14, 2018

Member

This should take a reference instead of a pointer because it's a constructor.

@msoucy

This comment has been minimized.

Contributor

msoucy commented Aug 15, 2018

  • I can change the name when I get home tonight.
  • I can also make a separate PR to add the dependency injection, but that might take a bit longer depending on how tonight goes.
  • Honestly, my team has never run into a situation where a single command needs to require two subsystems, that wasn't better suited for a command group. So the thought didn't cross my mind. I personally don't particularly care for having the lambda in the middle of the argument list, but I asked around and I seem to be in the minority on that one.
  • Action was mainly typedefed to provide an easy name to access the appropriate type from, it can be removed as it is unlikely that people will want to pass around functions prior to putting them into a command.
  • Though that's possible, and I appreciate the sentiment, I feel that it would do more harm than good. By adding a default implementation of the initialize method, you'd remove all the checks that the compiler can currently do to prevent you from writing malformed commands. By doing this, you could inherit from InstantCommand and not define the initialize method, and it would compile cleanly, only potentially throwing an error at run time. EDIT: looking back, apparently initialize isn't purely abstract (pure virtual in C++) so those checks don't exist right now anyways. Is it worth it to change the default InstantCommand definition to run a Runnable/function?
@msoucy

This comment has been minimized.

Contributor

msoucy commented Aug 15, 2018

A few questions that came up while I was making changes (most only tangentally related):

  • Why is there a custom Set class?
  • Why does the command architecture use Enumeration instead of the recommended Iterable?
  • Should the function being wrapped be called in initialize() or _initialize()? Why are there two?

@msoucy msoucy force-pushed the msoucy:actioncommand branch 2 times, most recently from c2da73f to ad331cf Aug 16, 2018

@calcmogul

This comment has been minimized.

Member

calcmogul commented Aug 16, 2018

The CommandGroup thing makes sense. Fair enough. In that case, the lambda can stay at the end of the constructor.

I'm against the typedef because std::function<void()> is a standard language construct and is a great way to tell the user what kind of function to pass in just from the function signature. Hiding that information seems like a mistake.

I'm assuming by changing the "default definition" you mean the default constructor of InstantCommand. We need to keep the empty default constructor because things subclass InstantCommand, and removing that breaks those subclasses. We can add a constructor overload that takes a Runnable though.

The custom Set class is an artifact from the pre-8 Java days. We really should clean that up (probably a "separate PR" thing). Enumeration vs Iterable is probably for a similar reason.

_initialize() ensures that if the user forgets to call super.initialize() after overriding initialize(), that the stuff in _initialize() still runs. I don't really agree with that legacy design decision, but it is what it is. It should go in _initialize() since initialize() is reserved for user things.

@msoucy msoucy force-pushed the msoucy:actioncommand branch 2 times, most recently from 3f912cc to e3e101f Aug 16, 2018

}
void frc::FunctionalCommand::_Initialize() {
if (m_action) {

This comment has been minimized.

@calcmogul

calcmogul Aug 16, 2018

Member

This should call InstantCommand::_Initialize() as well since Command::_Initialize() is non-empty.

*/
@Override
protected void _initialize() {
if (m_func != null) {

This comment has been minimized.

@calcmogul

calcmogul Aug 16, 2018

Member

This should call super._initialize() as well since Command._initialize() is non-empty.

@msoucy

This comment has been minimized.

Contributor

msoucy commented Aug 17, 2018

I've pushed a fix that calls the parent methods.

By "default definition" I was actually referring to the definition of the _initialize function. If we're ok with having a check for a null function, as well as having every instant command carry around a few extra bytes of data for the pointer, then that could be done. It wasn't done that way to begin with because this class started out as a library for use within a team.

@calcmogul

This comment has been minimized.

Member

calcmogul commented Aug 17, 2018

Considering how frivolously Commands are created, a few extra bytes in InstantCommand likely isn't a concern.

if (m_action) {
m_action();
}
InstantCommand::_Initialize();

This comment has been minimized.

@calcmogul

calcmogul Aug 17, 2018

Member

This should be called before the contents of this function because m_completed (in Command) should probably be set to false before the rest of FunctionalCommand::_Initialize() runs.

if (m_func != null) {
m_func.run();
}
super._initialize();

This comment has been minimized.

@calcmogul

calcmogul Aug 17, 2018

Member

This should be called before the contents of this function because m_completed (in Command) should probably be set to false before the rest of FunctionalCommand._initialize() runs.

*
* @param action The action to take when the command is run.
*/
explicit FunctionalCommand(std::function<void()> action);

This comment has been minimized.

@calcmogul

calcmogul Aug 17, 2018

Member

Java uses func instead of action for the variable name. C++ should be changed to match.

*/
@Override
protected void _initialize() {
if (m_func != null) {

This comment has been minimized.

@calcmogul

calcmogul Aug 17, 2018

Member

The constructor bails out on null, so this if statement will always evaluate to true.

}
void frc::FunctionalCommand::_Initialize() {
if (m_action) {

This comment has been minimized.

@calcmogul

calcmogul Aug 17, 2018

Member

If the user is required to pass a std::function in the constructor, it's exceedingly likely that m_action isn't nullptr. The only way it could be nullptr is if the user explicitly passed in nullptr as the argument. That is definitely not normal usage or even a sane thing to do. As such, you could just remove the if statement here.

@msoucy msoucy changed the title from Add Action Command for C++ and Java to Add Functional Command for C++ and Java Aug 18, 2018

@msoucy msoucy force-pushed the msoucy:actioncommand branch 3 times, most recently from 54245ba to 4e0895f Aug 21, 2018

InstantCommand() = default;
virtual ~InstantCommand() = default;
protected:
std::function<void()> m_func;

This comment has been minimized.

@calcmogul

calcmogul Aug 23, 2018

Member

This needs to be initialized with nullptr.

Show resolved Hide resolved wpilibc/src/main/native/cpp/commands/InstantCommand.cpp
Show resolved Hide resolved wpilibc/src/main/native/include/frc/commands/InstantCommand.h
InstantCommand::InstantCommand(const wpi::Twine& name, Subsystem& subsystem)
: Command(name, subsystem) {}
InstantCommand::InstantCommand(std::function<void()> func)
: InstantCommand() {

This comment has been minimized.

@calcmogul

calcmogul Aug 23, 2018

Member

No need to call the default constructor InstantCommand(). It's called automatically.

@msoucy msoucy force-pushed the msoucy:actioncommand branch 2 times, most recently from 04f48b6 to 7630b0b Aug 23, 2018

@msoucy

This comment has been minimized.

Contributor

msoucy commented Sep 2, 2018

Is there anything remaining to do on this?

/**
* Create a command that calls the given function when run.
*
* @param func The func to take when the command is run.

This comment has been minimized.

@calcmogul

calcmogul Sep 2, 2018

Member

"The func to take" is inaccurate because _initialize() doesn't take the function; it runs the function. I'd say "The function to run when Command::Initialize() is called." for C++ and "The function to run when Command.initialize() is called." for Java. Same applies to the other constructors.

* Creates a new {@link InstantCommand InstantCommand}.
* @param name the name for this command
* @param requirement the subsystem this command requires
* @param func the function to run on initialize

This comment has been minimized.

@calcmogul

calcmogul Sep 2, 2018

Member

These comments aren't consistent with the C++ ones (and vice versa). We try to keep them identical except for javadoc-specific things like {@link}.

@msoucy msoucy force-pushed the msoucy:actioncommand branch 5 times, most recently from bbea543 to d640b18 Sep 3, 2018

@msoucy msoucy changed the title from Add Functional Command for C++ and Java to Add Lambda Support to InstantCommand for C++ and Java Sep 8, 2018

@msoucy

This comment has been minimized.

Contributor

msoucy commented Sep 8, 2018

The comments should be the same between versions now, apart from capitalization (which lines up with the convention of the files in question) and links.

@calcmogul

For doxygen and javadoc comments, there should be an empty comment line between the description and the @ tags. Some comments aren't doing that. Otherwise, this looks good to me.

@msoucy msoucy force-pushed the msoucy:actioncommand branch from d640b18 to c9b18fa Sep 8, 2018

@msoucy

This comment has been minimized.

Contributor

msoucy commented Sep 8, 2018

Changed - I was following the example of the rest of the file.

@PeterJohnson

This comment has been minimized.

Member

PeterJohnson commented Sep 8, 2018

@frcjenkins test this please

@PeterJohnson PeterJohnson merged commit 8b5dc53 into wpilibsuite:master Sep 12, 2018

7 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
frcjenkins - Athena Build finished.
Details
frcjenkins - Linux Build finished.
Details
frcjenkins - Mac Build finished.
Details
frcjenkins - Windows Build finished.
Details

@msoucy msoucy deleted the msoucy:actioncommand branch Sep 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment