[Nut-upsdev] Sweex 1000VA UPS (Lakeview Research)

[Arjen de Korte]

Citeren Peter van Valderen <dosperios at gmail.com>:

> I was not aware that it had been imported into the trunk. Last time I
> looked noone seemed to have done anything with the driver or have any
> interest, so I figured there was no point. If there's any real
> interest in keeping it in the trunk, I can have a look with my device
> to see if I have the same problem mentioned above. If so, it should
> probably be fixable pretty easily.

In addition to the issues already mentioned, there are some others  
(maybe not so easy to fix):

1) It is not allowed to call upsdrv_initups() in the reconnect  
function. In upsdrv_initups() one should exit() from the driver if the  
UPS is not found. However, once you have reached upsdrv_updateinfo()  
the driver must not exit() if the UPS isn't found, but rather attempt  
to reconnect forever.

2) Similar to the above, the behavior of (re)connecting to the UPS  
must be different in upsdrv_initups() and upsdrv_updateinfo(). In the  
first case, one would probably retry several times (32 now) before  
deciding the UPS isn't there. In the latter case, the driver should  
attempt only once for each call to upsdrv_updateinfo() and return  
immediately if things don't work out. You don't have time to do  
repeated attempts here, otherwise the server will not hear from the  
driver too long.

3) Similar to the above, reading garbage data should be limited to  
once per pass upon reconnecting, like the below rudimentary example:

#define IGNORE_QUERY	5

static int	ignore = 0;

[...]

void upsdrv_updateinfo(void)
{
	char	reply[REPLY_PACKETSIZE];
	int	ret;

	if ((ignore == IGNORE_QUERY) && (reconnect() != SUCCESS)) {
		usb_comm_fail("Reconnecting to UPS failed");
		return;
	}

	ret = query_ups(reply);

	if (ret < 4) {
		usb_comm_fail("Query to UPS failed");
		dstate_datastale();
		ignore = IGNORE_QUERY;
		return;
	}

	usb_comm_good();
	dstate_dataok();

	if (ignore > 0) {
		ignore--;
		return;
	}

	[...]

4) Don't use upslog*() too often. System administrators don't like it  
if their syslog is spammed with information that is for debugging  
purposes only.

5) For the usb_control_msg() and usb_interrupt_read() calls, make a  
difference between error (ret < 0) and timeout (ret == 0), like in the  
following example:

	ret = usb_control_msg(upsdev, STATUS_REQUESTTYPE, REQUEST_VALUE,
		MESSAGE_VALUE, INDEX_VALUE, query, sizeof(query), 1000);

	if (ret < 0) {
		upsdebug_with_errno(3, "send");
		return ret;
	}

	if (ret == 0) {
		upsdebugx(3, "send: timeout");
		return ret;
	}

	upsdebug_hex(3, "send", query, ret);

Similarly, you would do the same after the usb_interrupt_read() call.  
This will help in diagnosing communication problems should this ever  
be needed.

6) Watch out with sleep() in the driver. With a timeout value of 1  
second in the usb_* calls, adding another 3 seconds will already bring  
you dangerously close to the five (5) seconds that are allowed in  
upsdrv_updateinfo().

Best regards, Arjen
-- 
Please keep list traffic on the list