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

[RST-10642] Allow the optimizer to remain disabled at startup #374

Merged
merged 2 commits into from
Jul 31, 2024

Conversation

svwilliams
Copy link
Contributor

Instead of automatically starting all of the plugins during construction, we should allow the optimizer to remain in the offline state until someone calls start(). The robot_localization package supported this feature with a disabled_at_startup flag. This PR only affects the operation of the fixed-lag smoother. The batch optimizer is missing most of this functionality.

Note: There is a subtle API change here. The base Optimizer class no longer starts the plugins during construction. This PR patches the BatchOptimizer is start the plugins during its constructor instead. But if there exists a 3rd party optimizer derived from the base Optimizer class, that 3rd party class will potentially break. I considered placing the plugin startup check inside the base Optimizer class. That would prevent this breakage and it would mean both the Fixed-lag Smoother and Batch Optimizer would benefit from the disabled_at_startup flag. However, I feel this reduces the flexibility of the derived Optimizer classes. Allowing the derived classes to have control over when the plugins are started and stopped feels better. Derived classes can implement additional logic and bookkeeping, handle any locks required, etc at the same time the plugins change state.

But feel free to disagree with me.

@svwilliams svwilliams merged commit 18d60b4 into devel Jul 31, 2024
4 checks passed
@svwilliams svwilliams deleted the RST-10642-allow-disabled-at-startup branch July 31, 2024 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants