Updated Phoenix Contact modbus driver for newer models by RikyPlaza · Pull Request #2689 · networkupstools/nut

RikyPlaza · 2024-11-22T16:19:42Z

Updated Phoenix Contact modbus driver for newer models which are using different modbus addresses. Choice is made basing on the UPS firmware version and the model information have been changed in order to have a quick feedback over the new/old model detection.

…es. Choice is made basing on firmware version. Signed-off-by: RikyPlaza <rikyrod2001@gmail.com>

jimklimov

Thanks for a solid-looking enhancement. A NEWS.adoc entry would be good, beside addressing the specific comments I mentioned during a look at code.

jimklimov · 2024-11-22T20:18:15Z

 		fatalx(EXIT_FAILURE, "Invalid modbus slave ID %d",MODBUS_SLAVE_ID);
 	}
-
+	


Here and several times above - needless added whitespace

jimklimov · 2024-11-22T20:19:25Z

-	if (CHECK_BIT(tab_reg[0], 16))
-		alarm_set("Low Battery (Service)");
+
+	uint32_t alarms_word;


Here and above, variable definitions are added in the middle of scope. NUT coding style requires all definitions to be before code, so older systems can build current NUT too. Probably that is what CI failed about all over the place.

jimklimov · 2024-11-22T20:21:28Z

+
+		mrir(modbus_ctx, 0x2000, 1, &actual_code_functions);
+
+		tab_reg[0] = CHECK_BIT(actual_code_functions, 2); //battery mode is the 2nd bit of the register 0x2000


Please use block comment style /* ... */

jimklimov · 2024-11-22T20:23:46Z

+		break;
+	case OLD_UPS:
+
+		mrir(modbus_ctx, 29697, 3, tab_reg); /* LB is actually called "shutdown event" on this ups */


Any idea if these numbers make more sense in hex? Are they perhaps bit-masks of some command combos?

The only reason why i kept the old addresses in dec and put the new ones in hex is the way they are reported in the UPSes datasheet. If you want to use a standard for coding i can change all addresses in the same way, but for maintenence in the furure it will be necessary to convery all numbers.

No, got no particular preference here. If maintenance would be easier with decimal numbers - so be it. Maybe worth a short comment at start of the file though.

jimklimov · 2024-11-22T20:25:17Z

 #define MODBUS_SLAVE_ID 192

+#define OLD_UPS 0
+#define NEW_UPS 1


I wonder how soon "old" vs. "new" would become obsolete? Maybe more symbolic names would be better (just a thought)?

You're right, i've put the models. To be honest there would be another model (TRIO) but i don't have the hardware to test the communication. As soon as it will arrive i can test the driver and see if changes are needed.

Another options can be an enum to also constrain the values accepted at compile-time (and get warnings like that a switch does not look at all newly defined options).

jimklimov · 2024-11-22T20:27:08Z

+
+	uint16_t FWVersion;
+
+	mrir(modbus_ctx, 0x0004, 1, &FWVersion);


Makes sense to also populate ups.firmware

I agree, not sure if the variable should be defined somewhere else or i can just call the dstate_setinfo function.

Well, the global variable makes sense if this info would be used elsewhere (e.g. instead of those OLD_UPS/NEW_UPS defines?)

Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

RikyPlaza · 2024-11-23T11:54:20Z

@jimklimov Hello, regarding the NEWS.adoc, could you give me some references please? I've looked in other pull requests but i can't find any example. Thank you!

AppVeyorBot · 2024-11-23T12:18:37Z

❌ Build nut 2.8.2.2394-master failed (commit bc3ad182ba by @RikyPlaza)

…cted alarm trigger (wrong bit used) Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

jimklimov · 2024-11-25T08:56:45Z

@jimklimov Hello, regarding the NEWS.adoc, could you give me some references please? I've looked in other pull requests but i can't find any example. Thank you!

Probably just add a paragraph (bullet point) similar to ones that are already there.

Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

AppVeyorBot · 2024-11-25T13:37:29Z

❌ Build nut 2.8.2.2400-master failed (commit 81417050d0 by @RikyPlaza)

Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

RikyPlaza

Files reviewed.

AppVeyorBot · 2024-11-25T15:10:04Z

✅ Build nut 2.8.2.2401-master completed (commit 82692db556 by @RikyPlaza)

Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

jimklimov · 2024-11-26T07:55:24Z

FYI: If the lack of default label would also prove a problem with some other compilers, check other drivers for use of pragmas around that. Typically we end up with a monster like:

#if (defined HAVE_PRAGMA_GCC_DIAGNOSTIC_PUSH_POP) && ( (defined HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_COVERED_SWITCH_DEFAULT) || (defined HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_UNREACHABLE_CODE) )
# pragma GCC diagnostic push
#endif
#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_COVERED_SWITCH_DEFAULT
# pragma GCC diagnostic ignored "-Wcovered-switch-default"
#endif
#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_UNREACHABLE_CODE
# pragma GCC diagnostic ignored "-Wunreachable-code"
#endif
/* Older CLANG (e.g. clang-3.4) seems to not support the GCC pragmas above */
#ifdef __clang__
# pragma clang diagnostic push
# pragma clang diagnostic ignored "-Wunreachable-code"
# pragma clang diagnostic ignored "-Wcovered-switch-default"
#endif
                /* All enum cases defined as of the time of coding
                 * have been covered above. Handle later definitions,
                 * memory corruptions and buggy inputs below...
                 */
                default:
                        fatalx(EXIT_FAILURE, "no suitable definition found!");
#ifdef __clang__
# pragma clang diagnostic pop
#endif
#if (defined HAVE_PRAGMA_GCC_DIAGNOSTIC_PUSH_POP) && ( (defined HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_COVERED_SWITCH_DEFAULT) || (defined HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_UNREACHABLE_CODE) )
# pragma GCC diagnostic pop
#endif

jimklimov · 2024-11-26T11:27:10Z

Also remembered: a man page would be needed to describe the supported devices and the driver options (per addvar() in code) and maybe other nuances - known caveats, etc. Ideally this comes from a driver author, as they best know if there is anything special to know about the devices.

RikyPlaza · 2024-11-26T20:42:14Z

the lack of default label would also prove a problem with some other compilers, check other drivers for use of pragmas around that. Typically we end up with a monster like:

@jimklimov I'm not sure if the checks are running, for now i don't see any error so i would let the check finish. Do you know how much time do they ususally take?

RikyPlaza · 2024-11-26T20:44:29Z

embered: a man page would be needed to describe the supported devices and the driver options (per addvar() in code) and maybe other nuances - known caveats, etc. Ideally this comes from a driver author, as they best know if there is anything special to know about the devices.

@jimklimov Ok, as soon as the checks are ok i'll take a look to the man page. Should i modify something already present or should i make a new man page?

jimklimov · 2024-11-26T23:01:44Z

New driver - new man page :)

As for checks, it's about 3-5hrs for all scenarios to run; overall time depends on how maby other PR iterations are queued...

RikyPlaza · 2024-11-27T16:26:32Z

@jimklimov Hi, now i see that the there are errors regarding the missing default label in switch statements, but i had to remove them in commit f19c8e0 since the checks were also failing. I really don't understand what it the issue...
In other drivers (blazer_usb.c, clone-outlet.c ecc) i see that the default label is preset.

jimklimov · 2024-11-27T16:51:18Z

@RikyPlaza : did you try the construct with pragmas per #2689 (comment) ?

RikyPlaza · 2024-11-28T19:31:01Z

@jimklimov Hello, if i consider the driver blazer_usb.c i don't see any pragma directive. I'm a little bit confused on how those drivers, without any pragma construct, pass the checks related to the default labels in switch statements...

RikyPlaza · 2024-11-28T19:31:55Z

@jimklimov In any case now i try to insert that statemets and commit the driver. Let's see :)

…ault" label in switch statements. Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

jimklimov · 2024-11-28T21:22:20Z

Different compilers and their different warning settings require different rules to be satisfied, and are sonetimes at odds:

a switch should definitively handle all cases, so must have a default label - works well for general numeric variables;
an enum's valid values are known at compile time, and each must be handled explicitly (even if implemented as many case value: labels preceding the same code block), so...
...a default label is redundant (should never be reachable) in a switch that handles all enum values - this notion is head-on crash vs. the first rule above, hence the pragma's against warnings at this spot and the default label handled to be sure, as the least-worst solution.

Also note fall-through cases (lack of break/return/continue/... before next label) - these are considered a likely oversight by a programmer and in fact often are. Explicit goto superfluous_label to the next line defining this label solves the issue portably.

…etworkupstools#2689, networkupstools#823] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>

Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

RikyPlaza · 2024-11-30T14:06:33Z

@jimklimov no idea why the checks failed :)

RikyPlaza

No idea on the fails.

…ools#2689] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>

…h cases [networkupstools#2689] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>

…rkupstools#2689] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>

…dbus_drv Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>

jimklimov · 2024-12-01T17:22:21Z

CI setup issue, mis-added new workers :\

…_modbus_drv Updated Phoenix Contact modbus driver for newer models SVN-Git-Bridge: nut-3e341d2d9c-3fc79f1e8124 3984321 Bridge-Author: Jim Klimov <jimklimov+nut@gmail.com> Bridge-Author-Date: 2024-12-01T18:22:00+01:00 Bridge-Committer: GitHub <noreply@github.com> Bridge-Committer-Date: 2024-12-01T18:22:00+01:00 From: Jim Klimov <jimklimov+nut@gmail.com> git-svn-id: file:///mnt/export/subversion/nut/trunk@4172 5f2f2009-99cd-4845-9af7-39d0aa6e3748

Added management for newer models which have different modbus address…

3d332d9

…es. Choice is made basing on firmware version. Signed-off-by: RikyPlaza <rikyrod2001@gmail.com>

jimklimov requested changes Nov 22, 2024

View reviewed changes

jimklimov added enhancement modbus labels Nov 22, 2024

jimklimov added this to the 2.8.3 milestone Nov 22, 2024

First review of the driver for pull request networkupstools#2689

6fa780d

Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

Avoid to use uint32_t* reading more than one modbus address and corre…

3314e72

…cted alarm trigger (wrong bit used) Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

RikyPlaza added 4 commits November 25, 2024 11:22

Review of variable declaration due to clang build failure

f34b150

Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

Alligned device.model variable

673ccff

Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

Used enum for model type instead of defines

c30f75c

Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

Updated NEWS.adoc

1bd7eb2

Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

Corrected enum definition.

9490b2f

Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

RikyPlaza commented Nov 25, 2024

View reviewed changes

RikyPlaza requested a review from jimklimov November 26, 2024 05:36

Removed default case label since clang complains.

f19c8e0

Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

Added pragma statemets as per tip to avoid compiler complains on "def…

9dde188

…ault" label in switch statements. Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

jimklimov added a commit to jimklimov/nut that referenced this pull request Nov 28, 2024

docs/developers.txt: clarify pragmas in switch/case/default vs. enum [n…

4c26207

…etworkupstools#2689, networkupstools#823] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>

jimklimov mentioned this pull request Nov 28, 2024

docs/developers.txt: clarify pragmas in switch/case/default vs. enum #2700

Merged

RikyPlaza added 3 commits November 29, 2024 07:28

Extended comment to "default" label example

02c9974

Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

Re-implemented default cases.

9eca86f

Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

Handled default case with error

1e3a830

Signed-off-by: RikyPlaza <158032745+RikyPlaza@users.noreply.github.com>

RikyPlaza commented Nov 30, 2024

View reviewed changes

jimklimov added 4 commits December 1, 2024 01:23

drivers/phoenixcontact_modbus.c: no break after fatalx() [networkupst…

53b0025

…ools#2689] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>

drivers/phoenixcontact_modbus.c: fix use of pragmas for default switc…

74e65e1

…h cases [networkupstools#2689] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>

NEWS.adoc: moved phoenixcontact_modbus update to other drivers [netwo…

b250346

…rkupstools#2689] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>

Merge remote-tracking branch 'upstream/master' into phoenixcontact_mo…

ba36376

…dbus_drv Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>

jimklimov merged commit 3984321 into networkupstools:master Dec 1, 2024

		fatalx(EXIT_FAILURE, "Invalid modbus slave ID %d",MODBUS_SLAVE_ID);
		}


		mrir(modbus_ctx, 0x2000, 1, &actual_code_functions);

		tab_reg[0] = CHECK_BIT(actual_code_functions, 2); //battery mode is the 2nd bit of the register 0x2000

Conversation

RikyPlaza commented Nov 22, 2024

Uh oh!

jimklimov 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!

RikyPlaza commented Nov 23, 2024

Uh oh!

AppVeyorBot commented Nov 23, 2024

Uh oh!

jimklimov commented Nov 25, 2024

Uh oh!

AppVeyorBot commented Nov 25, 2024

Uh oh!

RikyPlaza left a comment

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Nov 25, 2024

Uh oh!

jimklimov commented Nov 26, 2024

Uh oh!

jimklimov commented Nov 26, 2024

Uh oh!

RikyPlaza commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikyPlaza commented Nov 26, 2024

Uh oh!

jimklimov commented Nov 26, 2024

Uh oh!

RikyPlaza commented Nov 27, 2024

Uh oh!

jimklimov commented Nov 27, 2024

Uh oh!

RikyPlaza commented Nov 28, 2024

Uh oh!

RikyPlaza commented Nov 28, 2024

Uh oh!

jimklimov commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikyPlaza commented Nov 30, 2024

Uh oh!

RikyPlaza left a comment

Choose a reason for hiding this comment

Uh oh!

jimklimov commented Dec 1, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

RikyPlaza commented Nov 26, 2024 •

edited

Loading

jimklimov commented Nov 28, 2024 •

edited

Loading