Prevent storing of SDO write data in local store when using callbacks in LocalNode

[sveinse]

It is possible to create a custom SDO reader when using LocalNode. If this function returns anything else than None this value will be used as a response to the remote client.

canopen/canopen/node/local.py

Lines 79 to 83 in 09f6e9e

	# Try callback
	for callback in self._read_callbacks:
	result = callback(index=index, subindex=subindex, od=obj)
	if result is not None:
	return obj.encode_raw(result)

However, this is not the case on SDO write. There is callbacks that can be used to deliver the value to a custom system. There is, however, no ability to prevent it from storing the data in its data store.

canopen/canopen/node/local.py

Lines 118 to 124 in 09f6e9e

	# Try callbacks
	for callback in self._write_callbacks:
	callback(index=index, subindex=subindex, od=obj, data=data)
	# Store data
	self.data_store.setdefault(index, {})
	self.data_store[index][subindex] = bytes(data)

I believe a similar solution should be used here. If the write callbacks return anything else than None then the data should not be stored in the data store.

The use-case for this is when implementing a simulator of our hardware devices using LocalNode. We have some special SDOs that require some special responses. The callbacks for both reads and writes provides everything that's needed for that, except the data store is filling up with data that should not be present in the store.

[sveinse]

Either (A) where it bails out on the first non-None reply. This is similar to the reader callback logic.

# Try callbacks 
for callback in self._write_callbacks: 
    result = callback(index=index, subindex=subindex, od=obj, data=data) 
    if result is not None:
        return
 
# Store data 
self.data_store.setdefault(index, {}) 
self.data_store[index][subindex] = bytes(data) 

Or (B) goes through them all and then exits

# This allows running all callbacks
skip_store = any(
    callback(index=index, subindex=subindex, od=obj, data=data)
    for callback in self._write_callbacks)
)

if not skip_store:
    self.data_store.setdefault(index, {}) 
    self.data_store[index][subindex] = bytes(data)

[acolomb]

I'd much prefer option B here. I don't see how it would hurt much to store the data anyway. But if that is becoming a problem for your use case, sure why not make it opt-out.

But if callbacks are registered, they should not be skipped because of some other callback's behavior.

For reads, of course there can be only one return value. We should consider making the logic there consistent, as in invoking all registered callbacks, then using the last returned non-None value. That's currently not the case, judging from your quoted code excerpt.

[sveinse]

That's what I like about returning non-None. This is the opt-out of the default behavior. If the default behavior is what's wanted, but still need callbacks on either read or write? Return None in the callbacks.

[acolomb]

Yes, and it gives you the ability to add logging, GUI update, etc. callbacks that should always happen, but should never influence the actual SDO transaction.