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

[Peter van Valderen]

I see you already did some work on the drivers just as I was going to
do some tests.

The version that's in SVN now (r1645) almost works, only one thing is
wrong (one of the few things that I'd already fixed in the
aforementioned patch for 2.2.2).

One thing needs to be changed to get it to work, I just confirmed it
by pulling the plug as the driver was running and it correctly
recognised the changed states.

This is the line that needs to change:

In 'static int execute_and_retrieve_query(char *query, char *reply)', line 59

	ret = usb_interrupt_read(upsdev, REPLY_REQUESTTYPE, reply,
sizeof(REPLY_PACKETSIZE), 1000);

becomes

	ret = usb_interrupt_read(upsdev, REPLY_REQUESTTYPE, reply,
REPLY_PACKETSIZE, 1000);

This makes the driver work for me, it would be great if we could get a
tested by Tom Cassimon or someone else with this device.

Best regards,

Peter van Valderen

On Tue, Dec 16, 2008 at 10:57 PM, Arjen de Korte <nut+devel at de-korte.org> wrote:
> 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
>
>