-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add RDM Personality support #39
base: master
Are you sure you want to change the base?
Conversation
@@ -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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strncpy not required with the fixed length string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check should be performed for the length of the memcpy too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a redundant check as the length is validated on input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the above, in #37 but should perhaps just be removed.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use a memcpy as both are known lengths
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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: