Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Comments

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

Open
zaporozhets wants to merge 1 commit intocanopen-python:master from
zaporozhets:feat/add_xdd_parser
Open

xdd: Implement option to load object dictionary from XDD file. #614
zaporozhets wants to merge 1 commit intocanopen-python:master from
zaporozhets:feat/add_xdd_parser

Conversation

@zaporozhets
Copy link

@zaporozhets zaporozhets commented Sep 11, 2025

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>
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
Copy link
Author

Hi @acolomb, could you please take a look?
acolomb reacted with thumbs up emoji

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
returnod
returnod


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", lambdaval: 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

Reviewers

@acolomb acolomb acolomb left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /