Add RDM Personality support #39

peternewman · 2021-07-14T13:31:14Z

Closes #29

Blocked behind #37 (builds on the changes in there).

Obviously this adds new fields to the RDMINIT struct, currently in a way that isn't very backwards compatible, so I don't know what we want to do about that in terms of future releases.

There are also some other bits it would make sense to add while making a breaking change such as:

Manufacturer ID
Device part of the UID (or effectively combine these two to cover the whole thing)
Product category
Software version/software version label
Possibly default device label

…ial2 into personalities

chrisstaite · 2021-07-14T19:17:59Z

src/DMXSerial2.cpp

@@ -341,6 +342,13 @@ void DMXSerialClass2::init(struct RDMINIT *initData, RDMCallbackFunction func, R

  _baseInit();

+  // Sanity check/fix the defaultPersonalityNumber
+  if ((_initData->defaultPersonalityNumber < DMXSERIAL_MIN_PERSONALITY_VALUE) || (_initData->defaultPersonalityNumber > min(_initData->personalityCount, DMXSERIAL_MAX_PERSONALITY_VALUE))) {
+    #warning "Invalid default personality number in RDMINIT"


Warning is a compile time thing, this uses non-static data in the if. A static_cast would be more appropriate.

Do you mean a static_cast or static_assert?

The warning correctly appeared when I'd commented out all the personalities and forgot to tweak it, I wasn't too worried at runtime if someone does something odd, more flagging up that their config is a bit wonky when working around it, rather than just silently fixing it.

Yes, the latter. It's been quite a long day and I've been writing in every language but C++....
Perhaps the compiler is being clever here. But it's certainly not part of the language.

chrisstaite · 2021-07-14T19:19:24Z

src/DMXSerial2.cpp

@@ -364,7 +378,8 @@ void DMXSerialClass2::init(struct RDMINIT *initData, RDMCallbackFunction func, R
  } else {
    // set default values
    _startAddress = 1;
-    strcpy (deviceLabel, "new");
+    _personalityNumber = _initData->defaultPersonalityNumber;
+    strncpy(deviceLabel, "new", DMXSERIAL_MAX_RDM_STRING_LENGTH);


strncpy not required with the fixed length string.

Agreed, I assume the compiler will optimise it out. I'm also trying to safety proof it from someone going "I want this to be a longer string" and overflowing.

chrisstaite · 2021-07-14T19:22:07Z

src/DMXSerial2.cpp

        } else {
          memcpy(deviceLabel, _rdm.packet.Data, _rdm.packet.DataLength);
-          deviceLabel[_rdm.packet.DataLength] = '\0';
+          deviceLabel[min(_rdm.packet.DataLength, DMXSERIAL_MAX_RDM_STRING_LENGTH)] = '\0';


This check should be performed for the length of the memcpy too.

This change is actually originally in #37 (I wanted to fix the issues OLA discovered independently of adding the personalities, as the former should be simpler to get in).

I think it effectively already is via the if statement above, but I went a bit belt and braces, perhaps it should be removed or restructured.

chrisstaite · 2021-07-14T19:22:58Z

src/DMXSerial2.cpp

@@ -715,30 +771,43 @@ void DMXSerialClass2::_processRDMMessage(byte CmdClass, uint16_t Parameter, bool
          nackReason = E120_NR_SUB_DEVICE_OUT_OF_RANGE;
        } else {
          _rdm.packet.DataLength = strlen(deviceLabel);
+          _rdm.packet.DataLength = min(_rdm.packet.DataLength, DMXSERIAL_MAX_RDM_STRING_LENGTH);


This is a redundant check as the length is validated on input.

Similar to the above, in #37 but should perhaps just be removed.

chrisstaite · 2021-07-14T19:26:34Z

src/DMXSerial2.cpp

@@ -922,12 +1082,14 @@ void DMXSerialClass2::_saveEEPRom()
  eeprom.sig1 = 0x6D;
  eeprom.sig2 = 0x68;
  eeprom.startAddress = _startAddress;
-  strcpy (eeprom.deviceLabel, deviceLabel);
+  // This needs restricting to 32 chars (potentially no null), for backwards compatibility
+  strncpy(eeprom.deviceLabel, deviceLabel, DMXSERIAL_MAX_RDM_STRING_LENGTH);


Should use a memcpy as both are known lengths

Again #37

I believe abc\0def is a valid RDM label, but only abc should be used, if we memcpy we'd needlessly write def to the EEPROM wouldn't we? Also it's max length is known, but not it's current length.

peternewman · 2021-07-14T19:51:11Z

Thanks for the review too @chrisstaite !

peternewman · 2021-07-14T19:54:03Z

src/DMXSerial2.h

-  // uint16_t personalityCount;
-  // RDMPERSONALITY *personalities;
+//  uint16_t footprint; // This now depends on the personality data
+  uint8_t defaultPersonalityNumber; // Is this excessive flexibility?


I've had personal feedback suggesting it's not and probably matches commercial fixtures, so I'm tempted to leave this if we're messing with the struct anyway...

midikidi · 2021-07-21T14:27:22Z

Nice work Peter I will check it out

grahamrobertslx · 2022-02-09T23:05:21Z

How about adding sensor data to the RDM like adding ability to add a DHT22 for temperature sensor data.
PID SENSOR_VALUE

peternewman and others added 8 commits July 6, 2021 02:22

Fix a minor typo nit

6cdfbfc

Ensure we don't get or set labels that are too long

71cfd3c

Return unsupported command class if we don't support set

2977f7f

Undo an accidental commit of some testing tweaks

b99ff96

Initial RDM Personality support

a32139d

Merge branch 'master' into personalities

8b8685d

Correct the size of a field

7682d33

Merge branch 'personalities' of https://github.com/peternewman/DmxSer…

2682cb1

…ial2 into personalities

This was referenced Jul 14, 2021

Personality example #29

Open

DMX Personality Support chrisstaite/TeensyDmx#39

Open

chrisstaite reviewed Jul 14, 2021

View reviewed changes

peternewman commented Jul 14, 2021

View reviewed changes

Improve the example code

88734a5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RDM Personality support #39

Add RDM Personality support #39

peternewman commented Jul 14, 2021 •

edited

Loading

chrisstaite Jul 14, 2021

peternewman Jul 14, 2021

chrisstaite Jul 14, 2021 •

edited

Loading

chrisstaite Jul 14, 2021

peternewman Jul 14, 2021

chrisstaite Jul 14, 2021

peternewman Jul 14, 2021

chrisstaite Jul 14, 2021

peternewman Jul 14, 2021

chrisstaite Jul 14, 2021

peternewman Jul 14, 2021

peternewman commented Jul 14, 2021

peternewman Jul 14, 2021 •

edited

Loading

midikidi commented Jul 21, 2021

grahamrobertslx commented Feb 9, 2022

Add RDM Personality support #39

Are you sure you want to change the base?

Add RDM Personality support #39

Conversation

peternewman commented Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisstaite Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peternewman commented Jul 14, 2021

peternewman Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

midikidi commented Jul 21, 2021

grahamrobertslx commented Feb 9, 2022

peternewman commented Jul 14, 2021 •

edited

Loading

chrisstaite Jul 14, 2021 •

edited

Loading

peternewman Jul 14, 2021 •

edited

Loading