From 3ee5536a13ea74a4ad5818e2f290b6b1747df4e9 Mon Sep 17 00:00:00 2001 From: Benjamin Berg Date: Wed, 7 Oct 2020 16:54:41 +0200 Subject: [PATCH] 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 | 20 ++++++--- libfprint/drivers/uru4000.c | 25 +++++------ libfprint/drivers/virtual-image.c | 6 +-- libfprint/fpi-image-device.c | 70 ++++++++++++++++++++++++++----- libfprint/fpi-image-device.h | 32 +++++++++++--- 5 files changed, 114 insertions(+), 39 deletions(-) diff --git a/libfprint/drivers/elan.c b/libfprint/drivers/elan.c index f88ec3e..b29a204 100644 --- a/libfprint/drivers/elan.c +++ b/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); } } diff --git a/libfprint/drivers/uru4000.c b/libfprint/drivers/uru4000.c index 2ba4d39..0e67b2d 100644 --- a/libfprint/drivers/uru4000.c +++ b/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; } } diff --git a/libfprint/drivers/virtual-image.c b/libfprint/drivers/virtual-image.c index 612fbcf..aa06080 100644 --- a/libfprint/drivers/virtual-image.c +++ b/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 diff --git a/libfprint/fpi-image-device.c b/libfprint/fpi-image-device.c index 0ad06d2..a4eb1ad 100644 --- a/libfprint/fpi-image-device.c +++ b/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) - g_warning ("Deactivating image device while waiting for finger, this should not happen."); - - priv->state = FPI_IMAGE_DEVICE_STATE_INACTIVE; - g_object_notify (G_OBJECT (self), "fpi-image-device-state"); + if (!cancelling && priv->state != FPI_IMAGE_DEVICE_STATE_IDLE) + g_warning ("Deactivating image device while it is not idle, this should not happen."); 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. */ - g_assert (state != FPI_IMAGE_DEVICE_STATE_INACTIVE); + struct + { + 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); } diff --git a/libfprint/fpi-image-device.h b/libfprint/fpi-image-device.h index 43492d8..71472db 100644 --- a/libfprint/fpi-image-device.h +++ b/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).