Custom Options for EDS file by FedericoSpada · Pull Request #615 · canopen-python/canopen

FedericoSpada · 2025-09-21T15:15:46Z

Vector CANeds tool groups all unknown options contained in an EDS File in a dedicated section called "Unknown Entries".
We use that section to save additional information that is not considered by the CANopen specifications (for example: unit, factor, offset, category, etc.), and so I had to modify this library to read and write those options.

If you open a file that contains unknown option names, they will be grouped in the "custom_options" dictionary.
If you desire to save a value with a specific option name, you can update the "custom_options" dictionary accordingly.

P.S.: In the meantime, I've improved some of the bad code.

Allowed to Read/Write options not defined by the EDS Standard.

codecov · 2025-09-22T19:48:12Z

Codecov Report

❌ Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.

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

📢 Thoughts on this report? Let us know!

bizfsc · 2026-04-28T11:25:46Z

Thanks for adding custom options support! I reviewed the changes and have a few observations:

1. Duplicate _get_custom_options() call for VAR types

For VAR / DOMAIN objects, _get_custom_options() is called twice:

Once in import_eds() (line 134): var.custom_options = _get_custom_options(eds, section)
Once at the end of build_variable() (line 352): var.custom_options = _get_custom_options(eds, section)

The second call overwrites the first. One of them should be removed. Since build_variable() is also called for subindexes, keeping it only there seems cleanest – then the calls in import_eds() for ARR/RECORD containers still make sense (they parse the parent section's custom options).

2. _STANDARD_OPTIONS is incomplete

The list is missing at least:

"NrOfEntries" (used in [index]Name sections and SubNumber calculation)
"SubNumber" (used in export_record)
"ObjFlags" and "Denotation" (valid per CiA 306)
Numeric keys like "0", "1", etc. (used as subindex names in CompactSubObj sections)

Any standard option not in this list will incorrectly end up in custom_options. Consider also making the comparison case-insensitive (e.g. using a lowercase set) as a safety measure, even though optionxform = str is set.

3. No tests

It would be great to add a test that:

Imports an EDS with a non-standard option (e.g. Category=MyCategory) on a variable
Verifies it appears in var.custom_options and standard options do not
Exports back to EDS and verifies the custom option is preserved

4. Typo fix should be a separate commit

The manufacturer_idices → manufacturer_indices rename is a good fix but is unrelated to the custom options feature. Splitting it into its own commit would keep the history clean.

FedericoSpada · 2026-05-01T10:21:34Z

Hi,
Regarding the absence of "NrOfEntries" and "SubNumber", I've found the problem myself, but I didn't report it here since no one has been interested in this PR for 6 months.
For "ObjFlags" and "Denotation", since they are not managed in this Library, maybe we can make them appear in "custom_options", so the user can still access their value. What do you think about it?
For the tests, could you please write them yourself after my next commit? I've never used the "unittest" library, and testing in general is not my cup of tea. Thanks in advance.

bizfsc · 2026-05-04T08:57:03Z

@FedericoSpada no problem at all, I created a new PR (#653) because I can not edit your fork.
I split the whole thing into three separate PRs (#642, already merged; #653 open, #654 open).

acolomb · 2026-05-04T09:01:01Z

Sorry for not reacting earlier, there is definitely interest in your work @FedericoSpada. Just not enough spare time on my side to work on the backlog regarding this library. I'm planning on it though in the next few days. Good to see @bizfsc picking it up and pushing it forward.

- Resolved double function invocation - Added forgotten standard options and made the check case-insensitive - Reverted minor changes not related to the main focus of the PR

FedericoSpada · 2026-05-04T19:57:19Z

I've implemented the requested changes.

acolomb

Thank you very much for this contribution. Structurally, I think it's alright and a useful addition. There are some issues regarding code style though, especially the missing type hints. While that could be easily fixed, I think #653 and #654 are already more extensive and a bit cleaner, so let's continue on those instead.

Still added some comments here as guidance, mostly also relevant for the other PRs.

acolomb · 2026-05-04T21:04:20Z

        self.subindices = {}
        self.names = {}
+        #: Key-Value pairs not defined by the standard
+        self.custom_options = {}


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] = {}

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

acolomb · 2026-05-04T21:11:30Z

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


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

]

acolomb · 2026-05-04T21:11:50Z

+                     "storagelocation" , "compactsubobj" , "nrofentries" , "subnumber"    ,
+                     "objflags"        , "denotation"                                     ]
+
+def _get_custom_options(eds, section):


Type hints, please, for new code.

acolomb · 2026-05-04T21:15:45Z

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


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.

acolomb · 2026-05-04T21:26:37Z

        self.storage_location = None
        self.subindices = {}
        self.names = {}
+        #: Key-Value pairs not defined by the standard


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

acolomb · 2026-05-04T21:33:47Z

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


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

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())}

Custom Options for EDS file

6bc9305

Allowed to Read/Write options not defined by the EDS Standard.

bizfsc mentioned this pull request Apr 28, 2026

eds: Fix typo in manufacturer_idices → manufacturer_indices #642

Merged

bizfsc mentioned this pull request May 4, 2026

eds: Support custom_options for unknown EDS fields #653

Open

Changes requested by the maintainers

ee7fdf4

- Resolved double function invocation - Added forgotten standard options and made the check case-insensitive - Reverted minor changes not related to the main focus of the PR

acolomb requested changes May 4, 2026

View reviewed changes

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

-_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",
+]

Conversation

FedericoSpada commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bizfsc commented Apr 28, 2026

Uh oh!

FedericoSpada commented May 1, 2026

Uh oh!

bizfsc commented May 4, 2026

Uh oh!

acolomb commented May 4, 2026

Uh oh!

FedericoSpada commented May 4, 2026

Uh oh!

acolomb 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FedericoSpada commented Sep 21, 2025 •

edited

Loading

codecov Bot commented Sep 22, 2025 •

edited

Loading