Enrollment: improved error reporting

Convert enrollment function to return a signed integer, which is negative
on error or corresponds into fp_enroll_result otherwise.
Now we can treat a 'FAIL' condition differently from an actual error: fail
means that enrollment didn't complete because the data was nonsense or
whatever (e.g. scanned a different finger for each stage?).

Updated upekts accordingly.

examples/verify.c

@@ -19,6 +19,7 @@
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <unistd.h>
 
 #include <libfprint/fprint.h>
 
@@ -39,15 +40,20 @@ struct fp_dscv_dev *discover_device(struct fp_dscv_dev **discovered_devs)
 
 struct fp_print_data *enroll(struct fp_dev *dev) {
 	struct fp_print_data *enrolled_print = NULL;
-	enum fp_enroll_status status;
+	int r;
 
 	printf("You will need to successfully scan your finger %d times to "
 		"complete the process.\n", fp_dev_get_nr_enroll_stages(dev));
 
 	do {
-		printf("Scan your finger now.\n");
-		status = fp_enroll_finger(dev, &enrolled_print);
-		switch (status) {
+		sleep(1);
+		printf("\nScan your finger now.\n");
+		r = fp_enroll_finger(dev, &enrolled_print);
+		if (r < 0) {
+			printf("Enroll failed with error %d\n", r);
+			return NULL;
+		}
+		switch (r) {
 		case FP_ENROLL_COMPLETE:
 			printf("Enroll complete!\n");
 			break;
@@ -67,15 +73,19 @@ struct fp_print_data *enroll(struct fp_dev *dev) {
 			printf("Didn't catch that, please center your finger on the "
 				"sensor and try again.\n");
 			break;
+		case FP_ENROLL_RETRY_REMOVE_FINGER:
+			printf("Scan failed, please remove your finger and then try "
+				"again.\n");
+			break;
 		}
-	} while (status != FP_ENROLL_COMPLETE);
+	} while (r != FP_ENROLL_COMPLETE);
 
 	if (!enrolled_print) {
 		fprintf(stderr, "Enroll complete but no print?\n");
 		return NULL;
 	}
 
-	printf("got a print!\n");
+	printf("Enrollment completed!\n");
 	return enrolled_print;
 }
 

libfprint/core.c

@@ -18,10 +18,11 @@
  */
 
 #include <config.h>
+#include <errno.h>
+#include <stdio.h>
 
 #include <glib.h>
 #include <usb.h>
-#include <stdio.h>
 
 #include "fp_internal.h"
 
@@ -227,18 +228,18 @@ API_EXPORTED const char *fp_driver_get_full_name(const struct fp_driver *drv)
 	return drv->full_name;
 }
 
-API_EXPORTED enum fp_enroll_status fp_enroll_finger(struct fp_dev *dev,
+API_EXPORTED int fp_enroll_finger(struct fp_dev *dev,
 	struct fp_print_data **print_data)
 {
 	const struct fp_driver *drv = dev->drv;
-	enum fp_enroll_status ret;
+	int ret;
 	int stage = dev->__enroll_stage;
 	gboolean initial = FALSE;
 
 	if (!dev->nr_enroll_stages || !drv->enroll) {
 		fp_err("driver %s has 0 enroll stages or no enroll func",
-			dev->drv->name);
-		return FP_ENROLL_FAIL;
+			drv->name);
+		return -ENOTSUP;
 	}
 
 	if (stage == -1) {
@@ -248,13 +249,19 @@ API_EXPORTED enum fp_enroll_status fp_enroll_finger(struct fp_dev *dev,
 
 	if (stage >= dev->nr_enroll_stages) {
 		fp_err("exceeding number of enroll stages for device claimed by "
-			"driver %s (%d stages)", dev->drv->name, dev->nr_enroll_stages);
-		return FP_ENROLL_FAIL;
+			"driver %s (%d stages)", drv->name, dev->nr_enroll_stages);
+		dev->__enroll_stage = -1;
+		return -EINVAL;
 	}
 	fp_dbg("%s will handle enroll stage %d/%d%s", drv->name, stage,
 		dev->nr_enroll_stages - 1, initial ? " (initial)" : "");
 
 	ret = drv->enroll(dev, initial, stage, print_data);
+	if (ret < 0) {
+		fp_err("enroll failed with code %d", ret);
+		dev->__enroll_stage = -1;
+		return ret;
+	}
 	switch (ret) {
 	case FP_ENROLL_PASS:
 		fp_dbg("enroll stage passed");
@@ -273,6 +280,9 @@ API_EXPORTED enum fp_enroll_status fp_enroll_finger(struct fp_dev *dev,
 	case FP_ENROLL_RETRY_CENTER_FINGER:
 		fp_dbg("finger was not centered, enroll should retry");
 		break;
+	case FP_ENROLL_RETRY_REMOVE_FINGER:
+		fp_dbg("scan failed, remove finger and retry");
+		break;
 	case FP_ENROLL_FAIL:
 		fp_err("enroll failed");
 		dev->__enroll_stage = -1;
@@ -280,7 +290,7 @@ API_EXPORTED enum fp_enroll_status fp_enroll_finger(struct fp_dev *dev,
 	default:
 		fp_err("unrecognised return code %d", ret);
 		dev->__enroll_stage = -1;
-		return FP_ENROLL_FAIL;
+		return -EINVAL;
 	}
 	return ret;
 }

libfprint/drivers/upekts.c

@@ -461,7 +461,6 @@ static int dev_init(struct fp_dev *dev)
 		goto err;
 	}
 
-	seq = 0;
 	dummy = 0x04;
 	r = send_cmd28(dev, 0x06, &dummy, 1);
 	if (r < 0)
@@ -523,7 +522,7 @@ static const unsigned char scan_comp[] = {
 	0x12, 0xff, 0xff, 0xff, 0xff /* scan completion, prefixes print data */
 };
 
-static enum fp_enroll_status enroll(struct fp_dev *dev, gboolean initial,
+static int enroll(struct fp_dev *dev, gboolean initial,
 	int stage, struct fp_print_data **_data)
 {
 	unsigned char poll_data[] = { 0x30, 0x01 };
@@ -537,11 +536,11 @@ static enum fp_enroll_status enroll(struct fp_dev *dev, gboolean initial,
 		r = send_cmd28(dev, 0x02, (unsigned char *) enroll_init,
 			sizeof(enroll_init));
 		if (r < 0)
-			return FP_ENROLL_FAIL;
+			return r;
 		/* FIXME: protocol misunderstanding here. device receives response
 		 * to subcmd 0 after submitting subcmd 2? */
 		if (read_msg28(dev, 0x00, NULL, NULL) < 0)
-			return FP_ENROLL_FAIL;
+			return -EPROTO;
 	}
 
 	while (!result) {
@@ -551,12 +550,12 @@ static enum fp_enroll_status enroll(struct fp_dev *dev, gboolean initial,
 		if (r < 0)
 			return r;
 		if (read_msg28(dev, 0x00, &data, &data_len) < 0)
-			return FP_ENROLL_FAIL;
+			return -EPROTO;
 
 		if (data_len != 14) {
 			fp_err("received 3001 poll response of %d bytes?", data_len);
 			g_free(data);
-			return FP_ENROLL_FAIL;
+			return -EPROTO;
 		}
 
 		status = data[5];
@@ -573,9 +572,11 @@ static enum fp_enroll_status enroll(struct fp_dev *dev, gboolean initial,
 				result = FP_ENROLL_PASS;
 			break;
 		case 0x1c: /* FIXME what does this one mean? */
-		case 0x0f: /* scan taking too long, remove finger and try again */
 			result = FP_ENROLL_RETRY;
 			break;
+		case 0x0f: /* scan taking too long, remove finger and try again */
+			result = FP_ENROLL_RETRY_REMOVE_FINGER;
+			break;
 		case 0x1e: /* swipe too short */
 			result = FP_ENROLL_RETRY_TOO_SHORT;
 			break;
@@ -583,6 +584,9 @@ static enum fp_enroll_status enroll(struct fp_dev *dev, gboolean initial,
 			result = FP_ENROLL_RETRY_CENTER_FINGER;
 			break;
 		case 0x20:
+			/* finger scanned successfully */
+			/* don't break out immediately, need to look at the next
+			 * value to determine if enrollment is complete or not */
 			passed = 1;
 			break;
 		case 0x00:
@@ -591,12 +595,10 @@ static enum fp_enroll_status enroll(struct fp_dev *dev, gboolean initial,
 			break;
 		default:
 			fp_err("unrecognised scan status code %02x", status);
-			result = FP_ENROLL_FAIL;
+			result = -EPROTO;
 			break;
 		}
-
-		if (result != FP_ENROLL_COMPLETE)
-			g_free(data);
+		g_free(data);
 	}
 
 	/* FIXME: need to extend protocol research to handle the case when
@@ -612,26 +614,30 @@ static enum fp_enroll_status enroll(struct fp_dev *dev, gboolean initial,
 		/* FIXME: protocol misunderstanding here. device receives response
 		 * to subcmd 0 after submitting subcmd 2? */
 		if (read_msg28(dev, 0x02, &data, &data_len) < 0)
-			return FP_ENROLL_FAIL;
+			return -EPROTO;
 
 		if (data_len < sizeof(scan_comp)) {
 			fp_err("fingerprint data too short (%d bytes)", data_len);
-			return FP_ENROLL_FAIL;
+			result = -EPROTO;
+			goto comp_out;
 		}
 		if (memcmp(data, scan_comp, sizeof(scan_comp)) != 0) {
 			fp_err("unrecognised data prefix %x %x %x %x %x",
 				data[0], data[1], data[2], data[3], data[4]);
-			return FP_ENROLL_FAIL;
+			result = -EPROTO;
+			goto comp_out;
 		}
 		if (!_data) {
 			fp_err("complete but no data storage!");
-			return FP_ENROLL_COMPLETE;
+			result = FP_ENROLL_COMPLETE;
+			goto comp_out;
 		}
 
 		fdata = fpi_print_data_new(dev, data_len - sizeof(scan_comp));
 		buf = fpi_print_data_get_buffer(fdata);
 		memcpy(buf, data + sizeof(scan_comp), data_len - sizeof(scan_comp));
 		*_data = fdata;
+comp_out:
 		g_free(data);
 	}
 

libfprint/fp_internal.h

@@ -96,8 +96,8 @@ struct fp_driver {
 	/* Device operations */
 	int (*init)(struct fp_dev *dev);
 	void (*exit)(struct fp_dev *dev);
-	enum fp_enroll_status (*enroll)(struct fp_dev *dev, gboolean initial,
-		int stage, struct fp_print_data **print_data);
+	int (*enroll)(struct fp_dev *dev, gboolean initial, int stage,
+		struct fp_print_data **print_data);
 };
 
 extern const struct fp_driver upekts_driver;

libfprint/fprint.h

@@ -41,18 +41,18 @@ int fp_dev_get_nr_enroll_stages(struct fp_dev *dev);
 const char *fp_driver_get_name(const struct fp_driver *drv);
 const char *fp_driver_get_full_name(const struct fp_driver *drv);
 
-/* Enrolment */
-enum fp_enroll_status {
+/* Enrollment */
+enum fp_enroll_result {
 	FP_ENROLL_COMPLETE = 1,
 	FP_ENROLL_FAIL,
 	FP_ENROLL_PASS,
-	FP_ENROLL_RETRY,
+	FP_ENROLL_RETRY = 100,
 	FP_ENROLL_RETRY_TOO_SHORT,
 	FP_ENROLL_RETRY_CENTER_FINGER,
+	FP_ENROLL_RETRY_REMOVE_FINGER,
 };
 
-enum fp_enroll_status fp_enroll_finger(struct fp_dev *dev,
-	struct fp_print_data **print_data);
+int fp_enroll_finger(struct fp_dev *dev, struct fp_print_data **print_data);
 
 /* Data handling */
 void fp_print_data_free(struct fp_print_data *data);