From bf972c1a9e14ac442aae19764e9b50528ad935ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frieder=20Sch=C3=BCler?= Date: Thu, 7 May 2026 10:21:16 +0200 Subject: [PATCH 1/5] eds: fix LowLimit/HighLimit parsing for all signed integer types - Replace _calc_bit_length() (only handled INTEGER8/16/32/64) with a lookup dict _SIGNED_BIT_LENGTHS covering all 8 SIGNED_TYPES (INTEGER8/16/24/32/40/48/56/64) - Log a warning instead of silently ignoring invalid limit values (except ValueError: pass) - Add EDS test entries in sample.eds for INTEGER24/40/48/56 with hex-encoded negative limits - Extend test_record_with_limits and add test_invalid_limit_logs_warning Closes part of #352. --- canopen/objectdictionary/eds.py | 31 +++++++++++++--------------- test/sample.eds | 36 +++++++++++++++++++++++++++++++++ test/test_eds.py | 22 ++++++++++++++++++++ 3 files changed, 72 insertions(+), 17 deletions(-) diff --git a/canopen/objectdictionary/eds.py b/canopen/objectdictionary/eds.py index d47a3019..3231d4f6 100644 --- a/canopen/objectdictionary/eds.py +++ b/canopen/objectdictionary/eds.py @@ -204,19 +204,16 @@ def import_from_node(node_id: int, network: canopen.network.Network): return od -def _calc_bit_length(data_type): - if data_type == datatypes.INTEGER8: - return 8 - elif data_type == datatypes.INTEGER16: - return 16 - elif data_type == datatypes.INTEGER32: - return 32 - elif data_type == datatypes.INTEGER64: - return 64 - else: - raise ValueError( - f"Invalid data_type '{data_type}', expecting a signed integer data_type." - ) +_SIGNED_BIT_LENGTHS = { + datatypes.INTEGER8: 8, + datatypes.INTEGER16: 16, + datatypes.INTEGER24: 24, + datatypes.INTEGER32: 32, + datatypes.INTEGER40: 40, + datatypes.INTEGER48: 48, + datatypes.INTEGER56: 56, + datatypes.INTEGER64: 64, +} def _signed_int_from_hex(hex_str, bit_length): @@ -305,20 +302,20 @@ def build_variable( try: min_string = eds.get(section, "LowLimit") if var.data_type in datatypes.SIGNED_TYPES: - var.min = _signed_int_from_hex(min_string, _calc_bit_length(var.data_type)) + var.min = _signed_int_from_hex(min_string, _SIGNED_BIT_LENGTHS[var.data_type]) else: var.min = int(min_string, 0) except ValueError: - pass + logger.warning("Failed to parse LowLimit for %s: %r", name, min_string) if eds.has_option(section, "HighLimit"): try: max_string = eds.get(section, "HighLimit") if var.data_type in datatypes.SIGNED_TYPES: - var.max = _signed_int_from_hex(max_string, _calc_bit_length(var.data_type)) + var.max = _signed_int_from_hex(max_string, _SIGNED_BIT_LENGTHS[var.data_type]) else: var.max = int(max_string, 0) except ValueError: - pass + logger.warning("Failed to parse HighLimit for %s: %r", name, max_string) if eds.has_option(section, "DefaultValue"): try: var.default_raw = eds.get(section, "DefaultValue") diff --git a/test/sample.eds b/test/sample.eds index ad00a12e..49c95ce7 100644 --- a/test/sample.eds +++ b/test/sample.eds @@ -976,6 +976,42 @@ HighLimit=0xFFFFFFFF LowLimit=0x80000000 PDOMapping=0 +[3031] +ParameterName=INTEGER24 value range -1 to 0 +ObjectType=0x7 +DataType=0x10 +AccessType=rw +HighLimit=0x000000 +LowLimit=0xFFFFFF +PDOMapping=0 + +[3032] +ParameterName=INTEGER40 value range -1 to 0 +ObjectType=0x7 +DataType=0x12 +AccessType=rw +HighLimit=0x0000000000 +LowLimit=0xFFFFFFFFFF +PDOMapping=0 + +[3033] +ParameterName=INTEGER48 value range -1 to 0 +ObjectType=0x7 +DataType=0x13 +AccessType=rw +HighLimit=0x000000000000 +LowLimit=0xFFFFFFFFFFFF +PDOMapping=0 + +[3034] +ParameterName=INTEGER56 value range -1 to 0 +ObjectType=0x7 +DataType=0x14 +AccessType=rw +HighLimit=0x00000000000000 +LowLimit=0xFFFFFFFFFFFFFF +PDOMapping=0 + [3040] ParameterName=INTEGER64 value range -10 to +10 ObjectType=0x7 diff --git a/test/test_eds.py b/test/test_eds.py index 7a19ffeb..9250197a 100644 --- a/test/test_eds.py +++ b/test/test_eds.py @@ -148,6 +148,28 @@ def test_record_with_limits(self): int64 = self.od[0x3040] self.assertEqual(int64.min, -10) self.assertEqual(int64.max, +10) + # Verify all remaining SIGNED_TYPES are handled (INTEGER24/40/48/56) + for index in (0x3031, 0x3032, 0x3033, 0x3034): + var = self.od[index] + self.assertEqual(var.min, -1, f"min mismatch at 0x{index:04X}") + self.assertEqual(var.max, 0, f"max mismatch at 0x{index:04X}") + + def test_invalid_limit_logs_warning(self): + import io + import logging + + with open(SAMPLE_EDS) as f: + content = f.read() + invalid_eds = content.replace( + "LowLimit=0x02\nPDOMapping=0\n\n[3030]", + "LowLimit=INVALID\nPDOMapping=0\n\n[3030]", + ) + with io.StringIO(invalid_eds) as buf: + buf.name = "mock.eds" + with self.assertLogs("canopen.objectdictionary.eds", level=logging.WARNING) as cm: + od = canopen.import_od(buf) + self.assertIsNone(od[0x3021].min) + self.assertTrue(any("LowLimit" in msg for msg in cm.output)) def test_signed_int_from_hex(self): for data_type, test_cases in self.test_data.items(): From f65a1fb148b398b7d7ab181746eef9fccdac7245 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frieder=20Sch=C3=BCler?= Date: Thu, 7 May 2026 21:47:59 +0200 Subject: [PATCH 2/5] eds: revert LowLimit/HighLimit parse error to silent pass, remove warning test --- canopen/objectdictionary/eds.py | 4 ++-- test/test_eds.py | 17 ----------------- 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/canopen/objectdictionary/eds.py b/canopen/objectdictionary/eds.py index 3231d4f6..1967b22e 100644 --- a/canopen/objectdictionary/eds.py +++ b/canopen/objectdictionary/eds.py @@ -306,7 +306,7 @@ def build_variable( else: var.min = int(min_string, 0) except ValueError: - logger.warning("Failed to parse LowLimit for %s: %r", name, min_string) + pass if eds.has_option(section, "HighLimit"): try: max_string = eds.get(section, "HighLimit") @@ -315,7 +315,7 @@ def build_variable( else: var.max = int(max_string, 0) except ValueError: - logger.warning("Failed to parse HighLimit for %s: %r", name, max_string) + pass if eds.has_option(section, "DefaultValue"): try: var.default_raw = eds.get(section, "DefaultValue") diff --git a/test/test_eds.py b/test/test_eds.py index 9250197a..ab7cf613 100644 --- a/test/test_eds.py +++ b/test/test_eds.py @@ -154,23 +154,6 @@ def test_record_with_limits(self): self.assertEqual(var.min, -1, f"min mismatch at 0x{index:04X}") self.assertEqual(var.max, 0, f"max mismatch at 0x{index:04X}") - def test_invalid_limit_logs_warning(self): - import io - import logging - - with open(SAMPLE_EDS) as f: - content = f.read() - invalid_eds = content.replace( - "LowLimit=0x02\nPDOMapping=0\n\n[3030]", - "LowLimit=INVALID\nPDOMapping=0\n\n[3030]", - ) - with io.StringIO(invalid_eds) as buf: - buf.name = "mock.eds" - with self.assertLogs("canopen.objectdictionary.eds", level=logging.WARNING) as cm: - od = canopen.import_od(buf) - self.assertIsNone(od[0x3021].min) - self.assertTrue(any("LowLimit" in msg for msg in cm.output)) - def test_signed_int_from_hex(self): for data_type, test_cases in self.test_data.items(): for test_case in test_cases: From 4ed45ddbfbaed93a691d75a91b4ceb5df835c5ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frieder=20Sch=C3=BCler?= Date: Mon, 11 May 2026 13:55:51 +0200 Subject: [PATCH 3/5] add back and add more exception handling via warning instead of silent pass --- canopen/objectdictionary/eds.py | 85 +++++++++++++++++++++------------ test/test_eds.py | 39 ++++++++------- 2 files changed, 76 insertions(+), 48 deletions(-) diff --git a/canopen/objectdictionary/eds.py b/canopen/objectdictionary/eds.py index 1967b22e..da29b73a 100644 --- a/canopen/objectdictionary/eds.py +++ b/canopen/objectdictionary/eds.py @@ -204,26 +204,28 @@ def import_from_node(node_id: int, network: canopen.network.Network): return od -_SIGNED_BIT_LENGTHS = { - datatypes.INTEGER8: 8, - datatypes.INTEGER16: 16, - datatypes.INTEGER24: 24, - datatypes.INTEGER32: 32, - datatypes.INTEGER40: 40, - datatypes.INTEGER48: 48, - datatypes.INTEGER56: 56, - datatypes.INTEGER64: 64, -} +def _calc_bit_length(data_type: int) -> int: + if data_type in datatypes.SIGNED_TYPES: + st = ODVariable.STRUCT_TYPES[data_type] + if isinstance(st, datatypes.IntegerN): + return st.width + return st.size * 8 + else: + raise ValueError( + f"Invalid data_type '{data_type}', expecting a signed integer data_type." + ) def _signed_int_from_hex(hex_str, bit_length): number = int(hex_str, 0) + if number < 0 or number >= (1 << bit_length): + raise ValueError( + f"Value {hex_str!r} is out of range for a {bit_length}-bit signed integer" + ) max_value = (1 << (bit_length - 1)) - 1 - if number > max_value: return number - (1 << bit_length) - else: - return number + return number def _convert_variable(node_id, var_type, value): @@ -242,6 +244,18 @@ def _convert_variable(node_id, var_type, value): return int(value, 0) +def _int_to_hex(data_type: int, value: int) -> str: + """Format an integer as EDS hex string. + + Signed types with a negative value are written as two's-complement hex + (e.g. INTEGER8 -1 → 0xFF) so the output is a valid EDS literal. + """ + if data_type in datatypes.SIGNED_TYPES and value < 0: + bit_length = _calc_bit_length(data_type) + return f"0x{value + (1 << bit_length):0{bit_length // 4}X}" + return f"0x{value:02X}" + + def _revert_variable(var_type, value): if value is None: return None @@ -252,7 +266,7 @@ def _revert_variable(var_type, value): elif var_type in datatypes.FLOAT_TYPES: return value else: - return f"0x{value:02X}" + return _int_to_hex(var_type, value) def build_variable( @@ -302,20 +316,26 @@ def build_variable( try: min_string = eds.get(section, "LowLimit") if var.data_type in datatypes.SIGNED_TYPES: - var.min = _signed_int_from_hex(min_string, _SIGNED_BIT_LENGTHS[var.data_type]) + var.min = _signed_int_from_hex(min_string, _calc_bit_length(var.data_type)) else: var.min = int(min_string, 0) except ValueError: - pass + logger.warning( + "Invalid LowLimit %r for %s (0x%X), ignoring", + eds.get(section, "LowLimit"), var.name, var.index, + ) if eds.has_option(section, "HighLimit"): try: max_string = eds.get(section, "HighLimit") if var.data_type in datatypes.SIGNED_TYPES: - var.max = _signed_int_from_hex(max_string, _SIGNED_BIT_LENGTHS[var.data_type]) + var.max = _signed_int_from_hex(max_string, _calc_bit_length(var.data_type)) else: var.max = int(max_string, 0) except ValueError: - pass + logger.warning( + "Invalid HighLimit %r for %s (0x%X), ignoring", + eds.get(section, "HighLimit"), var.name, var.index, + ) if eds.has_option(section, "DefaultValue"): try: var.default_raw = eds.get(section, "DefaultValue") @@ -323,30 +343,33 @@ def build_variable( var.relative = True var.default = _convert_variable(node_id, var.data_type, var.default_raw) except ValueError: - pass + logger.warning( + "Invalid DefaultValue %r for %s (0x%X), ignoring", + var.default_raw, var.name, var.index, + ) if eds.has_option(section, "ParameterValue"): try: var.value_raw = eds.get(section, "ParameterValue") var.value = _convert_variable(node_id, var.data_type, var.value_raw) except ValueError: - pass + logger.warning( + "Invalid ParameterValue %r for %s (0x%X), ignoring", + var.value_raw, var.name, var.index, + ) # Factor, Description and Unit are not standard according to the CANopen specifications, but # they are implemented in the python canopen package, so we can at least try to use them if eds.has_option(section, "Factor"): try: var.factor = float(eds.get(section, "Factor")) except ValueError: - pass + logger.warning( + "Invalid Factor %r for %s (0x%X), ignoring", + eds.get(section, "Factor"), var.name, var.index, + ) if eds.has_option(section, "Description"): - try: - var.description = eds.get(section, "Description") - except ValueError: - pass + var.description = eds.get(section, "Description") if eds.has_option(section, "Unit"): - try: - var.unit = eds.get(section, "Unit") - except ValueError: - pass + var.unit = eds.get(section, "Unit") return var @@ -411,9 +434,9 @@ def export_variable(var, eds): eds.set(section, "PDOMapping", hex(var.pdo_mappable)) if getattr(var, 'min', None) is not None: - eds.set(section, "LowLimit", var.min) + eds.set(section, "LowLimit", _int_to_hex(var.data_type, var.min)) if getattr(var, 'max', None) is not None: - eds.set(section, "HighLimit", var.max) + eds.set(section, "HighLimit", _int_to_hex(var.data_type, var.max)) if getattr(var, 'description', '') != '': eds.set(section, "Description", var.description) diff --git a/test/test_eds.py b/test/test_eds.py index ab7cf613..e9efefed 100644 --- a/test/test_eds.py +++ b/test/test_eds.py @@ -136,23 +136,21 @@ def test_record(self): self.assertFalse(var.is_domain) def test_record_with_limits(self): - int8 = self.od[0x3020] - self.assertEqual(int8.min, 0) - self.assertEqual(int8.max, 127) - uint8 = self.od[0x3021] - self.assertEqual(uint8.min, 2) - self.assertEqual(uint8.max, 10) - int32 = self.od[0x3030] - self.assertEqual(int32.min, -2147483648) - self.assertEqual(int32.max, -1) - int64 = self.od[0x3040] - self.assertEqual(int64.min, -10) - self.assertEqual(int64.max, +10) - # Verify all remaining SIGNED_TYPES are handled (INTEGER24/40/48/56) - for index in (0x3031, 0x3032, 0x3033, 0x3034): - var = self.od[index] - self.assertEqual(var.min, -1, f"min mismatch at 0x{index:04X}") - self.assertEqual(var.max, 0, f"max mismatch at 0x{index:04X}") + cases = [ + (0x3020, 0, 127), # INTEGER8 + (0x3021, 2, 10), # UNSIGNED8 + (0x3030, -2147483648, -1), # INTEGER32 + (0x3031, -1, 0), # INTEGER24 + (0x3032, -1, 0), # INTEGER40 + (0x3033, -1, 0), # INTEGER48 + (0x3034, -1, 0), # INTEGER56 + (0x3040, -10, +10), # INTEGER64 + ] + for index, expected_min, expected_max in cases: + with self.subTest(index=f"0x{index:04X}"): + var = self.od[index] + self.assertEqual(var.min, expected_min) + self.assertEqual(var.max, expected_max) def test_signed_int_from_hex(self): for data_type, test_cases in self.test_data.items(): @@ -161,6 +159,13 @@ def test_signed_int_from_hex(self): result = _signed_int_from_hex('0x' + test_case["hex_str"], test_case["bit_length"]) self.assertEqual(result, test_case["expected"]) + def test_signed_int_from_hex_rejects_out_of_range(self): + # Value must fit in bit_length bits (unsigned representation). + with self.assertRaises(ValueError): + _signed_int_from_hex("0xFFFF", 8) # 16-bit value into 8-bit field + with self.assertRaises(ValueError): + _signed_int_from_hex("-1", 8) # negative inputs are never valid + def test_array_compact_subobj(self): array = self.od[0x1003] self.assertIsInstance(array, canopen.objectdictionary.ODArray) From 4319d4af947a9b7f64153885c787645bd5be55f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frieder=20Sch=C3=BCler?= Date: Mon, 11 May 2026 14:04:58 +0200 Subject: [PATCH 4/5] eds: use eds.get() in warning for DefaultValue/ParameterValue (mypy fix) --- canopen/objectdictionary/eds.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/canopen/objectdictionary/eds.py b/canopen/objectdictionary/eds.py index da29b73a..d29acb9b 100644 --- a/canopen/objectdictionary/eds.py +++ b/canopen/objectdictionary/eds.py @@ -345,7 +345,7 @@ def build_variable( except ValueError: logger.warning( "Invalid DefaultValue %r for %s (0x%X), ignoring", - var.default_raw, var.name, var.index, + eds.get(section, "DefaultValue"), var.name, var.index, ) if eds.has_option(section, "ParameterValue"): try: @@ -354,7 +354,7 @@ def build_variable( except ValueError: logger.warning( "Invalid ParameterValue %r for %s (0x%X), ignoring", - var.value_raw, var.name, var.index, + eds.get(section, "ParameterValue"), var.name, var.index, ) # Factor, Description and Unit are not standard according to the CANopen specifications, but # they are implemented in the python canopen package, so we can at least try to use them From 2382b96a02646ce2b285c52abe015c8d52830f35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frieder=20Sch=C3=BCler?= Date: Mon, 11 May 2026 14:24:54 +0200 Subject: [PATCH 5/5] eds: support decimal and hex literals for signed LowLimit/HighLimit --- canopen/objectdictionary/eds.py | 28 ++++++++++++++++++++-------- test/sample.eds | 18 ++++++++++++++++++ test/test_eds.py | 27 +++++++++++++++++---------- 3 files changed, 55 insertions(+), 18 deletions(-) diff --git a/canopen/objectdictionary/eds.py b/canopen/objectdictionary/eds.py index d29acb9b..96f4bdda 100644 --- a/canopen/objectdictionary/eds.py +++ b/canopen/objectdictionary/eds.py @@ -218,14 +218,26 @@ def _calc_bit_length(data_type: int) -> int: def _signed_int_from_hex(hex_str, bit_length): number = int(hex_str, 0) - if number < 0 or number >= (1 << bit_length): - raise ValueError( - f"Value {hex_str!r} is out of range for a {bit_length}-bit signed integer" - ) - max_value = (1 << (bit_length - 1)) - 1 - if number > max_value: - return number - (1 << bit_length) - return number + min_signed = -(1 << (bit_length - 1)) + max_signed = (1 << (bit_length - 1)) - 1 + max_unsigned = (1 << bit_length) - 1 + + if number < 0: + # Negative decimal literal (e.g. LowLimit=-32768) + if number < min_signed: + raise ValueError( + f"Value {hex_str!r} is out of range for a {bit_length}-bit signed integer" + ) + return number + else: + # Unsigned hex literal, two's-complement (e.g. LowLimit=0xFFFF → -1 for INTEGER16) + if number > max_unsigned: + raise ValueError( + f"Value {hex_str!r} is out of range for a {bit_length}-bit signed integer" + ) + if number > max_signed: + return number - (1 << bit_length) + return number def _convert_variable(node_id, var_type, value): diff --git a/test/sample.eds b/test/sample.eds index 49c95ce7..2d38ab7b 100644 --- a/test/sample.eds +++ b/test/sample.eds @@ -967,6 +967,24 @@ HighLimit=0x0A LowLimit=0x02 PDOMapping=0 +[3022] +ParameterName=UNSIGNED16 decimal limits +ObjectType=0x7 +DataType=0x06 +AccessType=rw +LowLimit=100 +HighLimit=1000 +PDOMapping=0 + +[3023] +ParameterName=INTEGER16 decimal limits +ObjectType=0x7 +DataType=0x03 +AccessType=rw +LowLimit=-100 +HighLimit=100 +PDOMapping=0 + [3030] ParameterName=INTEGER32 only negative values ObjectType=0x7 diff --git a/test/test_eds.py b/test/test_eds.py index e9efefed..7b14bb8f 100644 --- a/test/test_eds.py +++ b/test/test_eds.py @@ -137,14 +137,16 @@ def test_record(self): def test_record_with_limits(self): cases = [ - (0x3020, 0, 127), # INTEGER8 - (0x3021, 2, 10), # UNSIGNED8 - (0x3030, -2147483648, -1), # INTEGER32 - (0x3031, -1, 0), # INTEGER24 - (0x3032, -1, 0), # INTEGER40 - (0x3033, -1, 0), # INTEGER48 - (0x3034, -1, 0), # INTEGER56 - (0x3040, -10, +10), # INTEGER64 + (0x3020, 0, 127), # INTEGER8 hex limits + (0x3021, 2, 10), # UNSIGNED8 hex limits + (0x3022, 100, 1000), # UNSIGNED16 decimal limits + (0x3023, -100, 100), # INTEGER16 decimal limits + (0x3030, -2147483648, -1), # INTEGER32 hex limits + (0x3031, -1, 0), # INTEGER24 hex limits + (0x3032, -1, 0), # INTEGER40 hex limits + (0x3033, -1, 0), # INTEGER48 hex limits + (0x3034, -1, 0), # INTEGER56 hex limits + (0x3040, -10, +10), # INTEGER64 hex limits ] for index, expected_min, expected_max in cases: with self.subTest(index=f"0x{index:04X}"): @@ -159,12 +161,17 @@ def test_signed_int_from_hex(self): result = _signed_int_from_hex('0x' + test_case["hex_str"], test_case["bit_length"]) self.assertEqual(result, test_case["expected"]) + def test_signed_int_from_hex_accepts_decimal(self): + # Negative decimal values are valid EDS literals (CiA 306 allows both formats). + self.assertEqual(_signed_int_from_hex("-1", 8), -1) + self.assertEqual(_signed_int_from_hex("-128", 8), -128) + self.assertEqual(_signed_int_from_hex("-2147483648", 32), -2147483648) + def test_signed_int_from_hex_rejects_out_of_range(self): - # Value must fit in bit_length bits (unsigned representation). with self.assertRaises(ValueError): _signed_int_from_hex("0xFFFF", 8) # 16-bit value into 8-bit field with self.assertRaises(ValueError): - _signed_int_from_hex("-1", 8) # negative inputs are never valid + _signed_int_from_hex("-129", 8) # below minimum for 8-bit signed def test_array_compact_subobj(self): array = self.od[0x1003]