Adds pacemaker to LS372 custom PID loop

simonsobs / socs

Simons Observatory specific OCS agents.

BSD 2-Clause "Simplified" License

12 stars 12 forks source link

Adds pacemaker to LS372 custom PID loop #687

Closed jlashner closed 2 weeks ago

jlashner commented 3 months ago

Adds pacemaker to regulate temp reading rate to LS372 custom PID loop.

Description

Adds pacemaker, to LS372 custom pid acquisition loop.

Motivation and Context

Nick noticed variable readout rates for temp data with the custom PID. Part of the reason is the lack of timing regulation in the PID control loop. This PR adds a OCS Pacemaker to the body of the pid loop to limit sampling to a specified rate, defaulting to 10 Hz. This will hopefully also speed up and improve PID processing by reducing the number of repeated samples.

How Has This Been Tested?

Not yet tested.

Types of changes

[X] 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:

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

BrianJKoopman commented 2 months ago

I held off on merging this since it wasn't tested. Let me know the state of testing/what we want to do. Also see relevant discussion in https://github.com/simonsobs/socs/discussions/689.

BrianJKoopman commented 1 month ago

Just wanted to check in on the status of testing on this again.

jlashner commented 1 month ago

I think the code itself probably doesn't need to be tested beyond the CI since its a simple change, however I'm not sure if we want to merge it without knowing the effect on the PID... @mjrand or @ngalitzki, are you planning on doing any sort of LS372 PID tests and can we maybe incorporate this?

ngalitzki commented 3 weeks ago

I'm starting to pick up the PID testing thread again, doing a test on SATP1 seems like a good place to start. @mjrand is planning to do so on 9/6. I'm working with JB to analyze impacts of different PID settings on CMB scan data so we can do some follow up if it at least appears to work.

mjrand commented 3 weeks ago

I can base a new branch off of this branch I think for my customPID changes so that I can test and merge both changes at once, if everyone is ok with that.

mjrand commented 3 weeks ago

I think this branch is behind another 372 commit so we may be on another github timeline with this. I will add this pacemaker change to another branch based on main because I'm too github illiterate to do good rebasing

BrianJKoopman commented 3 weeks ago

I think this branch is behind another 372 commit so we may be on another github timeline with this. I will add this pacemaker change to another branch based on main because I'm too github illiterate to do good rebasing

Sorry, I don't see another 372 PR open related to the custom PID. Is that something else you're working on?

mjrand commented 2 weeks ago

I think this branch is behind another 372 commit so we may be on another github timeline with this. I will add this pacemaker change to another branch based on main because I'm too github illiterate to do good rebasing

Sorry, I don't see another 372 PR open related to the custom PID. Is that something else you're working on?

whoops sorry, yes I have not PR'd it yet. I will now

BrianJKoopman commented 2 weeks ago

Superseded by #753.