Skip to content

od: Extend physical conversion to integer factors, fix typing #673

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

Open
acolomb wants to merge 8 commits into
base: master
Choose a base branch
from
Open

od: Extend physical conversion to integer factors, fix typing #673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4ed62ae
Fix type handling for physical variable representation.
acolomb Jun 4, 2026
5aec961
Extend physical conversion to float types.
acolomb Jun 4, 2026
af462c9
Allow integer factors for physical representation.
acolomb Jun 4, 2026
aa8a166
Accept integer Factor= settings from EDS.
acolomb Jun 4, 2026
9ad88a7
Test integer factors.
acolomb Jun 4, 2026
b556966
Test integer factor in EDS.
acolomb Jun 4, 2026
eb85634
Test real32 and pass-through of non-numeric variables.
acolomb Jun 4, 2026
f1838f3
suggested changes from review
bizfsc Jun 24, 2026
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
25 changes: 17 additions & 8 deletions canopen/objectdictionary/__init__.py
View file
Open in desktop

@bizfsc bizfsc Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

    def decode_phys(
        self, value: Union[int, bool, float, str, bytes]
    ) -> Union[int, bool, float, str, bytes]:
        if self.data_type in NUMBER_TYPES:
            numeric = cast(Union[int, float], value)
            value = numeric * self.factor
        return value

    def encode_phys(
        self, value: Union[int, bool, float, str, bytes]
    ) -> Union[int, bool, float, str, bytes]:
        if self.data_type in NUMBER_TYPES:
            numeric = cast(Union[int, float], value)
            if self.factor != 1:
                numeric = numeric / self.factor
            if self.data_type in INTEGER_TYPES:
                numeric = round(numeric)
            value = numeric
        return value

Used too much brain power to solve this issue for the current temperatures. But here is the dilemma:

  1. don't use assert in production
  2. don't change signature
  3. don't ask for permission, ask for forgiveness

=> cast is noop in runtime = zero loss and the linter is happy, because the can not see that only int or float can be passed. This should be the best solution. Also numeric / self.factor will raise natural type error for wrong types

@acolomb acolomb Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use assert in production

Why? I feel like it's the perfect solution.

"In production" means that one should enable optimizations I guess, so the assertions become no-ops. A quick AI summary (without even hinting at type checks) cooked up these three use cases as OK to use assert:

  • Internal consistency checks: assert tree.root is not None
  • Conditions that indicate programmer bugs: assert isinstance(ast_node, Expression) (Yay!)
  • Debugging assumptions: assert cache_entry.owner is self

I'd rather reach a consensus here, before applying suggestions to the PR which are not clearly fixes for an oversight. 😉

Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import logging
import struct
from collections.abc import Collection, Iterator, Mapping, MutableMapping
from typing import Optional, TextIO, Union
from typing import Optional, TextIO, Union, cast

from canopen.objectdictionary.datatypes import *
from canopen.objectdictionary.datatypes import IntegerN, UnsignedN
Expand Down Expand Up @@ -354,7 +354,7 @@ def __init__(self, name: str, index: int, subindex: int = 0):
#: Physical unit
self.unit: str = ""
#: Factor between physical unit and integer value
self.factor: float = 1
self.factor: Union[int, float] = 1
#: Minimum allowed value
self.min: Optional[int] = None
#: Maximum allowed value
Expand Down Expand Up @@ -486,15 +486,24 @@ def encode_raw(self, value: Union[int, float, str, bytes, bytearray]) -> bytes:
raise TypeError(
f"Do not know how to encode {value!r} to data type 0x{self.data_type:X}")

def decode_phys(self, value: int) -> Union[int, bool, float, str, bytes]:
if self.data_type in INTEGER_TYPES:
value *= self.factor
def decode_phys(
self, value: Union[int, bool, float, str, bytes]
) -> Union[int, bool, float, str, bytes]:
if self.data_type in NUMBER_TYPES:
numeric = cast(Union[int, float], value)
value = numeric * self.factor
return value

def encode_phys(self, value: Union[int, bool, float, str, bytes]) -> int:
if self.data_type in INTEGER_TYPES:
def encode_phys(
self, value: Union[int, bool, float, str, bytes]
) -> Union[int, bool, float, str, bytes]:
if self.data_type in NUMBER_TYPES:
numeric = cast(Union[int, float], value)
if self.factor != 1:
value = round(value / self.factor)
numeric = numeric / self.factor
if self.data_type in INTEGER_TYPES:
numeric = round(numeric)
value = numeric
return value

def decode_desc(self, value: int) -> str:
Expand Down
10 changes: 8 additions & 2 deletions canopen/objectdictionary/eds.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,9 +337,15 @@ def build_variable(
# 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"))
var.factor = int(eds.get(section, "Factor"))
except ValueError:
pass
try:
var.factor = float(eds.get(section, "Factor"))
except ValueError:
logger.warning(
"Could not parse Factor for %s in section [%s], ignoring",
var.name, section,
)
if eds.has_option(section, "Description"):
try:
var.description = eds.get(section, "Description")
Expand Down
12 changes: 11 additions & 1 deletion test/sample.eds
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -995,7 +995,7 @@ ParameterName=Highest subindex
ObjectType=0x7
DataType=0x0005
AccessType=ro
DefaultValue=0x02
DefaultValue=0x03
PDOMapping=0x0

[3050sub1]
Expand All @@ -1018,6 +1018,16 @@ Factor=ERROR
Description=
Unit=

[3050sub3]
ParameterName=Integer Factor
ObjectType=0x7
DataType=0x0004
AccessType=ro
PDOMapping=0x0
Factor=42
Description=Should be parsed as integer
Unit=answer

[3063]
ParameterName=DOMAIN object
ObjectType=0x2
Expand Down
5 changes: 4 additions & 1 deletion test/test_eds.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,11 +211,14 @@ def test_reading_factor(self):
var = self.od['EDS file extensions']['FactorAndDescription']
self.assertEqual(var.factor, 0.1)
self.assertEqual(var.description, "This is the a test description")
self.assertEqual(var.unit,'mV')
self.assertEqual(var.unit, 'mV')
var2 = self.od['EDS file extensions']['Error Factor and No Description']
self.assertEqual(var2.description, '')
self.assertEqual(var2.factor, 1)
self.assertEqual(var2.unit, '')
var3 = self.od['EDS file extensions']['Integer Factor']
self.assertEqual(var3.factor, 42)
self.assertIsInstance(var3.factor, int)

def test_read_domain_object(self):
var = self.od[0x3063]
Expand Down
40 changes: 39 additions & 1 deletion test/test_od.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,14 +183,33 @@ def test_unknown_data_type(self):

class TestAlternativeRepresentations(unittest.TestCase):

def test_phys(self):
def test_phys_integer(self):
var = od.ODVariable("Test INTEGER16", 0x1000)
var.data_type = od.INTEGER16
var.factor = 0.1
self.assertAlmostEqual(var.decode_phys(128), 12.8)
self.assertEqual(var.encode_phys(-0.1), -1)

def test_phys_real(self):
var = od.ODVariable("Test REAL32", 0x1000)
var.data_type = od.REAL32
var.factor = 0.1
self.assertAlmostEqual(var.decode_phys(128), 12.8)
self.assertEqual(var.encode_phys(-0.1), -1)

def test_phys_boolean(self):
var = od.ODVariable("Test BOOLEAN", 0x1000)
var.data_type = od.BOOLEAN
self.assertEqual(var.decode_phys(True), True)
self.assertEqual(var.decode_phys(False), False)
self.assertEqual(var.encode_phys(True), True)

def test_phys_string(self):
var = od.ODVariable("Test VISIBLE_STRING", 0x1000)
var.data_type = od.VISIBLE_STRING
self.assertEqual(var.decode_phys('foo'), 'foo')
self.assertEqual(var.encode_phys('bar'), 'bar')

def test_phys_factor_1_int64_roundtrip(self):
"""int64 values must survive encode_phys when factor is 1."""
var = od.ODVariable("Test UNSIGNED64", 0x1000)
Expand Down Expand Up @@ -227,6 +246,25 @@ def test_phys_float_factor_decodes_to_float(self):
var.factor = 1.0
self.assertIsInstance(var.decode_phys(42), float)

def test_phys_int_factor(self):
"""Integer factor uses float division + round."""
var = od.ODVariable("Test INTEGER16", 0x1000)
var.data_type = od.INTEGER16
var.factor = 3
# 10 / 3 = 3
encoded = var.encode_phys(10)
self.assertEqual(encoded, 3)
self.assertIsInstance(encoded, int)

def test_phys_int_factor_decodes_to_int(self):
"""decode_phys with float factor ensures a float result."""
Comment thread
acolomb marked this conversation as resolved.
var = od.ODVariable("Test INTEGER32", 0x1000)
var.data_type = od.INTEGER32
var.factor = 10
decoded = var.decode_phys(42)
self.assertEqual(decoded, 420)
self.assertIsInstance(decoded, int)

def test_desc(self):
var = od.ODVariable("Test UNSIGNED8", 0x1000)
var.data_type = od.UNSIGNED8
Expand Down
Loading