Maximiliano Curia wrote:
> I've upgraded the driver provided by the manufacturer of the C-MOS [1]
UPS, which
> is an argentinian company.
>
> The file I'm attaching works with nut 2.0.4, I have one C-MOS UPS and
I've
> succesfully tested it. It would be nice to have it included in the nut
> codebase.
Thanks for your effort. A couple of questions and remarks:
1) Do you have a compatibility list for this driver (a list of CMos
models/products it can be used with?)
2) It looks like the CMOS_BTEST commands toggles the battery test. In
that case, your implementation of the instcmd's is incorrect. Sending
the "test.battery.stop" command, should unconditionally stop a battery
test. So if one is running, it should be stopped, if not just don't do
anything. Likewise, "test.battery.start" should unconditionally start
the test (if one is not running) or leave it running.
3) Something similar goes for the CMOS_OFF command. It's impossible to
map this command to both "shutdown.reboot" and "load.off".
4) The switch statement for the UPS status is probably incorrect. The
"OL" and "OB" states are mutually exclusive.
5) Also, when a battery test is running, you should not indicate an
"OB"
status, but probably "CAL" instead.
6) It's not quite clear how your upsdrv_shutdown() function is intended
to work. I think you need a test if it is needed to send the CMOS_BTEST
command, possibly depending on the CMOS_FUSE and/or CMOS_LINE status as
well.
7) You seem to add a lot of hardcoded dstate_addinfo() variables. This
is undesireable, especially if this driver can be used with different
hardware. Since they really don't add any information from the UPS, I
suggest to remove them.
Before considering to commit this patch, I want to know if you have ran
at least the following tests and what the result was:
1) the drivers can monitor the OL, OB and LB states and acts accordingly
2) upsmon -c fsd (mains present)
3) upsmon -c fsd (mains not present)
4) verified that the instcmd's work as intended
Best regards, Arjen