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

[commands] Add static Trigger factories for robot mode changes #5902

Merged
merged 25 commits into from
Nov 14, 2023

Conversation

DeltaDizzy
Copy link
Contributor

@DeltaDizzy DeltaDizzy commented Nov 10, 2023

Included factories:

  • autonomous()
  • teleop()
  • disabled()
    - [ ] endOfMatch() (would use isTeleopDisabled())?

Are there any other conditions that should be included? I chose to use the is[Mode]Enabled() variants to ensure they only trigger when a match is in play.

Resolves #5151
Resolves #5916

Still needed:

  • Javadocs
  • C++ impl
  • C++ docs

@shueja
Copy link
Contributor

shueja commented Nov 10, 2023

Watch out for isTeleopDisabled() firing on driver station connection and/or code boot. I don't know if it does but it might not be the best thing to use on its own for an end-of-match trigger.

@DeltaDizzy
Copy link
Contributor Author

To my understanding Triggers fire on rising edges, not pre-existing conditions, so it should be fine?

@shueja
Copy link
Contributor

shueja commented Nov 10, 2023

If a team switches to autonomous mode and back to teleop, would a trigger on isAutonomousDisabled fire, followed by one on isTeleopDisabled? Point is, it's not necessarily the semantics of "onTeleopDisable", and you probably want to track the falling edge of isTeleopEnabled.

@ThadHouse
Copy link
Member

End of Match is undetectable. The only way to detect it is on a DS disconnect, but that will happen any time the DS disconnects, not only when the match ends. That is also delayed by a few seconds before it occurs.

@shueja
Copy link
Contributor

shueja commented Nov 10, 2023

Well, I suppose you can check isTeleop() && getMatchTime() == 0 but that's kind of imprecise still.

@ThadHouse
Copy link
Member

ThadHouse commented Nov 10, 2023

Well, I suppose you can check isTeleop() && getMatchTime() == 0 but that's kind of imprecise still.

That isn't guaranteed. getMatchTime() is not guaranteed to hit 0. Also the isTeleop() will keep on until the DS disconnects.

A mix of the DS connected bit and FMS connected bit would likely be the best we get, but that won't detect the difference between a radio reboot or actual match end. But to the robot those cases do look identical, so there is no way to detect them.

@Starlight220
Copy link
Member

What's the usecase for endOfMatch?

@DeltaDizzy
Copy link
Contributor Author

The usecase for endOfMatch would be for e.g. games like 2020, where very specific post-match (as opposed to just any disable) behaviors may be desired. I'm not super attached to it though, so if end of match is truly undetectable I am open to scrapping it.

@Starlight220
Copy link
Member

Detecting end of match seems to be quite nuanced with multiple possible semantics, so I think that the logic should be explicit in team code (developed by the team).

@KangarooKoala
Copy link
Contributor

Looking at the lifecycle methods of IterativeRobotBase, test mode is also missing a Trigger. (robotInit/Periodic() and simulationInit/Periodic(), however, shouldn't need a Trigger since their value would never change during robot execution)

@DeltaDizzy
Copy link
Contributor Author

DeltaDizzy commented Nov 11, 2023

The javadoc description and return sections feel a but repetitive but I dunno what a better way would be if it is even needed.

@DeltaDizzy DeltaDizzy marked this pull request as ready for review November 11, 2023 08:36
@DeltaDizzy DeltaDizzy requested a review from a team as a code owner November 11, 2023 08:36
@calcmogul
Copy link
Member

/format

@Starlight220
Copy link
Member

Please add tests.

@calcmogul
Copy link
Member

/format

Copy link
Member

@calcmogul calcmogul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation looks good. Just needs C++ tests.

@DeltaDizzy
Copy link
Contributor Author

C++ tests are blocked on resolution of #5916.

@DeltaDizzy DeltaDizzy requested a review from calcmogul November 12, 2023 19:30
@DeltaDizzy
Copy link
Contributor Author

/format

@DeltaDizzy
Copy link
Contributor Author

/format

ThadHouse
ThadHouse previously approved these changes Nov 13, 2023
rzblue
rzblue previously approved these changes Nov 13, 2023
Co-authored-by: Tyler Veness <[email protected]>
@PeterJohnson PeterJohnson merged commit 09f3ed6 into wpilibsuite:main Nov 14, 2023
24 checks passed
@DeltaDizzy DeltaDizzy deleted the gametrigger-static-factories branch November 15, 2023 15:46
Starlight220 pushed a commit to Starlight220/allwpilib that referenced this pull request Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[commands] C++ Trigger has no API to access state Add Trigger factories for common robot state events
8 participants