Skip to content

Change attribute getters and setters into methods #371

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

Closed
wants to merge 2 commits into from
Closed

Change attribute getters and setters into methods #371

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
23dad2d
Deprecation of attribute getters and setters
sveinse Mar 25, 2023
627537e
Merge branch 'master' into feature-deprecate-attr-ops
sveinse Apr 8, 2023
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
12 changes: 6 additions & 6 deletions README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,11 @@ The :code:`n` is the PDO index (normally 1 to 4). The second form of access is f
# network.connect(bustype='nican', channel='CAN0', bitrate=250000)

# Read a variable using SDO
device_name = node.sdo['Manufacturer device name'].raw
vendor_id = node.sdo[0x1018][1].raw
device_name = node.sdo['Manufacturer device name'].get_raw()
vendor_id = node.sdo[0x1018][1].get_raw()
Comment on lines -118 to +119
Copy link
Member

Choose a reason for hiding this comment

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

Well, in my eyes this really kills the spirit of the Python library, it's just more reading without any more information. The whole point of properties and their accessors is that the (average) API user should not care about where it comes from, they just want the value. In this case, it is stored somewhere else, so must be fetched.

Of course for advanced use cases, blocking I/O functions should be clearly noted. I'm fine with having variants that will work with async etc. We just shouldn't burden the user with more complicated names, or warn them when using a perfectly fine (synchronous, blocking) API.


# Write a variable using SDO
node.sdo['Producer heartbeat time'].raw = 1000
node.sdo['Producer heartbeat time'].set_raw(1000)

# Read PDO configuration from node
node.tpdo.read()
Expand All @@ -139,12 +139,12 @@ The :code:`n` is the PDO index (normally 1 to 4). The second form of access is f
network.sync.start(0.1)

# Change state to operational (NMT start)
node.nmt.state = 'OPERATIONAL'
node.nmt.set_state('OPERATIONAL')

# Read a value from TPDO[1]
node.tpdo[1].wait_for_reception()
speed = node.tpdo[1]['Velocity actual value'].phys
val = node.tpdo['Some group.Some subindex'].raw
speed = node.tpdo[1]['Velocity actual value'].get_phys()
val = node.tpdo['Some group.Some subindex'].get_raw()

# Disconnect from CAN bus
network.sync.stop()
Expand Down
11 changes: 9 additions & 2 deletions canopen/nmt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ def send_command(self, code: int):

@property
def state(self) -> str:
return self.get_state()

def get_state(self) -> str:
Comment on lines +76 to +78
Copy link
Member

Choose a reason for hiding this comment

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

If we want to go this route, this adds another layer of indirection, which has a small runtime penalty. Couldn't the properties be created by specifying the get_*() and set_*() methods directly as fget and fset (if that's what they're called...) parameters? If the decorated method is just a wrapper itself, there is no real advantage in using a decorator anymore.

"""Attribute to get or set node's state as a string.

Can be one of:
Expand All @@ -93,6 +96,10 @@ def state(self) -> str:

@state.setter
def state(self, new_state: str):
logger.warning("Accessing NmtBase.state setter is deprecated, use set_state()")
Copy link
Member

Choose a reason for hiding this comment

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

Please not this :-) It'll cause a truckload of log spam in existing code, without anyone digging in to fix things as long as they keep working somehow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The DRY principle in python guides us not to, well, repeat ourselves. Having an API with multiple set/get methods is in my opinion not ideal and thus I wanted to propose to inform the user that the property based attr access is no longer the preferred way to do it. However, I do acknowledge that this is how this library has been designed. I won't argue if we decide that either get methods or property based access is supported without any warnings.

Copy link
Member

Choose a reason for hiding this comment

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

Breaking existing code is even worse than repeating yourself. But I'd still like to see a solution where we don't need any repetition, or at least reduce it to the absolute minimum.

Haven't thought it through, but I imagine some very basic code like this to "extract" the existing functions:

class NmtBase:
    @state.setter
    def state(self, new_state: str):
        # ...    
    set_state = state.fset

My experience with asyncio is nearly nil though, so no clue what it would take to make this async-compatible then.

self.set_state(new_state)

def set_state(self, new_state: str):
if new_state in NMT_COMMANDS:
code = NMT_COMMANDS[new_state]
else:
Expand Down Expand Up @@ -148,7 +155,7 @@ def wait_for_heartbeat(self, timeout: float = 10):
self.state_update.wait(timeout)
if self._state_received is None:
raise NmtError("No boot-up or heartbeat received")
return self.state
return self.get_state()

def wait_for_bootup(self, timeout: float = 10) -> None:
"""Wait until a boot-up message is received."""
Expand Down Expand Up @@ -218,7 +225,7 @@ def send_command(self, code: int) -> None:
# The heartbeat service should start on the transition
# between INITIALIZING and PRE-OPERATIONAL state
if old_state == 0 and self._state == 127:
heartbeat_time_ms = self._local_node.sdo[0x1017].raw
heartbeat_time_ms = self._local_node.sdo[0x1017].get_raw()
self.start_heartbeat(heartbeat_time_ms)
else:
self.update_heartbeat()
Expand Down
4 changes: 2 additions & 2 deletions canopen/node/remote.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,9 @@ def __load_configuration_helper(self, index, subindex, name, value):
subindex=subindex,
name=name,
value=value)))
self.sdo[index][subindex].raw = value
self.sdo[index][subindex].set_raw(value)
else:
self.sdo[index].raw = value
self.sdo[index].set_raw(value)
logger.info(str('SDO [{index:#06x}]: {name}: {value:#06x}'.format(
index=index,
name=name,
Expand Down
44 changes: 22 additions & 22 deletions canopen/pdo/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,41 +315,41 @@ def add_callback(self, callback: Callable[["Map"], None]) -> None:

def read(self) -> None:
"""Read PDO configuration for this map using SDO."""
cob_id = self.com_record[1].raw
cob_id = self.com_record[1].get_raw()
self.cob_id = cob_id & 0x1FFFFFFF
logger.info("COB-ID is 0x%X", self.cob_id)
self.enabled = cob_id & PDO_NOT_VALID == 0
logger.info("PDO is %s", "enabled" if self.enabled else "disabled")
self.rtr_allowed = cob_id & RTR_NOT_ALLOWED == 0
logger.info("RTR is %s", "allowed" if self.rtr_allowed else "not allowed")
self.trans_type = self.com_record[2].raw
self.trans_type = self.com_record[2].get_raw()
logger.info("Transmission type is %d", self.trans_type)
if self.trans_type >= 254:
try:
self.inhibit_time = self.com_record[3].raw
self.inhibit_time = self.com_record[3].get_raw()
except (KeyError, SdoAbortedError) as e:
logger.info("Could not read inhibit time (%s)", e)
else:
logger.info("Inhibit time is set to %d ms", self.inhibit_time)

try:
self.event_timer = self.com_record[5].raw
self.event_timer = self.com_record[5].get_raw()
except (KeyError, SdoAbortedError) as e:
logger.info("Could not read event timer (%s)", e)
else:
logger.info("Event timer is set to %d ms", self.event_timer)

try:
self.sync_start_value = self.com_record[6].raw
self.sync_start_value = self.com_record[6].get_raw()
except (KeyError, SdoAbortedError) as e:
logger.info("Could not read SYNC start value (%s)", e)
else:
logger.info("SYNC start value is set to %d ms", self.sync_start_value)

self.clear()
nof_entries = self.map_array[0].raw
nof_entries = self.map_array[0].get_raw()
for subindex in range(1, nof_entries + 1):
value = self.map_array[subindex].raw
value = self.map_array[subindex].get_raw()
index = value >> 16
subindex = (value >> 8) & 0xFF
size = value & 0xFF
Expand All @@ -366,44 +366,44 @@ def save(self) -> None:
"""Save PDO configuration for this map using SDO."""
logger.info("Setting COB-ID 0x%X and temporarily disabling PDO",
self.cob_id)
self.com_record[1].raw = self.cob_id | PDO_NOT_VALID | (RTR_NOT_ALLOWED if not self.rtr_allowed else 0x0)
self.com_record[1].set_raw(self.cob_id | PDO_NOT_VALID | (RTR_NOT_ALLOWED if not self.rtr_allowed else 0x0))
if self.trans_type is not None:
logger.info("Setting transmission type to %d", self.trans_type)
self.com_record[2].raw = self.trans_type
self.com_record[2].set_raw(self.trans_type)
if self.inhibit_time is not None:
logger.info("Setting inhibit time to %d us", (self.inhibit_time * 100))
self.com_record[3].raw = self.inhibit_time
self.com_record[3].set_raw(self.inhibit_time)
if self.event_timer is not None:
logger.info("Setting event timer to %d ms", self.event_timer)
self.com_record[5].raw = self.event_timer
self.com_record[5].set_raw(self.event_timer)
if self.sync_start_value is not None:
logger.info("Setting SYNC start value to %d", self.sync_start_value)
self.com_record[6].raw = self.sync_start_value
self.com_record[6].set_raw(self.sync_start_value)

if self.map is not None:
try:
self.map_array[0].raw = 0
self.map_array[0].set_raw(0)
except SdoAbortedError:
# WORKAROUND for broken implementations: If the array has a
# fixed number of entries (count not writable), generate dummy
# mappings for an invalid object 0x0000:00 to overwrite any
# excess entries with all-zeros.
self._fill_map(self.map_array[0].raw)
self._fill_map(self.map_array[0].get_raw())
subindex = 1
for var in self.map:
logger.info("Writing %s (0x%X:%d, %d bits) to PDO map",
var.name, var.index, var.subindex, var.length)
if hasattr(self.pdo_node.node, "curtis_hack") and self.pdo_node.node.curtis_hack: # Curtis HACK: mixed up field order
self.map_array[subindex].raw = (var.index |
var.subindex << 16 |
var.length << 24)
self.map_array[subindex].set_raw(var.index |
var.subindex << 16 |
var.length << 24)
else:
self.map_array[subindex].raw = (var.index << 16 |
var.subindex << 8 |
var.length)
self.map_array[subindex].set_raw(var.index << 16 |
var.subindex << 8 |
var.length)
subindex += 1
try:
self.map_array[0].raw = len(self.map)
self.map_array[0].set_raw(len(self.map))
except SdoAbortedError as e:
# WORKAROUND for broken implementations: If the array
# number-of-entries parameter is not writable, we have already
Expand All @@ -415,7 +415,7 @@ def save(self) -> None:
self._update_data_size()

if self.enabled:
self.com_record[1].raw = self.cob_id | (RTR_NOT_ALLOWED if not self.rtr_allowed else 0x0)
self.com_record[1].set_raw(self.cob_id | (RTR_NOT_ALLOWED if not self.rtr_allowed else 0x0))
self.subscribe()

def subscribe(self) -> None:
Expand Down
Loading