Skip to content
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.

Sign up for GitHub

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

Fix some more OLA RDM test suite discovered issues #37

Merged
merged 7 commits into from
Jan 21, 2022
Merged

Fix some more OLA RDM test suite discovered issues #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6cdfbfc
Fix a minor typo nit
peternewman Jul 6, 2021
71cfd3c
Ensure we don't get or set labels that are too long
peternewman Jul 13, 2021
2977f7f
Return unsupported command class if we don't support set
peternewman Jul 13, 2021
b99ff96
Undo an accidental commit of some testing tweaks
peternewman Jul 13, 2021
e4be95e
Name an unknown field
peternewman Jul 14, 2021
f224f57
Switch to strnlen for string length safety
peternewman Jul 14, 2021
e1f293e
Correctly handle packets upto the maximum size allowed by RDM
peternewman Jul 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
174 changes: 99 additions & 75 deletions src/DMXSerial2.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ struct EEPROMVALUES {
byte sig1; // 0x6D signature 1, EPROM values are valid of both signatures match.
byte sig2; // 0x68 signature 2
uint16_t startAddress; // the DMX start address can be changed by a RDM command.
char deviceLabel[DMXSERIAL_MAX_RDM_STRING_LENGTH]; // the device Label can be changed by a RDM command.
char deviceLabel[DMXSERIAL_MAX_RDM_STRING_LENGTH]; // the device Label can be changed by a RDM command. Don't store the null in the EPROM for backwards compatibility.
DEVICEID deviceID; // store the device ID to allow easy software updates.
}; // struct EEPROMVALUES

Expand Down Expand Up @@ -255,7 +255,7 @@ DEVICEID _devIDAll = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };


// This is the buffer for RDM packets being received and sent.
// this structure is needed to RDM data separate from DMX data.
// This structure is needed to separate RDM data from DMX data.
union RDMMEM {
// the most common RDM packet layout for commands
struct RDMDATA packet;
Expand All @@ -264,10 +264,12 @@ union RDMMEM {
struct DISCOVERYMSG discovery;

// the byte array used while receiving and sending.
byte buffer[60];
// This is the max size of any RDM packet and it's max PDL, to allow an
// aribitrary length packet to be received. Thanks to the union it doesn't
// need any more memory
byte buffer[sizeof(packet)];
} _rdm; // union RDMMEM


// This flag will be set when a full RDM packet was received.
bool8 _rdmAvailable;

Expand Down Expand Up @@ -354,7 +356,8 @@ void DMXSerialClass2::init(struct RDMINIT *initData, RDMCallbackFunction func, R
// check if the EEEPROM values are from the RDM library
if ((eeprom.sig1 == 0x6D) && (eeprom.sig2 == 0x68)) {
_startAddress = eeprom.startAddress;
strcpy (deviceLabel, eeprom.deviceLabel);
// Restricting this to 32 characters or the length, whichever is shorter
strncpy(deviceLabel, eeprom.deviceLabel, DMXSERIAL_MAX_RDM_STRING_LENGTH);
DeviceIDCpy(_devID, eeprom.deviceID);

// setup the manufacturer addressing device-ID
Expand All @@ -364,7 +367,7 @@ void DMXSerialClass2::init(struct RDMINIT *initData, RDMCallbackFunction func, R
} else {
// set default values
_startAddress = 1;
strcpy (deviceLabel, "new");
strncpy(deviceLabel, "new", DMXSERIAL_MAX_RDM_STRING_LENGTH);
_devID[4] = random255(); // random(255);
_devID[5] = random255(); // random(255);
} // if
Expand Down Expand Up @@ -500,7 +503,7 @@ void DMXSerialClass2::tick(void)
// check if my _devID is in the discovery range
if ((DeviceIDCmp(rdm->Data, _devID) <= 0) && (DeviceIDCmp(_devID, rdm->Data+6) <= 0)) {

// respond a special discovery message !
// respond with a special discovery message !
struct DISCOVERYMSG *disc = &_rdm.discovery;
_rdmCheckSum = 6 * 0xFF;

Expand Down Expand Up @@ -595,7 +598,7 @@ void DMXSerialClass2::term(void)

// Process the RDM Command Message by changing the _rdm buffer and returning (true).
// if returning (false) a NAK will be sent.
// This method processes the commands/parameters regarding mute, DEviceInfo, devicelabel,
// This method processes the commands/parameters regarding mute, DeviceInfo, devicelabel,
// manufacturer label, DMX Start address.
// When parameters are changed by a SET command they are persisted into EEPROM.
// When doRespond is true, send an answer back to the controller node.
Expand Down Expand Up @@ -638,59 +641,74 @@ void DMXSerialClass2::_processRDMMessage(byte CmdClass, uint16_t Parameter, bool
}
} // if

} else if ((CmdClass == E120_GET_COMMAND) && (Parameter == SWAPINT(E120_DEVICE_INFO))) { // 0x0060
if (_rdm.packet.DataLength > 0) {
// Unexpected data
nackReason = E120_NR_FORMAT_ERROR;
} else if (_rdm.packet.SubDev != 0) {
// No sub-devices supported
nackReason = E120_NR_SUB_DEVICE_OUT_OF_RANGE;
} else {
// return all device info data
DEVICEINFO *devInfo = (DEVICEINFO *)(_rdm.packet.Data); // The data has to be responded in the Data buffer.

devInfo->protocolMajor = 1;
devInfo->protocolMinor = 0;
devInfo->deviceModel = SWAPINT(_initData->deviceModelId);
devInfo->productCategory = SWAPINT(E120_PRODUCT_CATEGORY_DIMMER_CS_LED);
devInfo->softwareVersion = SWAPINT32(0x01000000);// 0x04020900;
devInfo->footprint = SWAPINT(_initData->footprint);
devInfo->currentPersonality = 1;
devInfo->personalityCount = 1;
devInfo->startAddress = SWAPINT(_startAddress);
devInfo->subDeviceCount = 0;
devInfo->sensorCount = _initData->sensorsLength;

_rdm.packet.DataLength = sizeof(DEVICEINFO);
handled = true;
} else if (Parameter == SWAPINT(E120_DEVICE_INFO)) { // 0x0060
if (CmdClass == E120_GET_COMMAND) {
if (_rdm.packet.DataLength > 0) {
// Unexpected data
nackReason = E120_NR_FORMAT_ERROR;
} else if (_rdm.packet.SubDev != 0) {
// No sub-devices supported
nackReason = E120_NR_SUB_DEVICE_OUT_OF_RANGE;
} else {
// return all device info data
DEVICEINFO *devInfo = (DEVICEINFO *)(_rdm.packet.Data); // The data has to be responded in the Data buffer.

devInfo->protocolMajor = 1;
devInfo->protocolMinor = 0;
devInfo->deviceModel = SWAPINT(_initData->deviceModelId);
devInfo->productCategory = SWAPINT(E120_PRODUCT_CATEGORY_DIMMER_CS_LED);
devInfo->softwareVersion = SWAPINT32(0x01000000);// 0x04020900;
devInfo->footprint = SWAPINT(_initData->footprint);
devInfo->currentPersonality = 1;
devInfo->personalityCount = 1;
devInfo->startAddress = SWAPINT(_startAddress);
devInfo->subDeviceCount = 0;
devInfo->sensorCount = _initData->sensorsLength;

_rdm.packet.DataLength = sizeof(DEVICEINFO);
handled = true;
}
} else if (CmdClass == E120_SET_COMMAND) {
// Unexpected set
nackReason = E120_NR_UNSUPPORTED_COMMAND_CLASS;
}

} else if ((CmdClass == E120_GET_COMMAND) && (Parameter == SWAPINT(E120_MANUFACTURER_LABEL))) { // 0x0081
if (_rdm.packet.DataLength > 0) {
// Unexpected data
nackReason = E120_NR_FORMAT_ERROR;
} else if (_rdm.packet.SubDev != 0) {
// No sub-devices supported
nackReason = E120_NR_SUB_DEVICE_OUT_OF_RANGE;
} else {
// return the manufacturer label
_rdm.packet.DataLength = strlen(_initData->manufacturerLabel);
memcpy(_rdm.packet.Data, _initData->manufacturerLabel, _rdm.packet.DataLength);
handled = true;
} else if (Parameter == SWAPINT(E120_MANUFACTURER_LABEL)) { // 0x0081
if (CmdClass == E120_GET_COMMAND) {
if (_rdm.packet.DataLength > 0) {
// Unexpected data
nackReason = E120_NR_FORMAT_ERROR;
} else if (_rdm.packet.SubDev != 0) {
// No sub-devices supported
nackReason = E120_NR_SUB_DEVICE_OUT_OF_RANGE;
} else {
// return the manufacturer label
_rdm.packet.DataLength = strnlen(_initData->manufacturerLabel, DMXSERIAL_MAX_RDM_STRING_LENGTH);
memcpy(_rdm.packet.Data, _initData->manufacturerLabel, _rdm.packet.DataLength);
handled = true;
}
} else if (CmdClass == E120_SET_COMMAND) {
// Unexpected set
nackReason = E120_NR_UNSUPPORTED_COMMAND_CLASS;
}

} else if ((CmdClass == E120_GET_COMMAND) && (Parameter == SWAPINT(E120_DEVICE_MODEL_DESCRIPTION))) { // 0x0080
if (_rdm.packet.DataLength > 0) {
// Unexpected data
nackReason = E120_NR_FORMAT_ERROR;
} else if (_rdm.packet.SubDev != 0) {
// No sub-devices supported
nackReason = E120_NR_SUB_DEVICE_OUT_OF_RANGE;
} else {
// return the DEVICE MODEL DESCRIPTION
_rdm.packet.DataLength = strlen(_initData->deviceModel);
memcpy(_rdm.packet.Data, _initData->deviceModel, _rdm.packet.DataLength);
handled = true;
} else if (Parameter == SWAPINT(E120_DEVICE_MODEL_DESCRIPTION)) { // 0x0080
if (CmdClass == E120_GET_COMMAND) {
if (_rdm.packet.DataLength > 0) {
// Unexpected data
nackReason = E120_NR_FORMAT_ERROR;
} else if (_rdm.packet.SubDev != 0) {
// No sub-devices supported
nackReason = E120_NR_SUB_DEVICE_OUT_OF_RANGE;
} else {
// return the DEVICE MODEL DESCRIPTION
_rdm.packet.DataLength = strnlen(_initData->deviceModel, DMXSERIAL_MAX_RDM_STRING_LENGTH);
memcpy(_rdm.packet.Data, _initData->deviceModel, _rdm.packet.DataLength);
handled = true;
}
} else if (CmdClass == E120_SET_COMMAND) {
// Unexpected set
nackReason = E120_NR_UNSUPPORTED_COMMAND_CLASS;
}

} else if (Parameter == SWAPINT(E120_DEVICE_LABEL)) { // 0x0082
Expand All @@ -700,7 +718,7 @@ void DMXSerialClass2::_processRDMMessage(byte CmdClass, uint16_t Parameter, bool
nackReason = E120_NR_FORMAT_ERROR;
} 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';
_rdm.packet.DataLength = 0;
// persist in EEPROM
_saveEEPRom();
Expand All @@ -714,24 +732,29 @@ void DMXSerialClass2::_processRDMMessage(byte CmdClass, uint16_t Parameter, bool
// No sub-devices supported
nackReason = E120_NR_SUB_DEVICE_OUT_OF_RANGE;
} else {
_rdm.packet.DataLength = strlen(deviceLabel);
_rdm.packet.DataLength = strnlen(deviceLabel, DMXSERIAL_MAX_RDM_STRING_LENGTH);
memcpy(_rdm.packet.Data, deviceLabel, _rdm.packet.DataLength);
handled = true;
}
} // if

} else if ((CmdClass == E120_GET_COMMAND) && (Parameter == SWAPINT(E120_SOFTWARE_VERSION_LABEL))) { // 0x00C0
if (_rdm.packet.DataLength > 0) {
// Unexpected data
nackReason = E120_NR_FORMAT_ERROR;
} else if (_rdm.packet.SubDev != 0) {
// No sub-devices supported
nackReason = E120_NR_SUB_DEVICE_OUT_OF_RANGE;
} else {
// return the SOFTWARE_VERSION_LABEL
_rdm.packet.DataLength = strlen(_softwareLabel);
memcpy(_rdm.packet.Data, _softwareLabel, _rdm.packet.DataLength);
handled = true;
} else if (Parameter == SWAPINT(E120_SOFTWARE_VERSION_LABEL)) { // 0x00C0
if (CmdClass == E120_GET_COMMAND) {
if (_rdm.packet.DataLength > 0) {
// Unexpected data
nackReason = E120_NR_FORMAT_ERROR;
} else if (_rdm.packet.SubDev != 0) {
// No sub-devices supported
nackReason = E120_NR_SUB_DEVICE_OUT_OF_RANGE;
} else {
// return the SOFTWARE_VERSION_LABEL
_rdm.packet.DataLength = strnlen(_softwareLabel, DMXSERIAL_MAX_RDM_STRING_LENGTH);
memcpy(_rdm.packet.Data, _softwareLabel, _rdm.packet.DataLength);
handled = true;
}
} else if (CmdClass == E120_SET_COMMAND) {
// Unexpected set
nackReason = E120_NR_UNSUPPORTED_COMMAND_CLASS;
}

} else if (Parameter == SWAPINT(E120_DMX_START_ADDRESS)) { // 0x00F0
Expand Down Expand Up @@ -822,7 +845,7 @@ void DMXSerialClass2::_processRDMMessage(byte CmdClass, uint16_t Parameter, bool
// Out of range sensor
nackReason = E120_NR_DATA_OUT_OF_RANGE;
} else {
_rdm.packet.DataLength = 13 + strlen(_initData->sensors[sensorNr].description);
_rdm.packet.DataLength = 13 + strnlen(_initData->sensors[sensorNr].description, DMXSERIAL_MAX_RDM_STRING_LENGTH);
_rdm.packet.Data[0] = sensorNr;
_rdm.packet.Data[1] = _initData->sensors[sensorNr].type;
_rdm.packet.Data[2] = _initData->sensors[sensorNr].unit;
Expand Down Expand Up @@ -922,12 +945,13 @@ 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);
DeviceIDCpy(eeprom.deviceID, _devID);

for (unsigned int i = 0; i < sizeof(eeprom); i++) {
if (((byte *)(&eeprom))[i] != EEPROM.read(i))
EEPROM.write(i, ((byte *)(&eeprom))[i]);
// EEPROM.update does a read/write check and only writes on a change
EEPROM.update(i, ((byte *)(&eeprom))[i]);
} // for
} // _saveEEPRom

Expand Down
10 changes: 5 additions & 5 deletions src/DMXSerial2.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ struct RDMDATA {

byte _TransNo; // transaction number, not checked
byte ResponseType; // ResponseType
byte _unknown; // I don't know, ignore this
byte _MessageCount; // not used unless we support queued messages
uint16_t SubDev; // sub device number (root = 0)
byte CmdClass; // command class
uint16_t Parameter; // parameter ID
Expand Down Expand Up @@ -234,10 +234,10 @@ class DMXSerialClass2
/// Returns the Device ID. Copies the UID to the buffer passed through the uid argument.
void getDeviceID(DEVICEID id);

/// Return the current DMX start address that is the first dmx address used by the device.
/// Return the current DMX start address that is the first DMX address used by the device.
uint16_t getStartAddress();

/// Return the current DMX footprint, that is the number of dmx addresses used by the device.
/// Return the current DMX footprint, that is the number of DMX addresses used by the device.
uint16_t getFootprint();

/// Register a device-specific implemented function for RDM callbacks
Expand All @@ -252,8 +252,8 @@ class DMXSerialClass2
/// Terminate operation.
void term(void);

/// A short custom label given to the device.
char deviceLabel[DMXSERIAL_MAX_RDM_STRING_LENGTH];
/// A short custom label given to the device. Add an extra char for a null.
char deviceLabel[DMXSERIAL_MAX_RDM_STRING_LENGTH + 1];

/// don't use that method from extern.
void _processRDMMessage(byte CmdClass, uint16_t Parameter, bool8 isHandled);
Expand Down