eds: Add first-class support for CiA 306 ObjFlags and Denotation#654
eds: Add first-class support for CiA 306 ObjFlags and Denotation#654bizfsc wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
bizfsc
force-pushed
the
feat/cia306-objflags-denotation-v2
branch
from
5dff918 to
90dbe07
Compare
bizfsc
force-pushed
the
feat/cia306-objflags-denotation-v2
branch
from
90dbe07 to
bccbd5c
Compare
|
Is this YAGNI? |
|
Frankly, I never heard of it. But now that I have, I had an immediate idea where it could be used in my code, replacing a previous workaround. Some controllers have extra objects for "user variables" which can be utilized in a user-supplied small PLC program on the device. The function of this custom program defines what the variable is really used for (denotation), yet the device manufacturer can only assign a generic name of "user variable 1" in the generic device EDS. That sounds like a perfect application of this less widespread option. The ObjFlags option also seems to solve a problem I've previously worked around by hard-coding objects to skip. I'll have to take a deeper look to really judge if it works like I imagine. And get better acquainted with their definition in CiA 306. |
acolomb
left a comment
acolomb
left a comment
There was a problem hiding this comment.
Again just a partial review, forgot to send this a few days ago...
- ODVariable, ODRecord and ODArray gain obj_flags: int = 0 - ODVariable, ODRecord and ODArray gain denotation: str = "" (Denotation also added to container types per reviewer request) - New _get_obj_flags() helper reads and validates ObjFlags integer - ObjFlags parsed for VAR/DOMAIN (build_variable), ARRAY and RECORD - Denotation parsed for all object types - ObjFlags exported when non-zero; Denotation exported in DCF mode only - __repr__ shows flags=0xN when obj_flags is non-zero (all three types) Closes review comment by acolomb: denotation now applied to all object types; obj_flags visible in string representation.
bizfsc
force-pushed
the
feat/cia306-objflags-denotation-v2
branch
from
3aa08e4 to
4430483
Compare
|
@bizfsc It would help review a bit if you could push additional fix commits instead of force-pushing. Easier to see the difference after some time has passed and the PR got swapped out of brain. Thanks! |
|
Well with the rebase from master and the other PRs already merged, this does not work :( I could use merge instead of rebase, but then there will merge changes in the PR commit history which is often not wanted. |
|
The PR commit history is not important after (squash-)merging. But during review, it helps to see what changed, even if that is just "merged from master branch". If there are no conflicts, the rebase / merge isn't strictly needed either. I have deliberately disables the requirement to have an up-to-date upstream in the PR for merging. |
|
Ok, I will no longer rebase and force-push in my PRs from now on. Sorry! |
- test_eds.py: move io, contextlib, configparser to top-level imports; add 'from canopen import objectdictionary' to fix namespace errors; remove all local imports from test methods; assert node_id is not None before arithmetic to satisfy type checker - eds.py: move sys, datetime imports to top-level; fix optionxform type ignores; fix fp-unbound in finally; narrow od[index] type before [1] subscript in Name-section handler; add type ignores for dynamic attrs (default_raw, value_raw); convert all eds.set() int args to str; use integer division for bitrate; split bool/int branch in DeviceInfo export to avoid redundant int() wrapping
|
Sorry to keep nagging about your PR structure... But this last commit is really mixing in a load of unrelated changes. There may be good reasons for these changes, but we need to keep PRs focused on the actually intended change. Any roadside cleanup touching other code belongs in its own PR. Actually I have quite a bit of cleanup and simplifications on the table for this module already. Just wanted to wait for #653 and #658 to land before touching that file again and ending up with lots of conflicts. I will now concentrate on that effort first. Some of the added "fixes" here I am actually not happy with, because they are hiding type checker errors instead of fixing them. Others have already been fixed in other PRs, so a merge from master would be the better approach. |
2 participants
ObjFlags(UNSIGNED32 bitfield) andDenotation(DCF-only string) are standard CiA 306 fields that were previously silently dropped. This PR adds first-class support for both.Changes
ODVariable,ODRecordandODArraygain anobj_flags: int = 0attribute.ODVariablegains adenotation: str = ""attribute._get_obj_flags(eds, section)helper reads and validates theObjFlagsinteger value.ObjFlagsis parsed for VAR/DOMAIN (viabuild_variable()), ARRAY and RECORD top-level objects.ObjFlagsis exported whenever non-zero.Denotationis read inbuild_variable()and written only in DCF mode (device_commisioning=True), matching the CiA 306 specification.Tests
test_reading_obj_flags— VAR withObjFlags=0x1reads back correctlytest_reading_obj_flags_default— standard objects default to0test_roundtrip_obj_flags— full EDS export/import cycle preservesobj_flagstest_denotation_roundtrip_dcf—denotationsurvives a DCF round-triptest_denotation_not_exported_in_eds_mode—denotationis absent in plain EDS export