powercom: running battery test on startup should be configurable

[shapirus]

Currently, powercom driver sends a signal to the controlled UPS to execute a battery test every time the driver starts. The respective code is in upsdrv_initups() at https://github.com/networkupstools/nut/blob/6b9971a706ced64b95c978c6e0b62ba4761c3113/drivers/powercom.c#L1008

This code runs unconditionally. There are use cases when this behavior may be undesirable. For example, booting a system with considerable load connected to the nut-controlled UPS whose battery has been depleted by a preceding power outage. Or not wanting the UPS to produce a loud beep when nut starts. I personally would rather have a cron job that triggers battery tests on a comfortable schedule (which I do, along with commenting out the block of code in question altogether).

There should be a config option that controls this behavior and allows to turn it on or off.

[rouben]

Hate to resurrect an ancient issue, but I wanted to second this. Is there any traction on this at all?

[jimklimov]

The idea does sound reasonable, but I haben't seen a change like this proposed in recent PRs...

On Fri, Nov 12, 2021, 05:01 Rouben @.***> wrote:

Hate to resurrect an ancient issue, but I wanted to second this. Is there any traction on this at all?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/networkupstools/nut/issues/546#issuecomment-966801677, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMPTFAYBF6JOCFUGX4COMDULSGSRANCNFSM4EZM3CPA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[shapirus]

Resurrecting this is fine, because the issue is still there :).

Fixing it requires some skill and time to find out how working with configuration is implemented in nut.

Bringing this thread up increases the chances that someone sees it and finds time to submit a PR. I might even do this myself some day...

[rouben]

Looks like it's handled by upsdrv_initups(). Seems like getval() gets values from the config file. The culprit that triggers a test (hard-coded test) appears to be line 1025 of powercom.c. It looks like the authors use the battery test command to test basic serial write ability (ability to send commands to UPS). I suspect that may have been legacy code when the driver was in its infancy, because the function ups_getinfo(), which upsdrv_initups() calls, does call upon ser_send_char() (i.e. writes to the UPS serial interface), so doing this is redundant.

I'll check out other drivers to see if they do a battery test on UPS initialization. If not, I think it makes sense to issue a PR that simply removes that hard-coded test from the driver altogether, if it is redundant/inconsistent with other drivers' behaviours. I'd be interested to hear what others think about this.

The logic to detect the line voltage and model type is really clunky, IMHO... see lines 987-1030. So many nested ifs and elsifs. That could probably be cleaned up also, although that's a potential logic error minefield.

[rouben]

After reviewing 5 different drivers, I can confirm that none of them issue a test battery command when initializing the UPS. Therefore this appears to be unique to the powercom driver, and may be a legacy behaviour.

Proposal: rather than making the "test battery on startup" a configurable feature, I suggest removing that completely. Testing the battery will still be available as a driver command.

Rationale:

1. Testing the battery on UPS init appears to be non-standard behaviour unique to the powercom driver.
2. Testing the battery on startup is a bad idea, as the UPS can fail and cut power to the system due to the increased load on the system during boot.
3. Removing this feature does not interfere with the driver's ability to test the UPD battery on demand.

If there are no objecting to this, I can take a crack at issuing a PR that removes the UPS battery test on init.

[shapirus]

Sounds very reasonable to me.

Considering also the fact that the UPS (at least my KIN-1000AP does this) performs a battery test on its own when it is turned on, there is one less reason to run this test on the driver startup.

Would be nice to hear from the developers, but since this issue hasn't seen any attention since 2018 :), maybe submitting a PR right away is okay.

[jimklimov]

Thanks for the analysis, idea sounds reasonable - especially if the explicit command to test battery is and remains available. Eagerly awaiting the PR then, especially as it seems there are several active people ready to test it on HW at the moment ;)

We had some cheaper Powercoms (original and KIN series, approx 600-1000AP) as our first UPSes when I started using NUT at the turn of millenium, but just could not remember if anything did force a test at startup back then.

As for something from "driver infancy", often a series of git blame + git show can help gauge that (newest commit at the "strange line"); github Web-UI makes it more simple to step back into history (e.g. latest commit to this line might be cosmetic and recent, but the actual logic before it could be 15 years old).

[jimklimov]

FWIW, regarding configurability of the powercom battery test on startup, found a pending but abandoned PR #595

Wondering now if some users' use-cases relied on having that test, and whether it would be complicated instead to automate an "upscmd" shortly after boot for similar effect, would they need to. At least, if we remove this self-test by default or completely, that is something to communicate in release notes for minimal surprise.

[rouben]

@jimklimov I asked one of the maintainers to check the requirements for the PR (it was not merged because it didn't meet the project's requirements for documentation and config file parameter handling, and is now out of date). If they respond, I can attempt to reissue a PR that satisfies the project's requirements. If not, I can issue a simpler PR, with appropriate documentation and version number bump that simply removes the on startup battery test and suggests how to do regular battery tests with cron, for example.

[jimklimov]

I think the cleaner PR to drop the "non-standard" behavior sounds okay, maybe with a Release Note item for the change, TBD when the release time finally comes.

On Sun, Nov 14, 2021, 17:43 Rouben @.***> wrote:

@jimklimov https://github.com/jimklimov I asked one of the maintainers to check the requirements for the PR (it was not merged because it didn't meet the project's requirements for documentation and config file parameter handling, and is now out of date). If they respond, I can attempt to reissue a PR that satisfies the project's requirements. If not, I can issue a simpler PR, with appropriate documentation and version number bump that simply removes the on startup battery test and suggests how to do regular battery tests with cron, for example.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/networkupstools/nut/issues/546#issuecomment-968325115, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMPTFATV6SFNHASPHQ4SZ3UL7RMZANCNFSM4EZM3CPA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[jimklimov]

Ultimately, the PR adding the option was merged.