* Arjen de Korte <[EMAIL PROTECTED]> [080711 07:02]:
Thanks for the review! (and sorry for the duplicate -- it's been a while
since I've used this mail client)
> 1) Flushing buffers *after* a serial command, is (usually) pointless. If
> you flush buffers (which is a good idea), you should do it right *before*
> sending a command. Otherwise, junk characters after flushing the buffer
> will still garble up the communication.
>
> 2) Get rid of the 'goto' statements. See also the 'docs/developers.txt'
> document about the use of goto.
Flushing at the start of the function is a good idea. I'll make that
change, and it will nicely make the exception-emulation goto use
unnecessary (which neatly sidesteps the whole goto religion issue!).
> 3) Never, *ever* use a while loop in a driver that is not guaranteed to
> exit at some point. The driver will now retry W/L/V/X forever if the
> commands fail. It may be a bit harsh to give up on the first failure (like
> it did before), but the number of retries must be limited.
Sadly, the original code wouldn't fail here (unless the W command hidden
in the preceeding ups_sync() call failed) -- it would just carry on
and silently return bad data.
It actually turns out to be a missed cleanup -- I had the mistaken
assumption that NUT would call ups_sync(), but that's just a remnant
from the original tripplite code. ups_sync() now is more generalized
and its error handling logic is now used for all W/L/V/X commands.
> 4) The new code in hex2d() seems to be over complicated. You're right
> about your comments on the use of strncpy(), so we prefer to use
> snprintf() now in most cases. Something like
>
> char buf[32];
> snprintf(buf, sizeof(buf), "%.*s", len, start);
> return strtol(buf, NULL, 16);
>
> will do the job just fine and is much easier to follow.
Yup, that's nicer. What I have right now in the patch is an open-coded
strlcpy(): great semantics but not portable.
> 5) Don't call dstate_datastale() on the first failure, unless you already
> know retrying is not going to fix the problem. Otherwise, it is generally
> a good idea to mask out the first two or three failures before
> complaining. When in doubt, don't call neither dstate_dataok() nor
> dstate_datastale() so that the server will report the last state the
> driver was in.
The only reason I had the function call dstate_datastale() on the first
entry is that the documentation in new-developers.txt seemed to indicate
to me that all returns from upsdrv_updateinfo() should either call
dstate_datastale() or dstate_dataok().
I prefer your suggested approach greatly (it's what I originally did
before I scanned new-drivers.txt to refresh my memory), so I'll
change back to the old code.
Updated changelog and inlined patch (compiles and detects UPS fine) follows:
- Update comment for tripplite documentation URL; appears to be dead now.
- Move #defines to tripplite.h
- Remove a useless static float definition and replace with an inline
constant; saves .text space, easier to read, and optimizes better.
- Make hex2d() use snprintf instead of strncpy. strncpy's edge case is
ugly. The old code could never overflow, but was harder to read.
- send_cmd() now flushes both input and output buffers on entry.
This change guards against corrupted serial port i/o between send_cmd()
calls.
- Squash a harmless unsigned/signed char warning in send_cmd().
- ups_sync() is removed -- detection success is now non-failure of
upsdrv_initinfo().
- upsdrv_initinfo(): W/L/V/X commands now behave identically to the old W
command in ups_sync(); three tries for each, and failure means the driver
prints an error and exits.
- upsdrv_shutdown() is significantly more robust. All serial i/o is checked
for failure, errors are reported, and where possible, data returned is
checked for range validity.
- Wait time for ser_get calls is now 1.0s rather than 3.0s.
- Update copyright date.
--- nut-2.2.2/drivers/tripplite.c 2007-05-26 10:24:26.000000000 -0400
+++ nut-2.2.2-nk/drivers/tripplite.c 2008-07-11 10:22:24.000000000 -0400
@@ -6,7 +6,7 @@
Copyright (C) 1999 Russell Kroll <[EMAIL PROTECTED]>
Copyright (C) 2001 Rickard E. (Rik) Faith <[EMAIL PROTECTED]>
- Copyright (C) 2004 Nicholas J. Kain <[EMAIL PROTECTED]>
+ Copyright (C) 2004,2008 Nicholas J. Kain <[EMAIL PROTECTED]>
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
@@ -26,8 +26,8 @@
/* REFERENCE 1
A few magic numbers were derived from the GPL'd file
- opensrc_server/upscmd.cpp, available from Tripp Lite at
- http://www.tripplite.com/linux/.
+ opensrc_server/upscmd.cpp, formerly (but not longer) available from
+ Tripp Lite at http://www.tripplite.com/linux/.
*/
/* REFERENCE 2
@@ -111,30 +111,6 @@
#include <math.h>
#include <ctype.h>
-#define ENDCHAR '\n' /* replies end with CR LF -- use LF to end */
-#define IGNCHAR '\r' /* ignore CR */
-#define MAXTRIES 3
-#define SER_WAIT_SEC 3 /* allow 3.0 sec for ser_get calls */
-#define SER_WAIT_USEC 0
-#define DEFAULT_OFFDELAY 64 /* seconds (max 0xFF) */
-#define DEFAULT_STARTDELAY 60 /* seconds (max 0xFFFFFF) */
-#define DEFAULT_BOOTDELAY 64 /* seconds (max 0xFF) */
-#define MAX_VOLT 13.4 /* Max battery voltage (100%) */
-#define MIN_VOLT 11.0 /* Min battery voltage (10%) */
-
-/* We calculate battery charge (q) as a function of voltage (V).
- * It seems that this function probably varies by firmware revision or
- * UPS model - the Windows monitoring software gives different q for a
- * given V than the old open source Tripp Lite monitoring software.
- *
- * The discharge curve should be the same for any given battery chemistry,
- * so it should only be necessary to specify the minimum and maximum
- * voltages likely to be seen in operation.
- */
-
-/* Interval notation for Q% = 10% <= [minV, maxV] <= 100% */
-static float V_interval[2] = {MIN_VOLT, MAX_VOLT};
-
/* Time in seconds to delay before shutting down. */
static unsigned int offdelay = DEFAULT_OFFDELAY;
static unsigned int startdelay = DEFAULT_STARTDELAY;
@@ -143,9 +119,9 @@ static unsigned int bootdelay = DEFAULT_
static int hex2d(char *start, unsigned int len)
{
char buf[32];
- buf[31] = '\0';
+ unsigned int i;
- strncpy(buf, start, (len < (sizeof buf) ? len : (sizeof buf - 1)));
+ snprintf(buf, sizeof buf, "%.*s", len, start);
return strtol(buf, NULL, 16);
}
@@ -159,10 +135,11 @@ static int hex2d(char *start, unsigned i
* return: # of chars in buf, excluding terminating \0 */
static int send_cmd(const char *str, char *buf, size_t len)
{
- char c;
- int ret;
+ unsigned char c;
+ int ret = 0;
size_t i = 0;
+ ser_flush_io(upsfd);
ser_send(upsfd, str);
if (!len || !buf)
@@ -171,31 +148,29 @@ static int send_cmd(const char *str, cha
for (;;) {
ret = ser_get_char(upsfd, &c, SER_WAIT_SEC, SER_WAIT_USEC);
if (ret == -1)
- return -1;
+ return ret;
if (c == ENDCHAR)
break;
}
do {
ret = ser_get_char(upsfd, &c, SER_WAIT_SEC, SER_WAIT_USEC);
if (ret == -1)
- return -1;
+ return ret;
if (c == IGNCHAR || c == ENDCHAR)
continue;
buf[i++] = c;
} while (c != ENDCHAR && i < len);
buf[i] = '\0';
- ser_flush_in(upsfd, NULL, 0);
return i;
}
-static void ups_sync(void)
+static void get_letter_cmd(char *str, char *buf, size_t len)
{
- char buf[256];
int tries, ret;
for (tries = 0; tries < MAXTRIES; ++tries) {
- ret = send_cmd(":W\r", buf, sizeof buf);
+ ret = send_cmd(str, buf, len);
if ((ret > 0) && isdigit((unsigned char)buf[0]))
return;
}
@@ -301,14 +276,10 @@ void upsdrv_initinfo(void)
int va;
long w, l;
-
- /* Detect the UPS or die. */
- ups_sync();
-
- send_cmd(":W\r", w_value, sizeof w_value);
- send_cmd(":L\r", l_value, sizeof l_value);
- send_cmd(":V\r", v_value, sizeof v_value);
- send_cmd(":X\r", x_value, sizeof x_value);
+ get_letter_cmd(":W\r", w_value, sizeof w_value);
+ get_letter_cmd(":L\r", l_value, sizeof l_value);
+ get_letter_cmd(":V\r", v_value, sizeof v_value);
+ get_letter_cmd(":X\r", x_value, sizeof x_value);
dstate_setinfo("ups.mfr", "%s", "Tripp Lite");
@@ -364,58 +335,140 @@ void upsdrv_shutdown(void)
void upsdrv_updateinfo(void)
{
+ static int numfails;
char buf[256];
- int bp;
- float bv;
+ int bp, volt, temp, load, vmax, vmin, stest, len;
+ int bcond, lstate, tstate, mode;
+ float bv, freq;
+
+ len = send_cmd(":D\r", buf, sizeof buf);
+ if (len != 21) {
+ ++numfails;
+ if (numfails > MAXTRIES) {
+ ser_comm_fail("Data command failed: [%d] bytes != 21
bytes.", len);
+ dstate_datastale();
+ }
+ return;
+ }
+
+ volt = hex2d(buf + 2, 2);
+ temp = (int)(hex2d(buf + 6, 2)*0.3636 - 21.0);
+ load = hex2d(buf + 12, 2);
+ freq = hex2d(buf + 18, 3) / 10.0;
+ bcond = buf[0];
+ lstate = buf[1];
+ tstate = buf[4];
+ mode = buf[5];
+ if (volt > INVOLT_MAX || volt < INVOLT_MIN ||
+ temp > TEMP_MAX || temp < TEMP_MIN ||
+ load > LOAD_MAX || load < LOAD_MIN ||
+ freq > FREQ_MAX || freq < FREQ_MIN) {
+ ++numfails;
+ if (numfails > MAXTRIES) {
+ ser_comm_fail("Data out of bounds:
[%0d,%3d,%3d,%02.2f]",
+ volt, temp, load, freq);
+ dstate_datastale();
+ }
+ return;
+ }
+
+ send_cmd(":B\r", buf, sizeof buf);
+ bv = (float)hex2d(buf, 2) / 10.0;
+ if (bv > 50.0 || bv < 0.0) {
+ ++numfails;
+ if (numfails > MAXTRIES) {
+ ser_comm_fail("Battery voltage out of bounds:
[%02.1f]", bv);
+ dstate_datastale();
+ }
+ return;
+ }
- send_cmd(":D\r", buf, sizeof buf);
+ send_cmd(":M\r", buf, sizeof buf);
+ vmax = hex2d(buf, 2);
+ if (vmax > INVOLT_MAX || vmax < INVOLT_MIN) {
+ ++numfails;
+ if (numfails > MAXTRIES) {
+ ser_comm_fail("InVoltMax out of bounds: [%d]", vmax);
+ dstate_datastale();
+ }
+ return;
+ }
- if (strlen(buf) < 21) {
- ser_comm_fail("Failed to get data: short read from UPS");
- dstate_datastale();
+ send_cmd(":I\r", buf, sizeof buf);
+ vmin = hex2d(buf, 2);
+ if (vmin > INVOLT_MAX || vmin < INVOLT_MIN) {
+ ++numfails;
+ if (numfails > MAXTRIES) {
+ ser_comm_fail("InVoltMin out of bounds: [%d]", vmin);
+ dstate_datastale();
+ }
return;
}
- if (strlen(buf) > 21) {
- ser_comm_fail("Failed to get data: oversized read from UPS");
- dstate_datastale();
+ send_cmd(":C\r", buf, sizeof buf);
+ errno = 0;
+ stest = strtol(buf, 0, 10);
+ if (errno == ERANGE) {
+ ++numfails;
+ if (numfails > MAXTRIES) {
+ ser_comm_fail("Self test is out of range: [%d]", stest);
+ dstate_datastale();
+ }
+ return;
+ }
+ if (errno == EINVAL) {
+ ++numfails;
+ if (numfails > MAXTRIES) {
+ ser_comm_fail("Self test returned non-numeric data.");
+ dstate_datastale();
+ }
+ return;
+ }
+ if (stest > 3 || stest < 0) {
+ ++numfails;
+ if (numfails > MAXTRIES) {
+ ser_comm_fail("Self test out of bounds: [%d]", stest);
+ dstate_datastale();
+ }
return;
}
- dstate_setinfo("input.voltage", "%0d", hex2d(buf + 2, 2));
- dstate_setinfo("ups.temperature", "%3d",
- (int)(hex2d(buf + 6, 2)*0.3636 - 21.0));
- dstate_setinfo("ups.load", "%3d", hex2d(buf + 12, 2));
- dstate_setinfo("input.frequency", "%02.2f", hex2d(buf + 18, 3) / 10.0);
+ /* We've successfully gathered all the data for an update. */
+ numfails = 0;
+
+ dstate_setinfo("input.voltage", "%0d", volt);
+ dstate_setinfo("ups.temperature", "%3d", temp);
+ dstate_setinfo("ups.load", "%3d", load);
+ dstate_setinfo("input.frequency", "%02.2f", freq);
status_init();
/* Battery Voltage Condition */
- switch (buf[0]) {
+ switch (bcond) {
case '0': /* Low Battery */
status_set("LB");
break;
case '1': /* Normal */
break;
default: /* Unknown */
- upslogx(LOG_ERR, "Unknown battery state: %c", buf[0]);
+ upslogx(LOG_ERR, "Unknown battery state: %c", bcond);
break;
}
/* Load State */
- switch (buf[1]) {
+ switch (lstate) {
case '0': /* Overload */
status_set("OVER");
break;
case '1': /* Normal */
break;
default: /* Unknown */
- upslogx(LOG_ERR, "Unknown load state: %c", buf[1]);
+ upslogx(LOG_ERR, "Unknown load state: %c", lstate);
break;
}
/* Tap State */
- switch (buf[4]) {
+ switch (tstate) {
case '0': /* Normal */
break;
case '1': /* Reducing */
@@ -426,12 +479,12 @@ void upsdrv_updateinfo(void)
status_set("BOOST");
break;
default: /* Unknown */
- upslogx(LOG_ERR, "Unknown tap state: %c", buf[4]);
+ upslogx(LOG_ERR, "Unknown tap state: %c", tstate);
break;
}
/* Mode */
- switch (buf[5]) {
+ switch (mode) {
case '0': /* Off */
status_set("OFF");
break;
@@ -445,36 +498,28 @@ void upsdrv_updateinfo(void)
status_set("OB");
break;
default: /* Unknown */
- upslogx(LOG_ERR, "Unknown mode state: %c", buf[4]);
+ upslogx(LOG_ERR, "Unknown mode state: %c", mode);
break;
}
status_commit();
- send_cmd(":B\r", buf, sizeof buf);
- bv = (float)hex2d(buf, 2) / 10.0;
/* dq ~= sqrt(dV) is a reasonable approximation
* Results fit well against the discrete function used in the Tripp Lite
* source, but give a continuous result. */
- if (bv >= V_interval[1])
+ if (bv >= MAX_VOLT)
bp = 100;
- else if (bv <= V_interval[0])
+ else if (bv <= MIN_VOLT)
bp = 10;
else
- bp = (int)(100*sqrt((bv - V_interval[0])
- / (V_interval[1] -
V_interval[0])));
+ bp = (int)(100*sqrt((bv - MIN_VOLT) / (MAX_VOLT - MIN_VOLT)));
dstate_setinfo("battery.voltage", "%.1f", bv);
dstate_setinfo("battery.charge", "%3d", bp);
+ dstate_setinfo("input.voltage.maximum", "%d", vmax);
+ dstate_setinfo("input.voltage.minimum", "%d", vmin);
- send_cmd(":M\r", buf, sizeof buf);
- dstate_setinfo("input.voltage.maximum", "%d", hex2d(buf, 2));
-
- send_cmd(":I\r", buf, sizeof buf);
- dstate_setinfo("input.voltage.minimum", "%d", hex2d(buf, 2));
-
- send_cmd(":C\r", buf, sizeof buf);
- switch (atoi(buf)) {
+ switch (stest) {
case 0:
dstate_setinfo("ups.test.result", "%s", "OK");
break;
--- nut-2.2.2/drivers/tripplite.h 2005-09-12 08:38:36.000000000 -0400
+++ nut-2.2.2-nk/drivers/tripplite.h 2008-07-09 14:53:00.000000000 -0400
@@ -6,7 +6,7 @@
Copyright (C) 1999 Russell Kroll <[EMAIL PROTECTED]>
Copyright (C) 2001 Rickard E. (Rik) Faith <[EMAIL PROTECTED]>
- Copyright (C) 2004 Nicholas J. Kain <[EMAIL PROTECTED]>
+ Copyright (C) 2004,2008 Nicholas J. Kain <[EMAIL PROTECTED]>
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
@@ -23,4 +23,38 @@
Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
-#define DRV_VERSION "0.8"
+#define DRV_VERSION "0.9"
+
+#define ENDCHAR '\n' /* replies end with CR LF -- use LF to end */
+#define IGNCHAR '\r' /* ignore CR */
+#define MAXTRIES 3 /* max number of times we try to detect ups */
+#define SER_WAIT_SEC 1 /* allow 1.0 sec for ser_get calls */
+#define SER_WAIT_USEC 0
+#define DEFAULT_OFFDELAY 64 /* seconds (max 0xFF) */
+#define DEFAULT_STARTDELAY 60 /* seconds (max 0xFFFFFF) */
+#define DEFAULT_BOOTDELAY 64 /* seconds (max 0xFF) */
+
+/* Sometimes the UPS seems to return bad data, so we range check it. */
+#define INVOLT_MAX 270
+#define INVOLT_MIN 0
+#define FREQ_MAX 80.0
+#define FREQ_MIN 30.0
+#define TEMP_MAX 100
+#define TEMP_MIN 0
+#define LOAD_MAX 100
+#define LOAD_MIN 0
+
+/* We calculate battery charge (q) as a function of voltage (V).
+ * It seems that this function probably varies by firmware revision or
+ * UPS model - the Windows monitoring software gives different q for a
+ * given V than the old open source Tripp Lite monitoring software.
+ *
+ * The discharge curve should be the same for any given battery chemistry,
+ * so it should only be necessary to specify the minimum and maximum
+ * voltages likely to be seen in operation.
+ */
+
+/* Interval notation for Q% = 10% <= [minV, maxV] <= 100% */
+#define MAX_VOLT 13.4 /* Max battery voltage (100%) */
+#define MIN_VOLT 11.0 /* Min battery voltage (10%) */
+
--
Nicholas J. Kain <nicholas at kain dot us>
http://www.kain.us/~niklata/
_______________________________________________
Nut-upsdev mailing list
[email protected]
http://lists.alioth.debian.org/mailman/listinfo/nut-upsdev
