padelt / temper-python

libusb/PyUSB-based driver to read TEMPer USB HID devices (USB ID 0c45:7401) and serve as a NetSNMP passpersist module
Other
182 stars 76 forks source link

Suggested architecture improvement for supporting new sensors #97

Closed davet2001 closed 3 years ago

davet2001 commented 4 years ago

I have been looking at https://github.com/padelt/temper-python and thinking of ways to support more sensors more easily, specifically to make it easier for people to contribute.

Currently there is a lot of logic looking at the product identifier spread through the driver. I assume it has evolved this way rather than intended from the start.

I would like to propose something like this:

class TemperDeviceConfig:
    """
    Configuration for a particular type of TEMPer USB thermo/humidty sensor
    """
    def __init__(self, product, temp_sensors=[2,4], hum_sensors=None, 
            sensor_type="fm75"):
        self.product = product
        self.temp_sensors = temp_sensors
        self.hum_sensors = hum_sensors
        self.sensor_type = sensor_type

CONFIGS = [
    # product, [array of temp sensor offsets], [array of hum sensor offsets], sensor_type
    # Default generic profile must be the last one in the list.
    TemperDeviceConfig('TEMPerV1.2',       [2],     None, "fm75"),
    TemperDeviceConfig('TEMPer1F_V1.3',    [2],     None, "fm75"),
    TemperDeviceConfig('TEMPERHUM1V1.3',   [4],     [4],  "si7021"),
    TemperDeviceConfig('TEMPer1F_H1_V1.4', [2],     [4],  "fm75"),
    TemperDeviceConfig('TEMPerNTC1.O',     [2,4,6], None, "fm75"),
    TemperDeviceConfig('Generic',          [2],     None, "fm75"),
]

Essentially, this defines the details of each sensor type in one place, then the rest of the driver uses that information, firstly when interrogating the device type, then when reading from it. It makes the rest of the file much simpler and smaller.

Before I work on this in more detail and submit a PR, do you support this approach?

Dave

ps-jay commented 4 years ago

This looks great to me. Could it even be externalised into a configuration file, so new sensors don't even need code change to be supported?

padelt commented 3 years ago

Hey @davet2001 , sure I would love to have an improved source layout!

davet2001 commented 3 years ago

I am working on this now. Starting with some pytests so that I can verify the layout change later doesn't break something.

davet2001 commented 3 years ago

Ok, PR is ready for someone to review/approve it.

padelt commented 3 years ago

I just merged the changes. Thanks a lot to @davet2001 !

Can someone test the current master so I can prepare a new release?

davet2001 commented 3 years ago

Can close