xdd: Implement option to load object dictionary from XDD file. by zaporozhets · Pull Request #614 · canopen-python/canopen

zaporozhets · 2025-09-11T16:58:48Z

Hi All,

This is a WIP merge request that adds support for XDD import to CANopen. I'm still in the process of testing and making improvements, but I’d appreciate any feedback you might have in the meantime.

Feel free to share any questions or suggestions.

Best regards,
Taras

codecov · 2025-09-22T19:52:15Z

Codecov Report

❌ Patch coverage is 70.28302% with 63 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
canopen/objectdictionary/xdd.py	68.61%	44 Missing and 15 partials ⚠️
canopen/utils.py	76.47%	2 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

zaporozhets · 2025-10-16T20:30:12Z

Hi @acolomb, could you please take a look?

acolomb · 2025-10-26T21:45:59Z

Thank you very much for this contribution. Happy to see some progress on the XDD front. I started taking a look and overall I think it's going in a good direction.

Need to take some more time to put down my thoughts in detail, but some high-level pointers first:

For new code, we expect to have full type annotations. The mypy linter run in the PR checks can give you hints about what needs improvement, or best run it locally. Note that any functions without argument or return type annotations are not even checked by default.
I don't like the idea of inflating the canopen.utils module, even though avoiding code duplication is a worthy goal. I'll try to come up with a simplification soon.
Where I do want less duplication is the object code constants declared in both eds.py and xdd.py. Have a look at commits 7c6dee5 and db9110e in master, those can be cherry-picked easily.
A little more attention to code formatting would be nice. I use flake8 locally and try to keep it from complaining. Will report any outstanding issues during more thorough review.
I assume there is no copyright involved for the sample.xdd file? I don't know where the CANopenNode project got its references from, but just trying to be careful about legal issues with files we include here.

acolomb

Just a partial review again, but I hope you get the general idea... More to come when I get another look with a fresher brain.

tomasbarton-stemcell · 2026-03-13T21:29:56Z

hi @zaporozhets, thanks for the work on this PR! Are you still planning to continue with it? I have a project where I could really use xdd/xdc parsing right now, so I'd be glad to pick up where you left off and contribute to this effort if you don't have the bandwidth.

zaporozhets · 2026-03-16T14:52:34Z

@acolomb, thank you for the review, and @tomasbarton-stemcell for your interest in this feature.

I was extremely busy last year and didn’t have the chance to address all the comments.

I’m currently on sick leave until Monday, so I finally have some time to catch up. Please give me a few days to refresh my memory and go through all the suggestions. I’ll keep you posted. I’ll definitely appreciate a thorough review here, as Python isn’t really my main area of expertise.

BR,
Taras

Signed-off-by: Taras Zaporozhets <[email protected]>

zaporozhets · 2026-03-24T21:02:15Z

Thank you for your patience. I’ve addressed (hopefully all) the comments and refactored several sections to simplify the parsing logic.

1.Added full type annotations and verified the code with mypy --config-file pyproject.toml ....
2. Removed changes from canopen.utils.
3. Applied updates from esd.py to reduce duplicated code.
4. Fixed formatting and validated it with flake8.
5. Created a new sample.xdd based on sample.eds, without relying on third-party EDS/XDD files.
6. Squashed changes in one commit.

@acolomb, @tomasbarton-stemcell could you please review?

tomasbarton-stemcell

Hi, thanks for the update! I tested this on a few xdc and xdd files I have in my project (all generated by the editor that comes with the canopennode C library) and all that I tried worked fine. I added some comments, but I am not really a Python programmer so 🤷🏻‍♂️

As a side not, could the import_od be relaxed to load xdc files as well? I am not exactly sure what the difference is, but with the files we have I can load them just fine and talk to the devices.

tomasbarton-stemcell · 2026-03-30T20:21:45Z

+    _add_device_information(od, root)
+    _add_object_list(od, root)
+    _add_dummy_objects(od, root)


if xdd is None (which it can be according to the type signature) then this will throw here because root won't be defined.

thisone root.find('.//{*}DeviceCommissioning') as well

tomasbarton-stemcell · 2026-03-30T20:24:48Z

+        'USINT': datatypes.UNSIGNED8,
+        'UINT': datatypes.UNSIGNED16,
+        'UDINT': datatypes.UNSIGNED32,
+        'ULINT': datatypes.UNSIGNED32,


is this right here? LINT above is defined to be 64b, but ULINT here is 32b

tomasbarton-stemcell · 2026-03-30T20:29:30Z

+            return int(value, 0)
+
+
+def _convert_variable(


unused. Didn't check other files (shouldn't be as this is _), but there is no reference to it in this file.

tomasbarton-stemcell · 2026-03-30T20:31:03Z

+    device_identity = root.find('.//{*}DeviceIdentity')
+    if device_identity is None:
+        raise ValueError("Missing 'DeviceIdentity' section in XDD file")
+    if device_identity is not None:


redundant None check (statement above guarantees this is not None).

There is the same problem below: if general_features is not None:

tomasbarton-stemcell · 2026-03-30T20:37:33Z

+            ("granularity", "granularity", autoint, None),
+            ("nrOfRxPDO", "nr_of_RXPDO", autoint, "0"),
+            ("nrOfTxPDO", "nr_of_TXPDO", autoint, "0"),
+            ("bootUpSlave", "simple_boot_up_slave", bool, 0),


is bool going to work here? I am not sure what the xml parser gives you, but if it is a string "true" or "false" they will both get converted to True using the bool function.

I was just going by the example xdd you have in test: <CANopenGeneralFeatures granularity="8" nrOfRxPDO="4" nrOfTxPDO="4" bootUpSlave="true" />

tomasbarton-stemcell · 2026-03-30T20:38:46Z

+    value = value.replace(" ", "").upper()
+    if '$NODEID' in value:
+        if node_id is None:
+            logger.warn(


logger.warn is deprecated: https://docs.python.org/3/library/logging.html#logging.Logger.warning

tomasbarton-stemcell · 2026-03-30T20:42:13Z

+    var.description = par_tree.get('description', '')
+
+    # Extract data type
+    data_types = {


Could probably be a file level constant

tomasbarton-stemcell · 2026-04-01T21:10:48Z

+        device_commissioning_elem = root.find('.//{*}DeviceCommissioning')
+        if device_commissioning_elem is not None:
+            node_id_attr = device_commissioning_elem.get("nodeID")


One more thing I noticed with my xdc file from CANopenNode editor:

<deviceCommissioning NodeID="2" nodeName="scale" actualBaudRate="0" networkNumber="0" networkName="" CANopenManager="false" />

The casing is different, it is lower case deviceCommissioning and then upper case NodeID. I am not sure which is is correct according to the standard.

bizfsc

Automated review based on static analysis of the PR diff. Several correctness bugs need fixing before merge; design issues are suggestions.

bizfsc · 2026-05-04T09:48:06Z

+
+logger = logging.getLogger(__name__)
+autoint = functools.partial(int, base=0)
+hex = functools.partial(int, base=16)


Bug: hex shadows the builtins.hex function.

hex is a Python builtin. Redefining it here makes it unavailable for any downstream code in this module and is confusing.

Suggestion: rename to parse_hex or _from_hex.

parse_hex = functools.partial(int, base=16)

bizfsc · 2026-05-04T09:48:06Z

+) -> ObjectDictionary:
+    od = ObjectDictionary()
+
+    if xdd is not None:


Bug: NameError when xdd=None.

If xdd is None, root is never assigned, but it is used immediately at line 30 (root.find('.//{*}DeviceCommissioning')) and again at lines 42-44.

Either raise early or guard all usages:

if xdd is None: raise ValueError("source must not be None") root = etree.parse(xdd).getroot()

bizfsc · 2026-05-04T09:48:06Z

+    device_identity = root.find('.//{*}DeviceIdentity')
+    if device_identity is None:
+        raise ValueError("Missing 'DeviceIdentity' section in XDD file")
+    if device_identity is not None:


Dead branch: if device_identity is not None: is always True here.

The raise ValueError on the line above guarantees device_identity is not None at this point. The if guard is misleading — it implies the block could be skipped. Just dedent the body:

if device_identity is None: raise ValueError("Missing 'DeviceIdentity' section in XDD file") identity_fields = [...]

bizfsc · 2026-05-04T09:48:06Z

+    general_features = root.find('.//{*}CANopenGeneralFeatures')
+    if general_features is None:
+        raise ValueError("Missing 'CANopenGeneralFeatures' element")
+    if general_features is not None:


Same dead-branch issue as with device_identity.

After raise ValueError("Missing 'CANopenGeneralFeatures' element"), the if general_features is not None: guard is always True. Dedent the body.

bizfsc · 2026-05-04T09:48:06Z

+            ("granularity", "granularity", autoint, None),
+            ("nrOfRxPDO", "nr_of_RXPDO", autoint, "0"),
+            ("nrOfTxPDO", "nr_of_TXPDO", autoint, "0"),
+            ("bootUpSlave", "simple_boot_up_slave", bool, 0),


Bug: bool("false") == True in Python.

The XDD attribute value is the string "true" or "false". Passing it to bool(...) will always return True for any non-empty string, including "false".

Fix:

("bootUpSlave", "simple_boot_up_slave", lambda s: str(s).lower() == "true", "false"),

bizfsc · 2026-05-04T09:48:06Z

+        if entry is None:
+            raise ValueError(
+                "Missing 'entry' attribute for dummy object")
+        p = entry.split('=')


IndexError if entry contains no = character.

entry.split('=') returns a list with one element when = is absent; p[1] then raises IndexError.

Safer:

key, _, raw_value = entry.partition('=') if not _: logger.warning("Unexpected dummyUsage entry format: %r", entry) continue value = int(raw_value, 10)

bizfsc · 2026-05-04T09:48:06Z

+    }
+
+    for k, v in data_types.items():
+        if par_tree.find(f'{{*}}{k}') is not None:


Last-wins data-type lookup — missing break.

The loop iterates over all keys even after a match is found. If a <parameter> element happened to contain two type tags (malformed XDD), the last one in the dict wins silently.

Add break after setting var.data_type:

for k, v in data_types.items(): if par_tree.find(f'{{*}}{k}') is not None: var.data_type = v break

bizfsc · 2026-05-04T09:48:06Z

+    value = value.replace(" ", "").upper()
+    if '$NODEID' in value:
+        if node_id is None:
+            logger.warn(


logger.warn() is deprecated since Python 3.2.

Use logger.warning(...) instead.

bizfsc · 2026-05-04T09:48:06Z

+            return int(value, 0)
+
+
+def _convert_variable(


_convert_variable is defined but never called (dead code).

It handles OCTET_STRING, DOMAIN, VISIBLE_STRING, FLOAT_TYPES etc., but none of the callers use it — _build_variable and _set_parameters_from_xdd_canopen_object only call _convert_integer. This means default values for non-integer types (e.g. REAL32, VISIBLE_STRING) are silently ignored.

Either wire up _convert_variable for non-integer defaults, or remove it.

bizfsc · 2026-05-04T09:48:06Z

+        ],
+        "int32": [
+            {"hex_str": "7FFFFFFF", "bit_length": 32, "expected": 2147483647},
+            {"hex_str": "80000000", "bit_length": 32, "expected": -2147483648},


Test missing: bool("false") bug.

No test checks that bootUpSlave="false" (string) is parsed as False. This is the easiest way to catch the bool(str) bug in CI.

def test_boot_up_slave_false(self): # sample.xdd has bootUpSlave="true" self.assertTrue(self.od.device_information.simple_boot_up_slave)

And a complementary file with bootUpSlave="false" should assert False.

zaporozhets force-pushed the feat/add_xdd_parser branch 2 times, most recently from ea733fe to 466b1b2 Compare September 11, 2025 17:14

zaporozhets marked this pull request as ready for review September 25, 2025 15:52

acolomb reviewed Oct 27, 2025

View reviewed changes

zaporozhets force-pushed the feat/add_xdd_parser branch 4 times, most recently from 47656be to 95c8be2 Compare March 22, 2026 20:21

zaporozhets force-pushed the feat/add_xdd_parser branch 2 times, most recently from 1b06485 to 4b7fc1d Compare March 24, 2026 20:46

xdd: Implement option to load object dictionary from XDD file.

7dd46de

Signed-off-by: Taras Zaporozhets <[email protected]>

zaporozhets force-pushed the feat/add_xdd_parser branch from 4b7fc1d to 7dd46de Compare March 24, 2026 20:57

tomasbarton-stemcell reviewed Mar 30, 2026

View reviewed changes

tomasbarton-stemcell reviewed Apr 1, 2026

View reviewed changes

bizfsc reviewed May 4, 2026

View reviewed changes

Conversation

zaporozhets commented Sep 11, 2025

Uh oh!

codecov Bot commented Sep 22, 2025

Codecov Report

Uh oh!

zaporozhets commented Oct 16, 2025

Uh oh!

acolomb commented Oct 26, 2025

Uh oh!

acolomb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomasbarton-stemcell commented Mar 13, 2026

Uh oh!

zaporozhets commented Mar 16, 2026

Uh oh!

zaporozhets commented Mar 24, 2026

Uh oh!

tomasbarton-stemcell left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bizfsc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tomasbarton-stemcell left a comment •

edited

Loading