Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for Liebert GXE 1-3KVA series #2629

Merged
merged 8 commits into from
Sep 23, 2024

Conversation

goodspeed34
Copy link
Contributor

@goodspeed34 goodspeed34 commented Sep 14, 2024

Product Breif: https://www.vertiv.com/4a3d25/globalassets/products/critical-power/uninterruptible-power-supplies-ups/vertiv-liebert-gxe-1000-3000va-racktower-230v-single-phase-ups/vertiv-liebert-gxe-datasheet-en-emea---mka4l0ukvlg.pdf

This UPS has a RS232 port and a USB port. The USB port has an ACM interface which functions as a serial port. Both ports can be used managing the device.

General points

  • Described the changes in the PR submission or a separate issue, e.g.
    known published or discovered protocols, applicable hardware (expected
    compatible and actually tested/developed against), limitations, etc.

  • There may be multiple commits in the PR, aligned and commented with
    a functional change. Notably, coding style changes better belong in a
    separate PR, but certainly in a dedicated commit to simplify reviews
    of "real" changes in the other commits. Similarly for typo fixes in
    comments or text documents.

  • Please star NUT on GitHub, this helps with sponsorships! ;)

Frequent "underwater rocks" for driver addition/update PRs

  • Revised existing driver families and added a sub-driver if applicable
    (nutdrv_qx, usbhid-ups...) or added a brand new driver in the other
    case.

  • Did not extend obsoleted drivers with new hardware support features
    (notably blazer and other single-device family drivers for Qx protocols,
    except the new nutdrv_qx which should cover them all).

  • For updated existing device drivers, bumped the DRIVER_VERSION macro
    or its equivalent.

  • For USB devices (HID or not), revised that the driver uses unique
    VID/PID combinations, or raised discussions when this is not the case
    (several vendors do use same interface chips for unrelated protocols).

  • For new USB devices, built and committed the changes for the
    scripts/upower/95-upower-hid.hwdb file

  • Proposed NUT data mapping is aligned with existing docs/nut-names.txt
    file. If the device exposes useful data points not listed in the file, the
    experimental.* namespace can be used as documented there, and discussion
    should be raised on the NUT Developers mailing list to standardize the new
    concept.

  • Updated data/driver.list.in if applicable (new tested device info)

Frequent "underwater rocks" for general C code PRs

  • Did not "blindly assume" default integer type sizes and value ranges,
    structure layout and alignment in memory, endianness (layout of bytes and
    bits in memory for multi-byte numeric types), or use of generic int where
    language or libraries dictate the use of size_t (or ssize_t sometimes).
  • Progress and errors are handled with upsdebugx(), upslogx(),
    fatalx() and related methods, not with direct printf() or exit().
    Similarly, NUT helpers are used for error-checked memory allocation and
    string operations (except where customized error handling is needed,
    such as unlocking device ports, etc.)

  • Coding style (including whitespace for indentations) follows precedent
    in the code of the file, and examples/guide in docs/developers.txt file.

  • For newly added files, the Makefile.am recipes were updated and the
    make distcheck target passes.

General documentation updates

  • Updated docs/acknowledgements.txt (for vendor-backed device support)

  • Added or updated manual page information in docs/man/*.txt files
    and corresponding recipe lists in docs/man/Makefile.am for new pages

  • Passed make spellcheck, updated spell-checking dictionary in the
    docs/nut.dict file if needed (did not remove any words -- the make
    rule printout in case of changes suggests how to maintain it).

Additional work may be needed after posting this PR

  • Propose a PR for NUT DDL with detailed device data dumps from tests
    against real hardware (the more models, the better).

  • Address NUT CI farm build failures for the PR: testing on numerous
    platforms and toolkits can expose issues not seen on just one system.

  • Revise suggestions from LGTM.COM analysis about "new issues" with
    the changed codebase.

@AppVeyorBot
Copy link

Copy link
Member

@jimklimov jimklimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, a very solid first entry, paint me impressed :)

I've posted some review comments, perhaps CI build agents' warnings and errors would add more.

Generally, a NEWS.adoc entry would be welcome, and a man page required. Here I would welcome the man page to also have a description or examples how to use both USB and serial connections, given that the driver does not have USB-specific code (does the UPS present itself as just a /dev/ttyUSB0 sort of serial port in the OS?)

Maybe mention which USB vendor/product IDs can be seen on the device, are they specific to the UPS model, or generic for USB-Serial chips (FTDI etc.)?

Also, is this an "individual" contribution or do you work for that company (who is it now - Liebert, Vertiv, Epson...)? Especially if it is their official/sponsored driver, a note in acknowledgements.txt may be worthwhile.

Finally, an entry or two (serial and USB) in data/driver.list.in, including a suitable value for the "support level" column, would be great.

drivers/Makefile.am Outdated Show resolved Hide resolved
drivers/gxe.c Outdated Show resolved Hide resolved
drivers/gxe.c Outdated Show resolved Hide resolved
drivers/gxe.c Outdated Show resolved Hide resolved
drivers/gxe.c Outdated Show resolved Hide resolved
drivers/gxe.c Outdated Show resolved Hide resolved
drivers/gxe.c Outdated Show resolved Hide resolved
drivers/gxe.c Outdated Show resolved Hide resolved
drivers/gxe.c Outdated Show resolved Hide resolved
drivers/gxe.c Outdated Show resolved Hide resolved
@jimklimov
Copy link
Member

jimklimov commented Sep 14, 2024

Also, to set up the local build with fatal warnings among other things, you can run ./ci_build.sh and then iterate with make to gobble up error fixes as you edit the sources until no warnings are left.

If you can use a container or VM with as new a clang and/or gcc as you can (e.g. using a bleeding/experimental packaged OS distro flavour), that can help about newest static analyzer features.

@goodspeed34
Copy link
Contributor Author

goodspeed34 commented Sep 14, 2024

@jimklimov Thank you for the swift review. This PR is still work in progress. It looks like for now the endian function is not portable and gcc complains snprintf sometimes. I will try to split the checksum and frame functions into an individual header. It looks like that belongs to a protocol heavily used by Vertiv called YDN23 but without an available standard.

Also the UPS's eco mode can be toggled from serial line. But, I can't find a specific instcmd or var for it in NUT. Can you give a pointer for that?

@goodspeed34
Copy link
Contributor Author

Thanks, a very solid first entry, paint me impressed :)

I've posted some review comments, perhaps CI build agents' warnings and errors would add more.

Generally, a NEWS.adoc entry would be welcome, and a man page required. Here I would welcome the man page to also have a description or examples how to use both USB and serial connections, given that the driver does not have USB-specific code (does the UPS present itself as just a /dev/ttyUSB0 sort of serial port in the OS?)

It's a /dev/ttyACM device.

Maybe mention which USB vendor/product IDs can be seen on the device, are they specific to the UPS model, or generic for USB-Serial chips (FTDI etc.)?

Sure, I will check that.

Also, is this an "individual" contribution or do you work for that company (who is it now - Liebert, Vertiv, Epson...)? Especially if it is their official/sponsored driver, a note in acknowledgements.txt may be worthwhile.

It's individual I think. I asked the company for the driver to RE but they said they don't have one and gave me a protocol manual.

Finally, an entry or two (serial and USB) in data/driver.list.in, including a suitable value for the "support level" column, would be great.

OK, I will do the checklist when ready.

@jimklimov
Copy link
Member

ECO mode is part of some other discussions slowly progressing in different threads. Currently NUT naming space has had no concept for that, so it is worth introducing in some consistent manner so it becomes a sort of de-facto standard that best suits everyone.

Note the definition of "ECO mode" is also vague and may vary between vendors and models. Seems like a marketing buzzword, and may possibly mean an ability of an UPS to switch between what was known as "on-line" (always dual transformation with a battery in the middle) vs. "line-interactive" (glorified power strip, ~10ms gap in an outage, only then battery-fed load) modes at will, at least on some devices.

@jimklimov
Copy link
Member

Also, is this an "individual" contribution or do you work for that company (who is it now - Liebert, Vertiv, Epson...)? Especially if it is their official/sponsored driver, a note in acknowledgements.txt may be worthwhile.

It's individual I think. I asked the company for the driver to RE but they said they don't have one and gave me a protocol manual.

Sounds decent. Can you please ask if a copy of the protocol file can be published e.g. at https://networkupstools.org/ups-protocols.html ?

@jimklimov
Copy link
Member

jimklimov commented Sep 14, 2024

Regarding endianness, check git grep -i endian in the sources - some drivers (and configure-time support => macros) have precedents about dealing with that already.

UPDATE: Although that git grep had fewer hits than I remembered. Probably more code deals with it without matching comments...

UPDATE2: drivers/bicker_ser.c is a recent addition to NUT that did deal with this, so is what was on my mind, particularly work in commit c9ba796 .

@goodspeed34
Copy link
Contributor Author

Also, is this an "individual" contribution or do you work for that company (who is it now - Liebert, Vertiv, Epson...)? Especially if it is their official/sponsored driver, a note in acknowledgements.txt may be worthwhile.

It's individual I think. I asked the company for the driver to RE but they said they don't have one and gave me a protocol manual.

Sounds decent. Can you please ask if a copy of the protocol file can be published e.g. at https://networkupstools.org/ups-protocols.html ?

Maybe not... There is a confidentiality notice in the message and it's all in Chinese.

@jimklimov
Copy link
Member

Maybe not... There is a confidentiality notice in the message and it's all in Chinese.

Well, I am not sure what the social conventions are then... would it hurt to ask if they have a publishable variant (and perhaps in English)?

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@goodspeed34
Copy link
Contributor Author

The corresponding protocol is attached. Sadly, I can't find an English one.

YDT1363.3-2014通信局(站)电源、空调及环境集中监控管理系统第3部分:前端智能设备协议.pdf

Maybe not... There is a confidentiality notice in the message and it's all in Chinese.

Well, I am not sure what the social conventions are then... would it hurt to ask if they have a publishable variant (and perhaps in English)?

@goodspeed34
Copy link
Contributor Author

goodspeed34 commented Sep 16, 2024

poll_interval needs (a little) revision. Finished and squashed together.

drivers/liebert-gxe.c Outdated Show resolved Hide resolved
@goodspeed34 goodspeed34 changed the title WIP: Support for Liebert GXE 1-3KVA series Support for Liebert GXE 1-3KVA series Sep 21, 2024
@goodspeed34
Copy link
Contributor Author

It seems CI bugged out from the forced-push...

…d Liebert GXE Series driver

Signed-off-by: Gong Zhile <[email protected]>
Compiler wants both to handle `default` cases in `switch` lists,
and dislikes a `default` label in enum handling when all known
values are handled (and wants them all to be handled). The clash
is only resolvable by pragmas, like elsewhere in out codebase.

Signed-off-by: Jim Klimov <[email protected]>
Copy link
Member

@jimklimov jimklimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super! Thanks!

@jimklimov
Copy link
Member

@goodspeed34 : By the way, one more question: since the beginning of time, there already was a drivers/liebert.c with claimed support for "Liebert UPStation GXE" "with MultiLink cable". Out of curiosity - did you check if it worked for your device?

I do see that the old driver is very basic and less functional than yours; just wondering if it can be treated as obsolete in favor of new development, or if they cover different grounds (e.g. cables/protocols) and should both exist?

@goodspeed34
Copy link
Contributor Author

goodspeed34 commented Sep 23, 2024

@goodspeed34 : By the way, one more question: since the beginning of time, there already was a drivers/liebert.c with claimed support for "Liebert UPStation GXE" "with MultiLink cable". Out of curiosity - did you check if it worked for your device?
I do see that the old driver is very basic and less functional than yours; just wondering if it can be treated as obsolete in favor of new development, or if they cover different grounds (e.g. cables/protocols) and should both exist?

It doesn't work and shows always OL state. (This UPS model only recognize the data frame in ydn23.h, there won't be any responses if data gets sent in liebert.c way. It looks like that driver lacks a probing method and report false positively) I think they are for different models and I can't tell if it's obsolete or not.

@jimklimov
Copy link
Member

Thanks for clarification, much appreciated!

@jimklimov
Copy link
Member

Hm, did I miss a data dump as an example of the new driver's work in the comments above, or could you kindly post one? or a PR to nut-ddl repo right away :)

@goodspeed34
Copy link
Contributor Author

goodspeed34 commented Sep 23, 2024

I don't have my laptop with me so I'm afraid I can't raise a PR. Here is the dump for Liebert GXE 01k00TS1101C00:

goodspeed@radiata:~/nut/tools$ cat EmersonNetworkPower__HE1T0010__liebert-gxe__2.8.2.1__01.dev
# Please add if relevant: DEVICE:URL:REPORT: <GitHub issue/PR or Mailing list archive entry URL>
# Please add if relevant: DEVICE:URL:VENDOR: <Vendor/Manufacturer/Product page URL>

# Please add comments for humans here and perhaps raise concerns about the data points,
# see https://networkupstools.org/ddl/#devseq-files for comment-tag syntax
# strip sensitive data (passwords, SNMP community, serial number, IP address, host name...)
# and post as a pull request to https://github.com/networkupstools/nut-ddl/
# DEVICE:COMMENT:
# Device dump generated by ./nut-ddl-dump.sh on Tue, 24 Sep 2024 01:06:15 +0800
# DEVICE:EOC

# :; upsc emerson
battery.charger.status: charging
battery.runtime.low: 55.80
battery.voltage: 40.85
device.mfr: EmersonNetworkPower
device.model: HE1T0010
device.type: ups
driver.debug: 0
driver.flag.allow_killpower: 0
driver.name: liebert-gxe
driver.parameter.pollinterval: 5
driver.parameter.port: /dev/ttyACM0
driver.parameter.synchronous: auto
driver.state: quiet
driver.version: 2.8.2.1
driver.version.internal: 0.01
input.frequency: 50.02
input.transfer.bypass.high: 253.000000
input.transfer.bypass.low: 120
input.voltage: 223.00
output.current: 0.62
output.frequency: 49.99
output.frequency.nominal: 50
output.voltage: 224.94
output.voltage.nominal: 220
ups.id: 01
ups.mfr: EmersonNetworkPower
ups.model: HE1T0010
ups.power: 140
ups.realpower: 120
ups.status: OL BYPASS
ups.test.interval: 0
ups.test.result: Idle

# DEVICE:COMMENT-BLOCK:FIXME:UPSRW: emerson
# [driver.debug]
# Description unavailable
# Type: NUMBER
# Value: 0
#
# [driver.flag.allow_killpower]
# Description unavailable
# Type: NUMBER
# Value: 0
# DEVICE:EOC

# DEVICE:COMMENT-BLOCK:FIXME:UPSCMD: emerson
# Instant commands supported on UPS [emerson]:
#
# driver.killpower - Description unavailable
# driver.reload - Description unavailable
# driver.reload-or-error - Description unavailable
# driver.reload-or-exit - Description unavailable
# load.off - Turn off the load immediately
# load.on - Turn on the load immediately
# test.battery.start - Start a battery test
# test.battery.stop - Stop the battery test
# DEVICE:EOC

There are mismatches here since the driver reports the actual manufacture EmersonNetworkPower and model HE1T0010 (from the device).

Hm, did I miss a data dump as an example of the new driver's work in the comments above, or could you kindly post one? or a PR to nut-ddl repo right away :)

@jimklimov
Copy link
Member

Thanks!

@jimklimov jimklimov merged commit 0f36765 into networkupstools:master Sep 23, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

3 participants