Improve emulator event handling

There could be multiple subscription to the same event of the same
object. These subscription need to all be triggered.

The core's event handling on the Python side had a bug where only a
single handler for a particular object/event combination could be
registered.

This commit fixes this by reworking how event subscriptions are
handled. Now, each subscription will return a "token" - an opaque
unique value to identify the subscription. Using tokens allows to
uniquely identify each subscription, regarless for which event and
object they are.

Author: voidtrance

controllers/__init__.py

@@ -501,37 +501,34 @@ def exec_command(self, command_id, object_id, subcommand_id, opts=None):
             return self._virtual_objects[object_id].exec_command(command_id,
                                                                  subcommand_id, opts)
 
-    def event_register(self, object_type, event_type, object_name, handler):
-        if not super().event_register(object_type, event_type, object_name,
-                                      self._event_handler):
-            return False
-        self._event_handlers[(object_type, event_type, object_name)] = handler
-        return True
+    def event_register(self, klass, event_type, object_name, handler):
+        subscription_id = super().event_register(klass, event_type, object_name,
+                                                 self._event_handler)
+        if subscription_id is None:
+            return None
+        self._event_handlers[subscription_id] = handler
+        return subscription_id
 
-    def event_unregister(self, object_type, event_type, object_name):
-        self._event_handlers.pop((object_type, event_type, object_name))
-        return super().event_unregister(object_type, event_type, object_name)
+    def event_unregister(self, subscription_id):
+        self._event_handlers.pop(subscription_id)
+        return super().event_unregister(subscription_id)
 
     def _find_event_data(self, klass, event):
         for e, s in self.object_defs[klass].events.items():
             if e == event:
                 return s
         return None
 
-    def _event_handler(self, klass, object_name, event_type, data):
-        handler = self._event_handlers.get((klass, event_type, object_name),
-                                            None)
-        if handler is None:
-            return
-        if self.object_defs[klass].virtual:
-            handler(klass, event_type, object_name, data)
-            return
-        event_data_def = self._find_event_data(klass, event_type)
-        if event_data_def is None:
-            raise vortex.core.VortexCoreError(f"Unknown event type {event_type}")
-        pointer = ctypes.cast(data, ctypes.POINTER(event_data_def))
-        content = vortex.lib.ctypes_helpers.parse_ctypes_struct(pointer.contents)
-        handler(klass, event_type, object_name, content)
+    def _event_handler(self, token, klass, object_name, event_type, data):
+        if not self.object_defs[klass].virtual:
+            event_data_def = self._find_event_data(klass, event_type)
+            if event_data_def is None:
+                raise vortex.core.VortexCoreError(f"Unknown event type {event_type}")
+            pointer = ctypes.cast(data, ctypes.POINTER(event_data_def))
+            data = vortex.lib.ctypes_helpers.parse_ctypes_struct(pointer.contents)
+
+        handler = self._event_handlers.get(token)
+        handler(klass, event_type, object_name, data)
 
     def reset(self, objects=[]):
         if not objects:

frontends/__init__.py

@@ -43,8 +43,6 @@ def __init__(self, queue_size=0):
         self._queue = CommandQueue(queue_size)
         self._thread = None
         self.is_reset = False
-        self.event_register = lambda a, b, c, d: False
-        self.event_unregister = lambda a, b, c, d: False
         self.emulation_frequency = 0
         try:
             os.mkfifo(self.PIPE_PATH)
@@ -62,7 +60,7 @@ def set_controller(self, controller):
         self._set_controller_data()
 
     # Implement these as class methods so subclasses can
-    # override them. If they they are implemented as class
+    # override them. If they are implemented as class
     # attributes, any subclass lookup of those methods will
     # return the base class method instead of the overriden
     # one.

src/core/common_defs.h

@@ -43,7 +43,7 @@ typedef struct {
     object_list_cb_t object_list;
     complete_cb_t completion_callback;
     event_register_t event_register;
-    event_register_t event_unregister;
+    event_unregister_t event_unregister;
     event_submit_t event_submit;
     cmd_submit_cb_t cmd_submit;
     void *v_cmd_exec;
@@ -171,11 +171,9 @@ static inline core_object_t *core_id_to_object(core_object_id_t id) {
          ->call_data.event_register(                                       \
              (type), (event), (name), ((core_object_t *)(obj)), (handler), \
              ((core_object_t *)(obj))->call_data.cb_data))
-#define CORE_EVENT_UNREGISTER(obj, type, event, name)                      \
-    (((core_object_t *)(obj))                                              \
-         ->call_data.event_unregister(                                     \
-             (type), (event), (name), ((core_object_t *)(obj)), (handler), \
-             ((core_object_t *)(obj))->call_data.cb_data))
+#define CORE_EVENT_UNREGISTER(obj, token) \
+    (((core_object_t *)(obj))             \
+         ->call_data.event_unregister((token), ((core_object_t *)(obj))->call_data.cb_data))
 #define CORE_EVENT_SUBMIT(obj, event, data)                            \
     (((core_object_t *)(obj))                                          \
          ->call_data.event_submit(                                     \

src/core/core.c

@@ -160,15 +160,14 @@ static core_object_t **core_object_list(const core_object_klass_t klass,
 /* Object callbacks */
 static void core_object_command_complete(uint64_t command_id, int64_t result,
                                          void *, void *data);
-static int core_object_event_register(const core_object_klass_t klass,
-                                      const core_object_event_type_t event,
-                                      const char *name, core_object_t *object,
-                                      event_handler_t handler, void *user_data);
-static int core_object_event_unregister(const core_object_klass_t klass,
-                                        const core_object_event_type_t event,
-                                        const char *name, core_object_t *object,
-                                        event_handler_t handler,
-                                        void *user_data);
+static core_event_token_t core_object_event_register(const core_object_klass_t klass,
+                                                     const core_object_event_type_t event,
+                                                     const char *name,
+                                                     core_object_t *object,
+                                                     event_handler_t handler,
+                                                     void *user_data);
+
+static int core_object_event_unregister(const core_event_token_t token, void *user_data);
 static int core_object_event_submit(const core_object_event_type_t event,
                                     const core_object_id_t id, void *event_data,
                                     void *user_data);
@@ -265,8 +264,8 @@ static void core_process_events(void *arg) {
                                            event->data);
             else {
                 PyGILState_STATE state = PyGILState_Ensure();
-                PyObject *args = Py_BuildValue("(isik)", object->klass,
-                                               object->name, event->type,
+                PyObject *args = Py_BuildValue("(kisik)", (core_event_token_t)subscription,
+                                               object->klass, object->name, event->type,
                                                event->data);
                 if (!args) {
                     PyErr_Print();
@@ -1149,10 +1148,12 @@ static PyObject *vortex_core_get_status(PyObject *self, PyObject *args) {
     return NULL;
 }
 
-static int core_object_event_register(const core_object_klass_t klass,
-                                      const core_object_event_type_t event,
-                                      const char *name, core_object_t *object,
-                                      event_handler_t handler, void *data) {
+static unsigned long core_object_event_register(const core_object_klass_t klass,
+                                                const core_object_event_type_t event,
+                                                const char *name,
+                                                core_object_t *object,
+                                                event_handler_t handler,
+                                                void *data) {
     core_t *core = (core_t *)data;
     event_subscription_t *subscription;
 
@@ -1180,35 +1181,32 @@ static int core_object_event_register(const core_object_klass_t klass,
     pthread_mutex_lock(&core->events.handlers[event].lock);
     STAILQ_INSERT_TAIL(&core->events.handlers[event].list, subscription, entry);
     pthread_mutex_unlock(&core->events.handlers[event].lock);
-    return 0;
+    return (unsigned long)subscription;
 }
 
-static int core_object_event_unregister(const core_object_klass_t klass,
-                                        const core_object_event_type_t event,
-                                        const char *name, core_object_t *object,
-                                        event_handler_t handler, void *data) {
+static int core_object_event_unregister(const core_event_token_t token, void *data) {
     core_t *core = (core_t *)data;
     event_subscription_t *subscription;
     event_subscription_t *next;
-    core_object_t *obj = core_object_find(klass, name, core);
+    core_object_event_type_t event;
 
-    pthread_mutex_lock(&core->events.handlers[event].lock);
-    subscription = STAILQ_FIRST(&core->events.handlers[event].list);
-    pthread_mutex_unlock(&core->events.handlers[event].lock);
-    while (subscription != NULL) {
-        next = STAILQ_NEXT(subscription, entry);
-        if (subscription->klass == klass &&
-            (subscription->object_id == CORE_OBJECT_ID_INVALID ||
-             (object && subscription->object_id == core_object_to_id(obj)))) {
-            pthread_mutex_lock(&core->events.handlers[event].lock);
-            STAILQ_REMOVE(&core->events.handlers[event].list, subscription,
-                          event_subscription, entry);
-            pthread_mutex_unlock(&core->events.handlers[event].lock);
-            free(subscription);
-            return 0;
-        }
+    for (event = OBJECT_EVENT_STEPPER_MOVE_COMPLETE; event <= OBJECT_EVENT_MAX; event++) {
+        pthread_mutex_lock(&core->events.handlers[event].lock);
+        subscription = STAILQ_FIRST(&core->events.handlers[event].list);
+        pthread_mutex_unlock(&core->events.handlers[event].lock);
+        while (subscription != NULL) {
+            next = STAILQ_NEXT(subscription, entry);
+            if ((core_event_token_t)subscription == token) {
+                pthread_mutex_lock(&core->events.handlers[event].lock);
+                STAILQ_REMOVE(&core->events.handlers[event].list, subscription, event_subscription,
+                              entry);
+                pthread_mutex_unlock(&core->events.handlers[event].lock);
+                free(subscription);
+                return 0;
+            }
 
-        subscription = next;
+            subscription = next;
+        }
     }
 
     return -1;
@@ -1272,13 +1270,19 @@ static uint64_t core_object_command_submit(core_object_t *source,
     return (uint64_t)cmd;
 }
 
+#define EVENT_TOKEN(klass, type, name, sub) \
+    (core_event_token_t)(((unsigned long)type << 60) | ((unsigned long)sub & ((1UL << 60) - 1)))
+
+#define TYPE_FROM_TOKEN(token) (core_object_event_type_t)((token >> 60) & 0xf)
+
 static PyObject *vortex_core_python_event_register(PyObject *self,
                                                    PyObject *args) {
     core_t *core = (core_t *)self;
     event_subscription_t *subscription;
     core_object_klass_t klass;
     core_object_event_type_t type;
     PyObject *callback;
+    PyObject *token;
     char *name = NULL;
 
     if (!PyArg_ParseTuple(args, "iisO", &klass, &type, &name, &callback))
@@ -1287,15 +1291,18 @@ static PyObject *vortex_core_python_event_register(PyObject *self,
     Py_INCREF(callback);
 
     subscription = calloc(1, sizeof(*subscription));
-    if (!subscription)
-        Py_RETURN_FALSE;
+    if (!subscription) {
+        Py_DECREF(callback);
+        Py_RETURN_NONE;
+    }
 
     if (name) {
         core_object_t *object = core_object_find(klass, name, core);
 
         if (!object) {
             free(subscription);
-            Py_RETURN_FALSE;
+            Py_DECREF(callback);
+            Py_RETURN_NONE;
         }
 
         subscription->object_id = core_object_to_id(object);
@@ -1309,49 +1316,40 @@ static PyObject *vortex_core_python_event_register(PyObject *self,
     pthread_mutex_lock(&core->events.handlers[type].lock);
     STAILQ_INSERT_TAIL(&core->events.handlers[type].list, subscription, entry);
     pthread_mutex_unlock(&core->events.handlers[type].lock);
-    Py_RETURN_TRUE;
+    token = Py_BuildValue("k", EVENT_TOKEN(klass, type, name, subscription));
+    return token;
 }
 
 static PyObject *vortex_core_python_event_unregister(PyObject *self,
                                                      PyObject *args) {
     core_t *core = (core_t *)self;
-    event_subscription_t *subscription;
-    event_subscription_t *next;
-    core_object_klass_t klass;
+    core_event_token_t subscription;
     core_object_event_type_t type;
-    core_object_id_t object_id = CORE_OBJECT_ID_INVALID;
-    char *name;
+    event_subscription_t *iter;
+    event_subscription_t *next;
 
-    if (PyArg_ParseTuple(args, "iis", &klass, &type, &name))
+    if (!PyArg_ParseTuple(args, "k", &subscription))
         return NULL;
 
-    if (name) {
-        core_object_t *object = core_object_find(klass, name, core);
-
-        if (object)
-            object_id = core_object_to_id(object);
-    }
+    type = TYPE_FROM_TOKEN(subscription);
 
     pthread_mutex_lock(&core->events.handlers[type].lock);
-    subscription = STAILQ_FIRST(&core->events.handlers[type].list);
+    iter = STAILQ_FIRST(&core->events.handlers[type].list);
     pthread_mutex_unlock(&core->events.handlers[type].lock);
 
-    while (subscription != NULL) {
-        next = STAILQ_NEXT(subscription, entry);
-        if (subscription->klass == klass &&
-            (subscription->object_id == CORE_OBJECT_ID_INVALID ||
-             subscription->object_id == object_id)) {
+    while (iter != NULL) {
+        next = STAILQ_NEXT(iter, entry);
+        if (subscription == (core_event_token_t)iter) {
             pthread_mutex_lock(&core->events.handlers[type].lock);
-            STAILQ_REMOVE(&core->events.handlers[type].list, subscription,
-                          event_subscription, entry);
+            STAILQ_REMOVE(&core->events.handlers[type].list, iter, event_subscription, entry);
             pthread_mutex_unlock(&core->events.handlers[type].lock);
-            if (subscription->is_python)
-                Py_DECREF(subscription->python.handler);
-            free(subscription);
+            if (iter->is_python)
+                Py_DECREF(iter->python.handler);
+            free(iter);
             Py_RETURN_TRUE;
         }
 
-        subscription = next;
+        iter = next;
     }
 
     Py_RETURN_FALSE;

src/core/events.h

@@ -49,6 +49,8 @@ typedef struct {
     double position[AXIS_TYPE_MAX];
 } toolhead_origin_event_data_t;
 
+typedef unsigned long core_event_token_t;
+
 /*
  * Object event handler type.
  * Object event handlers are called to handle events from
@@ -63,17 +65,28 @@ typedef void (*event_handler_t)(core_object_t *, const char *,
                                 const core_object_event_type_t, void *);
 
 /*
- * Signature of call to (un)register for an object event.
+ * Signature of call to register for an object event.
  * The arguments are:
  *    - the event type to register for,
  *    - name of the object issuing the event,
  *    - object registering for the event,
  *    - the object's event handler function,
  *    - the data that needs to be passed.
  */
-typedef int (*event_register_t)(const core_object_klass_t,
-                                const core_object_event_type_t, const char *,
-                                core_object_t *, event_handler_t, void *);
+typedef core_event_token_t (*event_register_t)(const core_object_klass_t,
+                                               const core_object_event_type_t,
+                                               const char *,
+                                               core_object_t *,
+                                               event_handler_t,
+                                               void *);
+
+/*
+ * Signature of call to unregister for an object event.
+ * The arguments are:
+ *    = the event subscription token.
+ *    - the data that needs to be passed.
+*/
+typedef int (*event_unregister_t)(const core_event_token_t, void *);
 
 /*
  * Signature of call to issue an event.

src/core/objects/axis.c

@@ -67,6 +67,7 @@ typedef struct {
     float length;
     double start_position;
     double position;
+    core_event_token_t endstop_event_token;
     stepper_status_t stepper_status;
 } axis_t;
 
@@ -197,9 +198,9 @@ static int axis_init(core_object_t *object) {
             axis->endstop_is_max = false;
     }
 
-    CORE_EVENT_REGISTER(axis, OBJECT_KLASS_ENDSTOP,
-                        OBJECT_EVENT_ENDSTOP_TRIGGER, axis->endstop_name,
-                        axis_event_handler);
+    axis->endstop_event_token = CORE_EVENT_REGISTER(axis, OBJECT_KLASS_ENDSTOP,
+                                                    OBJECT_EVENT_ENDSTOP_TRIGGER,
+                                                    axis->endstop_name, axis_event_handler);
     axis_reset(object);
     return 0;
 }
@@ -347,6 +348,7 @@ static void axis_destroy(core_object_t *object) {
     axis_t *axis = (axis_t *)object;
     size_t i;
 
+    CORE_EVENT_UNREGISTER(axis, axis->endstop_event_token);
     core_object_destroy(object);
     object_cache_destroy(axis_event_cache);
     free((char *)axis->endstop_name);