Skip to content

Comments

fix: use computed PID frequency in pid2.py instead of hardcoded value (fixes #366)#417

Merged
pradeeban merged 1 commit intoControlCore-Project:devfrom
GaneshPatil7517:fix/pid2-use-computed-frequency
Feb 19, 2026
Merged

fix: use computed PID frequency in pid2.py instead of hardcoded value (fixes #366)#417
pradeeban merged 1 commit intoControlCore-Project:devfrom
GaneshPatil7517:fix/pid2-use-computed-frequency

Conversation

@GaneshPatil7517
Copy link

Summary

This PR resolves Issue #366 in pid2.py.

The controller correctly computed and clamped a PID-based freq value:

freq = KpF*PF + KiF*IF + KdF*DF
if freq > 30: freq = 30
if freq < 10: freq = 10

However, the computed frequency was ignored and replaced with a hardcoded value:

ustar = np.array([amp, 30])

This effectively disabled dynamic PID frequency control.

Changes Made

  • Replaced hardcoded 30 with computed and clamped freq
  • Preserved all PID gain logic
  • Preserved clamping behavior (10–30)

Impact

  • Restores correct PID frequency behavior
  • Enables dynamic frequency adjustment based on error feedback
  • Fixes functional bug without altering control structure

Scope

  • Single file modification (tools/pid2.py)
  • Single line changed (1 insertion, 1 deletion)
  • No concore-lite changes
  • No Verilog changes

Testing

  • Verified syntax with py_compile
  • Simulated PID controller over 50 timesteps: confirmed freq varies dynamically (11 unique values observed)
  • Verified clamping holds within [10, 30]
  • Confirmed output array now reflects computed PID frequency

This restores intended controller functionality.

Copilot AI review requested due to automatic review settings February 18, 2026 21:18
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a bug in tools/pid2.py where a computed PID frequency value was being ignored. The controller implemented a dual PID system controlling both amplitude and frequency, but the frequency output was hardcoded to 30 instead of using the computed and clamped freq variable. This fix restores the intended dynamic frequency control behavior.

Changes:

  • Replace hardcoded frequency value (30) with computed PID frequency variable (freq) in controller output array

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@pradeeban pradeeban merged commit 543d169 into ControlCore-Project:dev Feb 19, 2026
11 of 12 checks passed
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.

2 participants