Support manufacturer-specific parameters

TimMJN commented 1 year ago

Hi,

I've been working on implementing manufacturer-specific parameters (PID 0x8000- 0xFFDF), including E120_GET_COMMAND, E120_SET_COMMAND and E120_PARAMETER_DESCRIPTION.

Right now, the parameter values are simply taken from / dumped in a array of bytes and processed in a user-callback function. It's up to the user to store/process/whatever the values.

mathertel commented 1 year ago

That's a great improvement. Thanks a lot. I will test...

peternewman commented 1 year ago

Have you run the OLA RDM Tests against this too? https://www.openlighting.org/rdm-tools/rdm-responder-tests/ https://www.openlighting.org/rdm-tools/rdm-responder-tests/running-the-tests/

TimMJN commented 1 year ago

@peternewman

Thanks for the review! I've processed most of your comments.

I don't think the library should enforce using unique manufacturer-specific PIDs, but perhaps @mathertel could add a remark on this on his website, similarly to how he mentions unique manufacturer codes.

Regarding the RDM test, working on it.. I'm having some python3 compatibility issues with running the tests.

TimMJN commented 1 year ago

To do: add support for cold reset using RESET_DEVICE 0x01

void(* resetFunc) (void) = 0;
resetFunc()

(leave the warm reset RESET_DEVICE 0xFF to be user-defined)

peternewman commented 1 year ago

@peternewman

Thanks for the review! I've processed most of your comments.

Great thanks.

I don't think the library should enforce using unique manufacturer-specific PIDs, but perhaps @mathertel could add a remark on this on his website, similarly to how he mentions unique manufacturer codes.

I don't think the library can technically enforce it (unless PIDs had to be centrally registered in a list of defines or something), so a note/remark is all we can manage. It shouldn't actually be an issue until two individual devices meet. But I guess it will stop someone tripping themselves up by not appreciating the subtlety of how the system should work.

Regarding the RDM test, working on it.. I'm having some python3 compatibility issues with running the tests.

Our 0.10.9 release or the 0.10 or master branches should all solve the Python 3 issues.

TimMJN commented 1 year ago

RDM test results:

OLA RDM Responder Tests

GetMaxPacketSize: FAILED
Check if the responder can handle a packet of the maximum size.
 GET: uid: 0987:2012fb3b, pid: DEVICE_INFO (0x0060), sub device: 0, data: b'\x00\x01\x02\x03\x04\x05\x06\x07\x08\t\n\x0b\x0c\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f !"#$%&\'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~\x7f\xc2\x80\xc2\x81\xc2\x82\xc2\x83\xc2\x84\xc2\x85\xc2\x86\xc2\x87\xc2\x88\xc2\x89\xc2\x8a\xc2\x8b\xc2\x8c\xc2\x8d\xc2\x8e\xc2\x8f\xc2\x90\xc2\x91\xc2\x92\xc2\x93\xc2\x94\xc2\x95\xc2\x96\xc2\x97\xc2\x98\xc2\x99\xc2\x9a\xc2\x9b\xc2\x9c\xc2\x9d\xc2\x9e\xc2\x9f\xc2\xa0\xc2\xa1\xc2\xa2\xc2\xa3\xc2\xa4\xc2\xa5\xc2\xa6\xc2\xa7\xc2\xa8\xc2\xa9\xc2\xaa\xc2\xab\xc2\xac\xc2\xad\xc2\xae\xc2\xaf\xc2\xb0\xc2\xb1\xc2\xb2\xc2\xb3\xc2\xb4\xc2\xb5\xc2\xb6\xc2\xb7\xc2\xb8\xc2\xb9\xc2\xba\xc2\xbb\xc2\xbc\xc2\xbd\xc2\xbe\xc2\xbf\xc3\x80\xc3\x81\xc3\x82\xc3\x83\xc3\x84\xc3\x85\xc3\x86\xc3\x87\xc3\x88\xc3\x89\xc3\x8a\xc3\x8b\xc3\x8c\xc3\x8d\xc3\x8e\xc3\x8f\xc3\x90\xc3\x91\xc3\x92\xc3\x93\xc3\x94\xc3\x95\xc3\x96\xc3\x97\xc3\x98\xc3\x99\xc3\x9a\xc3\x9b\xc3\x9c\xc3\x9d\xc3\x9e\xc3\x9f\xc3\xa0\xc3\xa1\xc3\xa2\xc3\xa3\xc3\xa4\xc3\xa5\xc3\xa6'
 Response status: Failed to send request
 Failed: expected one of:
  CC: Get, PID 0x0060, NACK RDMNack(value=1, desc="Format Error")
  CC: Get, PID 0x0060, NACK RDMNack(value=8, desc="Packet size unsupported")
  CC: Get, PID 0x0060, ACK, fields [], values {}
  RDM_INVALID_RESPONSE
  RDM_TIMEOUT

SetEmptyDeviceLabel: FAILED
SET the device label with no data.
 SET: uid: 0987:2012fb3b, pid: DEVICE_LABEL (0x0082), sub device: 0, args: ['']
 Response status: Response Timeout
 Failed: expected one of:
  CC: Set, PID 0x0082, NACK RDMNack(value=4, desc="Write protect")
  CC: Set, PID 0x0082, NACK RDMNack(value=5, desc="Unsupported command class")
  CC: Set, PID 0x0082, ACK, fields [], values {}
 SET: uid: 0987:2012fb3b, pid: DEVICE_LABEL (0x0082), sub device: 0, args: ['this is a string with 32 charact']
 Response status: Response Timeout

SetNonPrintableAsciiDeviceLabel: FAILED
SET the device label to something that contains non-printable ASCII
     characters.

 SET: uid: 0987:2012fb3b, pid: DEVICE_LABEL (0x0082), sub device: 0, args: ['str w\\r non\\x1bprint ASCII\\x7f']
 Response status: Response Timeout
 Failed: expected one of:
  CC: Set, PID 0x0082, NACK RDMNack(value=4, desc="Write protect")
  CC: Set, PID 0x0082, NACK RDMNack(value=6, desc="Data out of range")
  CC: Set, PID 0x0082, NACK RDMNack(value=1, desc="Format Error")
  CC: Set, PID 0x0082, NACK RDMNack(value=5, desc="Unsupported command class")
  CC: Set, PID 0x0082, ACK, fields [], values {}
 SET: uid: 0987:2012fb3b, pid: DEVICE_LABEL (0x0082), sub device: 0, args: ['this is a string with 32 charact']
 Response: RDMResponse(type=ACK, command_class=SET), PID: 0x0082, TN: 53, Error: Expected 0 bytes but got 32
 Failed: Invalid Param data: Expected 0 bytes but got 32

SetFullSizeDeviceLabel: FAILED
SET the device label.
 SET: uid: 0987:2012fb3b, pid: DEVICE_LABEL (0x0082), sub device: 0, args: ['this is a string with 32 charact']
 Response status: Response Timeout
 Failed: expected one of:
  CC: Set, PID 0x0082, NACK RDMNack(value=4, desc="Write protect")
  CC: Set, PID 0x0082, NACK RDMNack(value=5, desc="Unsupported command class")
  CC: Set, PID 0x0082, ACK, fields [], values {}
 SET: uid: 0987:2012fb3b, pid: DEVICE_LABEL (0x0082), sub device: 0, args: ['this is a string with 32 charact']
 Response status: Response Timeout

SetDeviceLabel: FAILED
SET the device label.
 SET: uid: 0987:2012fb3b, pid: DEVICE_LABEL (0x0082), sub device: 0, args: ['test label']
 Response status: Response Timeout
 Failed: expected one of:
  CC: Set, PID 0x0082, NACK RDMNack(value=4, desc="Write protect")
  CC: Set, PID 0x0082, NACK RDMNack(value=5, desc="Unsupported command class")
  CC: Set, PID 0x0082, ACK, fields [], values {}
 SET: uid: 0987:2012fb3b, pid: DEVICE_LABEL (0x0082), sub device: 0, args: ['this is a string with 32 charact']
 Response status: Response Timeout

------------------- Summary --------------------
OLA Version: 0.10.9
Test Run: 2023-06-07 10:33:28 PM 
UID: 0987:2012fb3b
Manufacturer: mathertel.de
Model Description: Arduino RDM Device
Software Version: 16777216
------------------- Warnings --------------------
------------------ Advisories -------------------
----------------- By Category -------------------
              Configuration:     7 /   7    100%
                    Control:    19 /  19    100%
         Core Functionality:     4 /   4    100%
               DMX512 Setup:    22 /  22    100%
            Dimmer Settings:    13 /  13    100%
           Display Settings:     2 /   2    100%
           Error Conditions:   312 / 313    100%
   IP and DNS Configuration:     5 /   5    100%
         Network Management:    25 /  25    100%
      Power / Lamp Settings:     9 /   9    100%
        Product Information:    15 /  19    79%
            RDM Information:     1 /   1    100%
                    Sensors:     5 /   5    100%
          Status Collection:     3 /   3    100%
                Sub Devices:    76 /  76    100%
-------------------------------------------------
523 / 583 tests run, 518 passed, 5 failed, 0 broken

TimMJN commented 1 year ago

Seems like we should add a catch for Set on the device label, return nack Write protected.

The first fail is related to https://github.com/mathertel/DmxSerial2/issues/46?

TimMJN commented 1 year ago

found the problem with device labels. Writing up to 32 characters to EEPROM takes too long to send the response in time. I've moved the _saveEEPRom call after the response has been sent. This still fails some tests, as it now takes too long to read back the results immediately after setting a new label. Unless @mathertel wants to eg rewrite the EEPROM saving to a background process that writes max 1 byte per tick, this is what it is.

TimMJN commented 1 year ago

Test output:

OLA RDM Responder Tests

GetMaxPacketSize: FAILED
Check if the responder can handle a packet of the maximum size.
 GET: uid: 0987:2012fb3b, pid: DEVICE_INFO (0x0060), sub device: 0, data: b'\x00\x01\x02\x03\x04\x05\x06\x07\x08\t\n\x0b\x0c\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f !"#$%&\'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~\x7f\xc2\x80\xc2\x81\xc2\x82\xc2\x83\xc2\x84\xc2\x85\xc2\x86\xc2\x87\xc2\x88\xc2\x89\xc2\x8a\xc2\x8b\xc2\x8c\xc2\x8d\xc2\x8e\xc2\x8f\xc2\x90\xc2\x91\xc2\x92\xc2\x93\xc2\x94\xc2\x95\xc2\x96\xc2\x97\xc2\x98\xc2\x99\xc2\x9a\xc2\x9b\xc2\x9c\xc2\x9d\xc2\x9e\xc2\x9f\xc2\xa0\xc2\xa1\xc2\xa2\xc2\xa3\xc2\xa4\xc2\xa5\xc2\xa6\xc2\xa7\xc2\xa8\xc2\xa9\xc2\xaa\xc2\xab\xc2\xac\xc2\xad\xc2\xae\xc2\xaf\xc2\xb0\xc2\xb1\xc2\xb2\xc2\xb3\xc2\xb4\xc2\xb5\xc2\xb6\xc2\xb7\xc2\xb8\xc2\xb9\xc2\xba\xc2\xbb\xc2\xbc\xc2\xbd\xc2\xbe\xc2\xbf\xc3\x80\xc3\x81\xc3\x82\xc3\x83\xc3\x84\xc3\x85\xc3\x86\xc3\x87\xc3\x88\xc3\x89\xc3\x8a\xc3\x8b\xc3\x8c\xc3\x8d\xc3\x8e\xc3\x8f\xc3\x90\xc3\x91\xc3\x92\xc3\x93\xc3\x94\xc3\x95\xc3\x96\xc3\x97\xc3\x98\xc3\x99\xc3\x9a\xc3\x9b\xc3\x9c\xc3\x9d\xc3\x9e\xc3\x9f\xc3\xa0\xc3\xa1\xc3\xa2\xc3\xa3\xc3\xa4\xc3\xa5\xc3\xa6'
 Response status: Failed to send request
 Failed: expected one of:
  CC: Get, PID 0x0060, NACK RDMNack(value=1, desc="Format Error")
  CC: Get, PID 0x0060, NACK RDMNack(value=8, desc="Packet size unsupported")
  CC: Get, PID 0x0060, ACK, fields [], values {}
  RDM_INVALID_RESPONSE
  RDM_TIMEOUT

SetNonPrintableAsciiDeviceLabel: FAILED
SET the device label to something that contains non-printable ASCII
     characters.

 SET: uid: 0987:2012fb3b, pid: DEVICE_LABEL (0x0082), sub device: 0, args: ['str w\\r non\\x1bprint ASCII\\x7f']
 Response: RDMResponse(type=ACK, command_class=SET), PID: 0x0082, TN: 117, PDL: 0, data: {}
 GET: uid: 0987:2012fb3b, pid: DEVICE_LABEL (0x0082), sub device: 0, args: []
 Response status: Response Timeout
 Failed: expected one of:
  CC: Get, PID 0x0082, ACK, fields ['label'], values {}
 SET: uid: 0987:2012fb3b, pid: DEVICE_LABEL (0x0082), sub device: 0, args: ['somethingelse']
 Response: RDMResponse(type=ACK, command_class=SET), PID: 0x0082, TN: 119, PDL: 0, data: {}

SetFullSizeDeviceLabel: FAILED
SET the device label.
 SET: uid: 0987:2012fb3b, pid: DEVICE_LABEL (0x0082), sub device: 0, args: ['this is a string with 32 charact']
 Response: RDMResponse(type=ACK, command_class=SET), PID: 0x0082, TN: 205, PDL: 0, data: {}
 GET: uid: 0987:2012fb3b, pid: DEVICE_LABEL (0x0082), sub device: 0, args: []
 Response status: Response Timeout
 Failed: expected one of:
  CC: Get, PID 0x0082, ACK, fields ['label'], values {}
 SET: uid: 0987:2012fb3b, pid: DEVICE_LABEL (0x0082), sub device: 0, args: ['somethingelse']
 Response status: Response Timeout

SetZeroDMXStartAddress: FAILED
Check the DMX address can't be set to 0.
 SET: pid: DMX_START_ADDRESS (0x00f0), sub device: 0, data: b'\x00\x00'
 Response status: Response Timeout
 Failed: expected one of:
  CC: Set, PID 0x00f0, NACK RDMNack(value=6, desc="Data out of range")

------------------- Summary --------------------
OLA Version: 0.10.9
Test Run: 2023-06-10 03:03:15 PM 
UID: 0987:2012fb3b
Manufacturer: mathertel.de
Model Description: Arduino RDM Device
Software Version: 16777216
------------------- Warnings --------------------
------------------ Advisories -------------------
----------------- By Category -------------------
              Configuration:     7 /   7    100%
                    Control:    19 /  19    100%
         Core Functionality:     4 /   4    100%
               DMX512 Setup:    22 /  22    100%
            Dimmer Settings:    13 /  13    100%
           Display Settings:     2 /   2    100%
           Error Conditions:   311 / 313    99%
   IP and DNS Configuration:     5 /   5    100%
         Network Management:    25 /  25    100%
      Power / Lamp Settings:     9 /   9    100%
        Product Information:    19 /  21    90%
            RDM Information:     1 /   1    100%
                    Sensors:     5 /   5    100%
          Status Collection:     3 /   3    100%
                Sub Devices:    76 /  76    100%
-------------------------------------------------
525 / 583 tests run, 521 passed, 4 failed, 0 broken

notice that SetZeroDMXStartAddress was ran directly after SetFullSizeDeviceLabel, causing the timeout.

peternewman commented 1 year ago

Seems like we should add a catch for Set on the device label, return nack Write protected.

No, there's little point implementing a read only device label, but I think you've got to the bottom of that.

The first fail is related to #46?

I thought we'd fixed that before a few times.

found the problem with device labels. Writing up to 32 characters to EEPROM takes too long to send the response in time.

Agreed.

I've moved the _saveEEPRom call after the response has been sent. This still fails some tests, as it now takes too long to read back the results immediately after setting a new label. Unless @mathertel wants to eg rewrite the EEPROM saving to a background process that writes max 1 byte per tick, this is what it is.

However I believe the correct fix is to implement an Ack Timer, which tells the controller to try again in a time you specify when the device should be ready. I think technically it shouldn't ack until it's actually completed the write (e.g. consider if there's a power cut before it finishes writing, the controller will think the write succeeded when it didn't. Obviously the balance is implementation complexity, so TBH as long as the DMX addresses ones work it may be easiest to just revert that change and put it in the two hard pile, or at least push it to another PR.

TimMJN commented 1 year ago

No, there's little point implementing a read only device label, but I think you've got to the bottom of that.

You're right, I was misreading the test output.

However I believe the correct fix is to implement an Ack Timer, which tells the controller to try again in a time you specify when the device should be ready. I think technically it shouldn't ack until it's actually completed the write (e.g. consider if there's a power cut before it finishes writing, the controller will think the write succeeded when it didn't. Obviously the balance is implementation complexity, so TBH as long as the DMX addresses ones work it may be easiest to just revert that change and put it in the two hard pile, or at least push it to another PR.

I have one issue with the current (before my changes) method. Correct me if I'm wrong, but the delayed ACK message would be able to mangle up other communication in the universe happening at that time, right? Queued messages are not implemented right now, but how about this:

When a SET on the device label is received, we compare the new label to the existing one.
If it is identical, we issue an ACK.
If it's not, we issue an ACK_TIMER (300ms should be sufficient), then rewrite the EEPROM.

That way, we keep queued messages on the to-do list, but the controller can re-issue the SET and will receive an ACK.

peternewman commented 1 year ago

I have one issue with the current (before my changes) method. Correct me if I'm wrong, but the delayed ACK message would be able to mangle up other communication in the universe happening at that time, right? Queued messages are not implemented right now, but how about this:

You don't send it gratuitously, the controller will wait and then re-query it when the timeout has expired. IIRC queued messages are mostly about things like when I change the DMX address via a local UI, or temperature sensor readings and stuff.

* When a SET on the device label is received, we compare the new label to the existing one.

* If it is identical, we issue an ACK.

* If it's not, we issue an ACK_TIMER (300ms should be sufficient), then rewrite the EEPROM.
That way, we keep queued messages on the to-do list, but the controller can re-issue the SET and will receive an ACK.

Yeah I think that ought to work, but it will want a chunk more testing.

Could I suggest reverting the changes around this from this PR, which is already quite big and complicated, and pushing it into a separate PR. Realistically there will be other equally buggy responders, so controllers probably deal with them already, which means this is mostly just about making us more compliant and allowing us to pass more tests. Which is a great thing to do, but it would be a shame if it delayed the addition of useful functionality to people in the mean time.

mathertel / DmxSerial2

Support manufacturer-specific parameters #47