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

[wpilib, commands] Cache controller BooleanEvents/Triggers and directly construct Triggers #6738

Merged
merged 7 commits into from
Jul 29, 2024

Conversation

Gold856
Copy link
Contributor

@Gold856 Gold856 commented Jun 13, 2024

Fixes #5903, resolves #6445 by updating it to work with the HID templates. (Updating those is great by the way, you just have to change a few lines and regenerate!)

@Gold856 Gold856 requested review from a team as code owners June 13, 2024 03:42
Copy link
Contributor

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

@Gold856
Copy link
Contributor Author

Gold856 commented Jun 13, 2024

I don't think I need to do anything for Python? Python Commands already directly constructs Triggers, and since objects are ref-counted, I don't think there's much of a GC concern either.

@Gold856 Gold856 changed the title [wpilib, cmd] Cache controller BooleanEvents/Triggers and directly construct Triggers [wpilib, commands] Cache controller BooleanEvents/Triggers and directly construct Triggers Jun 13, 2024
Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

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

I'm concerned about the axis caches, since there's no guarantee that the threshold is in the range [0, 50). (First, it's plausible that someone would want a negative threshold for e.g. the left x axis (which would lead to a conflict between the axisGreaterThan and axisLessThan caches), and second, although it wouldn't make sense, a user could pass a really high number, which should be handled in some way to avoid having a really difficult to track bug.)

@Gold856
Copy link
Contributor Author

Gold856 commented Jun 14, 2024

Yeah, I'm not sure how to handle axis caches because they're done in a unique way. I suppose I could nest the Map even further (Map<EventLoop, Map<Integer, Map<Double, Trigger>>>), but I feel like that's starting to just be extra overhead for a feature that people might not use.

@Alextopher
Copy link
Contributor

Alextopher commented Jun 14, 2024

I try to lean towards keeping maps as flat a possible. How about Map<EventLoop, Map<Pair<Integer, Double>, Trigger>. Then there is no chance a custom key function can be wrong.

@PeterJohnson
Copy link
Member

The C++ classes need mutexes as well.

@Gold856
Copy link
Contributor Author

Gold856 commented Jul 28, 2024

C++ classes don't cache BooleanEvents or Triggers, so they don't need mutexes.

@PeterJohnson PeterJohnson merged commit 3c2bdaf into wpilibsuite:main Jul 29, 2024
33 checks passed
@Gold856 Gold856 deleted the commands-joystick-cache branch July 29, 2024 19:49
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.

Periodically calling CommandXboxController button functions causes CommandScheduler loop overruns
5 participants