Merge pull request #2812 from jimklimov/issue-2708-spaces

Test for and fix `status_set("ALARM")`; update code and docs on correct Numeric format usage

NEWS.adoc

@@ -288,6 +288,11 @@ https://github.com/networkupstools/nut/milestone/11
    * fixed `netvision-mib`: sync `netvision_output_info` with currently
      available `SOCOMECUPS-MIB.txt`. [#2803]
 
+ - mge-utalk driver will no longer set non-standard status values `COMMFAULT`
+   and `ALARM` (for a specific status bit); instead, it will set modern
+   `ups.alarm` with values `COMMFAULT` and/or `DEVICEALARM` (and raise
+   an `ALARM` in `ups.status` for either, as standard alarms go). [#2708]
+
  - Introduced a new driver concept for interaction with OS-reported hardware
    monitoring readings. Currently instantiated as `hwmon_ina219` specifically
    made for Texas Instruments INA219 chip as exposed in the Linux "hwmon"

UPGRADING.adoc

@@ -117,6 +117,14 @@ Changes from 2.8.2 to 2.8.3
   other devices using that MIB (negatively, if the older mappings were
   indeed correct for any practical cases, and were not a typo). [#2803]
 
+- mge-utalk driver will no longer set non-standard status values `COMMFAULT`
+  and `ALARM` (for a specific status bit); instead, it will set modern
+  `ups.alarm` with values `COMMFAULT` and/or `DEVICEALARM` (and raise
+  an `ALARM` in `ups.status` for either, as standard alarms go). If your
+  clients (e.g. custom parsing scripts) for devices supported by this driver
+  depended on those non-standard tokens in `ups.status`, they would have to
+  be updated to handle the new token values in `ups.alarm` instead. [#2708]
+
 - Added support for `lbrb_log_delay_sec=N` setting to delay propagation of
   `LB` or `LB+RB` state (buggy with APC BXnnnnMI devices/firmwares issued
   circa 2023-2024 which flood the logs with spurious LOWBATT and REPLACEBATT

docs/nut-names.txt

@@ -87,6 +87,36 @@ and in certain cases detailed below there may be a variable component
 in the practical values (e.g. the `n` in `ambient.n.temperature.alarm`
 variable or `outlet.n.load.off` command names).
 
+Numeric format
+--------------
+
+Depending on the use-case, decimal numbers may be represented as integers
+or as floating-point numbers with a dot character as the separator for the
+fractional part (typically one or two digits long).  Leading zeroes may be
+present. Leading (negative) sign characters are possible, although use-cases
+for them are rare (if any).
+
+Spaces or commas must not be used inside the numeric values.
+
+Scientific notation (with mantissa/exponent) must not be used to represent
+numeric values set into variables, to serve the values as exact as we have
+them and keep the client-side parsing simple and predictable.
+
+For example: "01200.2" and "1200.20" are valid, while "1,200.20" and "1200,20"
+and "1 200.20" and "1.2e4" are invalid.
+
+Programming note: floating-point numbers should be emitted using the `%f`
+format specifier in the C `printf` family of methods and derived methods
+(including NUT `dstate_setinfo()` in the driver code). Specifiers like `%e`
+and `%g` which can emit the scientific notation should be avoided when setting
+variable values (directly in code or when providing format string patterns in
+mapping tables). They may however be used in debug traces, where reasonable.
+
+Note that in some cases (e.g. USB vendor and product identifiers) technically
+numeric values may be reported as hexadecimal and should be treated generally
+as opaque strings (with the consumer ascribing a known meaning to certain
+variable names).
+
 Time and Date format
 --------------------
 
@@ -111,6 +141,21 @@ link:http://tools.ietf.org/html/rfc3339[RFC 3339].
 
 Other representations from those specifications are not necessarily supported.
 
+[NOTE]
+======
+Values of certain variables may be propagated from device reports formally
+as opaque strings, which happen to convey a date/time value (commonly the
+device or battery manufacture date, replacement date, last self-test or
+calibration time stamp, device clock, etc.) in some format, not necessarily
+a standard one.
+
+While the drivers may convert them from original vendor-provided markup
+to the standard Time and Date format described above (if the formula is
+known for certain -- e.g. which locale is used by the device, which part
+of that string is the year/month/day, or how to add offset or prefix for
+the year, etc.), they are not generally required to do so.
+======
+
 Variables
 ---------
 

docs/nut.dict

@@ -1,4 +1,4 @@
-personal_ws-1.1 en 3288 utf-8
+personal_ws-1.1 en 3289 utf-8
 AAC
 AAS
 ABI
@@ -243,6 +243,7 @@ DELPHYS
 DELRANGE
 DES
 DESTDIR
+DEVICEALARM
 DEVNAME
 DF
 DHEA

drivers/clone.c

@@ -147,15 +147,15 @@ static int parse_args(size_t numargs, char **arg)
 		if (!strcasecmp(arg[1], "battery.charge")) {
 			battery.charge.act = strtod(arg[2], NULL);
 
-			dstate_setinfo("battery.charge.low", "%g", battery.charge.low);
+			dstate_setinfo("battery.charge.low", "%f", battery.charge.low);
 			dstate_setflags("battery.charge.low", ST_FLAG_RW | ST_FLAG_STRING);
 			dstate_setaux("battery.charge.low", 3);
 		}
 
 		if (!strcasecmp(arg[1], "battery.runtime")) {
 			battery.runtime.act = strtod(arg[2], NULL);
 
-			dstate_setinfo("battery.runtime.low", "%g", battery.runtime.low);
+			dstate_setinfo("battery.runtime.low", "%f", battery.runtime.low);
 			dstate_setflags("battery.runtime.low", ST_FLAG_RW | ST_FLAG_STRING);
 			dstate_setaux("battery.runtime.low", 4);
 		}
@@ -518,13 +518,13 @@ static int setvar(const char *varname, const char *val)
 {
 	if (!strcasecmp(varname, "battery.charge.low")) {
 		battery.charge.low = strtod(val, NULL);
-		dstate_setinfo("battery.charge.low", "%g", battery.charge.low);
+		dstate_setinfo("battery.charge.low", "%f", battery.charge.low);
 		return STAT_SET_HANDLED;
 	}
 
 	if (!strcasecmp(varname, "battery.runtime.low")) {
 		battery.runtime.low = strtod(val, NULL);
-		dstate_setinfo("battery.runtime.low", "%g", battery.runtime.low);
+		dstate_setinfo("battery.runtime.low", "%f", battery.runtime.low);
 		return STAT_SET_HANDLED;
 	}
 

drivers/dstate.c

@@ -47,14 +47,14 @@
 #ifndef WIN32
 	static char	*sockfn = NULL;
 #else
-	static OVERLAPPED connect_overlapped;
+	static OVERLAPPED	connect_overlapped;
 	static char	*pipename = NULL;
 #endif
-	static int	stale = 1, alarm_active = 0, ignorelb = 0;
+	static int	stale = 1, alarm_active = 0, alarm_status = 0, ignorelb = 0;
 	static char	status_buf[ST_MAX_VALUE_LEN], alarm_buf[ST_MAX_VALUE_LEN];
-	static st_tree_t	*dtree_root = NULL;
 	static conn_t	*connhead = NULL;
-	static cmdlist_t *cmdhead = NULL;
+	static st_tree_t	*dtree_root = NULL;
+	static cmdlist_t	*cmdhead = NULL;
 
 	struct ups_handler	upsh;
 
@@ -1629,6 +1629,7 @@ void status_init(void)
 	}
 
 	memset(status_buf, 0, sizeof(status_buf));
+	alarm_status = 0;
 }
 
 /* check if a status element has been set, return 0 if not, 1 if yes
@@ -1650,7 +1651,7 @@ int status_get(const char *buf)
 		return 0;
 
 	offset = s - status_buf;
-#if 0
+#ifdef DEBUG
 	upsdebugx(3, "%s: '%s' in '%s': offset=%" PRIuSIZE" buflen=%" PRIuSIZE" s[buflen]='0x%2X'\n",
 		__func__, buf, status_buf, offset, buflen, s[buflen]);
 #endif
@@ -1670,7 +1671,7 @@ int status_get(const char *buf)
 /* add a status element */
 void status_set(const char *buf)
 {
-#if 0
+#ifdef DEBUG
 	upsdebugx(3, "%s: '%s'\n", __func__, buf);
 #endif
 	if (strstr(buf, " ")) {
@@ -1708,6 +1709,20 @@ void status_set(const char *buf)
 		return;
 	}
 
+	if (!strcasecmp(buf, "ALARM")) {
+		/* Drivers really should not raise alarms this way,
+		 * but for the sake of third-party forks, we handle
+		 * the possibility...
+		 */
+		upsdebugx(2, "%s: (almost) ignoring ALARM set as a status", __func__);
+		if (!alarm_status && !alarm_active && strlen(alarm_buf) == 0) {
+			alarm_init();	/* no-op currently, but better be proper about it */
+			alarm_set("[N/A]");
+		}
+		alarm_status++;
+		return;
+	}
+
 	if (status_get(buf)) {
 		upsdebugx(2, "%s: status was already set: %s", __func__, buf);
 		return;
@@ -1749,6 +1764,32 @@ void status_commit(void)
 		break;
 	}
 
+	/* NOTE: Not sure if any clients rely on ALARM being first if raised,
+	 * but note that if someone also uses status_set("ALARM") we can end
+	 * up with a "[N/A]" alarm value injected (if no other alarm was set)
+	 * and only add the token here so it remains first.
+	 *
+	 * NOTE: alarm_commit() must be executed before status_commit() for
+	 * this report to work!
+	 * * If a driver only called status_set("ALARM") and did not bother
+	 *   with alarm_commit(), the "ups.alarm" value queries would have
+	 *   returned NULL if not for the "sloppy driver" fix below, although
+	 *   the "ups.status" value would report an ALARM token.
+	 * * If a driver properly used alarm_init() and alarm_set(), but then
+	 *   called status_commit() before alarm_commit(), the "ups.status"
+	 *   value would not know to report an ALARM token, as before.
+	 * * If a driver used both status_set("ALARM") and alarm_set() later,
+	 *   the injected "[N/A]" value of the alarm (if that's its complete
+	 *   value) would be overwritten by the explicitly assigned contents,
+	 *   and an explicit alarm_commit() would be required for proper
+	 *   reporting from a non-sloppy driver.
+	 */
+
+	if (!alarm_active && alarm_status && !strcmp(alarm_buf, "[N/A]")) {
+		upsdebugx(2, "%s: Assume sloppy driver coding that ignored alarm methods and used status_set(\"ALARM\") instead: commit the injected N/A ups.alarm value", __func__);
+		alarm_commit();
+	}
+
 	if (alarm_active) {
 		dstate_setinfo("ups.status", "ALARM %s", status_buf);
 	} else {
@@ -1775,10 +1816,10 @@ void alarm_init(void)
 void alarm_set(const char *buf)
 {
 	int ret;
-	if (strlen(alarm_buf) > 0) {
-		ret = snprintfcat(alarm_buf, sizeof(alarm_buf), " %s", buf);
+	if (strlen(alarm_buf) < 1 || (alarm_status && !strcmp(alarm_buf, "[N/A]"))) {
+		ret = snprintf(alarm_buf, sizeof(alarm_buf), "%s", buf);
 	} else {
-		ret = snprintfcat(alarm_buf, sizeof(alarm_buf), "%s", buf);
+		ret = snprintfcat(alarm_buf, sizeof(alarm_buf), " %s", buf);
 	}
 
 	if (ret < 0) {

drivers/mge-utalk.c

@@ -69,7 +69,7 @@
 /* --------------------------------------------------------------- */
 
 #define DRIVER_NAME	"MGE UPS SYSTEMS/U-Talk driver"
-#define DRIVER_VERSION	"0.97"
+#define DRIVER_VERSION	"0.98"
 
 
 /* driver description structure */
@@ -775,7 +775,8 @@ static int get_ups_status(void)
 		if (exit_flag != 0)
 			return FALSE;
 
-		/* must clear status buffer before each round */
+		/* must clear alarm and status buffers before each round */
+		alarm_init();
 		status_init();
 
 		/* system status */
@@ -803,11 +804,10 @@ static int get_ups_status(void)
 			}
 			/* buf[2] not used */
 			if (buf[1] == '1')
-				status_set("COMMFAULT"); /* self-invented */
+				alarm_set("COMMFAULT"); /* self-invented */
 				/* FIXME: better to call datastale()?! */
 			if (buf[0] == '1')
-				status_set("ALARM");     /* self-invented */
-				/* FIXME: better to use ups.alarm */
+				alarm_set("DEVICEALARM");     /* self-invented */
 		}  /* if strlen */
 
 		/* battery status */
@@ -866,6 +866,7 @@ static int get_ups_status(void)
 
 	} while ( !ok && tries++ < MAXTRIES );
 
+	alarm_commit();
 	status_commit();
 
 	return ok;

drivers/powerp-txt.c

@@ -36,7 +36,7 @@
 
 #include <ctype.h>
 
-#define POWERPANEL_TEXT_VERSION	"Powerpanel-Text 0.61"
+#define POWERPANEL_TEXT_VERSION	"Powerpanel-Text 0.62"
 
 typedef struct {
 	float          i_volt;
@@ -267,13 +267,13 @@ static void powpan_initinfo(void)
 	if (powpan_command("P3\r") > 0) {
 
 		if ((s = strtok(&powpan_answer[1], ",")) != NULL) {
-			dstate_setinfo("battery.voltage.nominal", "%g", strtod(s, NULL));
+			dstate_setinfo("battery.voltage.nominal", "%f", strtod(s, NULL));
 		}
 		if ((s = strtok(NULL, ",")) != NULL) {
 			dstate_setinfo("battery.packs", "%li", strtol(s, NULL, 10));
 		}
 		if ((s = strtok(NULL, ",")) != NULL) {
-			dstate_setinfo("battery.capacity", "%g", strtod(s, NULL));
+			dstate_setinfo("battery.capacity", "%f", strtod(s, NULL));
 		}
 	}
 
@@ -502,7 +502,7 @@ static int powpan_updateinfo(void)
 	if (status.has_b_volt) {
 		dstate_setinfo("battery.voltage", "%.1f", status.b_volt);
 		if (status.b_volt > 20.0 && status.b_volt < 28.0) {
-			dstate_setinfo("battery.voltage.nominal", "%g", 24.0);
+			dstate_setinfo("battery.voltage.nominal", "%f", 24.0);
 		}
 	}
 	if (status.has_o_freq) {