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

Teresadev #14

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -128,4 +128,4 @@ venv.bak/
dmypy.json

# Pyre type checker
.pyre/
.pyre/
159 changes: 159 additions & 0 deletions ZXY6005S.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
# This file enables communication with ZXY-6005S Power Supply Units
# Documentation for Serial Commands: https://docs.google.com/document/d/1sobaDhIX-cFmzpiBEMtt-2S7j5h_ogMujiykYV2Dyv8/edit?usp=sharing
# Data Sheet for ZXY-6005S Power Supply Unit https://drive.google.com/drive/u/0/folders/1QRbtmRMBnCshlBtOnuKX8HSpySGEhDeM
# Author: Teresa LaBolle

import serial
import serial.tools.list_ports
from enum import Enum

class Commands(Enum):
Copy link
Member

Choose a reason for hiding this comment

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

See comment on the power supply class for in-depth detail on docstrings, but for Enums specifically, it looks nicer if the Enum pairs are defined in one doctring like so:

class Commands(Enum):
    '''
    :param MODEL: Description of param
    :type MODEL: str
    '''

'''Get power supply unit model. Response: ZXY-6005S'''
Copy link
Member

Choose a reason for hiding this comment

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

The syntax of ''' qutations in a row are usually reserved for python docstrings.

For inline comments use #

Suggested for line 12 and similar scenarios:

MODEL = 'a'  # Get power supply unit model. Response: ZXY-6005S

MODEL = 'a'
'''Get firmware version. Response: R2.7Z'''
FIRMWARE_VERSION = 'v'
'''Set amp hour counter to specified value. Response: Asa(value)'''
SET_AMP_HOUR = 'sa'
'''Return the amp hour reading. Response: Ara(5 digit value)'''
RETURN_AMP_HOUR = 'ra'
'''Set voltage to specified value. Response: Asu(value)'''
SET_VOLTAGE = 'su'
'''Return voltage measurement. Response: Aru(5 digit value)'''
RETURN_VOLTAGE = 'ru'
'''Set current limit to specified value. Response: Asi(value)'''
SET_CURRENT_LIMIT = 'si'
'''Return current in amps. Response: Ari(4 digit value)'''
RETURN_CURRENT = 'ri'
'''Return Mode [Constant Voltage(CV) or Constant Current(CC)] Response: Arc0 or Arc1'''
RETURN_MODE = 'rc'
'''Return temperature in Celsius. Response: Art(3 digit value)'''
RETURN_TEMP = 'rt'
'''Set power output (On/OFF). Response: Aso(1 OR 0)'''
SET_OUTPUT = 'so'


class ZXY6005S:
Copy link
Member

Choose a reason for hiding this comment

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

Docstrings in general are greatly encouraged for:

  1. Public Functions (which are by default all functions in python)
  2. Class Definitions

Suggested add for this class and similar definitions

class ZXY6005S:
    '''
    A short description of what the class does.

    :param MODEL: Get power supply unit model. Response: ZXY-6005S
    :param FIRMWARE_VERSION: Get firmware version. Response: R2.7Z
    '''
    
    def __some_function__(param1, param2) -> int:
        '''
        A short description of what the function does.
        
        :param param1: Description of param1
        :param param2: Description of param2
        :returns: Description of what is returned
        :rtype: int
        ''''
    ...

BAUDRATE = 9600
INPUT_DELAY = 0.001
BYTESIZE = serial.EIGHTBITS
PARITY = serial.PARITY_NONE
STOPBITS = serial.STOPBITS_ONE
TIMEOUT = 1

def __init__(self):
Copy link
Member

@dmitri-mcguckin dmitri-mcguckin Nov 8, 2023

Choose a reason for hiding this comment

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

Is this class intended to manage the state of all three power supplies at the same time? Or just one supply and have three instances?

Personally, I would recommend the latter of the two because this scopes the purpose of the class and you can use handy things in client code such as:

for p in [supply_1, supply_2, supply_3]:
    p.send_command(...)

'''construct objects, using location for X, Y, and Z power supplies'''
Copy link
Member

Choose a reason for hiding this comment

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

It is also recommended that parameters such as device location/name is a configurable item for the constructor as this could change with different supplies or different environment configurations. It is also acceptable to add defaults to the initializers if they are generic settings that may be the same across environments.

Example:

def __init__(self, dev_1: str = '/dev/TTYUSBS10'):
    ...

names = ['X', 'Y', 'Z']
locations = ['1-1.5.4.3', '1-1.5.4.2', '1-1.5.4.1']
self.devices = {}
for name, location in zip(names,locations):
serial_port = None
for i in serial.tools.list_ports.comports():
if i.location == location:
serial_port = i.device
break
if serial_port is None:
raise Exception(f'Could not find device with location of {location}')

self.devices[name] = serial.Serial(
port = serial_port,
baudrate = self.BAUDRATE,
parity = self.PARITY,
stopbits = self.STOPBITS,
bytesize = self.BYTESIZE,
timeout = self.TIMEOUT,
)

def write_message(self, device_name, msg):
Copy link
Member

@dmitri-mcguckin dmitri-mcguckin Nov 8, 2023

Choose a reason for hiding this comment

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

Since this object seems to be a stateful object It may make sense to store the device name as an attribute on the object so it can be omitted as a parameter here.

Same applies to all similar functions below.

'''writes a command to serial port'''
ser = self.devices[device_name]
if ser.out_waiting != 0:
ser.flush()
ser.write(msg)
ser.flush()

def read_message(self, device_name, ending_token = '\n'):
'''reads from the serial port until a specified character'''
ser = self.devices[device_name]
data = ser.read_until(ending_token)
return data.decode().strip()

def send_command(self, device_name, msg):
'''sends a command to serial port and reads the message returned'''
self.write_message(device_name, msg)
return self.read_message(device_name)

def create_command(self, command):
Copy link
Member

@dmitri-mcguckin dmitri-mcguckin Nov 8, 2023

Choose a reason for hiding this comment

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

This is actually excellent that you've separated the command generation from the command sending.

The core functionality of this class, (i.e. directly communicating with the power supply on a serial channel), is not really unit-testable. However the most important part that we can unit-test is making sure that the commands are being generated correctly per the power-supply spec.

I would recommend making a set of unit tests around this function asserting that this function generates all of the correct things else fails safely in a way that will stop it from being sent to the PSU otherwise.

'''creates command to send through serial port'''
'''<A> is address, ends on <\n>, and encode as bytes'''
msg = f'A{command}\n'.encode()
return msg

def model(self, device_name: str) -> str:
'''takes a device name and returns the model name'''
msg = self.create_command(Commands.MODEL.value)
return self.send_command(device_name, msg)

def firmware_version(self, device_name: str) -> str:
'''takes a device name and returns the firmware version'''
msg = self.create_command(Commands.FIRMWARE_VERSION.value)
return self.send_command(device_name, msg)

def set_output(self, device_name: str, value: bool):
'''takes a device name and a boolean: 1 for ON, 0 for OFF to set output ON/OFF'''
if value:
msg = f'{Commands.SET_OUTPUT.value}1'
else:
msg = f'{Commands.SET_OUTPUT.value}0'
msg = self.create_command(msg)
reply = self.send_command(device_name, msg)
if reply != msg.decode().strip():
raise ValueError(f'Invalid reply was {reply}, expected {msg.decode().strip()}')

def set_amp_hour(self, device_name: str, value: int):
'''takes a device name and an integer, sets amp hour counter to that value'''
msg = f'{Commands.SET_AMP_HOUR.value}{str(value)}'
msg = self.create_command(msg)
reply = self.send_command(device_name, msg)
if reply != msg.decode().strip():
raise ValueError(f'Invalid reply was {reply}, expected {msg.decode().strip()}')

def return_amp_hour(self, device_name: str) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

For getters and setters I would recommend either the verbiage get/set or if you wanted to get fancy, (and more pythonic), you could also do:

@amp_hour.setter
def amp_hour(self, value) -> None:
    <methods to set value>
    
@amp_hour.getter
def amp_hour() -> str:
    return <value>

So that calling code would look like:

supply_1.amp_hour = '1.3'
print(supply_1.amp_hour)

'''takes a device name and returns amp hour reading'''
msg = self.create_command(Commands.RETURN_AMP_HOUR.value)
return self.send_command(device_name, msg)

def set_voltage(self, device_name: str, value: int):
'''takes a device name and an integer, sets voltage to that value'''
msg = f'{Commands.SET_VOLTAGE.value}{str(value)}'
msg = self.create_command(msg)
reply = self.send_command(device_name, msg)
if reply != msg.decode().strip():
raise ValueError(f'Invalid reply was {reply}, expected {msg.decode().strip()}')

def return_voltage(self, device_name: str) -> str:
'''takes a device name and returns voltage measurement'''
msg = self.create_command(Commands.RETURN_VOLTAGE.value)
return self.send_command(device_name, msg)

def set_current_limit(self, device_name: str, value: int):
'''takes a device name and an integer, sets current limit to that value'''
msg = f'{Commands.SET_CURRENT_LIMIT.value}{str(value)}'
msg = self.create_command(msg)
reply = self.send_command(device_name, msg)
if reply != msg.decode().strip():
raise ValueError(f'Invalid reply was {reply}, expected {msg.decode().strip()}')

def return_current(self, device_name: str) -> str:
'''takes a device name and returns current in amps'''
msg = self.create_command(Commands.RETURN_CURRENT.value)
return self.send_command(device_name, msg)

def return_mode(self, device_name: str) -> str:
'''takes a device name and returns mode (CV or CC), see Data Sheet pg 6, Item 4'''
msg = self.create_command(Commands.RETURN_MODE.value)
return self.send_command(device_name, msg)

def return_temp(self, device_name: str) -> str:
'''takes a device name and returns temperature in Celsius (of PSU?)'''
msg = self.create_command(Commands.RETURN_TEMP.value)
return self.send_command(device_name, msg)