Teresadev

[Tlabting]

Adding ZXY6005S Powersupply Driver

[dmitri-mcguckin]

In general looks fine.

I would recommend moving any code to the src/ directory so it's organized a bit more and can later be grouped into a callable python module.

Left other comments to be addressed as well.

[dmitri-mcguckin]

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

[dmitri-mcguckin]

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
    '''

[dmitri-mcguckin]

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
        ''''
    ...

[dmitri-mcguckin]

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(...)

[dmitri-mcguckin]

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'):
    ...

[dmitri-mcguckin]

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.

[dmitri-mcguckin]

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.

[dmitri-mcguckin]

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)