Repository navigation
riello_usb.c: option for driver to calculate values of battery.runtime and battery.charge "locally" - #1692
Conversation
Added driver setting flag `localcalculation`. When enabled, driver will calculate values of battery.runtime and battery.load locally. This is for some Riello models (iPlug and iDialog series) that provides incorrect values. Local calculation is done according to nominal battery capacity, nominal battery voltage, actual battery charge, maximum and actual UPS load. Added condition to filter off situation when battery temperature variable is incorrectly set to variable type maximum (255) - which is typical issue of some Riello models (iPlug and iDialog series). If incorrect value is detected temperature is set to 0.
Added localcalculation flag description
|
Looks great for a PR, more so if it is nearly your first as claimed ;) I just updated the docs impacted for feature updates. Thinking about the flag name (if other drivers have something similar enough to name it identically) and purpose, I remembered |
jimklimov
left a comment
jimklimov
left a comment
There was a problem hiding this comment.
Suggested some improvements (try them in practice first). One more sore point is whitespace style (got guidelines in docs, did not get to automate them yet) - notably TABs for indentation.
Before further changes, if any, please don't forget to git pull the changes I made for the docs into your workspace, so commit history remains compatible.
jimklimov
added
USB
Riello
Run time in this patch depends on various percentage loads as well. It also depends on battery percentage factor and that is calculated linearly.
Actual Charge Percentage is simple if we use linear scheme - like here: 10.7 - fully discharged, 12.9 - fully charged assuming that 50% is in the middle. In fact 50% is not at 11.8. but rather around 12.1. So linear scheme will show 50% while in fact it's already 40%. Battery age reduce that problem because it speed up voltage loss at higher voltages. Situation looks similar to the cars where fuel tank 50% on dashboard not always represent 50% of real tank. But indeed fill is full and empty i empty :D |
This is because I've copied the code from editor in terminal to the github on website. All tabs has been translated to spaces :(
Heh ... most confusing are git related tasks. |
From If you don't yet have that workspace, you can instantiate it from your github fork and branch (by default will make a new If you had cloned NUT master earlier, you can add your fork as another "remote" repo and check out the branch: Then as you edit stuff, and are satisfied with results, commit them and update the source branch on github that the PR is spawned from, so new iteration would be tested by CI: |
|
Later on you can just For the purposes of this PR, this would update your workspace with any edits I might have made like for the docs yesterday. Generally, over time it might help sync (merge) the upstream |
|
FWIW, there are many Git tutorials out there. I liked https://www.atlassian.com/git/tutorials ages ago. Also google up some "git games" trainings, there should be a lot on github or other sites that walk you from level to level by completing certain git commands. |
|
Cheers, I've updated the PR regarding my comments. Can you please fetch a fresh copy of your "patch-1" branch to build and test that the driver still works "sanely" (and e.g. does not spam logs about need for the flag when it is not set)? |
Use "guesstimate" as common NUT term for future searches
Log that guesstimation will be used instead of device readings (if any)
|
I must confess I'm not sure if I know what's happening here. |
|
Wait a minute - I've cloned git on my PC with added my fork repository as you guided me in one of last comments. I've done as you said and there is some conflicts detected in riello_ups.c and they are commented with angle brackets. |
|
As a maintainer I can and did apply some fixes to your branch in your fork on github. The angle brackets appear during To test the currently published version with my changes, you can To |
|
The changes as you have done are good - all works perfect. |
…on" related support [networkupstools#1692] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…tery.voltage.low/.high for localcalculation guesstimates [networkupstools#1692] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
|
Thanks @grifferz for the comments - I was on the verge of merging this PR "as is" to let it benefit at least some population of NUT users, but having seen your review - I've updated this PR with a shot at letting the battery voltage low/high range for these calculations be configurable. Maybe the defaults for a range ( If you have a chance to test whether this branch now serves your HW well with currently the two added settings in your |
…range via known or configured battery.voltage.nominal [networkupstools#1692] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…to match nutdrv_qx data for PbAc batteries [networkupstools#1692] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
|
@grifferz : updated with a few ways to honour the There is decent chance it would now work out of the box (with just the |
|
CI hiccups unrelated to this change set:
|
jimklimov
added
the
ready / gonna merge
|
Cheers all, any chance for practical testing with recent changes, to merge the PR this weekend? <3 :) |
Working on it… |
@jimklimov Okay, so if I just run this with and nothing else special, I get this from However, if I try to add my own lower bound for voltage as discovered by running my unit to exhaustion, 22.9: I get this: Am I doing something wrong with my configuration? As it stands then, I cannot use this (without a more generous margin) because the |
|
I guess it might be a bit of an unsupported blind spot to only set one of the values in configuration. Still, it was not too expected that the values would revert to a multiplier of 1 (factors of 12V nominal base) given the other numbers like nominal "24" from HW are over 20 :\ For a quick test, would it fare better if you set "default.battery.voltage.high" in ups.conf also? |
|
Adding the "high" setyting doesn't seem to make any difference unfortunately: |
|
Thanks, trying to mock it up now. Some debug can help make sense of codepath walked, e.g.: |
|
Okay here we go. (An APC is also attached and configured so ignore its mention of that) |
…rom settings or initial reading [networkupstools#1692] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…rom nominal voltage alignment, then apply from settings or initial reading [networkupstools#1692] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
|
Checking another approach: to set high/low values based on nominal first, then overlay with configs if given (even if just one limit is customized), e.g.: |
|
With this pull and running like you suggested: So that seems better? No runtime though, I suppose because no Adding Running it like this so it uses my config file: So that's got it? |
|
Seems like it, I hope :) |
|
I see the 4095 and 409.5 mentions (all bits set in a 12-bit streak) are still a problem, but for some other day and tracked in some other ticket (PRs welcome though - possibly a few if's to ignore a reading in the right places). At least, it's not something I'd comfortably address from just a phone with Termux in the middle of the ocean. Oh, the times and the things made possible! |
…g lines [networkupstools#1692] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…calculation" feature from riello_ser [networkupstools#1692, networkupstools#2390] Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
jimklimov
added
the
Incorrect or missing readings
riello_usb.c: option for driver to calculate values of battery.runtime and battery.charge "locally" SVN-Git-Bridge: nut-3e341d2d9c-3fc79f1e8124 f6bff66 Bridge-Author: Jim Klimov <jimklimov+nut@gmail.com> Bridge-Author-Date: 2024-03-31T08:15:15+02:00 Bridge-Committer: GitHub <noreply@github.com> Bridge-Committer-Date: 2024-03-31T08:15:15+02:00 From: Jim Klimov <jimklimov+nut@gmail.com> git-svn-id: file:///mnt/export/subversion/nut/trunk@3950 5f2f2009-99cd-4845-9af7-39d0aa6e3748
MAINTAINER NOTE: Planning to merge this fix as part of upcoming NUT v2.8.1 but probably will refactor later the configuration options involved -- and possibly this would only land in a subsequent release, see #1842. For now the newly introduced
localcalculationoption is documented as "may get deprecated soon" in favor ofruntimecalas in other drivers.Closes: #1685
This PR introduces actually two features: the local calculation of the runtime/charge data, and constraints on valid runtime/charge/temperature values. Unlike the original description and implementation, invalid data points would not cause a zero reading.
According to issue #1685 :
Added driver setting flag
localcalculation. When enabled, driver will calculate values of battery.runtime and battery.charge locally. This is for some Riello models (iPlug and iDialog series) that provides incorrect values. Local calculation is done according to nominal battery capacity, nominal battery voltage, actual battery charge, maximum and actual UPS load.Also used lead battery charge graphs as guidance to estimate current battery charge. Graph is not linear, so obtained charge value may not be perfectly accurate.
However value should be good enough to determine battery actual status and estimate rough time it can power the system.
Added condition to filter off situation when battery temperature variable is incorrectly set to variable type maximum (255) - which is typical issue of some Riello models (iPlug and iDialog series). If incorrect value is detected temperature is set to 0.
UPDATE: initially claimed calculation of
battery.load, but code setsbattery.chargein fact.