Order of operations in NmtMaster.on_heartbeat

[sveinse]

In the implementation of NmtMaster.on_heatbeat, is it deliberate that the callbacks are called before the _state of the class is updated and waiters notified? The emcy.py implementation is opposite., where it process the data, updates the class state, notify waiters and calls the callbacks.

canopen/canopen/nmt.py

Lines 122 to 137 in e840449

	def on_heartbeat(self, can_id, data, timestamp):
	with self.state_update:
	self.timestamp = timestamp
	new_state, = struct.unpack_from("B", data)
	# Mask out toggle bit
	new_state &= 0x7F
	logger.debug("Received heartbeat can-id %d, state is %d", can_id, new_state)
	for callback in self._callbacks:
	callback(new_state)
	if new_state == 0:
	# Boot-up, will go to PRE-OPERATIONAL automatically
	self._state = 127
	else:
	self._state = new_state
	self._state_received = new_state
	self.state_update.notify_all()

When adopting for async, these callbacks will be deferred for later execution. If it is expected that the callbacks run before the class state (like _state) is updated, I will need to adopt specially for that. Otherwise I propose changing the order in NmtMaster.on_heartbeat to mirror the order in EmcyConsumer.on_emcy.

@christiansandberg I see from git blame that this is your code. Your input would be much appreciated. Thanks.

edit:
The lock is held while the callbacks of on_heartbeat() is called, while on_emcy its not. Is that also of importance?

canopen/canopen/emcy.py

Lines 26 to 40 in e840449

	def on_emcy(self, can_id, data, timestamp):
	code, register, data = EMCY_STRUCT.unpack(data)
	entry = EmcyError(code, register, data, timestamp)
	with self.emcy_received:
	if code & 0xFF00 == 0:
	# Error reset
	self.active = []
	else:
	self.active.append(entry)
	self.log.append(entry)
	self.emcy_received.notify_all()
	for callback in self.callbacks:
	callback(entry)

[christiansandberg]

Don't remember that was intentional. If someone uses the current state of the NMT object to compare old and new state, it would be relying on undocumented behavior.

[sveinse]

I'd like to propose the structure and order as outlined below to make it feasible to coexist with async. It does change the behavior so it should not be introduced earlier than 3.0.

We should change the callback prototype to something like callback(new_state, old_state=old_state). Today the callback is synchronous, where call callbacks are run to completion before the next event is processed. With async, however, the callbacks won't run immediately, but rather be scheduled for later execution. Another on_heartbeat() event might arrive before the former callbacks has been run, so the callbacks should be self-contained of the state.

    def on_heartbeat(self, can_id, data, timestamp):

        # --> 1) Move this out of the critical section
        new_state, = struct.unpack_from("B", data)
        # Mask out toggle bit
        new_state &= 0x7F
        logger.debug("Received heartbeat can-id %d, state is %d", can_id, new_state)

        with self.state_update:

            # --> 2) Remove calling the callbacks from here to below
            self.timestamp = timestamp
            old_state = self._state
            if new_state == 0:
                # Boot-up, will go to PRE-OPERATIONAL automatically
                self._state = 127
            else:
                self._state = new_state
            self._state_received = new_state
            self.state_update.notify_all()

        # --> 3) Calling the callbacks without holding the lock
        for callback in self._callbacks:
            call(callback, new_state, old_state=old_state)

[acolomb]

I agree with that structure in general, but I don't see a need to modify the callback signature. Why would an interested party that registers a callback be interested in the previous state? And in the (probably rare) case it is, then it's very simple to cache the previous value in that component individually. I'd rather not break code using a callback which doesn't accept the second (keyword) argument.