Updated Phoenix Contact modbus driver for newer models

[RikyPlaza]

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.

[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]

Here and several times above - needless added whitespace

[jimklimov]

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.

[RikyPlaza]

Undersoot!

[jimklimov]

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

[jimklimov]

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

[RikyPlaza]

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.

[jimklimov]

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]

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

[RikyPlaza]

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.

[jimklimov]

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]

Makes sense to also populate ups.firmware

[RikyPlaza]

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

[jimklimov]

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

[RikyPlaza]

@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]

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

[jimklimov]

@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.

[AppVeyorBot]

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

[RikyPlaza]

Files reviewed.

[AppVeyorBot]

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

[jimklimov]

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]

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]

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]

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]

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]

@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]

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

[RikyPlaza]

@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]

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

[jimklimov]

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.

[RikyPlaza]

@jimklimov no idea why the checks failed :)

[RikyPlaza]

No idea on the fails.

[jimklimov]

CI setup issue, mis-added new workers :\