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

Adding Hi6200 Agent for reading LN2 on SATp #555

Merged
merged 19 commits into from
Feb 26, 2024
Merged

Conversation

mjrand
Copy link
Contributor

@mjrand mjrand commented Oct 25, 2023

Description

Added the agent.py and driver.py files for the hi6200 agent in socs.

Motivation and Context

This agent controls the hi6200 weight controller that reads the scale measuring the LN2 depth on SATp1.

How Has This Been Tested?

I tested this in daq-dev and was able to verify the agent ran and uploaded data correctly.

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. (Likely my agent naming scheme is off...)
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@BrianJKoopman BrianJKoopman added the new agent New OCS agent needs to be created label Oct 30, 2023
@BrianJKoopman BrianJKoopman self-requested a review October 30, 2023 17:09
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.

Thanks for the PR @mjrand!

In addition to the inline comments below, this agent needs a docs page. Please add a page for it under docs/agents/. For the page to show up, it must be added to the index file. There are instructions for building the docs here.

socs/agents/hi6200/.driver.py.swo Outdated Show resolved Hide resolved
socs/agents/hi6200/agent.py Outdated Show resolved Hide resolved
socs/agents/hi6200/agent.py Show resolved Hide resolved
socs/agents/hi6200/agent.py Outdated Show resolved Hide resolved
socs/agents/hi6200/agent.py Outdated Show resolved Hide resolved
socs/agents/hi6200/agent.py Show resolved Hide resolved
socs/agents/hi6200/drivers.py Outdated Show resolved Hide resolved
socs/agents/hi6200/drivers.py Outdated Show resolved Hide resolved
socs/agents/hi6200/drivers.py Outdated Show resolved Hide resolved
socs/agents/hi6200/drivers.py Outdated Show resolved Hide resolved
@BrianJKoopman BrianJKoopman self-requested a review November 6, 2023 19:41
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.

Thanks for the updates @mjrand. A few more comments and responses to some of the modifications below.

socs/agents/hi6200/agent.py Outdated Show resolved Hide resolved
socs/agents/hi6200/drivers.py Outdated Show resolved Hide resolved
docs/agents/hi6200.rst Outdated Show resolved Hide resolved
docs/agents/hi6200.rst Outdated Show resolved Hide resolved
socs/agents/hi6200/drivers.py Outdated Show resolved Hide resolved
docs/agents/hi6200.rst Outdated Show resolved Hide resolved
@mjrand
Copy link
Contributor Author

mjrand commented Dec 20, 2023

whoops I forgot to actually incorporate half the comments.... give me a little bit to go through them before reviewing

@mjrand
Copy link
Contributor Author

mjrand commented Dec 20, 2023

Fixed comments, agent works and is ready for review.

@BrianJKoopman BrianJKoopman self-requested a review January 2, 2024 16:30
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.

Hi @mjrand, thanks for the updates! (And sorry this has sat for so long.) It's looking good, I think we're basically ready to merge.

I added the docs page to the index, and then cleaned up some formatting of docstrings, mostly in the drivers. I also did some very minor restructuring in the agent to remove a layer of indents and add some info to the locks (though I don't really expect any locking issues here with only one task and one process...) Other changes were mostly wrapping long lines as I reviewed.

I'll plan to merge at the beginning of next week (avoiding doing so as it approaches 5pm on a Friday.) Let me know if you have any concerns.

@BrianJKoopman BrianJKoopman merged commit ee538a0 into main Feb 26, 2024
8 checks passed
@BrianJKoopman BrianJKoopman deleted the add_Hi6200_Agent branch February 26, 2024 15:14
hnakata-JP pushed a commit that referenced this pull request Apr 12, 2024
* Adding Hi6200 Agent for reading LN2 on SATp

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

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

* Corrected some whitespace and style issues

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

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

* Fixed my fight with the pre-commit bot

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

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

* Added in edits from Brian's PR comments. Added docs. (Attempted) removal of all swap files

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

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

* Attempting to fix disconnect error catching

* Added catch in monitor weight for AttributeError. Cleaned up code as per Brian's comments.

* Actually fixed Brian's comments regarding pacemaker, privatizing decoding function, etc.

* Remove layer of nesting in main process

* Add job names to lock

* Wrap agent code to 80 characters

* Add agent to docs index

* Add driver code to docs page

* Format driver docstrings and wrap code to 80 characters

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Brian Koopman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new agent New OCS agent needs to be created
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants