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

add IMPULSE SNMP-card for 1-10kVA UPS in NUT #2641

Conversation

seregega
Copy link

@seregega seregega commented Sep 26, 2024

Card name: IMPULSE SNMP-card for 1-10kVA UPS
Prod. code: CNASNMP1
FW version: 1110.5.200.1.10.00

add: impuls_magnetic in snmp-ups list
fix: upsload oid

Signed-off-by: Sergei Kartavtsev [email protected]

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.

Card name: IMPULSE SNMP-card for 1-10kVA UPS
Prod. code: CNASNMP1
FW version: 1110.5.200.1.10.00

add: impuls_magnetic in snmp-ups list
fix: upsload oid
Signed-off-by: sergega <[email protected]>
Signed-off-by: Sergei Kartavtsev <[email protected]>
Signed-off-by: sergega <[email protected]>
@seregega seregega force-pushed the IMPULSE-SNMP-card-for-1-10kVA-UPS branch from 775bf17 to 9ad4f63 Compare September 26, 2024 10:12
@jimklimov jimklimov added this to the 2.8.3 milestone Sep 26, 2024
@jimklimov
Copy link
Member

  • Why "impuls" in the file names and not "impulse"?
  • Can you please add a man page, perhaps pointing to vendor and their MIB if published?
  • If this is a vendor-backed contribution, feel free to add the company info into docs/acknowledgements.txt


#include "impuls-magnetic-mib.h"

#define IMPULS_MAGNETIC_MIB_VERSION "1.0"
Copy link
Member

Choose a reason for hiding this comment

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

2-digit minor version, e.g. 1.00 please

Comment on lines +3 to +4
* Copyright (C) 2002-2006
* 2012-2024 Sergei Kartavtsev <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

  • I think the first set of years (2002-2006) with no name can go away
  • Your own (C) is likely just 2024?
  • Please align indentation with other lines

@@ -0,0 +1,281 @@
/* impuls_magnetic_mib.c - (ietf-mib.c + fix for upsload for IMPULSE SNMP-card for 1-10kVA UPS) - data to monitor SNMP UPS (RFC 1628 compliant) with NUT
Copy link
Member

Choose a reason for hiding this comment

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

Please wrap into 2-3 lines to fit in ~76 char columns

* 2002-2006 Niels Baggesen <[email protected]>
* 2002-2006 Arjen de Korte <[email protected]>
*
* Sponsored by MGE UPS SYSTEMS <http://www.mgeups.com>
Copy link
Member

Choose a reason for hiding this comment

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

This specific driver was not sponsored by MGE :) Was it by the makers of Impuls(e)?

Comment on lines +274 to +280
/* FIXME: Rename the structure here (or even relocate to new file)
* and in snmp-ups.c when the real TrippLite mappings get defined. */
/* FIXME: Duplicate the line below to fix an issue with the code generator (nut-snmpinfo.py -> line is discarding) */
/*mib2nut_info_t tripplite_ietf = { "tripplite", IETF_MIB_VERSION, NULL, NULL, ietf_mib, TRIPPLITE_SYSOID, NULL };*/

/* FIXME: Duplicate the line below to fix an issue with the code generator (nut-snmpinfo.py -> line is discarding) */
/*mib2nut_info_t ietf = { "ietf", IETF_MIB_VERSION, IETF_OID_UPS_MIB "4.1.0", IETF_OID_UPS_MIB "1.1.0", ietf_mib, IETF_SYSOID, NULL };*/
Copy link
Member

Choose a reason for hiding this comment

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

Can remove the commented-away lines originating from ietf-mib

@@ -133,6 +134,7 @@ static mib2nut_info_t *mib2nut[] = {
*/
&tripplite_ietf, /* This struct comes from : ietf-mib.c */
&ietf, /* This struct comes from : ietf-mib.c */
&impuls_magnetic, /* This struct comes from : impuls_magnetic.c for fix upsload oid and works only manualy selected*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
&impuls_magnetic, /* This struct comes from : impuls_magnetic.c for fix upsload oid and works only manualy selected*/
&impuls_magnetic, /* This struct comes from : impuls_magnetic.c for fixed upsload oid; based on ietf MIB and works only when manually selected*/


/* SNMP OIDs set */
#define IETF_OID_UPS_MIB "1.3.6.1.2.1.33.1."
#define IETF_SYSOID ".1.3.6.1.2.1.33"
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific SYSOID for Impulse MIB, so we could auto-detect that the SNMP server is this device (and so it could be listed at higher priority than &ietf in snmp-ups.c file, and not require "manual configuration" to be used).

@jimklimov
Copy link
Member

jimklimov commented Oct 16, 2024

@seregega : gentle bump, there are some questions and suggestions still raised for your PR...

@seregega seregega closed this Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants