drivers: delta-hid: Add Delta HID subdriver#807
drivers: delta-hid: Add Delta HID subdriver#807jimklimov merged 31 commits intonetworkupstools:masterfrom
Conversation
Delta HID subdriver is added to support Delta RT Series, Single Phase, 1/2/3 kVA UPS devices. The driver was tested and tweaked on Delta RT 3 kVA UPS. Signed-off-by: Luka Kovačič <luka.kovacic@builtin.io>
|
flag all |
Update entry flags to use HU_FLAG_QUICK_POLL for status and alarm variables, HU_FLAG_SEMI_STATIC for variables that can change on user changes and HU_FLAG_STATIC for variables that don't need to be updated after init. Signed-off-by: Luka Kovačič <luka.kovacic@builtin.io>
|
@LynxChaus thanks for pointing this out! |
|
@lukakovacica Don't bother - this project seems dead. My PRs are still not merged after over 3 months (very few lines of code). Not sure what's going on ... |
|
@flobernd : sorry about making that impression, there was quite a bit of turbulence in private lives of people involved, and change of leadership direction, and so on... |
|
@jimklimov No problem at all! Glad to see this project alive. A short information some months ago would have helped tho 😋 |
|
@jimklimov Thanks for reviewing the driver. |
|
hey there
|
aquette
left a comment
aquette
left a comment
There was a problem hiding this comment.
looks good but needs a few changes and complement, as pointed. Don't hesitate to ask for more info if needed
| static hid_info_t delta_hid2nut[] = { | ||
|
|
||
| { "battery.voltage.nominal", 0, 0, "UPS.BatterySystem.Battery.ConfigVoltage", NULL, "%.0f", HU_FLAG_STATIC, NULL }, | ||
| { "battery.voltage", 0, 0, "UPS.BatterySystem.Battery.Voltage", NULL, "%.1f", HU_FLAG_QUICK_POLL, NULL }, |
There was a problem hiding this comment.
HU_FLAG_QUICK_POLL should be limited to status data and closely monitored ones, such as timers , that need refresh more often. Just put 0 for other vars
There was a problem hiding this comment.
Polling Battery.Voltage once in minute - not good, under high load battery may discharge really fast within minute.
There was a problem hiding this comment.
I suppose that would mean that the UPS is under-spec'ed for its load. The battery capacity should allow for some overhead of detection of a non-negligibly long outage, and then some for wall power to maybe come back quickly, and when the battery level gets to suspicious levels - initiate graceful shutdown of systems and survive long enough (deployment-dependent, servers can take tens of minutes for interconnected webs of services to go down correctly).
And also batteries age, so it might not hold as long in practice when the outage bites as when it did when you bought and tested it.
Rule of thumb is that you should have at least 15 minutes under your full load (e.g. server busy checksumming its disks over and over with all cores and disks busy).
So if the "once a minute" is too infrequent, something is wrong on the hardware side.
Also, voltage the battery produces is often non-linear compared to its remaining capacity or run-time; if the UPS hardware and protocol provides those data points, and does not mislead doing so (I think we had an issue open for some devices that publish unreliable values), they should be preferred for determining when you should start a shutdown.
That all ranted out, @aquette : in case of going on-battery, 1) Does an USB UPS actively push that notice into the driver or is it only seen with latency of regular polling? 2) Does the poll frequency increase when we are at risk?
There was a problem hiding this comment.
- supposed to push notifications, but really depends on the mfr implem. Visible in the initial dump, if this var is both tagged notif and polled (don't remember the wording I put though)
- no, but could be an improvement to consider. Still, some device may not support well too much polling when on batt, since they are busy processing internals. And still, interrupts notif are supposed to be here for that!
| { "battery.runtime", 0, 0, "UPS.PowerSummary.RunTimeToEmpty", NULL, "%.0f", HU_FLAG_QUICK_POLL, NULL }, | ||
| { "battery.charge.warning", 0, 0, "UPS.PowerSummary.WarningCapacityLimit", NULL, "%.0f", HU_FLAG_SEMI_STATIC, NULL }, | ||
| { "battery.type", 0, 0, "UPS.PowerSummary.iDeviceChemistry", NULL, "%s", HU_FLAG_STATIC, stringid_conversion }, | ||
| { "ups.mfr", 0, 0, "UPS.PowerSummary.iManufacturer", NULL, "%s", HU_FLAG_STATIC, stringid_conversion }, |
There was a problem hiding this comment.
All usages with a leaf starting by a "i" are in fact indexes to strings in the strings table. (hence the stringid_conversion callback), but conversely to battery.type above, these 3 declarations (mfr, model and serial) can be removed, since these are addressed by the delta_format_*() functions hooks.
| { "output.frequency.nominal", 0, 0, "UPS.Flow.ConfigFrequency", NULL, "%.0f", HU_FLAG_SEMI_STATIC, NULL }, | ||
| { "output.voltage.nominal", 0, 0, "UPS.Flow.ConfigVoltage", NULL, "%.0f", HU_FLAG_SEMI_STATIC, NULL }, | ||
| { "ups.realpower", 0, 0, "UPS.OutletSystem.Outlet.ActivePower", NULL, "%.1f", HU_FLAG_QUICK_POLL, NULL }, | ||
| { "ups.delay.reboot", 0, 0, "UPS.OutletSystem.Outlet.DelayBeforeReboot", NULL, "%.0f", HU_FLAG_SEMI_STATIC, NULL }, |
There was a problem hiding this comment.
these 3 delays are configs & RW, and used (duplicated) later to declare the commandss.
See
https://github.com/networkupstools/nut/blob/master/drivers/mge-hid.c#L1188
https://github.com/networkupstools/nut/blob/master/drivers/mge-hid.c#L1394
note that you just need to declare the "delay" version of the command, the instant one (delay = 0) will be automatically added
| { "BOOL", 0, 0, "UPS.PowerSummary.PresentStatus.NeedReplacement", NULL, NULL, HU_FLAG_QUICK_POLL, replacebatt_info }, | ||
| { "BOOL", 0, 0, "UPS.PowerSummary.PresentStatus.ShutdownImminent", NULL, NULL, HU_FLAG_QUICK_POLL, shutdownimm_info }, | ||
| { "battery.charge", 0, 0, "UPS.PowerSummary.RemainingCapacity", NULL, "%.0f", HU_FLAG_QUICK_POLL, NULL }, | ||
| { "battery.charge.low", 0, 0, "UPS.PowerSummary.RemainingCapacityLimit", NULL, "%.0f", HU_FLAG_SEMI_STATIC, NULL }, |
There was a problem hiding this comment.
this is probably a settings (ST_FLAG_RW).
See https://github.com/networkupstools/nut/blob/master/drivers/mge-hid.c#L1151
|
Hard to say if this device do well implement usb interrupts (notification sent by the device, between millings). A driver debug output with level 1 (dump HID data tree) should show us, or otherwise higher debug when going on battery |
|
Hi everyone, I'll send some debug output from the driver and upsc output to you this weekend. |
|
By the way, just in case: @lukakovacica did/do you work for the maker of this UPS? One more point came up, it is worth mentioning the vendors' efforts at https://github.com/networkupstools/nut/blob/master/docs/acknowledgements.txt |
|
@jimklimov No, I have no association with Delta in any way in preparing this driver. |
|
As I realized, maybe a bit too late, this effort potentially duplicates #987 which (for good or bad) had no objections so was merged recently. I suppose this PR should now be revisited to check what else it brings to the table, and/or if the merged code did have the weaknesses highlighted in this review. And ultimately, if the two contributions can be merged into the same sources and subdriver, to support both hopefully different device series (claiming "Delta UPS Amplon R Series" in #987, "Delta RT series UPS devices" in #807). |
Sync the vendor-name comment (for udev generation among others) with another Delta HID driver merged to master recently.
|
It seems the two PRs are quite compatible, working on a series of commits that would make this one seem to add fixes to one already merged. Main difference seems to be in the mapping fields seen by, I suppose, generation scripts on the two authors' systems and UPSes, and different flags to many of the fields that are present in both codebases. |
…ter branch: move delta_ups_format_model() to same position as in master
…ter branch: import delta_ups_format_model() content from master
…ter branch: import named delta_ups_usage_lkp[] content from master
…ter branch: change delta_ups_hid2nut[] content order by names to be same as in master
…ter branch: import Helper lookup tables and mapping functions from master
…ter branch: import delta_ups_hid2nut[] entries "as is"
…ter branch: set battery.charge* precision to %.0f same for all entries
…ter branch: set voltage/current/frequency precision to %.1f same for all entries
…ter branch: comment away ups.mfr/model/serial setting per PR comments
…ter branch: remove (read-only) mappings for ups.delay* in favor of same points polled for ups.timer*
…ter branch: sync polling frequencies
|
Note: I have prepared the branch to converge the work on a new Delta UPS USB support already merged into NUT master, and the independent work done here with other devices from that vendor adding more to the support. Currently that change is proposed at lukakovacica#1 and stored at https://github.com/networkupstools/nut/tree/PR-807 branch. It was prepared starting from an earlier commit in this PR (before it got master merged into it), renaming the delta-hid files of this PR into delta_usb-hid, and respectively adding the "_ups" into func/table names there, and resynchronizing the contents. While the changeset itself is large and seems to delete one file and modify a lot in another, the trail of numerous commits with smaller-scoped goals can help retrace and review this convergence effort. @aquette: I would welcome very much if you could go over that resulting delta_usb-hid.c regarding your comments above, about which fields should be polled at which frequencies, and how delay vs. timer fit together, etc :) @jungeonkim @lukakovacica: would you please have a chance to try a build of https://github.com/networkupstools/nut/tree/PR-807 to check if the combined code still works for the UPSes you developed with? |
|
A fast 1mn review since good and showing that you applied my comments. Though reviewing on a phone... Trusting you. |
…ARIABLE(device) arg
|
This pull request introduces 1 alert when merging 4a24894 into 82a984b - view on LGTM.com new alerts:
|
|
Gentle bump: @jungeonkim @lukakovacica: would you please have a chance to try a build of https://github.com/networkupstools/nut/tree/PR-807 or of this PR's branch directly as https://github.com/networkupstools/LukaKovacica/nut/tree/nut-rebase to check if the combined code still works for the UPSes you developed with? I am rather eager to merge this contribution, but would not like to break by that the use-cases that apparently did work for either of you :) |
|
@jimklimov it looks like work |
|
@jimklimov I will take a look and try this on the UPS during this weekend. |
|
Thanks to both of you!
…On Wed, May 5, 2021, 08:14 Luka Kovačič ***@***.***> wrote:
@jimklimov <https://github.com/jimklimov> I will take a look and try this
on the UPS during this weekend.
I'll get back to you.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#807 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMPTFF7BHIPCKRRM6HBW7LTMDO2RANCNFSM4PAJ4YIQ>
.
|
|
Hello @jimklimov, I've tested this today on my machine. I've attached some logs below. On mains: Discharging: I have also tested the following commands: 0.005233 [D2] Checking device (05DD/041B) (001/010) I won't be able to do any further testing now, as the machine is in production. |
|
Super, thanks! |
6 participants
Hello,
This commit adds initial support for the Delta RT series UPS devices (Single Phase, 1/2/3 kVA).
A new Delta HID subdriver has been added, the driver was tested and tweaked on the Delta RT 3 kVA UPS.
Some features, like shutting down the UPS, will be added gradually over use and more testing.
Basic UPS monitoring is supported for now.