[cmd] Allow specifying initial condition of Trigger bindings

[KangarooKoala]

If someone has a better name for NEG_CONDITION, please let me know!

Fixes #7413 and fixes #5608 by letting users specify NEG_CONDITION to force an initial edge or FALSE to force an initial edge if the condition is true.

[github-actions]

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

[KangarooKoala]

Mac failures seem unrelated. (One is Mac CMake having trouble with dependencies, the other is a flaky ntcore test)

[KangarooKoala]

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

Running into issues making the PR (See robotpy/robotpy-commands-v2#77), if anyone wants to do it for me that'd be greatly appreciated. (If not, I'll write the PR eventually and rely on CI)

[tom131313]

This correction is a step in the right direction but it is only a partial fix. My example code in issue #7413 shows how to assure that the initial value is checked when the command initially runs and not prematurely checked as can happen if the robot mode doesn't match the command's allowed modes to run.
if ((command.runsWhenDisabled() || DriverStation.isEnabled()) // command can be scheduled

[KangarooKoala]

This correction is a step in the right direction but it is only a partial fix. My example code in issue #7413 shows how to assure that the initial value is checked when the command initially runs and not prematurely checked as can happen if the robot mode doesn't match the command's allowed modes to run. if ((command.runsWhenDisabled() || DriverStation.isEnabled()) // command can be scheduled

You bring up an excellent point, but the better solution is to logical AND the trigger with the robot being enabled (most likely in team code at the command binding site so you can tell if the command can run when disabled), because the code in the referenced issue will only affect the initial enable but not subsequent ones, which would arguably be worse than no check because of the greater complexity of the behavior and the potential to mask issues that can (and by Murphy's law will) show up in a match (either entering teleop or during a match restart).

[tom131313]

I don't know how to find and check the final code version that will be put into WPILib but your intermediate version of the Java (I didn't check C++) has an extraneous or out of context comment , kCondition (no initial edge) by default on the two-parameter version of all the command specifiers. I understand it's intended to be a reference to the default behavior of the one-parameter version and it wouldn't hurt to cross-reference the two possible versions.

For the user to get with 100% surety what likely is the expected behavior, more complicated coding must be specified (your implemented change plus my suggested change plus whatever more complex behavior is actually desired for multiple enables/disables). I suggest more complete instructions on how to use these methods. I'd like to see that in the coding Javadoc.

[KangarooKoala]

I don't know how to find and check the final code version that will be put into WPILib

If you want to see all of the files on the PR branch (but not updated against main), you can just go to https://github.com/KangarooKoala/allwpilib/tree/trigger-initial-state. (From the top of the page, right below the title)

If you want to view the files after the merge, I think you have to merge it in locally:

1. Clone WPILib if you haven't already (git clone https://github.com/wpilibsuite/allwpilib; cd allwpilib)
2. Add KangarooKoala as a remote (git remote add KangarooKoala https://github.com/KangarooKoala/allwpilib)
3. Fetch KangarooKoala remote (git fetch KangarooKoala)
4. Update your main branch (git checkout main; git pull)
5. Checkout a new branch tracking the PR branch (git checkout --track KangarooKoala/trigger-initial-state)
6. Merge in main (git merge main)

Then you can view the files locally in your IDE or text editor of choice.

your intermediate version of the Java (I didn't check C++) has an extraneous or out of context comment , kCondition (no initial edge) by default on the two-parameter version of all the command specifiers. I understand it's intended to be a reference to the default behavior of the one-parameter version and it wouldn't hurt to cross-reference the two possible versions.

Could you clarify this? I'm not sure whether you're saying that the note is unnecessary ("extraneous or out of context comment") or necessary ("I understand it's intended to be a reference to the default behavior of the one-parameter version")?

For the user to get with 100% surety what likely is the expected behavior, more complicated coding must be specified (your implemented change plus my suggested change plus whatever more complex behavior is actually desired for multiple enables/disables)

I agree that we could make it easier to get the (probably) expected behavior*. I will note though that I think this is an overestimate of the complexity required. The second two changes can be replaced with just using trigger.and(RobotModeTriggers.disabled().negate()) instead of trigger, since by forcing the condition low when the robot is disabled, we delay the rising edge until the robot is enabled.

* To make sure we're on the same page, here's a definition of the behavior for normal commands (that don't run when disabled):

• onTrue is triggered if the condition is true when the robot goes from disabled to enabled or when the robot starts enabled
• onFalse is triggered if the condition is false when the robot goes from disabled to enabled or when the robot starts enabled

I suggest more complete instructions on how to use these methods.

I agree that the documentation should be improved. I'll try to work on that tonight, though I might be busy. (Update: I was in fact busy, so I couldn't get to it today. Not sure when I'll have time to work on it.)

[tom131313]

Could you clarify this?

For example, you see in the two-parameter version of whileTrue there is a reference to a default value. There is no default value in Java for parameters. The comment only makes sense if browsing the code for the one-parameter version of whileTrue and we notice that the one-parameter version is the two-parameter version with a built-in (default) value. I tried to highlight the code below:
/**

• Starts the given command when the condition changes to true and cancels it when the condition
• changes to false. The command is never started immediately.

• Doesn't re-start the command if it ends while the condition is still `true`. If the command

• should restart, see {@link edu.wpi.first.wpilibj2.command.RepeatCommand}.
• @param command the command to start
• @return this trigger, so calls can be chained
  */
  public Trigger whileTrue(Command command) {
  return whileTrue(command, InitialState.kCondition);
  }

/**

• Starts the given command when the condition changes to true and cancels it when the condition
• changes to false.

• Doesn't re-start the command if it ends while the condition is still `true`. If the command

• should restart, see {@link edu.wpi.first.wpilibj2.command.RepeatCommand}.
• @param command the command to start
• VVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVV
• VVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVV
• VVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVV
• @param initialState the initial state to use, kCondition (no initial edge) by default
• AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
• AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
• AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
• @return this trigger, so calls can be chained
  */
  public Trigger whileTrue(Command command, InitialState initialState) {
  requireNonNullParam(command, "command", "whileTrue");
  m_loop.bind(
  new Runnable() {
  private boolean m_pressedLast = getInitialState(initialState);

• To make sure we're on the same page, here's a definition of the behavior for normal commands (that don't run when disabled):

• onTrue is triggered if the condition is true when the robot goes from disabled to enabled or when the robot starts enabled

I assume you mean this behavior is possible with your new onTrue AND your suggested addition to the Trigger condition of checking for disabled/enabled. Without that extra check the CommandScheduler just ignores the command but the m_pressedLast is set and we've burned our "first-time" switch.

I agree the coding isn't complex but it is rather subtle for inexperienced high-schoolers and mentors who are not immersed in this thus my call for documentation. I'd be glad to help any way I can.

• when the robot starts enabled

Is this possible? I've not been on the drive team for a competition so I guess I don't know what happens when the robot restarts in the middle of a match. I had always assumed it was like without an FMS and robot started disabled and the technician had to click the driverstation Enable.

[KangarooKoala]

Still working out cleaner names that better match what users care about that also make sense for all of the different binding methods (onChange(), onTrue(), onFalse(), whileTrue(), whileFalse(), toggleOnTrue(), and toggleOnFalse()).

[tom131313]

Still working out cleaner names that better match what users care about that also make sense for all of the different binding methods (onChange(), onTrue(), onFalse(), whileTrue(), whileFalse(), toggleOnTrue(), and toggleOnFalse()).

I suggest deprecating the two toggle methods. The documentation discourages their use and I agree with the documentation not to use them.

The onChange, of course, I have no experience but it sounds like a toggle to me with the same problem as the toggle. But it isn't at all like a toggle and could use a better name. I don't have a good suggestion at the moment since I have not used it but I'll start off a discussion with onValueChange or listenForChange or listenForNewData or listenForNewValue.

[KangarooKoala]

I suggest deprecating the two toggle methods.

Maybe- I see why, but it can also be useful for cases besides binding to a controller button. (Maybe toggle a command when entering a certain state in the code)

I'd love to talk about this further, but that should be done in a separate issue.

The onChange, of course, I have no experience but it sounds like a toggle to me with the same problem as the toggle.

As the name implies, it acts like onTrue() and onFalse(), except instead of only scheduling the command when the condition goes from false to true or from true to false, it schedules the command whenever the condition makes any change. You can view the implementation here.

Could you please clarify a) why you think it sounds like a toggle and b) what you would want in a better name?

By the way, in general, if you want to see what something in WPILib does, you can go to the repository's GitHub page, type /, and then type in the name of the method, and it should show you a list of results. (You can also scroll to the top and click the "Type / to search box" instead of typing /)

[tom131313]

deprecating the two toggle methods

Since you where evaluating name changes I thought making the problem go away was a possibility. They are dangerous commands whose use is not to be done carelessly but if there is a need for them, then they work as advertised.

All the Trigger action methods refer to the Trigger condition state except for toggleOnTrue/False refers to changing the Command state in addition to the Trigger condition state. Clarification could be toggleCommandOnTrue/False.

onChange ... schedules the command whenever the condition makes any change

I looked at some code I thought was the user interface for this but apparently I saw the wrong code and completely misunderstood what onChange does. I'm sorry for injecting my confusion.

Thanks for the GitHub tip.