⚡ Match Highlights: networkupstools/nut/pull/

mtenberge · 2018-03-23T12:21:04Z

Please consider this new driver for inclusion in NUT.

Siemens has a line of small UPSes, specially designed for use in machine control cabinets with embedded industrial PCs: they operate at 24V DC instead of AC line voltages, and are suited for DIN-rail mounting.
Machines are mostly switched off by simply interrupting the mains voltage, so with the growing popularity of embedded Linux systems in industry, I expect these UPSes to be used more and more.
There are models with lead acid batteries, but also maintenance-free models with supercaps. Some models have serial ports, others have USB interfaces.

Siemens only provides a Windows application to interface with these UPSes, but they have published the protocol in the device manual.
The protocol is not compatible with existing drivers in NUT, so I have written a new driver and corresponding documentation.
Theoretically, all models from the series (both serial and USB) should work, but as mentioned in the docs, I have only tested the driver with only 1 model variant with USB port.

zykh

I've nitpicked a bit (and today wasn't even nitpicking day, otherwise I would've marked all the remaining 'nits' -- I'll let you find all of them).
No, seriously, on a first look, overall it looks good to me.
Ah, one last remark, I think that our new drivers are supposed to have a name starting with 'nutdrv_' (and with underscores instead of dashes).

zykh · 2018-03-24T23:05:37Z

What about an AsciiDoc admonition paragraph here?

Thanks for your review and comments.
I'm new to writing manpages, and just found out that 'AsciiDoc' is the magic word to google... ;-)
I reformatted the text in various locations.

zykh · 2018-03-24T23:06:30Z

zykh · 2018-03-24T23:26:09Z

These would be better served by an AsciiDoc listing block.

zykh · 2018-03-24T23:27:35Z

Listing block, too.

zykh · 2018-03-24T23:28:39Z

Guess what?

zykh · 2018-03-25T00:03:21Z

I'd remove these redundant 'ups.*' items.

I got those from the skeleton driver skel.c, so that one might needs a revision as well.

zykh · 2018-03-25T00:06:51Z

This is a blocker. If you add a new var you are supposed to:

document it in nut-names.txt / cmdvartab

get it approved.

I have considered to use the existing variable battery.charge, but people (and code?) would probably assume that it contains an integer (percentage), not a string ("<85%" or ">85%").
As far as I could see, the list of existing variables does not contain a variable that suits this purpose, or does it?

zykh · 2018-03-25T00:10:01Z

hmm... I'm not so sure this is the best thing to do (without even documenting it in the man page): people will complain that the driver doesn't respect their settings.

You are right. Instead, I added a log message if the interval is set higher than 5.

zykh · 2018-03-25T00:11:50Z

Why not moving these remarks in instcmd() and simply call instcmd("shutdown.return", NULL) here?

Yes, better.

zykh · 2018-03-25T00:14:09Z

What about using our str_to_uint() (the sign is ignored)?

If it only would have been mentioned somewhere in the Developer Guide, I would have known about it ;-)

mtenberge · 2018-03-26T10:52:17Z

One issue remaining: the naming of new drivers.
Looking in the source tree, there are only a few driver names that start with nutdrv_. Most of them are qx drivers, and they contain both dashes and underscores in their names (e.g. nutdrv_qx_voltronic-qs-hex).
Compilation results in e.g. nutdrv_qx-nutdrv_qx_voltronic-qs-hex.o. I'm not sure if that should be the desired result...
Apart from the qx-drivers, there is only nutdrv_atcl_usb (which apparently uses underscores). If this is the desired new naming convention, I will rename the siemens driver to follow this convention.
Please consider adding this information to Chapter 4 of the Developer Guide ('Creating a new driver'), as there is no use in having a naming convention if nobody knows about it...

zykh · 2018-05-01T23:32:39Z

Thanks for addressing those issues.
And yes, you're right on the need to update skel.c and document str_*() functions and the new naming convention (speaking of the latter, the built drivers are nutdrv_qx and nutdrv_atcl_usb... and there are only these two simply because those are the only ones added after we decided to adopt the new convention): I'll either do that in the coming days or add issues here on GitHub so that we don't forget about that.

Two (three) things left:

I agree with you on battery.charge (that's why my comment was about the things to do when adding a new var) -- I'm more for a name like battery.charge.approximation, though, and I'm not completely sure on what should be the format of its value, apart from the fact that it should not have the '%' explicitly expressed in the value to go along with other similar vars, but, probably, something like you did (< 85 / > 85) should be just fine with me, or maybe we could use an interval (e.g. 85-100), but I'm not so fond of it, either,
in the manpage, I think it should be better to actually mention pollinterval (that is, something like: 'The serial port is polled periodically, every pollinterval, for new data (recommended setting: every second) ...').
(my git complains about whitespace errors).

In my opinion this is (almost) ready, so I'm adding it to 2.7.5 milestone.

mtenberge · 2018-05-04T08:34:41Z

About the charge level indication variable: I started a thread about this on the nut-upsdev list, but up to now it remains quiet there… (https://alioth-lists.debian.net/pipermail/nut-upsdev/2018-April/thread.html). For now, I removed the % characters from the indication values.

I added a separate section to the man page about polling, and fixed the line endings.

aquette · 2019-02-28T12:57:57Z

@zykh and @mtenberge what's the status on this one?
I'm back on trying (again) to push 2.7.5...

zykh · 2019-03-02T23:52:20Z

@aquette: code-wise, this should be ready (after a rebase).

As mentioned in the last comment, the only thing left is the publication of the (approximate) level of battery charge reported (these devices only report if they are below or above 85%) and the formalisation of the var used for that purpose and of its content (i.e. document it in docs/nut-names.txt and in data/cmdvartab): what do we want to do?

a. add a new variable for things like that?
b. or, don't use that data at all?

I'm more for the 'a' option, which is the one @mtenberge already implemented, but with a name of battery.charge.approx (in future, we could also consider to make it a general suffix and extend its use to other vars), instead of the implemented battery.charge.indication.
And, for the published content, the already implemented option (<85 / >85) looks (not beautiful, but) good to me.

But I'm not good at naming things...

mtenberge · 2019-03-04T09:22:45Z

I don't have a strong opinion on the variable naming, I'll leave that up to you core devs to decide.

I'll see if I can do a rebase this week.

…es UPSes

- The number of delimiter characters is no longer hard-coded, but will be detected and thrown away at runtime - Discard any remains in the RX buffer after communication errors - collapse repeating code - upsdrv_shutdown now uses instcmd (removes duplicate code) - obsolete ups.* variables removed - poll_interval not hard-coded, but give a warning when a large value is encountered - option parsing improved

As requested in networkupstools#545 (review)

zykh · 2019-07-30T04:38:55Z

@mtenberge, thanks and sorry for the huge delay.
Since, meanwhile, no one objected, later today, I'll merge this and apply the change I suggested.
So.. last chance to shout at me...

…oc: Fixed man page naming for `nutdrv_siemens-sitop(.8)` (dash vs. underscore) to match the driver program name [networkupstools#1209, networkupstools#545] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>

zykh requested changes Mar 25, 2018

View reviewed changes

zykh approved these changes May 1, 2018

View reviewed changes

zykh added this to the 2.7.5 milestone May 1, 2018

mtenberge force-pushed the add-siemens-sitop-driver branch from be2a4cc to dc5eebb Compare March 4, 2019 14:20

mtenberge added 9 commits May 15, 2019 10:12

Initial commit of siemens-sitop: driver for Siemens SITOP UPS500-seri…

3d20109

…es UPSes

Spellcheck fixes

551a5b2

Improve manpage layout

05f8290

Rename driver siemens-sitop to nutdrv_siemens_sitop

6be12cd

As requested in networkupstools#545 (review)

Better explanation of serial port polling

51993e0

Fix line endings

7ddc280

Remove %-sign from variable values

9272297

Spelling fix

8dcb9fb

mtenberge force-pushed the add-siemens-sitop-driver branch from dc5eebb to 8dcb9fb Compare May 15, 2019 08:20

zykh merged commit 8dcb9fb into networkupstools:master Jul 31, 2019

mtenberge deleted the add-siemens-sitop-driver branch August 27, 2019 08:15

Conversation

mtenberge commented Mar 23, 2018

Uh oh!

zykh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtenberge commented Mar 26, 2018

Uh oh!

zykh commented May 1, 2018

Uh oh!

mtenberge commented May 4, 2018

Uh oh!

aquette commented Feb 28, 2019

Uh oh!

zykh commented Mar 2, 2019

Uh oh!

mtenberge commented Mar 4, 2019

Uh oh!

zykh commented Jul 30, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants