refactor: consolidate duplicate PID controllers (pidmayuresh3 → wrapper) — fixes #378#435
Merged
pradeeban merged 3 commits intoControlCore-Project:devfrom Feb 20, 2026
Conversation
Replace the duplicate PID implementation in tools/pidmayuresh3.py with a thin backward-compatibility wrapper that re-exports from pidmayuresh.py. - pidmayuresh.py remains the canonical implementation (uses logging) - pidmayuresh3.py now imports from pidmayuresh.py with a DeprecationWarning - No PID logic, API, or default parameters changed - All existing imports of pidmayuresh3 continue to work Closes ControlCore-Project#378
There was a problem hiding this comment.
Pull request overview
Consolidates the duplicate PID controller implementation by turning tools/pidmayuresh3.py into a backward-compatibility wrapper around the canonical tools/pidmayuresh.py, while also cleaning up minor whitespace in two bang-bang controller scripts.
Changes:
- Replaced the duplicate implementation in
tools/pidmayuresh3.pywith a wrapper that emits aDeprecationWarningand re-exports the canonical module. - Removed stray whitespace in
tools/bangbang.pyandhumanc/bangbang.py.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/pidmayuresh3.py | Replaces duplicate PID implementation with a deprecation wrapper that re-exports the canonical controller. |
| tools/bangbang.py | Whitespace cleanup in controller function. |
| humanc/bangbang.py | Whitespace cleanup in controller function. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Apply Copilot review suggestion — try relative import first (from .pidmayuresh) for package-style usage, with fallback to absolute import for direct script execution.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
hey @pradeeban Sir
Resolves #378 —
tools/pidmayuresh.pyandtools/pidmayuresh3.pycontainedidentical PID controller implementations differing only in logging (
loggingmodule vs
print()).Changes
tools/pidmayuresh3.py: Replaced the full duplicate implementation witha thin backward-compatibility wrapper that re-exports from
pidmayuresh.pyand emits a
DeprecationWarning.tools/pidmayuresh.py: No changes — remains the canonical implementationusing the
loggingmodule.What stays the same
pid_controller(state, ym, sp, Kp, Ki, Kd, sigout, cin, low, up)tryparamvaluesWhy this approach
pidmayuresh.pyprint()removed; all output usesloggingpidmayuresh3.pywrapper re-exports everythingTesting
py_compilepasses for both filestest_openjupyter_security.py(unrelatedFLASK_SECRET_KEYissue)