Skip to content

eds: fix signed integer limit parsing and export for all SIGNED_TYPES#658

Open
friederschueler wants to merge 5 commits into
canopen-python:masterfrom
friederschueler:fix/eds-limit-parsing
Open

eds: fix signed integer limit parsing and export for all SIGNED_TYPES#658
friederschueler wants to merge 5 commits into
canopen-python:masterfrom
friederschueler:fix/eds-limit-parsing

Conversation

@friederschueler
Copy link
Copy Markdown
Collaborator

@friederschueler friederschueler commented May 6, 2026

Fixes incomplete and incorrect signed integer handling for LowLimit/HighLimit in EDS parsing and export, as noted in #352.

Changes

_calc_bit_length — extended to all SIGNED_TYPES

The existing function only handled INTEGER8/16/32/64. It now covers all 8 SIGNED_TYPES (INTEGER8/16/24/32/40/48/56/64) by deriving the bit width dynamically from ODVariable.STRUCT_TYPES instead of a hardcoded if/elif chain.

_signed_int_from_hex — decimal and hex support, range validation

The parser now accepts both formats for LowLimit/HighLimit:

  • Unsigned two's-complement hex (e.g. 0xFFFF for INTEGER16 → -1)
  • Signed decimal (e.g. -32768 for INTEGER16 → -32768)

An explicit ValueError is raised if the value does not fit in the type's bit width. Previously, a value like 0xFFFF for an 8-bit field would silently produce a wrong result.

_int_to_hex — new helper for EDS integer serialization

Formats any integer as a valid EDS hex literal. Negative signed values are written as two's-complement unsigned hex (e.g. INTEGER32 -10xFFFFFFFF), which is the canonical EDS format. Used by _revert_variable and the export of LowLimit/HighLimit.

_revert_variable — fix for negative integers

Previously used f"0x{value:02X}" unconditionally, which raises ValueError for negative values and broke export of any negative DefaultValue. Now delegates to _int_to_hex.

Export of LowLimit/HighLimit

Previously written as raw Python integers (e.g. -2147483648). Now uses _int_to_hex to produce canonical unsigned two's-complement hex.

Silent parse errors → logger.warning

All except ValueError: pass blocks in build_variable now log the field name, value, object name and index:

  • LowLimit, HighLimit, DefaultValue, ParameterValue, Factor

Dead try/except ValueError around Description and Unit removed — ConfigParser.get() never raises ValueError.

Tests

  • Add sample.eds entries covering all four combinations of signed/unsigned × hex/decimal limits
  • Refactor test_record_with_limits as a table-driven test with subTest
  • Add test_signed_int_from_hex_accepts_decimal and test_signed_int_from_hex_rejects_out_of_range

Closes part of #352.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2026

Codecov Report

❌ Patch coverage is 87.50000% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
canopen/objectdictionary/eds.py 87.50% 3 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@acolomb
Copy link
Copy Markdown
Member

acolomb commented May 7, 2026

Please always separate style changes from functional changes. Except of course fixing in the immediate vicinity of the functional changes.

Comment thread canopen/objectdictionary/eds.py Outdated
Comment thread canopen/objectdictionary/eds.py Outdated
Comment thread test/test_eds.py Outdated
- 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 canopen-python#352.
@friederschueler friederschueler force-pushed the fix/eds-limit-parsing branch from 27e94d8 to f65a1fb Compare May 7, 2026 20:15
@friederschueler friederschueler changed the title eds: fix LowLimit/HighLimit parsing for all signed integer types eds: fix signed integer limit parsing and export for all SIGNED_TYPES May 11, 2026
Copy link
Copy Markdown
Collaborator Author

@friederschueler friederschueler left a comment

Choose a reason for hiding this comment

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

As the old importer couldn't use decimal format for signed limits, but we exported decimal there was definitively a bug. Now we also export in hex format by default, which is more common in eds I think. We could change that, or we could remember on import the exact format and then export it in that format, but this seems like an overkill.

@acolomb
Copy link
Copy Markdown
Member

acolomb commented May 12, 2026

Thanks for drilling deeper, the incomplete number parsing really was a nuisance. I don't know which is more common, hex or decimal for the limit values in EDS files. My random sample shows both in use, but never hexadecimal coding for negative limits.

But definitely, any format with a leading minus sign is easier to grasp than writing two's complement hex numbers. Since that is very uncommon for hexadecimal numbers though, I'd favor defaulting to plain decimal notation on export. CiA 306 never mentions negative numbers, let alone writing any number in two's complement. But we can keep this special parsing feature of course.

Need another slot of free time to finish reviewing, sorry for this incomplete feedback...

"""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.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

According to CiA 306:

Integer numbers shall be written as decimal numbers, hexadecimal numbers or octal numbers.

I don't see any need to restrict ourselves to writing hex or two's complement at all. We may still accept it, though, as the data type rules are very clear on the possible interpretation. But the spec doesn't mention this at all.

It's also completely unclear how to handle floating point numbers in EDS files. I guess if we can parse them in Python, we can accept them. But I wouldn't assume any particular conversion to apply for a literal in hex notation: It's fine to write -0xFFFF.FF as a theoretical floating-point number noted in hex, just that Python only parses decimal floats.

>>> float(int("-0xFFFF", 16))  # this works fine, but not directly
-65535.0

Just because CiA 301 defines a wire format for REAL types, doesn't mean we should try parsing hexadecimal byte sequences as IEEE floats. When exporting, the best approach I can see is to simply use Python's float to string conversion. This function gets in the way though, prohibiting us from writing anything but integer limits at all. Why not simply use _revert_variable() for the limits as well? The fall-back (integer) case in there can still do some limit checking, but no hex conversion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants