image-device: Redefine internal states and valid transitions

This adds a number of new internal states to better capture what is
going on. Also added are checks that all transitions we make are in the
set of expected and valid transitions.

Only three drivers use the state_change notification. These drivers are
updated accordingly.

libfprint/drivers/elan.c

@@ -983,11 +983,14 @@ elan_change_state (FpImageDevice *idev)
       elan_calibrate (dev);
       break;
 
+    case FPI_IMAGE_DEVICE_STATE_IDLE:
+    case FPI_IMAGE_DEVICE_STATE_ACTIVATING:
+    case FPI_IMAGE_DEVICE_STATE_INACTIVE:
     case FPI_IMAGE_DEVICE_STATE_CAPTURE:
       /* not used */
       break;
 
-    case FPI_IMAGE_DEVICE_STATE_INACTIVE:
+    case FPI_IMAGE_DEVICE_STATE_DEACTIVATING:
     case FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_OFF:
       elan_stop_capture (dev);
       break;
@@ -1012,6 +1015,11 @@ dev_change_state (FpImageDevice *dev, FpiImageDeviceState state)
 
   /* Inactive and await finger off are equivalent for the elan driver. */
   if (state == FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_OFF)
+    state = FPI_IMAGE_DEVICE_STATE_DEACTIVATING;
+
+  /* The internal state may already be inactive, ignore deactivation then. */
+  if (self->dev_state_next == FPI_IMAGE_DEVICE_STATE_INACTIVE &&
+      state == FPI_IMAGE_DEVICE_STATE_DEACTIVATING)
     state = FPI_IMAGE_DEVICE_STATE_INACTIVE;
 
   if (self->dev_state_next == state)
@@ -1022,7 +1030,7 @@ dev_change_state (FpImageDevice *dev, FpiImageDeviceState state)
 
   switch (state)
     {
-    case FPI_IMAGE_DEVICE_STATE_INACTIVE:
+    case FPI_IMAGE_DEVICE_STATE_DEACTIVATING:
     case FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_ON:
     case FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_OFF: {
         char *name;
@@ -1041,14 +1049,14 @@ dev_change_state (FpImageDevice *dev, FpiImageDeviceState state)
         break;
       }
 
+    case FPI_IMAGE_DEVICE_STATE_IDLE:
+    case FPI_IMAGE_DEVICE_STATE_ACTIVATING:
+    case FPI_IMAGE_DEVICE_STATE_INACTIVE:
     case FPI_IMAGE_DEVICE_STATE_CAPTURE:
       /* TODO MAYBE: split capture ssm into smaller ssms and use this state */
       self->dev_state = state;
       self->dev_state_next = state;
       break;
-
-    default:
-      g_assert_not_reached ();
     }
 }
 
@@ -1070,7 +1078,7 @@ dev_deactivate (FpImageDevice *dev)
        * need to signal back deactivation) and then ensure we will change
        * to the inactive state eventually. */
       self->deactivating = TRUE;
-      dev_change_state (dev, FPI_IMAGE_DEVICE_STATE_INACTIVE);
+      dev_change_state (dev, FPI_IMAGE_DEVICE_STATE_DEACTIVATING);
     }
 }
 

libfprint/drivers/uru4000.c

@@ -412,18 +412,6 @@ dev_change_state (FpImageDevice *dev, FpiImageDeviceState state)
 {
   FpiDeviceUru4000 *self = FPI_DEVICE_URU4000 (dev);
 
-  switch (state)
-    {
-    case FPI_IMAGE_DEVICE_STATE_INACTIVE:
-    case FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_ON:
-    case FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_OFF:
-    case FPI_IMAGE_DEVICE_STATE_CAPTURE:
-      break;
-
-    default:
-      g_assert_not_reached ();
-    }
-
   self->activate_state = state;
   if (self->img_transfer != NULL)
     return;
@@ -1185,7 +1173,10 @@ deactivate_write_reg_cb (FpiUsbTransfer *transfer, FpDevice *dev,
 static void
 dev_deactivate (FpImageDevice *dev)
 {
-  dev_change_state (dev, FPI_IMAGE_DEVICE_STATE_INACTIVE);
+  /* This is started/handled by execute_state_change in order to delay the
+   * action until after the image transfer has completed.
+   * We just need to override the function so that the complete handler is
+   * not called automatically. */
 }
 
 static void
@@ -1196,7 +1187,7 @@ execute_state_change (FpImageDevice *dev)
 
   switch (self->activate_state)
     {
-    case FPI_IMAGE_DEVICE_STATE_INACTIVE:
+    case FPI_IMAGE_DEVICE_STATE_DEACTIVATING:
       fp_dbg ("deactivating");
       self->irq_cb = NULL;
       self->irq_cb_data = NULL;
@@ -1251,6 +1242,12 @@ execute_state_change (FpImageDevice *dev)
       write_reg (dev, REG_MODE, MODE_AWAIT_FINGER_OFF,
                  change_state_write_reg_cb, NULL);
       break;
+
+    /* Ignored states */
+    case FPI_IMAGE_DEVICE_STATE_IDLE:
+    case FPI_IMAGE_DEVICE_STATE_ACTIVATING:
+    case FPI_IMAGE_DEVICE_STATE_INACTIVE:
+      break;
     }
 }
 

libfprint/drivers/virtual-image.c

@@ -195,7 +195,7 @@ recv_image (FpDeviceVirtualImage *self, GInputStream *stream)
   g_debug ("Starting image receive (if active), state is: %i", state);
 
   /* Only register if the state is active. */
-  if (state != FPI_IMAGE_DEVICE_STATE_INACTIVE)
+  if (state >= FPI_IMAGE_DEVICE_STATE_IDLE)
     {
       g_input_stream_read_all_async (stream,
                                      self->recv_img_hdr,
@@ -338,10 +338,10 @@ dev_activate (FpImageDevice *dev)
 {
   FpDeviceVirtualImage *self = FPI_DEVICE_VIRTUAL_IMAGE (dev);
 
+  fpi_image_device_activate_complete (dev, NULL);
+
   if (self->connection)
     recv_image (self, g_io_stream_get_input_stream (G_IO_STREAM (self->connection)));
-
-  fpi_image_device_activate_complete (dev, NULL);
 }
 
 static void

libfprint/fpi-image-device.c

@@ -41,6 +41,9 @@ fp_image_device_get_instance_private (FpImageDevice *self)
                             g_type_class_get_instance_private_offset (img_class));
 }
 
+static void fp_image_device_change_state (FpImageDevice      *self,
+                                          FpiImageDeviceState state);
+
 /* Private shared functions */
 
 void
@@ -52,6 +55,7 @@ fpi_image_device_activate (FpImageDevice *self)
   g_assert (!priv->active);
 
   fp_dbg ("Activating image device");
+  fp_image_device_change_state (self, FPI_IMAGE_DEVICE_STATE_ACTIVATING);
   cls->activate (self);
 }
 
@@ -62,40 +66,77 @@ fpi_image_device_deactivate (FpImageDevice *self, gboolean cancelling)
   FpImageDevicePrivate *priv = fp_image_device_get_instance_private (self);
   FpImageDeviceClass *cls = FP_IMAGE_DEVICE_GET_CLASS (device);
 
-  if (!priv->active || priv->state == FPI_IMAGE_DEVICE_STATE_INACTIVE)
+  if (!priv->active || priv->state == FPI_IMAGE_DEVICE_STATE_DEACTIVATING)
     {
       /* XXX: We currently deactivate both from minutiae scan result
        *      and finger off report. */
       fp_dbg ("Already deactivated, ignoring request.");
       return;
     }
-  if (!cancelling && priv->state == FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_ON)
+  if (!cancelling && priv->state != FPI_IMAGE_DEVICE_STATE_IDLE)
-    g_warning ("Deactivating image device while waiting for finger, this should not happen.");
+    g_warning ("Deactivating image device while it is not idle, this should not happen.");
-
-  priv->state = FPI_IMAGE_DEVICE_STATE_INACTIVE;
-  g_object_notify (G_OBJECT (self), "fpi-image-device-state");
 
   fp_dbg ("Deactivating image device");
+  fp_image_device_change_state (self, FPI_IMAGE_DEVICE_STATE_DEACTIVATING);
   cls->deactivate (self);
 }
 
 /* Static helper functions */
 
+/* This should not be called directly to activate/deactivate the device! */
 static void
 fp_image_device_change_state (FpImageDevice *self, FpiImageDeviceState state)
 {
   FpImageDevicePrivate *priv = fp_image_device_get_instance_private (self);
   g_autofree char *prev_state_str = NULL;
   g_autofree char *state_str = NULL;
+  gboolean transition_is_valid = FALSE;
+  gint i;
 
-  /* Cannot change to inactive using this function. */
+  struct
-  g_assert (state != FPI_IMAGE_DEVICE_STATE_INACTIVE);
+  {
+    FpiImageDeviceState from;
+    FpiImageDeviceState to;
+  } valid_transitions[] = {
+    { FPI_IMAGE_DEVICE_STATE_INACTIVE, FPI_IMAGE_DEVICE_STATE_ACTIVATING },
+
+    { FPI_IMAGE_DEVICE_STATE_ACTIVATING, FPI_IMAGE_DEVICE_STATE_IDLE },
+    { FPI_IMAGE_DEVICE_STATE_ACTIVATING, FPI_IMAGE_DEVICE_STATE_INACTIVE },
+
+    { FPI_IMAGE_DEVICE_STATE_IDLE, FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_ON },
+    { FPI_IMAGE_DEVICE_STATE_IDLE, FPI_IMAGE_DEVICE_STATE_CAPTURE }, /* raw mode -- currently not supported */
+    { FPI_IMAGE_DEVICE_STATE_IDLE, FPI_IMAGE_DEVICE_STATE_DEACTIVATING },
+
+    { FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_ON, FPI_IMAGE_DEVICE_STATE_CAPTURE },
+    { FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_ON, FPI_IMAGE_DEVICE_STATE_DEACTIVATING }, /* cancellation */
+
+    { FPI_IMAGE_DEVICE_STATE_CAPTURE, FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_OFF },
+    { FPI_IMAGE_DEVICE_STATE_CAPTURE, FPI_IMAGE_DEVICE_STATE_IDLE }, /* raw mode -- currently not supported */
+    { FPI_IMAGE_DEVICE_STATE_CAPTURE, FPI_IMAGE_DEVICE_STATE_DEACTIVATING }, /* cancellation */
+
+    { FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_OFF, FPI_IMAGE_DEVICE_STATE_IDLE },
+    { FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_OFF, FPI_IMAGE_DEVICE_STATE_DEACTIVATING }, /* cancellation */
+
+    { FPI_IMAGE_DEVICE_STATE_DEACTIVATING, FPI_IMAGE_DEVICE_STATE_INACTIVE },
+  };
 
   prev_state_str = g_enum_to_string (FPI_TYPE_IMAGE_DEVICE_STATE, priv->state);
   state_str = g_enum_to_string (FPI_TYPE_IMAGE_DEVICE_STATE, state);
   fp_dbg ("Image device internal state change from %s to %s",
           prev_state_str, state_str);
 
+  for (i = 0; i < G_N_ELEMENTS (valid_transitions); i++)
+    {
+      if (valid_transitions[i].from == priv->state && valid_transitions[i].to == state)
+        {
+          transition_is_valid = TRUE;
+          break;
+        }
+    }
+  if (!transition_is_valid)
+    g_warning ("Internal state machine issue: transition from %s to %s should not happen!",
+               prev_state_str, state_str);
+
   priv->state = state;
   g_object_notify (G_OBJECT (self), "fpi-image-device-state");
   g_signal_emit_by_name (self, "fpi-image-device-state-changed", priv->state);
@@ -199,7 +240,7 @@ fpi_image_device_minutiae_detected (GObject *source_object, GAsyncResult *res, g
       if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
         {
           fp_image_device_maybe_complete_action (self, g_steal_pointer (&error));
-          fpi_image_device_deactivate (self, FALSE);
+          fpi_image_device_deactivate (self, TRUE);
           return;
         }
 
@@ -231,7 +272,7 @@ fpi_image_device_minutiae_detected (GObject *source_object, GAsyncResult *res, g
             {
               fp_image_device_maybe_complete_action (self, g_steal_pointer (&error));
               /* We might not yet be deactivating, if we are enrolling. */
-              fpi_image_device_deactivate (self, FALSE);
+              fpi_image_device_deactivate (self, TRUE);
               return;
             }
         }
@@ -385,6 +426,8 @@ fpi_image_device_report_finger_status (FpImageDevice *self,
        * In the enroll case, the decision can only be made after minutiae
        * detection has finished.
        */
+      fp_image_device_change_state (self, FPI_IMAGE_DEVICE_STATE_IDLE);
+
       if (action != FPI_DEVICE_ACTION_ENROLL)
         fpi_image_device_deactivate (self, FALSE);
       else
@@ -432,6 +475,7 @@ fpi_image_device_image_captured (FpImageDevice *self, FpImage *image)
                             fpi_image_device_minutiae_detected,
                             self);
 
+  /* XXX: This is wrong if we add support for raw capture mode. */
   fp_image_device_change_state (self, FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_OFF);
 }
 
@@ -567,6 +611,7 @@ fpi_image_device_activate_complete (FpImageDevice *self, GError *error)
   action = fpi_device_get_current_action (FP_DEVICE (self));
 
   g_return_if_fail (priv->active == FALSE);
+  g_return_if_fail (priv->state == FPI_IMAGE_DEVICE_STATE_ACTIVATING);
   g_return_if_fail (action == FPI_DEVICE_ACTION_ENROLL ||
                     action == FPI_DEVICE_ACTION_VERIFY ||
                     action == FPI_DEVICE_ACTION_IDENTIFY ||
@@ -585,6 +630,7 @@ fpi_image_device_activate_complete (FpImageDevice *self, GError *error)
 
   /* We always want to capture at this point, move to AWAIT_FINGER
    * state. */
+  fp_image_device_change_state (self, FPI_IMAGE_DEVICE_STATE_IDLE);
   fp_image_device_change_state (self, FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_ON);
 }
 
@@ -601,7 +647,7 @@ fpi_image_device_deactivate_complete (FpImageDevice *self, GError *error)
   FpImageDevicePrivate *priv = fp_image_device_get_instance_private (self);
 
   g_return_if_fail (priv->active == TRUE);
-  g_return_if_fail (priv->state == FPI_IMAGE_DEVICE_STATE_INACTIVE);
+  g_return_if_fail (priv->state == FPI_IMAGE_DEVICE_STATE_DEACTIVATING);
 
   g_debug ("Image device deactivation completed");
 
@@ -610,6 +656,8 @@ fpi_image_device_deactivate_complete (FpImageDevice *self, GError *error)
   /* Assume finger was removed. */
   priv->finger_present = FALSE;
 
+  fp_image_device_change_state (self, FPI_IMAGE_DEVICE_STATE_INACTIVE);
+
   fp_image_device_maybe_complete_action (self, error);
 }
 

libfprint/fpi-image-device.h

@@ -25,6 +25,9 @@
 /**
  * FpiImageDeviceState:
  * @FPI_IMAGE_DEVICE_STATE_INACTIVE: inactive
+ * @FPI_IMAGE_DEVICE_STATE_ACTIVATING: State during activate callback
+ * @FPI_IMAGE_DEVICE_STATE_IDLE: Activated but idle
+ * @FPI_IMAGE_DEVICE_STATE_DEACTIVATING: State during deactivate callback
  * @FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_ON: waiting for the finger to be pressed or swiped
  * @FPI_IMAGE_DEVICE_STATE_CAPTURE: capturing an image
  * @FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_OFF: waiting for the finger to be removed
@@ -35,9 +38,33 @@
  * The driver needs to call fpi_image_device_report_finger_status() to move
  * between the different states. Note that the capture state might be entered
  * unconditionally if the device supports raw capturing.
+ *
+ * A usual run would look like:
+ *  - inactive -> activating: activate vfunc is called
+ *  - activating -> idle: fpi_image_device_activate_complete()
+ *  - idle -> await-finger-on
+ *  - await-finger-on -> capture: fpi_image_device_report_finger_status()
+ *  - capture -> await-finger-off: fpi_image_device_image_captured()
+ *  - await-finger-off -> idle: fpi_image_device_report_finger_status()
+ *  - idle -> deactivating: deactivate vfunc is called
+ *  - deactivating -> inactive: fpi_image_device_deactivate_complete()
+ *
+ * Raw mode is currently not supported (not waiting for finger), but in that
+ * case the following transitions are valid:
+ *  - idle -> capture
+ *  - capture -> idle
+ *
+ * Also valid are these transitions in case of errors or cancellations:
+ *  - activating -> inactive: fpi_image_device_activate_complete()
+ *  - await-finger-on -> deactivating: deactivate vfunc is called
+ *  - capture -> deactivating: deactivate vfunc is called
+ *  - await-finger-off -> deactivating: deactivate vfunc is called
  */
 typedef enum {
   FPI_IMAGE_DEVICE_STATE_INACTIVE,
+  FPI_IMAGE_DEVICE_STATE_ACTIVATING,
+  FPI_IMAGE_DEVICE_STATE_DEACTIVATING,
+  FPI_IMAGE_DEVICE_STATE_IDLE,
   FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_ON,
   FPI_IMAGE_DEVICE_STATE_CAPTURE,
   FPI_IMAGE_DEVICE_STATE_AWAIT_FINGER_OFF,
@@ -58,11 +85,6 @@ typedef enum {
  *   finger or image capture). Implementing this is optional, it can e.g. be
  *   used to flash an LED when waiting for a finger.
  *
- * These are the main entry points for image based drivers. For all but the
- * change_state vfunc, implementations *must* eventually call the corresponding
- * function to finish the operation. It is also acceptable to call the generic
- *
- *
  * These are the main entry points for drivers to implement. Drivers may not
  * implement all of these entry points if they do not support the operation
  * (or a default implementation is sufficient).