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 support for direct Ethernet SCPI PSU Agent #715

Merged
merged 29 commits into from
Sep 20, 2024

Conversation

CAWNoodle
Copy link
Contributor

@CAWNoodle CAWNoodle commented Aug 5, 2024

Adding support for direct Ethernet SCPI PSU Agent

Description

  • Adds the ScpiPsuInterface class to the socs/agents/scpi_psu/drivers.py file to allow sending SCPI commands directly over ethernet with a specified IP address and port.

  • Introduces code switching based on device Model number as returned by the '*idn?' SCPI command.

  • Adds the '--port' and '--mode' options to the agent's arguments. An 'acq' mode was added to conform to allow for automated collection of measurement data, conforming to the pattern of other SOCS agents.

  • Allows backwards compatibility with the older Prologix Adapter interface by omitting the '--port' option.

Motivation and Context

Many modern power supplies come equipped with an ethernet port or wifi connection. A new SCPI PSU agent was required to interface with such devices while maintaining backward compatibility for the legacy GPIB-to-Ethernet adapters. Furthermore, some means of distinguishing between various models (e.g. those with 3 outputs vs. single channel devices, devices with varying voltage and current limitation, and devices supporting different subsets of the SCPI protocol,etc.) was necessary in order to provide functionality without loss of flexibility.

Resolves #313.

How Has This Been Tested?

With no access to a Prologix GPIB-to-Ethernet adapter, regression testing was unable to be performed. However, testing proceeded with small incremental changes to the driver first. Then the agent code was built testing each change as it was made. Testing consisted of forcing a --no-cache build of a local image before running 'docker-compose up' for the full OCS stack and tracing container execution through the log files. This agent was attached to a Keithley 2280S power supply with which initialization, data acquisition, current measurement, voltage measurement, current setting, voltage setting, output enabling and disabling functions, as well as tests of the output state, and model self-identification were all tested -- first within the agent and then via an OCSClient object handle in python.

Other agent container run alongside this one during testing include:
The OCS host manager
Influxdb and publisher
crossbar
grafana
Aggregator
FakeData
Labjack
CryomechCPA
and other test agents

These tests have been useful for unit testing, but they are insufficient for regression or integration testing. I expect further edits will be required if this change is to be adopted by the wider community.

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.

Daniel Lee and others added 13 commits June 7, 2024 12:55
…ion to using the Prologix GPIB-to-ethernet adapter.
…monsobs/socs faciliting future pull requests.
….py to drivers.py; adding new ScpiPsuInterface class to implement drivers for SCPI PSUs that can be connected to directly by Ethernet.
problems related to number of channels being different for different
models of PSU.
acquisition of current and voltage measurements as found in other device
agents.
PSU container would crash when attempting to measure when the output was
disabled.
measurements, and current measurements of each channel of the PSU for
use by client scripts.
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 @CAWNoodle! See my inline comments below, and let me know if you have any questions/comments.

Out of curiosity, which model devices do you have access to?

requirements.txt Outdated Show resolved Hide resolved
socs/agents/scpi_psu/drivers.py Outdated Show resolved Hide resolved
socs/agents/scpi_psu/drivers.py Show resolved Hide resolved
socs/agents/scpi_psu/drivers.py Outdated Show resolved Hide resolved
socs/agents/scpi_psu/drivers.py Outdated Show resolved Hide resolved
socs/agents/scpi_psu/agent.py Outdated Show resolved Hide resolved
socs/agents/scpi_psu/agent.py Outdated Show resolved Hide resolved
socs/agents/scpi_psu/agent.py Outdated Show resolved Hide resolved
socs/agents/scpi_psu/agent.py Show resolved Hide resolved
socs/agents/scpi_psu/agent.py Outdated Show resolved Hide resolved
@CAWNoodle
Copy link
Contributor Author

Thanks for the PR @CAWNoodle! See my inline comments below, and let me know if you have any questions/comments.

Out of curiosity, which model devices do you have access to?

Hi Brian, Thanks for your work on this! I'll get right to work making these fixes. We have a Keithley 2280S-60-3 and a 2230G-30-1, but no GPIB adapter. I might end up making an agent to work with the 2230G over USB in the future.

@CAWNoodle
Copy link
Contributor Author

Oh also. Uh, how do I actually submit these changes once they're made?

@BrianJKoopman
Copy link
Member

Oh also. Uh, how do I actually submit these changes once they're made?

When you're ready you can push your new commits to this same branch. Then once the changes are ready for re-review click the little circular arrows next to my name in the reviewers list to the right to "re-request review". That lets me know it's time to take a look/no more changes are expected.

PS - Thanks for the quick turnaround, will try to re-review soon!

@BrianJKoopman
Copy link
Member

Responded to inline comments, let me know if you need anything else from me, else I'll wait for your push and re-review request to take another look. No rush, just checking I'm not blocking on this.

@CAWNoodle
Copy link
Contributor Author

I think I've addressed all the changes. Our lab is running a set of experiments at the moment, but I should be able to test these changes over the weekend.

I started reading manuals for various SCPI power supplies and I've come to the conclusion that supporting all of them with a general purpose SCPI driver is not feasible. Each manufacturer implements different commands to perform the same operations and they are usually not cross-compatible. For example, the 2230G 3-channel devices use the :INSTrument:SELect <CH1|CH2|CH3|> command to select a channel for subsequent commands. Whereas the 2280S devices do not even have a :INST: root command. And the BK Precision 9173 & 9174 dual-channel devices don't have a channel selection command at all. Instead they have separate commands for each channel such as :MEAS:CURR? for channel one and :MEAS:CURR2? for channel two. Even the command for enabling/disabling the output differs from model to model.

A generalized power supply agent could be constructed that would implement all the things you typically want to do with a power supply, but under the hood we would need a sort of implementation file for each model and/or manufacturer.

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 pushing the updates! I responded to the previous review's comments and made a couple additional small comments in this re-review.

It is too bad that the commanding isn't more uniform across manufacturers. Like I said in one of the comment responses, I believe most users have Keithley devices (this used to be named after one of those devices) -- I wouldn't recommend spending the time to generalize this further unless the demand is there for other PSUs.

socs/agents/scpi_psu/drivers.py Outdated Show resolved Hide resolved
socs/agents/scpi_psu/drivers.py Show resolved Hide resolved
socs/agents/scpi_psu/agent.py Outdated Show resolved Hide resolved
@BrianJKoopman BrianJKoopman mentioned this pull request Aug 30, 2024
6 tasks
@BrianJKoopman
Copy link
Member

Hi @CAWNoodle, just wanted to check in on this. #726, which introduces re-connection logic for when there's a network interruption. That should handle my concern I commented on in #715 (comment). Can you merge in the latest main? There's a small merge conflict to be handled.

Other than that the biggest thing to fix is the TypeError from my recent re-review: #715 (comment)

Then this should be good to merge. Let me know if there's anything I can do to help.

@CAWNoodle
Copy link
Contributor Author

Sorry for the delay, The Fall semester has just begun and I have been trying to adjust to the new schedule. I should have time to get this done during this week.

@BrianJKoopman
Copy link
Member

Sorry for the delay, The Fall semester has just begun and I have been trying to adjust to the new schedule. I should have time to get this done during this week.

Great, no rush on my end, mostly wanted to let you know about the #726 merge.

@CAWNoodle
Copy link
Contributor Author

CAWNoodle commented Sep 10, 2024

So it looks like the changes from #726 are catching the timeout error from trying to measure when the output is off. If there is a networking issue it would only be caught when running monitor_output. clearing the error queue also indicates that this is primarily addressing timeout errors that result from the PSU's command execution rather than networking problems.

Concurring with suggestions in #538, I think the best place to handle connectivity issues would be in the drivers. Likewise, the drivers should ensure that commands sent to the devices do not trigger internal errors, that way agent code doesn't have to be aware of any implementation details.

I've essentially left the #726 changes alone in the merge as they're not doing any harm. I've made the requested edits as well. I'm just going to run some tests to make sure everything is still working as expected and I'll push everything.

@CAWNoodle
Copy link
Contributor Author

Ok, I've pushed my changes. I had to comment out the call to _initialize_module() at the bottom of the init() function. It kept resulting in a timeout error, and I suspect it is because the PSU already had a connection established on the same port. So trying to connect a second time resulted in a timeout.

texmexlab and others added 9 commits September 13, 2024 22:30
This was the expected mode before 'acq' became the default. Fixes an issue with
the init test.
This ends up hanging on the auto-reconnection otherwise. This was a bit
unexpected, as test_mode should break us out before another communication.
This makes it look like initialize happened twice, since the message included
in the return is also logged in the same place.
This was commented because it was causing issues, but we should just drop it
altogether. It's better that 'init' doesn't loop forever until a connection is
made. Rather it should fail and report as failed.
This was missing, but needed since it's checked in init(). This is maybe a bit
of a hack, but by setting to 0 we skip setting 'acq_params' and end running
with the default three channel query given by the the default 'channels' on
'monitor_output()'. This is the old behavior before the addition of
'ScpiPsuInterface'.
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.

Ok, I've pushed my changes. I had to comment out the call to _initialize_module() at the bottom of the init() function. It kept resulting in a timeout error, and I suspect it is because the PSU already had a connection established on the same port. So trying to connect a second time resulted in a timeout.

Thanks, this is looking good. I have a few commits I'd like to push before merging that fix the problems in the tests. On your PR can you check "Allow edits from maintainers"? (I could also PR onto your branch, though this is a bit more straightforward if possible.)

@CAWNoodle
Copy link
Contributor Author

Hmm... I do not appear to have that checkbox shown in "Allow edits from maintainers". Any ideas on how I can fix that?

@BrianJKoopman
Copy link
Member

Hmm... I do not appear to have that checkbox shown in "Allow edits from maintainers". Any ideas on how I can fix that?

No clue. I just opened TexMExLab#3. If you merge that they should show up here.

@CAWNoodle
Copy link
Contributor Author

Hmm... I do not appear to have that checkbox shown in "Allow edits from maintainers". Any ideas on how I can fix that?

No clue. I just opened TexMExLab#3. If you merge that they should show up here.

Done

@BrianJKoopman BrianJKoopman self-requested a review September 16, 2024 21:04
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.

Great, thanks for all the work on this! I'm checking with @agthomas-uc to see if he can test this on the hardware with the GPIB converter to double check old behavior is unaffected. Assuming that'll work out I'm going to wait on that test to merge. Thanks again!

@agthomas-uc
Copy link
Contributor

I verified the new agent works on our hardware with a GPIB converter. I performed three reconnect tests of variable length, making sure the longest was at least a minute to ensure secondary timeout errors did not occur. Grafana view and corresponding log showing flawless performance are attached.
image
image

@BrianJKoopman
Copy link
Member

Great, thanks again for testing @agthomas-uc! Going to go ahead with the merge. Thanks @CAWNoodle also for the PR!

@BrianJKoopman BrianJKoopman merged commit cf13d18 into simonsobs:main Sep 20, 2024
5 checks passed
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.

SCPI PSU Agent should initialize and monitor at startup
3 participants