Add RDM Personality support #39
Conversation
…ial2 into personalities
|
|
||
| // 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" |
There was a problem hiding this comment.
Warning is a compile time thing, this uses non-static data in the if. A static_cast would be more appropriate.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| _startAddress = 1; | ||
| strcpy (deviceLabel, "new"); | ||
| _personalityNumber = _initData->defaultPersonalityNumber; | ||
| strncpy(deviceLabel, "new", DMXSERIAL_MAX_RDM_STRING_LENGTH); |
There was a problem hiding this comment.
strncpy not required with the fixed length string.
There was a problem hiding this comment.
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.
| } 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'; |
There was a problem hiding this comment.
This check should be performed for the length of the memcpy too.
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
This is a redundant check as the length is validated on input.
There was a problem hiding this comment.
Similar to the above, in #37 but should perhaps just be removed.
| 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); |
There was a problem hiding this comment.
Should use a memcpy as both are known lengths
There was a problem hiding this comment.
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.
|
Thanks for the review too @chrisstaite ! |
| // uint16_t personalityCount; | ||
| // RDMPERSONALITY *personalities; | ||
| // uint16_t footprint; // This now depends on the personality data | ||
| uint8_t defaultPersonalityNumber; // Is this excessive flexibility? |
There was a problem hiding this comment.
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...
|
Nice work Peter I will check it out |
|
How about adding sensor data to the RDM like adding ability to add a DHT22 for temperature sensor data. |
4 participants
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: