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

Fixed custom-PID response time after being above setpoint #752

Closed
wants to merge 2 commits into from

Conversation

mjrand
Copy link
Contributor

@mjrand mjrand commented Sep 12, 2024

Fixed the slow response time the custom PID had to going under the setpoint after it was above the setpoint for a long time.

Description

Updated the I term in the custom_PID function so it could no longer go negative.
Being above the setpoint for a long time would cause the I value to become very negative.
It would take a long time for the custom_PID to overcome that large negative value so the PID would be effectively off for that time.

Motivation and Context

This issue was prominent after IV curves when the IV curves would heat the whole focal plane above the setpoint
The PID could take upwards of 30 mins to come back online after IV curves because of this.
The custom PID should respond in <5 minutes now, depending on the I value it is set to.
It is not perfect but this relatively simple solution doesn't affect normal PID operations and should improve functionality a lot.

How Has This Been Tested?

I tested this code on both satp1 and satp2.
For satp2 I simple heated the FPA and then dropped the setpoint to something very low.
The FP was above the setpoint for a few minutes while the FPA cooled. The PID then responded nearly instantly after the FPA dipped below the setpoint and turned the heater on immediately (slowly because the I value was untuned and low).

On satp1 I ran multiple IVs with the updated code. The new custom_PID function turned back on immediately after the IVs were finished and resumed a normal PID operating temperature in < 5minutes after they were done (instead of the ~30 minutes without the changes). I also ran a test scan for 15 minutes with the new custom_PID function to make sure the changes did not affect the operation. The custom_PID operated normally as intended during the scans.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

jlashner and others added 2 commits September 9, 2024 10:58
* Adds grip and ungrip fns to hwp-supervisor

and safety checks

* Adds check of boresight angle

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Expose supervisor control state info with rest of hwp state

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add current_state_type to spin control function

* Adds ACU blocking flag and handshake to HWP Supervisor

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* fix typo and register gripper tasks

* Adds ACU blocking flag timestamp and timeout

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Adds acu instance id default and `--no-acu` option

* Adds checks on pid last update time for gripper safety

* Adds better logging to ensure_grip_safety

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: ykyohei <38639108+ykyohei@users.noreply.github.com>
* Created a new agent.py which dynamically reconnects sometimes, but still has some errors

* Fixed the code to dynamically reconnect. See the relevant DAQ discussion for more.

* Removed numpy

* Removed the commented out call for psu.test() as it ended up being unnecessary

* Removed status messages and added logging as per daq-discussions#106

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Made the easy changes

* Made the easy changes

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Merged main

* Changed the nested logic to use elif

* Finished requested changes

* Changed the reconnect logic again.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Uncommented the correct drivers.

* Remove extra local drivers import

* Remove extra word added to comment

I think this snuck in during a git merge, looks like a 'git' command got added
to the comment in 4567a0b.

---------

Co-authored-by: simonscryo <simonscryo@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@mjrand
Copy link
Contributor Author

mjrand commented Sep 12, 2024

what the heck did I do how does this PR have HWP changes in it

@mjrand
Copy link
Contributor Author

mjrand commented Sep 12, 2024

haha oops I think I'm dumb and PRd commits from main into my branch whoops

@mjrand mjrand closed this Sep 12, 2024
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.

3 participants