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.

Sign up for GitHub

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

Jump to bottom

[cmd] Revert Trigger implementation #4673

Merged
merged 5 commits into from
Nov 28, 2022
Merged

[cmd] Revert Trigger implementation #4673

merged 5 commits into from
Nov 28, 2022

Conversation

Starlight220
Copy link
Member

This might fix #4574 and similar.

There are no user-facing changes in this PR.

@Starlight220 Starlight220 requested review from a team as code owners November 20, 2022 20:48
@Starlight220
Copy link
Member Author

/wpiformat

Oblarg
Oblarg requested changes Nov 27, 2022
View reviewed changes
Copy link
Contributor

@Oblarg Oblarg left a comment

Choose a reason for hiding this comment

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

Taking back my approval temporarily: it occurs to me that perhaps this could be fixed at the Event level instead? The problem still exists there.

Oblarg
Oblarg approved these changes Nov 28, 2022
View reviewed changes
Copy link
Contributor

@Oblarg Oblarg left a comment

Choose a reason for hiding this comment

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

approved; stick a pin in this, though, we should use the delegation once the lower level is fixed. If we can't, the lower level stuff probably isn't really robust.

@Starlight220
Copy link
Member Author

This takes Trigger out of the problem's effect; fixing BooleanEvent is somewhat lower priority (because no one uses it) and can be done separately.

@PeterJohnson
Copy link
Member

PeterJohnson commented Nov 28, 2022 •
edited
Loading

Is there a specific previous PR this is reverting? What are you reverting the implementation from and to?

@Starlight220
Copy link
Member Author

Trigger was refactored to use BooleanEvent when it was introduced in #4104, and this reverts to the original implementation.

@PeterJohnson PeterJohnson merged commit cb38bac into wpilibsuite:main Nov 28, 2022
@Starlight220 Starlight220 deleted the simple-trigger branch November 29, 2022 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Oblarg Oblarg Oblarg approved these changes

@rzblue rzblue rzblue approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Edge-based BooleanEvents can't be used in multiple places
4 participants
@Starlight220 @PeterJohnson @rzblue @Oblarg