Skip to content

Conversation

@jlashner
Copy link
Contributor

@jlashner jlashner commented Jun 5, 2024

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

  • 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 jlashner requested review from BrianJKoopman and mjrand June 5, 2024 23:42
Copy link
Member

@BrianJKoopman BrianJKoopman left a comment

Choose a reason for hiding this comment

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

Looks good!

@BrianJKoopman
Copy link
Member

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 #689.

@BrianJKoopman
Copy link
Member

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

@jlashner
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor

mjrand commented Sep 6, 2024

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
Copy link
Contributor

mjrand commented Sep 6, 2024

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
Copy link
Member

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
Copy link
Contributor

mjrand commented Sep 12, 2024

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 BrianJKoopman mentioned this pull request Sep 16, 2024
6 tasks
@BrianJKoopman
Copy link
Member

Superseded by #753.

@BrianJKoopman BrianJKoopman deleted the ls372-custom-pid-pacemaker branch September 16, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants