From 79f8bf839fa531dc272ee61e005b001e8498688f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 12 May 2026 23:38:34 +0200 Subject: [PATCH 1/4] Correct type handling in decode_bits() and encode_bits(). Contrary to the existing type hints, the methods can handle strings for lookup as well as an arbitrary collection of integer bit offsets. Correct the hints to reflect that and check at runtime to only lookup from strings. Correct return type for encode_bits(), as it works only for integers. Document both functions to explain the intention and caveats to consider, especially the lookup KeyError exception. Extend test to cover an iterable and a failed lookup. --- canopen/objectdictionary/__init__.py | 35 +++++++++++++++++++++------- test/test_od.py | 6 +++++ 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/canopen/objectdictionary/__init__.py b/canopen/objectdictionary/__init__.py index 40ab8fbf..a505ef0d 100644 --- a/canopen/objectdictionary/__init__.py +++ b/canopen/objectdictionary/__init__.py @@ -6,7 +6,7 @@ import logging import struct -from collections.abc import Iterator, Mapping, MutableMapping +from collections.abc import Collection, Iterator, Mapping, MutableMapping from typing import Optional, TextIO, Union from canopen.objectdictionary.datatypes import * @@ -516,21 +516,38 @@ def encode_desc(self, desc: str) -> int: raise ValueError( f"No value corresponds to '{desc}'. Valid values are: {valid_values}") - def decode_bits(self, value: int, bits: list[int]) -> int: - try: + def decode_bits(self, value: int, bits: Union[str, Collection[int]]) -> int: + """Isolate and right-shift the specified bits from a given integer. + + :param value: Variable value holding the bits + :param bits: Registered lookup name or concrete list of bit offsets + :return: Extracted bits, right-shifted to cut off to lowest specified offset + :raises KeyError: For unknown lookup names + """ + if isinstance(bits, str): bits = self.bit_definitions[bits] - except (TypeError, KeyError): - pass mask = 0 for bit in bits: mask |= 1 << bit return (value & mask) >> min(bits) - def encode_bits(self, original_value: int, bits: list[int], bit_value: int): - try: + def encode_bits( + self, original_value: int, bits: Union[str, Collection[int]], bit_value: int + ) -> int: + """Replace the specified bits with the given (unshifted) pattern. + + The bit offsets sequence may be non-contiguous, but the replacement pattern + must specify all bits including the "holes". It is only shifted once, so the + LSB lands at the lowest specified bit offset. + + :param original_value: Variable value holding the bits + :param bits: Registered lookup name or concrete list of bit offsets + :param bit_value: Source pattern to overwrite with + :return: Merged value with the bits replaced + :raises KeyError: For unknown lookup names + """ + if isinstance(bits, str): bits = self.bit_definitions[bits] - except (TypeError, KeyError): - pass temp = original_value mask = 0 for bit in bits: diff --git a/test/test_od.py b/test/test_od.py index d6e3e984..559c1a3b 100644 --- a/test/test_od.py +++ b/test/test_od.py @@ -238,10 +238,16 @@ def test_bits(self): self.assertEqual(var.decode_bits(1, "BIT 0"), 1) self.assertEqual(var.decode_bits(1, [1]), 0) self.assertEqual(var.decode_bits(0xf, [0, 1, 2, 3]), 15) + self.assertEqual(var.decode_bits(0xf, range(4)), 15) self.assertEqual(var.decode_bits(8, "BIT 2 and 3"), 2) self.assertEqual(var.encode_bits(0xf, [1], 0), 0xd) self.assertEqual(var.encode_bits(0, "BIT 0", 1), 1) + with self.assertRaises(KeyError): + var.decode_bits(0, "DOES NOT EXIST") + with self.assertRaises(KeyError): + var.encode_bits(0, "DOES NOT EXIST", 0) + class TestObjectDictionary(unittest.TestCase): From a0ffffb73b5d1dd450c0b8d24594f033d9530c70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 12 May 2026 23:51:22 +0200 Subject: [PATCH 2/4] Add unit tests involving sparse bit offset enumerations. Test encoding and decoding with non-contiguous bit offsets. --- test/test_od.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/test_od.py b/test/test_od.py index 559c1a3b..8aa41d01 100644 --- a/test/test_od.py +++ b/test/test_od.py @@ -248,6 +248,16 @@ def test_bits(self): with self.assertRaises(KeyError): var.encode_bits(0, "DOES NOT EXIST", 0) + def test_bits_sparse(self): + var = od.ODVariable("Test UNSIGNED8", 0x1000) + var.data_type = od.UNSIGNED8 + + self.assertEqual(var.decode_bits(0b11111111, [2, 5]), 0b1001) + self.assertEqual(var.decode_bits(0b11011011, [2, 5]), 0) + self.assertEqual(var.encode_bits(0b11111111, [2, 5], 0), 0b11011011) + self.assertEqual(var.encode_bits(0b00000000, [2, 5], 0b1001), 0b00100100) + self.assertEqual(var.encode_bits(0b00000000, [2, 5], 0b1111), 0b00100100) + class TestObjectDictionary(unittest.TestCase): From dcf7bf4a848d52bd59ed44f07c3db832f32dc793 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Tue, 12 May 2026 23:53:46 +0200 Subject: [PATCH 3/4] Fix unintended bit overwriting with sparse bit offsets. Any bit for which the replacement pattern contained a 1 bit in one of the "holes", would be set despite not being part of the desired mask. --- canopen/objectdictionary/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/canopen/objectdictionary/__init__.py b/canopen/objectdictionary/__init__.py index a505ef0d..dbc6c855 100644 --- a/canopen/objectdictionary/__init__.py +++ b/canopen/objectdictionary/__init__.py @@ -553,7 +553,7 @@ def encode_bits( for bit in bits: mask |= 1 << bit temp &= ~mask - temp |= bit_value << min(bits) + temp |= (bit_value << min(bits)) & mask return temp From 5587d9a173325624468a6532a2ca020a302e97a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Wed, 13 May 2026 00:39:29 +0200 Subject: [PATCH 4/4] Correct type annotations for class Bits. This mechanism works only for integers, so add appropriate assertions. Document all expected types for indexing the Bits. --- canopen/variable.py | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/canopen/variable.py b/canopen/variable.py index 639a1839..ede20607 100644 --- a/canopen/variable.py +++ b/canopen/variable.py @@ -1,5 +1,7 @@ +from __future__ import annotations + import logging -from collections.abc import Mapping +from collections.abc import Collection, Mapping from typing import Union from canopen import objectdictionary @@ -118,7 +120,7 @@ def desc(self, desc: str): self.raw = self.od.encode_desc(desc) @property - def bits(self) -> "Bits": + def bits(self) -> Bits: """Access bits using integers, slices, or bit descriptions.""" return Bits(self) @@ -169,23 +171,23 @@ def write( class Bits(Mapping): def __init__(self, variable: Variable): + assert variable.od.data_type in objectdictionary.datatypes.INTEGER_TYPES self.variable = variable self.read() + self.raw: int @staticmethod - def _get_bits(key): + def _get_bits(key: Union[slice, int, str, Collection[int]]) -> Union[str, Collection[int]]: if isinstance(key, slice): - bits = range(key.start, key.stop, key.step) - elif isinstance(key, int): - bits = [key] - else: - bits = key - return bits - - def __getitem__(self, key) -> int: + return range(key.start, key.stop, key.step) + if isinstance(key, int): + return [key] + return key + + def __getitem__(self, key: Union[slice, int, str, Collection[int]]) -> int: return self.variable.od.decode_bits(self.raw, self._get_bits(key)) - def __setitem__(self, key, value: int): + def __setitem__(self, key: Union[slice, int, str, Collection[int]], value: int): self.raw = self.variable.od.encode_bits( self.raw, self._get_bits(key), value) self.write() @@ -197,7 +199,8 @@ def __len__(self): return len(self.variable.od.bit_definitions) def read(self): - self.raw = self.variable.raw + assert isinstance(raw_int := self.variable.raw, int) + self.raw = raw_int def write(self): self.variable.raw = self.raw