Skip to content

Custom Options for EDS file #615

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
FedericoSpada wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Custom Options for EDS file #615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6bc9305
Custom Options for EDS file
FedericoSpada Sep 21, 2025
ee7fdf4
Changes requested by the maintainers
FedericoSpada May 4, 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
6 changes: 6 additions & 0 deletions canopen/objectdictionary/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,8 @@ def __init__(self, name: str, index: int):
self.storage_location = None
self.subindices = {}
self.names = {}
#: Key-Value pairs not defined by the standard
Copy link
Copy Markdown
Member

@acolomb acolomb May 4, 2026

Choose a reason for hiding this comment

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

The library already uses some nonstandard options, so the description is a bit misleading. We should call them "not handled explicitly" instead.

self.custom_options = {}
Copy link
Copy Markdown
Member

@acolomb acolomb May 4, 2026

Choose a reason for hiding this comment

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

For new code, we require proper typing hints now, to avoid an ever-growing error list from the mypy checker. (Same thing below.)

Suggested change
self.custom_options = {}
self.custom_options: dict[str, str] = {}

Copy link
Copy Markdown
Contributor Author

@FedericoSpada FedericoSpada May 4, 2026

Choose a reason for hiding this comment

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

I always copy the format of existing code.
Maybe it is worth the effort to type-hint the whole library.


def __repr__(self) -> str:
return f"<{type(self).__qualname__} {self.name!r} at {pretty_index(self.index)}>"
Expand Down Expand Up @@ -268,6 +270,8 @@ def __init__(self, name: str, index: int):
self.storage_location = None
self.subindices = {}
self.names = {}
#: Key-Value pairs not defined by the standard
self.custom_options = {}

def __repr__(self) -> str:
return f"<{type(self).__qualname__} {self.name!r} at {pretty_index(self.index)}>"
Expand Down Expand Up @@ -374,6 +378,8 @@ def __init__(self, name: str, index: int, subindex: int = 0):
self.storage_location = None
#: Can this variable be mapped to a PDO
self.pdo_mappable = False
#: Key-Value pairs not defined by the standard
self.custom_options = {}

def __repr__(self) -> str:
subindex = self.subindex if isinstance(self.parent, (ODRecord, ODArray)) else None
Expand Down
23 changes: 23 additions & 0 deletions canopen/objectdictionary/eds.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,14 +140,17 @@ def import_eds(source, node_id):
arr.add_member(last_subindex)
arr.add_member(build_variable(eds, section, node_id, index, 1))
arr.storage_location = storage_location
arr.custom_options = _get_custom_options(eds, section)
od.add_object(arr)
elif object_type == ARR:
arr = objectdictionary.ODArray(name, index)
arr.storage_location = storage_location
arr.custom_options = _get_custom_options(eds, section)
od.add_object(arr)
elif object_type == RECORD:
record = objectdictionary.ODRecord(name, index)
record.storage_location = storage_location
record.custom_options = _get_custom_options(eds, section)
od.add_object(record)

continue
Expand Down Expand Up @@ -253,6 +256,19 @@ def _revert_variable(var_type, value):
else:
return f"0x{value:02X}"

_STANDARD_OPTIONS = ["objecttype" , "parametername" , "datatype" , "accesstype" ,
"pdomapping" , "lowlimit" , "highlimit" , "defaultvalue" ,
"parametervalue" , "factor" , "description" , "unit" ,
"storagelocation" , "compactsubobj" , "nrofentries" , "subnumber" ,
"objflags" , "denotation" ]

Comment on lines +259 to +264
Copy link
Copy Markdown
Member

@acolomb acolomb May 4, 2026

Choose a reason for hiding this comment

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

This is definitely funky formatting. We mostly follow "black" style, though not strictly enforded. A simple list, one item per row, would be fine here, however. Makes it easier to insert grouping comments.

As for the name, I think KNOWN_OBJECT_OPTIONS would be a better fit, as some of these are not strictly standard. And we don't strictly need to mark them "internal", thus moving toward the top of the file and removing the underscore seems appropriate.

This would be my approach, sorted as in CiA 306:

Suggested change
_STANDARD_OPTIONS = ["objecttype" , "parametername" , "datatype" , "accesstype" ,
"pdomapping" , "lowlimit" , "highlimit" , "defaultvalue" ,
"parametervalue" , "factor" , "description" , "unit" ,
"storagelocation" , "compactsubobj" , "nrofentries" , "subnumber" ,
"objflags" , "denotation" ]
KNOWN_OBJECT_OPTIONS = [
# EDS generic:
"parametername",
"objecttype",
"datatype",
"accesstype",
"defaultvalue",
"pdomapping",
"subnumber",
"lowlimit",
"highlimit",
"objflags",
"compactsubobj",
"nrofentries",
# DCF only:
"parametervalue",
"uploadfile",
"downloadfile",
"denotation",
# nonstandard extensions:
"factor",
"unit",
"description",
"storagelocation",
]

def _get_custom_options(eds, section):
Copy link
Copy Markdown
Member

@acolomb acolomb May 4, 2026

Choose a reason for hiding this comment

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

Type hints, please, for new code.

custom_options = {}
for option, value in eds.items(section):
if not (option.lower() in _STANDARD_OPTIONS or option.isdigit()) :
Copy link
Copy Markdown
Member

@acolomb acolomb May 4, 2026

Choose a reason for hiding this comment

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

Please pay attention to code formatting. The space before : violates PEP-8 for example. Having some linters running in the background really helps to catch these problems early, so I recommend it (flake8 and black --diff for example).

Same thing with the number of blank lines between top-level definitions, btw.

custom_options[option] = value
Copy link
Copy Markdown
Member

@acolomb acolomb May 4, 2026

Choose a reason for hiding this comment

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

This whole function would be a good candidate for a dict comprehension instead of an explicit loop.

Copy link
Copy Markdown
Contributor Author

@FedericoSpada FedericoSpada May 4, 2026

Choose a reason for hiding this comment

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

I think it would be too complicated/long:

{option: value for option, value in eds.items(section) if not (option.lower() in _STANDARD_OPTIONS or option.isdigit())}

return custom_options


def build_variable(eds, section, node_id, index, subindex=0):
"""Creates a object dictionary entry.
Expand Down Expand Up @@ -332,6 +348,8 @@ def build_variable(eds, section, node_id, index, subindex=0):
var.unit = eds.get(section, "Unit")
except ValueError:
pass

var.custom_options = _get_custom_options(eds, section)
return var


Expand Down Expand Up @@ -406,12 +424,17 @@ def export_variable(var, eds):
if getattr(var, 'unit', '') != '':
eds.set(section, "Unit", var.unit)

for option, value in var.custom_options.items():
eds.set(section, option, value)

def export_record(var, eds):
section = f"{var.index:04X}"
export_common(var, eds, section)
eds.set(section, "SubNumber", f"0x{len(var.subindices):X}")
ot = RECORD if isinstance(var, objectdictionary.ODRecord) else ARR
eds.set(section, "ObjectType", f"0x{ot:X}")
for option, value in var.custom_options.items():
eds.set(section, option, value)
for i in var:
export_variable(var[i], eds)

Expand Down
Loading