Added support for APC Galaxy 5500 UPS #2239

mateusz-jastrzebski · 2023-12-20T14:59:46Z

I am working with (APC) MGE Galaxy 5500 UPS and the default way of grabbing ups.status is different from other APC drivers sadly (talking about drivers/apc-mib.c). OID that works: 1.3.6.1.4.1.318.1.1.1.11.1.1

Easiest way I found out to make my ups.status work (as far as OL OB go and also not block other APC/snmp-ups devices's functionality) was to make a custom snmp-ups driver with special treatment; If I have used custom functions for it, I'd still need to modify macro (and compile whole project) that allows custom lookup functions to work but I might be wrong
The new driver is a modified clone of the apc-mib.c
It is in usable state; should get the job done for turning off devices.

Feel free to leave some comments - I am still learning coding.

Signed-off-by: mjastrzebski <jasmati09@gmail.com>

jimklimov · 2023-12-21T17:29:10Z

Hello, thanks for this! Just in case - we have many apc-something subdrivers; did you check that none of them fits your device?

If not, and the problem is about just a few mapping-table data points, it may work to just add them into an existing subdriver (not sure OTOH if the first or last initially probed working match would be used in fact). They are flexible enough to support vendor variant MIBs - so you can see a few duplicate names with different OIDs already.

mateusz-jastrzebski · 2023-12-29T12:24:18Z

I tried grepping for my status OID in other drivers/subdrivers lookup files with no result.
Well still I'll try to check if other APC subdrivers work but in the meanwhile I'll continue working on patching my code for PR.
See You in 2024!

jimklimov · 2024-01-03T09:16:08Z

From CI:

In file included from snmp-ups.c:45:
./apcgalaxy5500-mib.h:10:33: error: no newline at end of file [-Werror,-Wnewline-eof]
#endif /* APCGALAXY5500_MIB_H */
                                ^
snmp-ups.c:3512:9: error: mixing declarations and code is incompatible with standards before C99 [-Werror,-Wdeclaration-after-statement]
                        char pwr_status = buf[1];
                             ^
2 errors generated.
make[1]: *** [Makefile:2389: snmp_ups-snmp-ups.o] Error 1
In file included from apcgalaxy5500-mib.c:30:
./apcgalaxy5500-mib.h:10:33: error: no newline at end of file [-Werror,-Wnewline-eof]
#endif /* APCGALAXY5500_MIB_H */
                                ^
apcgalaxy5500-mib.c:303:3: error: no newline at end of file [-Werror,-Wnewline-eof]
*/
  ^
2 errors generated.

Signed-off-by: mjastrzebski <120177661+mateusz-jastrzebski@users.noreply.github.com>

jimklimov · 2024-01-11T13:01:34Z

drivers/snmp-ups.c

-			su_status_set(su_info_p, value);
-			upsdebugx(2, "=> value: %ld", value);
+			status = nut_snmp_get_str(su_info_p->OID, buf, sizeof(buf), su_info_p->oid2info);
+			char pwr_status;


This looks error-prone: strtol() deals with a (nul-terminated) string, but your copy of the first character from buf is not necessarily followed in RAM by a zero byte. Any random garbage can pop up here...

Trying to make sense of that MIB/OID with http://oidref.com/1.3.6.1.4.1.318.1.1.1.11.1.1.0 I see that it is a child of http://oidref.com/1.3.6.1.4.1.318.1.1.1.11.1.1 but otherwise got no clue of its own value. That parent however is a string representing 64 bit-flags (or UNKNOWN which you don't check for), and the second byte (your buf[1]?) would be the "On Battery" flag. There is a separate flag for "On Line" as well as bypass, self-test, and a united "Low Battery / On Battery" flag, so it may be prudent to check several values for this reading.

Also wondering if all this complex logic should be here or can be better served by "lookup functions" in the *-mib.c file (the later, generally NULL, fields of lookup tables like APCC_G5500_pwr_info[])?.. See e.g. drivers/eaton-pdu-marlin-mib.c for some practical examples.

I'll try to learn about strtol() and its functioning, thank you.

As far as I know, those lookup functions only work when (to put it simply)
the macro `WITH_SNMP_LKP_FUN' is set to 1

drivers/snmp-ups.h 137:142

# if (defined WITH_DMFMIB) && (WITH_DMFMIB != 0) # define WITH_SNMP_LKP_FUN 0 # else # define WITH_SNMP_LKP_FUN 1 # endif #endif

Does it mean that in best scenario I will still have to use a self-built version of NUT for this particular UPS model?
Or I might be missing something

Yes, I acknowledge the fact that ups.status is more than OL or OB, things would get easier with lookup functions, I understand

And umm I have put child OID (not parent) there because that's what actually works when using something like snmpget
The parent OID does not work

snmpget -v1 -c public 'redacted_ip' 1.3.6.1.4.1.318.1.1.1.11.1.1

Error in packet Reason: (noSuchName) There is no such variable name in this MIB. Failed object: iso.3.6.1.4.1.318.1.1.1.11.1.1

jimklimov · 2024-01-11T13:01:59Z

drivers/snmp-ups.c

+		if(!(strcasecmp(su_info_p->OID, ".1.3.6.1.4.1.318.1.1.1.11.1.1.0")))
 		{
-			su_status_set(su_info_p, value);
-			upsdebugx(2, "=> value: %ld", value);
+			status = nut_snmp_get_str(su_info_p->OID, buf, sizeof(buf), su_info_p->oid2info);
+			char pwr_status;
+			pwr_status = buf[1];
+			su_status_set(su_info_p, strtol(&pwr_status, NULL, 10));
+			if (status == TRUE)
+				upsdebugx(2, "=> value: %c", pwr_status);
+			else
+				upsdebugx(2, "=> Failed");
 		}
 		else
-			upsdebugx(2, "=> Failed");
-
-		free_info(tmp_info_p);
+		{
+			status = nut_snmp_get_int(su_info_p->OID, &value);
+			su_status_set(su_info_p, value);
+			if (status == TRUE)
+					upsdebugx(2, "=> value: %ld", value);
+			else
+					upsdebugx(2, "=> Failed");
+		}


Why did this block change so drastically - a lot more than indenting generally, losing the debug log and now calling su_status_set() regardless of a successful status to fetch the value, for example? Did a local experiment seep out here?

Alright I agree I did mess this up

jimklimov · 2024-01-11T13:03:02Z

drivers/apcgalaxy5500-mib.c

+    { 7, "smallMomentarySpike", NULL, NULL },
+    { 8, "largeMomentarySpike", NULL, NULL },
+    { 9, "selfTest", NULL, NULL },
+    { 10, "rateOfVoltageChange", NULL, NULL }


Here and above, lookup tables must end with a { 0, NULL, NULL, NULL } entry to stop iterations.

Didn't think of that, this code snippet is copied from drivers/apc-mib.c

jimklimov · 2024-01-11T13:05:36Z

drivers/apcgalaxy5500-mib.c

+	{ "ups.status", ST_FLAG_STRING, SU_INFOSIZE, APCC_G5500_OID_POWER_STATUS, "OFF",
+		SU_FLAG_OK | SU_STATUS_PWR, APCC_G5500_pwr_info },
+	{ "ups.status", ST_FLAG_STRING, SU_INFOSIZE, APCC_G5500_OID_BATT_STATUS, "",
+		SU_FLAG_OK | SU_STATUS_BATT, APCC_G5500_batt_info },
+	{ "ups.status", ST_FLAG_STRING, SU_INFOSIZE, APCC_G5500_OID_CAL_RESULTS, "",
+		SU_FLAG_OK | SU_STATUS_CAL, APCC_G5500_cal_info },
+	{ "ups.status", ST_FLAG_STRING, SU_INFOSIZE, APCC_G5500_OID_NEEDREPLBATT, "",
+		SU_FLAG_OK | SU_STATUS_RB, APCC_G5500_battrepl_info },


Aren't these all different statuses? Check with docs/nut-names.txt for suitable strings for the first column.

Also, I think the first hit (query that gets a response from a particular device for the reading name) wins, so the later lines would be effectively ignored at run time.

jimklimov · 2024-01-11T13:08:33Z

Did a bit of cursory review, for some things that stung my eye :)

Also, as you might have noticed by now, it is much less cumbersome to issue PRs from a dedicated git branch (avoid using your copy of master or any other branches named by upstream projects). Especially makes it easier to collaborate, e.g. when maintainer like me may push typo fixes to your unprotected branch that is a source of PR, and specifically to sync changes from the shared upstream codebase back into yours if the PR stays open for a non-trivial length of time.

aquette · 2024-01-14T13:09:39Z

Have you checked the subpart (after .318) do not match mge-mib? These Galaxy may just the rebadged MGE.

jimklimov · 2024-05-22T23:58:12Z

Odd, notifications here show a commit with nuked README.adoc... why?

mateusz-jastrzebski · 2024-05-23T00:24:19Z

Umm, I just wanted to have a clean readme for my fork, I acknowledge the fact that this PR's not going to be merged anytime soon. I have also just found out the importance of merging a non default branch from forked repo..
I think I'm going to just close this PR "for now" as I physically don't have time and patience for fixing my code.
If there will be next time, I'll make sure I do it with best manners, sorry!

jimklimov · 2024-05-23T07:17:44Z

Ah, I see. Just was worried someone stole your account to spam, or something.

Added support for APC Galaxy 5500 UPS

73b1c2e

Signed-off-by: mjastrzebski <jasmati09@gmail.com>

mateusz-jastrzebski force-pushed the master branch from 855951b to 73b1c2e Compare December 20, 2023 15:00

jimklimov added APC SNMP needs-work PR discussion concluded that some work is needed from the contributor labels Jan 2, 2024

Merge branch 'networkupstools:master' into master

7bbdbdc

Fixing apcG5500 SNMP driver for PR networkupstools#2239

ab76f3f

Signed-off-by: mjastrzebski <120177661+mateusz-jastrzebski@users.noreply.github.com>

mateusz-jastrzebski force-pushed the master branch from 449214f to ab76f3f Compare January 10, 2024 15:16

jimklimov requested changes Jan 11, 2024

View reviewed changes

jimklimov marked this pull request as draft January 14, 2024 18:36

jimklimov mentioned this pull request Jan 15, 2024

drivers/apc-mib.c: update MIB values #2264

Merged

jimklimov marked this pull request as ready for review February 27, 2024 09:06

Merge branch 'master' into master

81d6514

jimklimov marked this pull request as draft February 27, 2024 13:27

Update README.adoc

b37f71d

mateusz-jastrzebski closed this May 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for APC Galaxy 5500 UPS #2239

Added support for APC Galaxy 5500 UPS #2239

mateusz-jastrzebski commented Dec 20, 2023

jimklimov commented Dec 21, 2023

mateusz-jastrzebski commented Dec 29, 2023

jimklimov commented Jan 3, 2024

jimklimov Jan 11, 2024

mateusz-jastrzebski Jan 11, 2024

mateusz-jastrzebski Jan 11, 2024

jimklimov Jan 11, 2024

mateusz-jastrzebski Jan 11, 2024

jimklimov Jan 11, 2024

mateusz-jastrzebski Jan 11, 2024

jimklimov Jan 11, 2024

jimklimov commented Jan 11, 2024

aquette commented Jan 14, 2024

jimklimov commented May 22, 2024

mateusz-jastrzebski commented May 23, 2024

jimklimov commented May 23, 2024

Added support for APC Galaxy 5500 UPS #2239

Added support for APC Galaxy 5500 UPS #2239

Conversation

mateusz-jastrzebski commented Dec 20, 2023

jimklimov commented Dec 21, 2023

mateusz-jastrzebski commented Dec 29, 2023

jimklimov commented Jan 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimklimov commented Jan 11, 2024

aquette commented Jan 14, 2024

jimklimov commented May 22, 2024

mateusz-jastrzebski commented May 23, 2024

jimklimov commented May 23, 2024