Nut upsdev - Dec 2007 - Driver bestfcom - Timing problem with Fortress LI1420

Hi,
my UPS is a Best Fortress LI1420 with the monitoring master running fedora 7.
I have to replace the manufacturer's software "Checkups II"
because it's not
working with releases after fc4.

My choice is the nut package.
But it isn't just installing rpm's and configure the parameter files.

?
The first hurdle to take was realising the UPS doesn't send a prompt.
Function sync_serial() sends only <cr>, no command, and the UPS answers
with a
new line. Without prompt enabled bestfcom will loop forever. Called via 
upsdrvctl it will time out.
So debugging the source helped.
Setting the Fortress parameter 75 (prompt) to 1 solved this.

Next the driver doesn't work. Error message is:

[nut at ls1 ~]# bestfcom -D -D -D -a LI1420
Network UPS Tools - Best Ferrups/Fortress 0.11 (2.0.5)
debug level is '3'
UPS Time: Sunday, December 02, 2007 - 02:41:19
fc response: 80 ???????????????????????????????????????????????? ? ? 
120202411901000000010000022002200000001100024200000533500000230000000000E0010663
Bad response from formatconfig command in upsdrv_init_fc()

The line returned is not from command "fc" but from command
"f" called just
before. 
When stepping the source under gdb everything works fine.
Inserting some sleep(1) commands helps.

############################################################
?begin diff
############################################################
--- bestfcom.c??2006-11-08 03:08:45.000000000 +0100
+++ nut-2.0.5/drivers/bestfcom.c????????2007-12-02 01:21:51.000000000 +0100
@@ -181,7 +181,8 @@
?static int execute(const char *cmd, char *result, int resultsize)
?{
????????int ret;
-???????char ch, buf[256];
+???????unsigned char ch;
+???????char buf[256];
?
????????/* Check for the Inverter status alarm if pending :
???????? * "\r\n{Inverter: ? ? On}\r\n=>"
@@ -190,10 +191,12 @@
????????????????POLL_ALERT, alert_handler, 0, 20);
?
????????ser_send(upsfd, cmd);
+???????sleep(UPSDELAY);
?
????????/* delete command echo up to \012 but no further */
????????for (ch = '\0'; ch != '\012'; ser_get_char(upsfd,
&ch, 0, 10));
?
+???????sleep(UPSDELAY);
????????/* get command response?*/
????????ret = ser_get_line(upsfd, result, resultsize, '\015',
"\012", 3, 0);
?
@@ -741,7 +744,9 @@
????????????????exit(EXIT_FAILURE);
????????}
?
+???????sleep(UPSDELAY);
????????execute("fc\r", rstring, sizeof(rstring));
+???????sleep(UPSDELAY);
????????if (strlen(rstring) < 80 ) {
????????????????upsdrv_init_nofc();
????????} else {

############################################################
?end diff
############################################################
?
Am I the only one facing this? Or is my UPS (manufactured 1998) the last of 
it's kind?

I hope this helps other Fortress users.

H.Kegel

[...]
> Am I the only one facing this? Or is my UPS (manufactured 1998) the last
> of it's kind?
Looking at the sources, it looks this this problem was created with the
conversion to the new ser_* functions a couple of years ago. Given the age
of your UPS, chances are that this isn't a very popular one anymore, so
you may indeed be the first to notice that there is something wrong.

The problems are (as you empirically found out as well) in the timeouts
used in the driver:

173	ser_get_line(upsfd, buf, sizeof(buf), '\012', "", 0, 20);
189	ser_get_line_alert(upsfd, buf, sizeof(buf), '\012', "",
190		POLL_ALERT, alert_handler, 0, 20);
195	for (ch = '\0'; ch != '\012'; ser_get_char(upsfd, &ch,
0, 10));

The first problem is that the person that converted these functions from
an earlier version probably assumed that the timeout values here are
defined in seconds and milliseconds. The first is true, but the second
value reads in microseconds. In practice this will mean that these
functions will not wait at all and immediately timeout. This could have
been detected, but unfortunately in none of these cases is checked for the
return value (problem number two). Adding a lot of delay (like you did)
fixes this, by allowing the serial buffers in the kernel to receive all
the characters that the UPS sends, before doing the reads, so no
additional delays are needed and they will work as intended.

Could you try if fixing this (instead of adding these 1 second sleeps) by
changing the above calls in the driver to

173	ser_get_line(upsfd, buf, sizeof(buf), '\012', "", 0,
20000);
189	ser_get_line_alert(upsfd, buf, sizeof(buf), '\012', "",
190		POLL_ALERT, alert_handler, 0, 20000);
195	for (ch = '\0'; ch != '\012'; ser_get_char(upsfd, &ch,
0, 10000));

fixes this problem?

The reason why I don't want to use your patch ad verbatim, is that
unconditional 1 second sleeps makes the driver very sluggish, to the point
that we may get into trouble with the connection to the sever. It probably
isn't needed either.

Thanks for your efforts in diagnosing this problem, anyway.

Best regards, Arjen
-- 
Eindhoven - The Netherlands
Key fingerprint - 66 4E 03 2C 9D B5 CB 9B  7A FE 7E C1 EE 88 BC 57