Add KIKUSUI PCR500MA AC power supply agent #818
Conversation
BrianJKoopman
added
hardware: stimulator
new agent
|
|
||
| .. autoclass:: socs.agents.kikusui_pcr500ma.agent.PCRAgent | ||
| :members: | ||
|
|
There was a problem hiding this comment.
Add "Supporting APIs" header.
(I'm trying out this new-to-me suggested change feature, which I think you'll be able to just "accept". Though you can of course add independent of this too, if that's easier.)
| Supporting APIs | |
| --------------- | |
| @@ -55,6 +55,7 @@ API Reference Full API documentation for core parts of the SOCS library. | |||
| agents/holo_synth | |||
| agents/ibootbar | |||
| agents/ifm_sbn246_flowmeter | |||
| agents/kikusui_pcr500ma.rst | |||
There was a problem hiding this comment.
This works, but for consistency, drop the .rst extension.
| agents/kikusui_pcr500ma.rst | |
| agents/kikusui_pcr500ma |
| '''OCS agent for dS378 ethernet relay | ||
| ''' |
There was a problem hiding this comment.
Stale comment from the dS378 relay agent. You can just drop this, many of the agents don't have a docstring like this, and this isn't used in any of the auto documentation.
| '''OCS agent for dS378 ethernet relay | |
| ''' |
| ACQ_TIMEOUT = 20 | ||
|
|
||
|
|
||
| class PCRAgent: |
There was a problem hiding this comment.
Is the communication the same across other "PRC*" models? If not, consider renaming to PCR500MAAgent.
It might also make sense to either include the manufacturer name here, or the fact that this is for the stimulator. For reference there are two other Kikusui power supplies supported in this repo, one is the HWPPMXAgent and the other is the WiregridKikusuiAgent. I know we already added the DS378Agent, but we should evaluate this as a group with the other open stimulator PRs.
| This function measures output voltage when turning off | ||
| and check setting voltage value when turning on by default | ||
| to avoid abrupt change in applied voltage. |
There was a problem hiding this comment.
This wasn't clear to me when I read the comment, until I read the actual code. Can I suggest a rewording:
| This function measures output voltage when turning off | |
| and check setting voltage value when turning on by default | |
| to avoid abrupt change in applied voltage. | |
| This function measures the output voltage when turning off | |
| and checks the voltage setting when turning on by default. If the | |
| voltage is too high it will not change state to avoid an abrupt | |
| change in applied voltage. |
| return True, f'Got AC voltage setting: {volt_set}' | ||
|
|
||
| @ocs_agent.param('_') | ||
| def meas(self, session, params=None): |
There was a problem hiding this comment.
Why have a separate task to measure this? Isn't acq() grabbing these values too? (Except for f_ac -- should that be included in acq() for regular recording?)
|
|
||
| LOCK_RELEASE_SEC = 1. | ||
| LOCK_RELEASE_TIMEOUT = 10 | ||
| ACQ_TIMEOUT = 20 |
There was a problem hiding this comment.
This is a fairly high timeout. Does acq() take a long time to run? You might consider lowering this. Many other agents that set this get by with at most a 5 second timeout.
| if not message[-1] == '\n': | ||
| message += '\n' | ||
|
|
||
| self.__w(message.encode()) |
There was a problem hiding this comment.
Why have this additional layer of wrapping instead of using send directly?
self.send(message.encode())
Same question for __r().
| self._send(message) | ||
|
|
||
| def turn_on(self): | ||
| """Trun on""" |
There was a problem hiding this comment.
Small typo -- "Trun" -> "Turn". (Same in the turn_off() docstring.)
| pgroup = parser.add_argument_group('Agent Options') | ||
| pgroup.add_argument('--port', default=PORT_DEFAULT, type=int, | ||
| help='Port number for TCP communication.') | ||
| pgroup.add_argument('--ip_address', |
There was a problem hiding this comment.
Change --ip_address to --ip-address. By convention we use '-' for a separator in these commandline arguments. (Don't forget to change it on the example config file in the docs page too.)
Description
This pull request adds a new agent to operate KIKUSUI's PCR500MA AC power supply.
Motivation and Context
PCR500MA will be used as a power source to the stimulator heater to generate radiation.
Since the device will be connected to the site ethernet and be controlled from daq-pc, it should be included in this socs repository.
How Has This Been Tested?
Test was done using the actual PCR500MA device.
The device was connected to the lab network, and the ocs agent and client were run from a PC also connected to the same network.
Functions in the agent code was tested by checking the display and the light indicator of the device.
Torelance to the ethernet disconection was also tested by just unplugging the cable from the device.
Types of changes
Checklist: