SasaKaranovic / VU-Server

VU Dials Server Application
https://vudials.com
26 stars 12 forks source link

remove non-integer magic numbers for command and data type #16

Closed feeph closed 8 months ago

feeph commented 8 months ago

The ..._LAST values in dials/Comms_Hub_Server.py will cause issues with https://github.com/feeph/VU-Server/blob/ee237b6d6842b43d927727743737e846b1415b53/dial_driver.py#L31 because the function assumes cmd and dataType contain numerical values. There's no logic in place to handle non-integer values:

if dataLen == 0:
    payload = ">{:02X}{:02X}{:04X}".format(cmd, dataType, dataLen)
elif dataLen == 1:
    if data < 256:
        payload = ">{:02X}{:02X}{:04X}{:02X}".format(cmd, dataType, dataLen, data)
    elif data >= 256:
        payload = ">{:02X}{:02X}{:04X}{:04X}".format(cmd, dataType, dataLen+1, data)
    elif dataLen > 1:
        [...]
        payload = ">{:02X}{:02X}{:04X}{}".format(cmd, dataType, int(len(formattedData)/2), formattedData)

If cmd or dataType are 'None' a TypeError is raised:

TypeError: unsupported format string passed to NoneType.__format__

There's a comment that these files were autogenerated by generate_pyton_classes.py. If the files should be left unchanged it is recommended to modify the function 'dial_driver._sendCommand()' to properly handle cases where cmd and/or dataType are 'None'.

The 2 values in dials/Comms_Hub_Gauge.py have been removed as well. It does not look like anything uses this file.

feeph commented 8 months ago

The pull request's description is an updated version of the commit message. Please squash-merge this PR and use the updated message for the commit.

How are contributions handled? Do I need to sign a contributor's agreement somewhere?

SasaKaranovic commented 8 months ago

Hi @feeph,

Thank you for bringing this up. This is actually the "desired/expected" behaviour since _LAST items are used as boundary/guard elements. In our VU Server they have no way of appearing during the normal code execution.

It's a valid point that if they are not used, they should be removed. Since these files are auto-generated, there is no point in manually editing them. I'll consider updating the auto-generator script to not export these items for .py files and future release should have this "issue" resolved.