nutdrv_qx: fix Cypress 0665:5161 replies

NEWS.adoc

@@ -86,6 +86,12 @@ https://github.com/networkupstools/nut/milestone/13
     * Only claim a USB device as "supported" during discovery out of the box
       if `subdriver_command` was assigned (`1A86:7523` is used by CH340/341
       USB chips not only in UPSes). [issue #3410]
+    * Added a `cypress_0665_5161_subdriver` initialization method for devices
+      with USB ID `0665:5161` (Belkin, Voltronic, VOLT Polska) which enables
+      a quirk to drain stale bytes from USB buffers of the device before our
+      driver sends queries, and use a longer timeout. If this change breaks
+      your device, please add `cypress_drain_quirk = 0` in your `ups.conf`
+      and let us know. [issue #2534, PR #3563]
     * Modified `megatec` subdriver to allow `ups.firmware` with empty contents
       (when a device returns all-spaces in that part of the `I` query response)
       out of the box and not ignore the device as unsupported, by adding the

UPGRADING.adoc

@@ -79,6 +79,15 @@ The new `-A filename` option defaults to trying to use a `nutauth.conf` file
   This was revised to follow the new common setting `reconnect_max_tries`,
   which defaults to trying indefinitely now. [#3541]
 
+- Deployments using `nutdrv_qx` driver with `cypress` subdriver may be impacted
+  by a fix which added a `cypress_0665_5161_subdriver` initialization method for
+  devices with USB ID `0665:5161` (Belkin, Voltronic, VOLT Polska) which enables
+  a special quirk to drain stale bytes from USB buffers of the device before our
+  driver sends queries, and use a longer timeout. If this change breaks your
+  device, please add `cypress_drain_quirk = 0` in your `ups.conf` and let us
+  know. [issue #2534, PR #3563]
+
+
 Changes from 2.8.4 to 2.8.5
 ---------------------------
 

data/driver.list.in

@@ -1682,6 +1682,7 @@
 
 "Vivaldi"	"ups"	"1"	"EA200 LED"	"USB"	"richcomm_usb"
 
+"VOLT Polska"	"ups"	"2"	"RackUPS 1200VA/720W 2x7Ah"	"USB (USB ID 0665:5161)"	"nutdrv_qx"	# https://github.com/networkupstools/nut/issues/2534
 "Voltronic Power"	"ups"	"2"	"Apex 1KVA"	"Serial"	"nutdrv_qx"
 "Voltronic Power"	"ups"	"2"	"Apex 1KVA"	"USB"	"nutdrv_qx"
 "Voltronic Power"	"ups"	"2"	"Frigate TX 1KVA"	"Serial"	"nutdrv_qx"

docs/man/nutdrv_qx.txt

@@ -503,6 +503,15 @@ You must provide *value* (+0x409+ or +0x4095+), according to your device entry i
 *noscanlangid*::
 If this flag is set, don't autoscan valid range for langid.
 
+*cypress_drain_quirk =* 'value'::
+Some devices using the `cypress` subdriver, e.g. VOLT Polska identified as Cypress
+USB ID `0665:5161`, were seen to retain stale bytes in their USB buffer, returning
+old "garbage" as response to new queries and so corrupting discovery attempts.
+The `nutdrv_qx` driver can drain the USB buffer and use longer timeouts to communicate
+with such devices, but such a change may potentially misfire on other devices with
+same USB ID (Belkin, Voltronic).  This option allows to explicitly enable or disable
+this behavior; by default it is chosen according to device self-identification.
+
 
 IMPLEMENTATION NOTES
 ^^^^^^^^^^^^^^^^^^^^

docs/nut.dict

@@ -1,4 +1,4 @@
-personal_ws-1.1 en 3800 utf-8
+personal_ws-1.1 en 3801 utf-8
 AAC
 AAS
 ABI
@@ -1030,6 +1030,7 @@ PnP
 Poettering
 Pohle
 PointBre
+Polska
 Pos
 Potrans
 Poush

drivers/nutdrv_qx.c

@@ -58,7 +58,7 @@
 #	define DRIVER_NAME	"Generic Q* Serial driver"
 #endif	/* QX_USB */
 
-#define DRIVER_VERSION	"0.54"
+#define DRIVER_VERSION	"0.55"
 
 #ifdef QX_SERIAL
 #	include "serial.h"
@@ -730,12 +730,22 @@ static int				langid_fix = -1;
 
 static int	(*subdriver_command)(const char *cmd, size_t cmdlen, char *buf, size_t buflen) = NULL;
 
+/* -1: let the driver guess by USB ID; 0/1: explicit disable/enable */
+static int	cypress_drain_quirk_setting = -1;
+static bool_t	cypress_0665_5161_quirk = FALSE;
+
+#define CYPRESS_0665_5161_FLUSH_REPORTS	10
+#define CYPRESS_0665_5161_FLUSH_TIMEOUT	25
+#define CYPRESS_REPLY_TIMEOUT			1000
+#define CYPRESS_0665_5161_REPLY_TIMEOUT	3000
+
 /* Cypress communication subdriver */
 static int	cypress_command(const char *cmd, size_t cmdlen, char *buf, size_t buflen)
 {
 	char	tmp[SMALLBUF];
 	size_t	tmplen;
 	int	ret = 0;
+	int	reply_timeout = CYPRESS_REPLY_TIMEOUT;
 	size_t	i;
 
 	if (buflen > INT_MAX) {
@@ -745,6 +755,36 @@ static int	cypress_command(const char *cmd, size_t cmdlen, char *buf, size_t buf
 		buflen = (INT_MAX - 1);
 	}
 
+	/* Some USB devices with Cypress 0665:5161 can leave a reply from a previous
+	 * command queued on the interrupt endpoint. Do not apply this device quirk
+	 * to other Cypress-based UPSes.
+	 */
+	if (cypress_0665_5161_quirk) {
+		reply_timeout = CYPRESS_0665_5161_REPLY_TIMEOUT;
+
+		/* Read once more than the number of reports we accept, so a timeout
+		 * confirms that the endpoint is empty. Do not send a command if it
+		 * remains busy: its reply could not be associated reliably.
+		 */
+		for (i = 0; i <= CYPRESS_0665_5161_FLUSH_REPORTS; i++) {
+			ret = usb_interrupt_read(udev, 0x81,
+				(usb_ctrl_charbuf)tmp, 8, CYPRESS_0665_5161_FLUSH_TIMEOUT);
+
+			if (ret == LIBUSB_ERROR_TIMEOUT)
+				break;
+
+			if (ret <= 0)
+				return ret;
+
+			if (i == CYPRESS_0665_5161_FLUSH_REPORTS) {
+				upsdebugx(1, "cypress: input endpoint stayed busy before %s", cmd);
+				return LIBUSB_ERROR_BUSY;
+			}
+
+			upsdebugx(3, "cypress: discarded stale input report before %s", cmd);
+		}
+	}
+
 	/* Send command */
 	memset(tmp, 0, sizeof(tmp));
 	tmplen = cmdlen > sizeof(tmp) ? sizeof(tmp) : cmdlen;
@@ -779,7 +819,7 @@ static int	cypress_command(const char *cmd, size_t cmdlen, char *buf, size_t buf
 		/* ret = usb->get_interrupt(udev, (unsigned char *)&buf[i], 8, 1000); */
 		ret = usb_interrupt_read(udev,
 			0x81,
-			(usb_ctrl_charbuf)&buf[i], 8, 1000);
+			(usb_ctrl_charbuf)&buf[i], 8, reply_timeout);
 
 		/* Any errors here mean that we are unable to read a reply
 		 * (which will happen after successfully writing a command
@@ -2561,6 +2601,24 @@ static void	*cypress_subdriver(USBDevice_t *device)
 {
 	NUT_UNUSED_VARIABLE(device);
 
+	switch (cypress_drain_quirk_setting) {
+		case 0: cypress_0665_5161_quirk = FALSE; break;
+		case 1: cypress_0665_5161_quirk = TRUE; break;
+		default: cypress_0665_5161_quirk = FALSE;
+	}
+	subdriver_command = &cypress_command;
+	return NULL;
+}
+
+static void	*cypress_0665_5161_subdriver(USBDevice_t *device)
+{
+	NUT_UNUSED_VARIABLE(device);
+
+	switch (cypress_drain_quirk_setting) {
+		case 0: cypress_0665_5161_quirk = FALSE; break;
+		case 1: cypress_0665_5161_quirk = TRUE; break;
+		default: cypress_0665_5161_quirk = TRUE;
+	}
 	subdriver_command = &cypress_command;
 	return NULL;
 }
@@ -2705,7 +2763,7 @@ static qx_usb_device_id_t	qx_usb_id[] = {
 	{ USB_DEVICE(SYSGRATION_VENDORID,	0x0000),	NULL,		NULL,			&cypress_subdriver },	/* Agiler UPS */
 	{ USB_DEVICE(NONAMEFFFF_VENDORID,	0x0000),	NULL,		NULL,			&ablerex_subdriver_fun },	/* Ablerex 625L USB (Note: earlier best-fit was "krauler_subdriver" before PR #1135) */
 	{ USB_DEVICE(LEGRAND_VENDORID,	0x0035),	NULL,		NULL,			&krauler_subdriver },	/* Legrand Daker DK / DK Plus */
-	{ USB_DEVICE(CYPRESS_VENDORID,	0x5161),	NULL,		NULL,			&cypress_subdriver },	/* Belkin F6C1200-UNV/Voltronic Power UPSes */
+	{ USB_DEVICE(CYPRESS_VENDORID,	0x5161),	NULL,		NULL,			&cypress_0665_5161_subdriver },	/* Belkin F6C1200-UNV/Voltronic Power UPSes */
 	{ USB_DEVICE(PHOENIXTEC_VENDORID,	0x0002),	"Phoenixtec Power","USB Cable (V2.00)",	&phoenixtec_subdriver },/* Masterguard A Series */
 	{ USB_DEVICE(PHOENIXTEC_VENDORID,	0x0002),	NULL,		NULL,			&cypress_subdriver },	/* Online Yunto YQ450 */
 	{ USB_DEVICE(PHOENIXTEC_VENDORID,	0x0003),	NULL,		NULL,			&ippon_subdriver },	/* Mustek Powermust */
@@ -3506,6 +3564,10 @@ void	upsdrv_makevartable(void)
 		"Apply the language ID workaround to the krauler subdriver "
 		"(0x409 or 0x4095)");
 	addvar(VAR_FLAG, "noscanlangid", "Don't autoscan valid range for langid");
+
+	addvar(VAR_VALUE, "cypress_drain_quirk",
+		"Enable or disable USB buffer drain from stale data for devices "
+		"with cypress subdriver (default: enabled for 0665:5161)");
 #endif	/* QX_USB */
 
 #ifdef QX_SERIAL
@@ -3690,6 +3752,7 @@ void	upsdrv_initups(void)
 	char	*subdrv;
 # endif
 #endif
+	const char	*val;
 
 	upsdebugx(1, "%s...", __func__);
 
@@ -3705,7 +3768,8 @@ void	upsdrv_initups(void)
 		getval("product") ||
 		getval("serial") ||
 		getval("bus") ||
-		getval("langid_fix")
+		getval("langid_fix") ||
+		getval("cypress_drain_quirk")
 # if (defined WITH_USB_BUSPORT) && (WITH_USB_BUSPORT)
 		|| getval("busport")
 # endif
@@ -3750,7 +3814,6 @@ void	upsdrv_initups(void)
 		};
 
 		int		i;
-		const char	*val;
 		struct termios	tio;
 
 		/* Open and lock the serial port and set the speed to 2400 baud. */
@@ -3845,6 +3908,21 @@ void	upsdrv_initups(void)
 			}
 		}
 
+		if ((val = getval("cypress_drain_quirk"))) {
+			if (!strcmp(val, "1") || !strcasecmp(val, "on") || !strcasecmp(val, "yes") || !strcasecmp(val, "true")) {
+				upsdebugx(2, "cypress_drain_quirk explicitly enabled: %s", val);
+				cypress_drain_quirk_setting = 1;
+			} else if (!strcmp(val, "0") || !strcasecmp(val, "off") || !strcasecmp(val, "no") || !strcasecmp(val, "false")) {
+				upsdebugx(2, "cypress_drain_quirk explicitly disabled: %s", val);
+				cypress_drain_quirk_setting = 0;
+			} else if (!strcmp(val, "-1") || !strcasecmp(val, "null")) {
+				upsdebugx(2, "cypress_drain_quirk ignored (driver will choose by USB ID): %s", val);
+				cypress_drain_quirk_setting = -1;
+			} else {
+				upslogx(LOG_NOTICE, "Error enabling cypress_drain_quirk: unsupported value, ignored");
+			}
+		}
+
 		/* Pick up the subdriver name if set explicitly */
 		if (subdrv) {