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

[cmd] Allow specifying initial condition of Trigger bindings #7425

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

KangarooKoala
Copy link
Contributor

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.

@KangarooKoala KangarooKoala requested a review from a team as a code owner November 23, 2024 03:04
Copy link
Contributor

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

@KangarooKoala
Copy link
Contributor Author

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

@KangarooKoala
Copy link
Contributor Author

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
Copy link

tom131313 commented Dec 2, 2024

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

KangarooKoala commented Dec 2, 2024

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants