From 36c2ea3fa80661c3d2afcb1aa94d061f27efcd96 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Wed, 21 Apr 2021 00:22:34 +0100 Subject: [PATCH 1/5] Add some warnings if we've misconfigured the dummy responders in code --- common/rdm/ResponderPersonality.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/common/rdm/ResponderPersonality.cpp b/common/rdm/ResponderPersonality.cpp index 7a3df4858d..3eec3aebe1 100644 --- a/common/rdm/ResponderPersonality.cpp +++ b/common/rdm/ResponderPersonality.cpp @@ -47,6 +47,11 @@ Personality::Personality(uint16_t footprint, const string &description, : m_footprint(footprint), m_description(description), m_slot_data(slot_data) { + if (m_slot_data.SlotCount() > m_footprint) { + OLA_WARN << "Provided slot count of " << m_slot_data.SlotCount() + << " for personality \"" << description + << "\" is greater than it's footprint of " << m_footprint; + } } /** From c167d6d63bd7ad68f20c0bce12ee6ea8f231c663 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Wed, 21 Apr 2021 00:23:10 +0100 Subject: [PATCH 2/5] More thorough checking of SLOT_INFO and DEFAULT_SLOT_VALUE edge cases --- tools/rdm/TestDefinitions.py | 55 ++++++++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 3 deletions(-) diff --git a/tools/rdm/TestDefinitions.py b/tools/rdm/TestDefinitions.py index a7de95d21a..432b666b64 100644 --- a/tools/rdm/TestDefinitions.py +++ b/tools/rdm/TestDefinitions.py @@ -2440,6 +2440,7 @@ class GetSlotInfo(OptionalParameterTestFixture): """Get SLOT_INFO.""" CATEGORY = TestCategory.DMX_SETUP PID = 'SLOT_INFO' + REQUIRES = ['dmx_footprint'] PROVIDES = ['defined_slots', 'undefined_definition_slots', 'undefined_type_sec_slots'] @@ -2454,12 +2455,31 @@ def VerifyResult(self, response, fields): self.SetProperty('undefined_type_sec_slots', []) return - slots = [d['slot_offset'] for d in fields['slots']] - self.SetProperty('defined_slots', set(slots)) + slots = {} + # check for duplicates + for d in fields['slots']: + if d['slot_offset'] in slots: + self.AddWarning('SLOT_INFO contained slot %d more than once' % + (d['slot_offset'])) + else: + slots[d['slot_offset']] = d + + self.SetProperty('defined_slots', set(slots.keys())) undefined_definition_slots = [] undefined_type_sec_slots = [] + # do more thorough checking of each (non-duplicate) slot for slot in fields['slots']: + if slot['slot_offset'] > TestMixins.MAX_DMX_ADDRESS: + self.AddWarning( + "SLOT_INFO slot %d has an offset more than %d" + % (slot['slot_offset'], TestMixins.MAX_DMX_ADDRESS)) + # footprint is 1 based, offset is 0 based + if slot['slot_offset'] >= self.Property('dmx_footprint'): + self.AddWarning( + "SLOT_INFO slot %d has an offset greater than or equal to the " + "personality's defined footprint (%d)" + % (slot['slot_offset'], self.Property('dmx_footprint'))) if slot['slot_type'] not in RDMConstants.SLOT_TYPE_TO_NAME: self.AddWarning('Unknown slot type %d for slot %d' % (slot['slot_type'], slot['slot_offset'])) @@ -2477,10 +2497,25 @@ def VerifyResult(self, response, fields): undefined_definition_slots.append(slot['slot_offset']) else: # slot_label_id must reference a defined slot + # we've already validated the offset of the parent slot, so we don't + # need to check it again here if slot['slot_label_id'] not in slots: self.AddWarning( 'Slot %d is of type secondary and references an unknown slot %d' % (slot['slot_offset'], slot['slot_label_id'])) + else: + # the defined slot must be a primary slot + # slot exists, so find it + primary_slot = slots.get(slot['slot_label_id'], None) + # check the type of the parent slot + if primary_slot is None: + self.SetBroken('Failed to find primary slot for %d (%d)' + % (slot['slot_offset'], slot['slot_label_id'])) + elif primary_slot['slot_type'] != RDMConstants.SLOT_TYPES['ST_PRIMARY']: + self.AddWarning( + "Slot %d is of type secondary and references slot %d which " + "isn't a primary slot" + % (slot['slot_offset'], slot['slot_label_id'])) if slot['slot_type'] == RDMConstants.SLOT_TYPES['ST_SEC_UNDEFINED']: undefined_type_sec_slots.append(slot['slot_offset']) @@ -2660,7 +2695,7 @@ class GetDefaultSlotValues(OptionalParameterTestFixture): """Get DEFAULT_SLOT_VALUE.""" CATEGORY = TestCategory.DMX_SETUP PID = 'DEFAULT_SLOT_VALUE' - REQUIRES = ['defined_slots'] + REQUIRES = ['dmx_footprint', 'defined_slots'] def Test(self): self.AddIfGetSupported(self.AckGetResult()) @@ -2674,6 +2709,20 @@ def VerifyResult(self, response, fields): default_slots = set() for slot in fields['slot_values']: + if slot['slot_offset'] > TestMixins.MAX_DMX_ADDRESS: + self.AddWarning( + "DEFAULT_SLOT_VALUE slot %d has an offset more than %d" + % (slot['slot_offset'], TestMixins.MAX_DMX_ADDRESS)) + # footprint is 1 based, offset is 0 based + if slot['slot_offset'] >= self.Property('dmx_footprint'): + self.AddWarning( + "SLOT_INFO slot %d has an offset greater than or equal to the " + "personality's defined footprint (%d)" + % (slot['slot_offset'], self.Property('dmx_footprint'))) + if slot['slot_offset'] in default_slots: + self.AddWarning( + "DEFAULT_SLOT_VALUE contained slot %d more than once" % + slot['slot_offset']) if slot['slot_offset'] not in defined_slots: self.AddWarning( "DEFAULT_SLOT_VALUE contained slot %d, which wasn't in SLOT_INFO" % From ac1cccc7c3576bf0b326cc48e14ccf7004283253 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Wed, 21 Apr 2021 00:43:37 +0100 Subject: [PATCH 3/5] Add a warning if we try and set an invalid active personality --- common/rdm/ResponderPersonality.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/common/rdm/ResponderPersonality.cpp b/common/rdm/ResponderPersonality.cpp index 3eec3aebe1..356e039fc7 100644 --- a/common/rdm/ResponderPersonality.cpp +++ b/common/rdm/ResponderPersonality.cpp @@ -95,8 +95,13 @@ uint8_t PersonalityManager::PersonalityCount() const { } bool PersonalityManager::SetActivePersonality(uint8_t personality) { - if (personality == 0 || personality > m_personalities->PersonalityCount()) + if (personality == 0 || personality > m_personalities->PersonalityCount()) { + OLA_WARN << "Tried to set to invalid personality " + << static_cast(personality) << ", only 1 to " + << static_cast(m_personalities->PersonalityCount()) + << " are valid"; return false; + } m_active_personality = personality; return true; } From 66b6ccc2572fcbf15c17f21dafdea1b1a6f018ef Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Wed, 21 Apr 2021 01:25:16 +0100 Subject: [PATCH 4/5] Fix some Flake 8 issues --- tools/rdm/ModelCollector.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/rdm/ModelCollector.py b/tools/rdm/ModelCollector.py index 7882acd329..7f464a3d7e 100644 --- a/tools/rdm/ModelCollector.py +++ b/tools/rdm/ModelCollector.py @@ -239,8 +239,8 @@ def _HandleDeviceInfo(self, data): else: # We need software version to do anything, so abort and move onto the # next responder - print ('Failed to get device info for UID %s so moving onto the next one' - % self.uid) + print('Failed to get device info for UID %s so moving onto the next one' + % self.uid) self._FetchNextUID() def _HandleDeviceLabel(self, data): @@ -339,8 +339,8 @@ def _HandleLanguages(self, data): if not this_version: # We need software version to do anything, so abort and move onto the # next responder - print ('Failed to get software version for UID %s so moving onto the ' - 'next one' % self.uid) + print('Failed to get software version for UID %s so moving onto the ' + 'next one' % self.uid) self._FetchNextUID() for language in data['languages']: this_version['languages'].append(language['language']) @@ -386,8 +386,8 @@ def _NextState(self): if not self._GetVersion(): # We need software version to do anything, so abort and move onto the # next responder - print ('Failed to get software version for UID %s so moving onto the ' - 'next one' % self.uid) + print('Failed to get software version for UID %s so moving onto the ' + 'next one' % self.uid) self._FetchNextUID() else: # fetch device label From eb07fc2716434e64c674f67a7adffe53cbbac7a8 Mon Sep 17 00:00:00 2001 From: Peter Newman Date: Wed, 21 Apr 2021 01:33:34 +0100 Subject: [PATCH 5/5] Fix more flake8 issues --- tools/rdm/TestDefinitions.py | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/tools/rdm/TestDefinitions.py b/tools/rdm/TestDefinitions.py index 432b666b64..532337d72f 100644 --- a/tools/rdm/TestDefinitions.py +++ b/tools/rdm/TestDefinitions.py @@ -2472,14 +2472,14 @@ def VerifyResult(self, response, fields): for slot in fields['slots']: if slot['slot_offset'] > TestMixins.MAX_DMX_ADDRESS: self.AddWarning( - "SLOT_INFO slot %d has an offset more than %d" - % (slot['slot_offset'], TestMixins.MAX_DMX_ADDRESS)) + "SLOT_INFO slot %d has an offset more than %d" + % (slot['slot_offset'], TestMixins.MAX_DMX_ADDRESS)) # footprint is 1 based, offset is 0 based if slot['slot_offset'] >= self.Property('dmx_footprint'): self.AddWarning( - "SLOT_INFO slot %d has an offset greater than or equal to the " - "personality's defined footprint (%d)" - % (slot['slot_offset'], self.Property('dmx_footprint'))) + "SLOT_INFO slot %d has an offset greater than or equal to the " + "personality's defined footprint (%d)" + % (slot['slot_offset'], self.Property('dmx_footprint'))) if slot['slot_type'] not in RDMConstants.SLOT_TYPE_TO_NAME: self.AddWarning('Unknown slot type %d for slot %d' % (slot['slot_type'], slot['slot_offset'])) @@ -2511,7 +2511,8 @@ def VerifyResult(self, response, fields): if primary_slot is None: self.SetBroken('Failed to find primary slot for %d (%d)' % (slot['slot_offset'], slot['slot_label_id'])) - elif primary_slot['slot_type'] != RDMConstants.SLOT_TYPES['ST_PRIMARY']: + elif (primary_slot['slot_type'] != + RDMConstants.SLOT_TYPES['ST_PRIMARY']): self.AddWarning( "Slot %d is of type secondary and references slot %d which " "isn't a primary slot" @@ -2711,29 +2712,29 @@ def VerifyResult(self, response, fields): for slot in fields['slot_values']: if slot['slot_offset'] > TestMixins.MAX_DMX_ADDRESS: self.AddWarning( - "DEFAULT_SLOT_VALUE slot %d has an offset more than %d" - % (slot['slot_offset'], TestMixins.MAX_DMX_ADDRESS)) + "DEFAULT_SLOT_VALUE slot %d has an offset more than %d" + % (slot['slot_offset'], TestMixins.MAX_DMX_ADDRESS)) # footprint is 1 based, offset is 0 based if slot['slot_offset'] >= self.Property('dmx_footprint'): self.AddWarning( - "SLOT_INFO slot %d has an offset greater than or equal to the " - "personality's defined footprint (%d)" - % (slot['slot_offset'], self.Property('dmx_footprint'))) + "SLOT_INFO slot %d has an offset greater than or equal to the " + "personality's defined footprint (%d)" + % (slot['slot_offset'], self.Property('dmx_footprint'))) if slot['slot_offset'] in default_slots: self.AddWarning( - "DEFAULT_SLOT_VALUE contained slot %d more than once" % - slot['slot_offset']) + "DEFAULT_SLOT_VALUE contained slot %d more than once" % + slot['slot_offset']) if slot['slot_offset'] not in defined_slots: self.AddWarning( - "DEFAULT_SLOT_VALUE contained slot %d, which wasn't in SLOT_INFO" % - slot['slot_offset']) + "DEFAULT_SLOT_VALUE contained slot %d, which wasn't in SLOT_INFO" % + slot['slot_offset']) default_slots.add(slot['slot_offset']) for slot_offset in defined_slots: if slot_offset not in default_slots: self.AddAdvisory( - "SLOT_INFO contained slot %d, which wasn't in DEFAULT_SLOT_VALUE" % - slot_offset) + "SLOT_INFO contained slot %d, which wasn't in DEFAULT_SLOT_VALUE" % + slot_offset) class GetDefaultSlotValueWithData(TestMixins.GetWithDataMixin,