Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 2 additions & 0 deletions canopen/objectdictionary/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,8 @@ def __init__(self, name: str, index: int, subindex: int = 0):
self.data_type: Optional[int] = None
#: Access type, should be "rw", "ro", "wo", or "const"
self.access_type: str = "rw"
#: The variable represents a DOMAIN ObjectType
self.is_domain: bool = False
Comment thread
friederschueler marked this conversation as resolved.
#: Description of variable
self.description: str = ""
#: Dictionary of value descriptions
Expand Down
17 changes: 12 additions & 5 deletions canopen/objectdictionary/eds.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ def import_eds(source, node_id):
storage_location = None

if object_type in (objectcodes.VAR, objectcodes.DOMAIN):
var = build_variable(eds, section, node_id, index)
var = build_variable(eds, section, node_id, index, is_domain=object_type==objectcodes.DOMAIN)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

PEP 8: missing spaces around the == comparison inside the keyword argument.

Suggested change
var = build_variable(eds, section, node_id, index, is_domain=object_type==objectcodes.DOMAIN)
var = build_variable(eds, section, node_id, index, is_domain=object_type == objectcodes.DOMAIN)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've changed it, but for me, it is incredibly misleading: you are giving "priority" to the assignment instead of the operation that is performed before.
A possible solution would be is_domain=(object_type == objectcodes.DOMAIN), but Pylint complains about the unnecessary parentheses.
For me, the proper way to write it would be is_domain = object_type==objectcodes.DOMAIN.
I've never understood why PEP 8 requires no spaces for "=" in function invocations...

od.add_object(var)
elif object_type == objectcodes.ARRAY and eds.has_option(section, "CompactSubObj"):
arr = ODArray(name, index)
Expand Down Expand Up @@ -158,7 +158,11 @@ def import_eds(source, node_id):
subindex = int(match.group(2), 16)
entry = od[index]
if isinstance(entry, (ODRecord, ODArray)):
var = build_variable(eds, section, node_id, index, subindex)
try:
object_type = int(eds.get(section, "ObjectType"), 0)
except NoOptionError:
object_type = objectcodes.VAR
var = build_variable(eds, section, node_id, index, subindex, is_domain=object_type==objectcodes.DOMAIN)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same PEP 8 spacing issue as above.

Suggested change
var = build_variable(eds, section, node_id, index, subindex, is_domain=object_type==objectcodes.DOMAIN)
var = build_variable(eds, section, node_id, index, subindex, is_domain=object_type == objectcodes.DOMAIN)

entry.add_member(var)

# Match [index]Name
Expand Down Expand Up @@ -252,13 +256,14 @@ def _revert_variable(var_type, value):
return f"0x{value:02X}"


def build_variable(eds, section, node_id, index, subindex=0):
def build_variable(eds, section, node_id, index, subindex=0, is_domain=False):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Frankly I don't particularly like this function signature. The is_domain attribute could be set directly on the resulting object instead of adding a new call parameter.

Or even better, pass the whole object_code value and have the logic for the boolean attribute here in this function. That's why we have this function with centralized logic.

"""Creates a object dictionary entry.
:param eds: String stream of the eds file
:param section:
:param node_id: Node ID
:param index: Index of the CANOpen object
:param subindex: Subindex of the CANOpen object (if presente, else 0)
:param subindex: Subindex of the CANOpen object (if present, else 0)
:param is_domain: variable represents a DOMAIN ObjectType (if present, else False)
"""
name = eds.get(section, "ParameterName")
var = ODVariable(name, index, subindex)
Expand All @@ -268,6 +273,7 @@ def build_variable(eds, section, node_id, index, subindex=0):
var.storage_location = None
var.data_type = int(eds.get(section, "DataType"), 0)
var.access_type = eds.get(section, "AccessType").lower()
var.is_domain = is_domain
if var.data_type > 0x1B:
# The object dictionary editor from CANFestival creates an optional object if min max values are used
# This optional object is then placed in the eds under the section [A0] (start point, iterates for more)
Expand Down Expand Up @@ -370,7 +376,8 @@ def export_variable(var, eds):
section = f"{var.index:04X}sub{var.subindex:X}"

export_common(var, eds, section)
eds.set(section, "ObjectType", f"0x{objectcodes.VAR:X}")
object_type = objectcodes.DOMAIN if var.is_domain else objectcodes.VAR
eds.set(section, "ObjectType", f"0x{object_type:X}")
if var.data_type:
eds.set(section, "DataType", f"0x{var.data_type:04X}")
if var.access_type:
Expand Down
27 changes: 27 additions & 0 deletions test/sample.eds
Original file line number Diff line number Diff line change
Expand Up @@ -1017,3 +1017,30 @@ PDOMapping=0x0
Factor=ERROR
Description=
Unit=

[3063]
ParameterName=DOMAIN object
ObjectType=0x2
DataType=0x0007
AccessType=rw
PDOMapping=0

[3064]
ParameterName=Record with DOMAIN sub-object
SubNumber=0x2
ObjectType=0x9

[3064sub0]
ParameterName=Highest subindex
ObjectType=0x7
DataType=0x0005
AccessType=ro
DefaultValue=0x01
PDOMapping=0

[3064sub1]
ParameterName=DOMAIN sub-object
ObjectType=0x2
DataType=0x0007
AccessType=rw
PDOMapping=0
37 changes: 37 additions & 0 deletions test/test_eds.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ def test_variable(self):
self.assertEqual(var.name, 'Producer heartbeat time')
self.assertEqual(var.data_type, canopen.objectdictionary.UNSIGNED16)
self.assertEqual(var.access_type, 'rw')
self.assertEqual(var.is_domain, False)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: for boolean checks the unittest idiom is assertFalse/assertTrue rather than assertEqual(..., False/True). Same pattern applies to the other four analogous assertions added in this PR.

Suggested change
self.assertEqual(var.is_domain, False)
self.assertFalse(var.is_domain)

self.assertEqual(var.default, 0)
self.assertFalse(var.relative)

Expand All @@ -132,6 +133,7 @@ def test_record(self):
self.assertEqual(var.subindex, 1)
self.assertEqual(var.data_type, canopen.objectdictionary.UNSIGNED32)
self.assertEqual(var.access_type, 'ro')
self.assertEqual(var.is_domain, False)

def test_record_with_limits(self):
int8 = self.od[0x3020]
Expand Down Expand Up @@ -166,6 +168,7 @@ def test_array_compact_subobj(self):
self.assertEqual(var.subindex, 5)
self.assertEqual(var.data_type, canopen.objectdictionary.UNSIGNED32)
self.assertEqual(var.access_type, 'ro')
self.assertEqual(var.is_domain, False)

def test_explicit_name_subobj(self):
name = self.od[0x3004].name
Expand Down Expand Up @@ -197,6 +200,7 @@ def test_dummy_variable(self):
self.assertEqual(var.name, 'Dummy0003')
self.assertEqual(var.data_type, canopen.objectdictionary.INTEGER16)
self.assertEqual(var.access_type, 'const')
self.assertEqual(var.is_domain, False)
self.assertEqual(len(var), 16)

def test_dummy_variable_undefined(self):
Expand All @@ -213,6 +217,39 @@ def test_reading_factor(self):
self.assertEqual(var2.factor, 1)
self.assertEqual(var2.unit, '')

def test_read_domain_object(self):
var = self.od[0x3063]
self.assertIsInstance(var, canopen.objectdictionary.ODVariable)
self.assertEqual(var.index, 0x3063)
self.assertEqual(var.subindex, 0)
self.assertEqual(var.name, 'DOMAIN object')
self.assertEqual(var.data_type, canopen.objectdictionary.UNSIGNED32)
self.assertEqual(var.access_type, 'rw')
self.assertEqual(var.is_domain, True)

def test_read_domain_subobject(self):
record = self.od[0x3064]
var = record[1]
self.assertIsInstance(var, canopen.objectdictionary.ODVariable)
self.assertEqual(var.index, 0x3064)
self.assertEqual(var.subindex, 1)
self.assertEqual(var.name, 'DOMAIN sub-object')
self.assertEqual(var.data_type, canopen.objectdictionary.UNSIGNED32)
self.assertEqual(var.access_type, 'rw')
self.assertEqual(var.is_domain, True)

def test_roundtrip_domain_objects(self):
# ObjectType==DOMAIN survive an EDS export/import round-trip
import io
with io.StringIO() as dest:
canopen.export_od(self.od, dest, 'eds')
dest.name = 'mock.eds'
dest.seek(0)
od2 = canopen.import_od(dest)
self.assertEqual(od2['Producer heartbeat time'].is_domain, False)
self.assertEqual(od2['Identity object']['Vendor-ID'].is_domain, False)
self.assertEqual(od2[0x3063].is_domain, True)
self.assertEqual(od2[0x3064][1].is_domain, True)


def test_comments(self):
Expand Down