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

[wpilibc] Remove Alert magic static with map lookup #7712

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

virtuald
Copy link
Member

The magic static adds yet another thing that needs to be reset if you want to run a unit test with a completely reset wpilib state. Use the existing Sendable static map to store the data instead.

This leaks memory if ResetSmartDashboardInstance is called.

Alternative to #7711

The magic static adds yet another thing that needs to be reset if you
want to run a unit test with a completely reset wpilib state. Use the
existing Sendable static map to store the data instead.

This leaks memory if ResetSmartDashboardInstance is called.
@virtuald virtuald force-pushed the remove-static-alert branch from feaf3a5 to 5512e72 Compare January 20, 2025 16:11
@PeterJohnson
Copy link
Member

I'd prefer to figure out a global fix using ManagedStatic or similar for 2027, as this approach feels like a hack that we'd need to back out for Sendable changes. Is there a compelling reason to make this change for 2025?

@virtuald
Copy link
Member Author

virtuald commented Jan 20, 2025

Well, I tried to convert the C++ tests to python and every test after the first one fails, because we restart NT each time a new test occurs (unlike the C++ tests).

If anyone tries to use Alerts in their robotpy robot tests it will fail after the first time because robotpy resets all the reset of the wpilib state.

An alternative would be to add a frc::impl::ResetAlerts function to clear the static variable, adding yet another reset function. It would be more invasive because frc::alert holds a pointer to the set inside the static, so those would need to be converted to shared_ptr in addition to adding the reset function.

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

Successfully merging this pull request may close these issues.

2 participants