Skip to content

Conversation

@zaporozhets
Copy link

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

Signed-off-by: Taras Zaporozhets <zaporozhets.taras@gmail.com>
@codecov
Copy link

codecov bot commented Sep 22, 2025

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 zaporozhets marked this pull request as ready for review September 25, 2025 15:52
@zaporozhets
Copy link
Author

Hi @acolomb, could you please take a look?

@acolomb
Copy link
Member

acolomb commented Oct 26, 2025

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.

Copy link
Member

@acolomb acolomb left a comment

Choose a reason for hiding this comment

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

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.


import re
import xml.etree.ElementTree as etree
from configparser import NoOptionError
Copy link
Member

Choose a reason for hiding this comment

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

This is a leftover from eds?

@@ -0,0 +1,306 @@
import logging

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

import re
import xml.etree.ElementTree as etree
from configparser import NoOptionError
from typing import TYPE_CHECKING
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from typing import TYPE_CHECKING

Comment on lines +12 to +13
if TYPE_CHECKING:
import canopen.network
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if TYPE_CHECKING:
import canopen.network

RECORD = 9


def import_xdd(xdd, node_id):
Copy link
Member

Choose a reason for hiding this comment

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

With static type hints, something like this:

Suggested change
def import_xdd(xdd, node_id):
def import_xdd(
xdd: Union[etree.Element, str, bytes, os.PathLike],
node_id: Optional[int],
) -> ObjectDictionary:

try:
od.node_id = int(node_id, 0)
except (ValueError, TypeError):
pass
Copy link
Member

Choose a reason for hiding this comment

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

No logging of this failure?

Comment on lines +45 to +46

return od
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return od
return od


return od

def _add_device_information_to_od(od, root):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def _add_device_information_to_od(od, root):
def _add_device_information(od: ObjectDictionary, root: etree.Element):

Keep the names slightly shorter, it's all already in the context of the OD.

device_identity = root.find('.//{*}DeviceIdentity')
if device_identity is not None:
for src_prop, dst_prop, f in [
("vendorName", "vendor_name", lambda val: str(val)),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
("vendorName", "vendor_name", lambda val: str(val)),
("vendorName", "vendor_name", str),

This can be simplified in many places, no need for a lambda.

if device_identity is not None:
for src_prop, dst_prop, f in [
("vendorName", "vendor_name", lambda val: str(val)),
("vendorID", "vendor_number", lambda val: int(val, 0)),
Copy link
Member

Choose a reason for hiding this comment

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

You could create a autoint = functools.partial(int, base=0) constructor once in this module, then use it to simplify in these cases.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants