Skip to content

Commit 65f9395

Browse files
m4r1kclaude
andcommitted
fix(voltage,docs,e2e): CodeRabbit review fixes + README badge restyle
CodeRabbit review on PR #29 surfaced two real bugs and four quality-of-life issues. All addressed in one commit; piggy-backs the unrelated README badge restyle (Federico-requested) since it's trivial. Bugs fixed: 1. **voltage.py: reject wrong-side transfer points before clamping.** A NUT driver reporting a high value (e.g., `input.transfer.low=250`) in the LOW transfer field passed the existing ±25% sanity check (|250-207|=43 ≤ 230×0.25=57.5) and produced `warning_low=255V` -- making perfectly normal 230V mains read as a brownout on every poll. Symmetric bug on the high side (low value in high field would yield warning_high=195V, below the 207V low warning, an impossible band). Fix: low_transfer must be < nominal AND high_transfer must be > nominal before being accepted as a clamp candidate. Without the guard, the bogus value is silently ignored and we fall back to ±10%. Added regression tests covering 250/230V on the low side and 200/230V on the high side, plus boundary cases (transfer == nominal is also rejected). 2. **voltage.py: don't claim EN 50160 accepts the UPS switch threshold.** The mild-event notification text said `UPS will not switch to battery until 280.0V (firmware setting); EN 50160 considers up to that level acceptable` -- which read as if EN 50160 sanctions 280V on a 230V grid. EN 50160 actually caps at nominal × 1.1 (253V), well below typical UPS switch points. The two are independent thresholds with different purposes. Fix: rewritten as `UPS will not switch to battery until 280.0V (firmware setting); this is a grid-quality issue (outside the EN 50160 ±10% envelope), not an imminent power loss.` -- accurate to the spec and applies symmetrically to high and low events. Updated the corresponding test_mild_overvoltage test to assert the new wording AND that the buggy phrasing must NOT appear. Quality-of-life: 3. **docs/notifications.md, docs/triggers.md: "non-severe" instead of "10–15%".** With narrow UPS transfer points, the warning band can be tighter than ±10%, so non-severe events can fire at less than 10% deviation. The previous wording implied a fixed band. 4. **docs/triggers.md: replaced 122V-on-120V flap example.** 122V on a 120V grid is +1.7%, well within the normal band -- it would NOT trigger any warning, so it was a misleading example. Replaced with 105V (12.5% under, past the ±10% warning threshold but not severe). 5. **docs/triggers.md: language hint on new fenced code blocks.** Added `text` to the log-output blocks for markdownlint MD040 compliance. Existing yaml/shell blocks unchanged. 6. **tests/e2e/groups/single-ups.sh Test 6: tightened assertions.** The previous `grep -i "voltage\|brownout"` matched the startup `Voltage Monitoring Active` line, so a regression that broke BROWNOUT_DETECTED would still pass. Now requires the actual `BROWNOUT_DETECTED` event marker AND the rc9 `% below nominal` framing -- both as fail-closed `exit 1` instead of soft-warn. README badge restyle (Federico-requested): All 6 README badges switched to the dark/minimal `for-the-badge` style with `labelColor=090909` and `color=e8e8ed` for visual consistency. Codecov badge swapped from `codecov.io/...graph/badge.svg` (no `for-the-badge` variant) to `img.shields.io/codecov/c/...` -- same data source, same link target, but now the style is honoured. Codecov coverage % and PyPI version still display dynamically; only their colour is overridden to keep the row uniform. Tests: 747 -> 749 (+2 for the new transfer-side guards). All green. Co-Authored-By: Claude Opus 4.7 <[email protected]>
1 parent b53d668 commit 65f9395

6 files changed

Lines changed: 103 additions & 45 deletions

File tree

README.md

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,12 @@
44

55
**UPS monitoring and shutdown orchestration for NUT**
66

7-
[![License: MIT](https://img.shields.io/badge/License-MIT-yellow.svg)](https://opensource.org/licenses/MIT)
8-
[![Python 3.9+](https://img.shields.io/badge/python-3.9+-blue.svg)](https://www.python.org/downloads/)
9-
[![NUT Compatible](https://img.shields.io/badge/NUT-compatible-green.svg)](https://networkupstools.org/)
10-
[![codecov](https://codecov.io/gh/m4r1k/Eneru/branch/main/graph/badge.svg)](https://codecov.io/gh/m4r1k/Eneru)
11-
[![Documentation](https://img.shields.io/badge/docs-Read%20The%20Docs-blue.svg)](https://eneru.readthedocs.io/)
12-
[![PyPI](https://img.shields.io/pypi/v/eneru.svg)](https://pypi.org/project/eneru/)
7+
<a href="LICENSE"><img src="https://img.shields.io/badge/License-MIT-e8e8ed?style=for-the-badge&labelColor=090909" alt="MIT"></a>
8+
<a href="https://www.python.org/downloads/"><img src="https://img.shields.io/badge/Python-3.9+-e8e8ed?style=for-the-badge&labelColor=090909" alt="Python 3.9+"></a>
9+
<a href="https://networkupstools.org/"><img src="https://img.shields.io/badge/NUT-compatible-e8e8ed?style=for-the-badge&labelColor=090909" alt="NUT compatible"></a>
10+
<a href="https://codecov.io/gh/m4r1k/Eneru"><img src="https://img.shields.io/codecov/c/github/m4r1k/Eneru?style=for-the-badge&labelColor=090909&color=e8e8ed&label=Coverage" alt="Coverage"></a>
11+
<a href="https://eneru.readthedocs.io/"><img src="https://img.shields.io/badge/Docs-Read%20The%20Docs-e8e8ed?style=for-the-badge&labelColor=090909" alt="Documentation"></a>
12+
<a href="https://pypi.org/project/eneru/"><img src="https://img.shields.io/pypi/v/eneru?style=for-the-badge&labelColor=090909&color=e8e8ed&label=PyPI" alt="PyPI"></a>
1313

1414
<p align="center">
1515
<img src="https://raw.githubusercontent.com/m4r1k/Eneru/main/docs/images/eneru-diagram.svg" alt="Eneru Architecture" width="600">

docs/notifications.md

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -292,14 +292,18 @@ sqlite3 /var/lib/eneru/<UPS>.db \
292292
If the condition persists past the dwell, the notification fires
293293
with a `Persisted Ns.` annotation so you can see it was held.
294294

295-
**Severity bypass (rc9, 5.1.0).** Mild deviations (10–15% from
296-
nominal) go through the dwell as described above. **Severe
297-
deviations** (greater than ±15% from nominal) bypass the dwell
298-
entirely and notify immediately, with a `(severe, X.X% below/above
299-
nominal)` tag and an `Approaching UPS battery-switch threshold`
300-
callout when NUT exposes the UPS's transfer points. The reasoning:
301-
- Mild deviations are usually flap from neighbour appliances cycling
302-
— the 30s filter is the right call.
295+
**Severity bypass (rc9, 5.1.0).** Non-severe voltage warnings (up
296+
to and including ±15% from nominal) go through the dwell as
297+
described above. **Severe deviations** (greater than ±15% from
298+
nominal) bypass the dwell entirely and notify immediately, with a
299+
`(severe, X.X% below/above nominal)` tag and an
300+
`Approaching UPS battery-switch threshold` callout when NUT
301+
exposes the UPS's transfer points. The reasoning:
302+
- Non-severe events are usually flap from neighbour appliances
303+
cycling — the 30s filter is the right call. Note that with
304+
narrow UPS transfer points the warning thresholds may be tighter
305+
than ±10%, so the "non-severe" band can fire at less than 10%
306+
deviation; the dwell still applies.
303307
- Severe deviations indicate real grid trouble (utility fault,
304308
generator instability, site wiring issue) — the operator wants
305309
to know immediately, not 30s later.

docs/triggers.md

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -82,11 +82,14 @@ What Eneru does instead:
8282
`OVER_VOLTAGE_DETECTED` / `BROWNOUT_DETECTED` is always written
8383
immediately on transition. The *notification* dispatch is gated by
8484
severity:
85-
- **Mild deviations** (10–15% from nominal) go through
86-
`notifications.voltage_hysteresis_seconds` (default 30s). A
87-
2-second flap to 122V on a 120V grid no longer pages you; a
88-
sustained event still does — and arrives with a
89-
`Persisted Ns.` annotation.
85+
- **Non-severe voltage warnings** (up to and including ±15% from
86+
nominal) go through `notifications.voltage_hysteresis_seconds`
87+
(default 30s). A 2-second flap to 105V on a 120V grid (12.5%
88+
under nominal — past the warning threshold but not severe) no
89+
longer pages you; a sustained event still does, and arrives
90+
with a `Persisted Ns.` annotation. With narrow UPS transfer
91+
points the warning band can be tighter than ±10%, so non-severe
92+
events can fire at less than 10% deviation.
9093
- **Severe deviations** (`>±15%` from nominal) bypass the dwell
9194
and notify **immediately** with a `(severe, X.X% below/above
9295
nominal)` tag. These signal real grid trouble — utility fault,
@@ -105,7 +108,7 @@ What Eneru does instead:
105108

106109
### What you'll see in the log
107110

108-
```
111+
```text
109112
📊 Voltage Monitoring Active.
110113
Nominal: 230.0V (NUT=230.0).
111114
Grid-quality warnings: 207.0V / 253.0V (±10% nominal, EN 50160 envelope).
@@ -122,16 +125,17 @@ information, not an active power event).
122125

123126
**Mild brownout (UPS won't switch):**
124127

125-
```
128+
```text
126129
🔻 BROWNOUT_DETECTED: input voltage 200.0V is 13.0% below 230V nominal
127130
(warning threshold 207.0V). Persisted 30s.
128131
UPS will not switch to battery until 170.0V (firmware setting);
129-
this is a grid-quality issue, not an imminent power loss.
132+
this is a grid-quality issue (outside the EN 50160 ±10% envelope),
133+
not an imminent power loss.
130134
```
131135

132136
**Severe brownout (UPS may switch shortly):**
133137

134-
```
138+
```text
135139
🔻 BROWNOUT_DETECTED: (severe, 21.7% below nominal): input voltage 180.0V.
136140
Notifying immediately (bypassed hysteresis).
137141
Approaching UPS battery-switch threshold (170.0V) -- battery may

src/eneru/health/voltage.py

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -61,28 +61,44 @@ def _derive_warning_low(nominal: float, low_transfer) -> float:
6161
"""Pick the warning-low threshold: the *tighter* (higher) of ±10% and (transfer + buffer).
6262
6363
Tighter = warns earlier = better grid-quality signal. NUT's
64-
transfer point is honored only when it's plausibly close to the
65-
expected ±10% line (within ±25%); a wildly mis-reported transfer
66-
value is ignored so we always have at least the ±10% safety net.
64+
transfer point is honored only when:
65+
- it's numeric,
66+
- it's BELOW the nominal (a low-transfer at or above nominal is
67+
nonsense -- means the UPS would switch on perfectly normal mains),
68+
- and it's within ±25% of the expected ±10% line.
69+
70+
The "below nominal" guard catches the bug where a NUT driver
71+
reports a high value (e.g., 250 on a 230V grid) in the low-transfer
72+
field; without the guard we'd compute warning_low = 255V and then
73+
230V mains would falsely fire BROWNOUT_DETECTED. With the guard
74+
the bogus value is ignored and we fall back to ±10%.
75+
6776
Rounded to one decimal so the log line and notification text stay
6877
clean (avoids 253.00000000000003 from float multiplication).
6978
"""
7079
pct_band = nominal * (1 - GRID_QUALITY_DEVIATION_PCT)
7180
candidates = [pct_band]
7281
if is_numeric(low_transfer):
7382
lt = float(low_transfer)
74-
if abs(lt - pct_band) <= nominal * 0.25:
83+
if lt < nominal and abs(lt - pct_band) <= nominal * 0.25:
7584
candidates.append(lt + TRANSFER_BUFFER_V)
7685
return round(max(candidates), 1)
7786

7887

7988
def _derive_warning_high(nominal: float, high_transfer) -> float:
80-
"""Pick the warning-high threshold: the *tighter* (lower) of ±10% and (transfer - buffer)."""
89+
"""Pick the warning-high threshold: the *tighter* (lower) of ±10% and (transfer - buffer).
90+
91+
Symmetric guard to ``_derive_warning_low``: high transfer must be
92+
ABOVE nominal to be plausible. A NUT driver reporting 200 on a 230V
93+
grid in the high-transfer field would otherwise compute a warning
94+
of 195V, well below the 207V ±10% threshold -- forcing the warning
95+
band wider rather than tighter, which defeats the whole clamp.
96+
"""
8197
pct_band = nominal * (1 + GRID_QUALITY_DEVIATION_PCT)
8298
candidates = [pct_band]
8399
if is_numeric(high_transfer):
84100
ht = float(high_transfer)
85-
if abs(ht - pct_band) <= nominal * 0.25:
101+
if ht > nominal and abs(ht - pct_band) <= nominal * 0.25:
86102
candidates.append(ht - TRANSFER_BUFFER_V)
87103
return round(min(candidates), 1)
88104

@@ -337,22 +353,21 @@ def _format_voltage_detail(self, direction: str, voltage: float,
337353
# transfer point. For severe events, frame as "battery may
338354
# engage shortly"; for mild, frame as "this is a grid quality
339355
# issue, not an imminent power loss".
356+
# NOTE: do NOT imply EN 50160 considers the UPS switch point
357+
# acceptable. EN 50160 caps at nominal × 1.1 (e.g., 253V on
358+
# 230V), well below typical UPS switch points (e.g., 280V).
359+
# The two are independent thresholds with different purposes.
340360
ups_switch = (self.state.ups_transfer_low if direction == "low"
341361
else self.state.ups_transfer_high)
342362
if ups_switch is not None:
343363
if is_severe:
344364
tail = (f"Approaching UPS battery-switch threshold "
345365
f"({ups_switch}V) -- battery may engage shortly.")
346366
else:
347-
en50160_note = (
348-
"EN 50160 considers up to that level acceptable; "
349-
if direction == "high" else ""
350-
)
351367
tail = (f"UPS will not switch to battery until "
352-
f"{ups_switch}V (firmware setting); "
353-
f"{en50160_note}"
354-
f"this is a grid-quality issue, not an "
355-
f"imminent power loss.")
368+
f"{ups_switch}V (firmware setting); this is "
369+
f"a grid-quality issue (outside the EN 50160 "
370+
f"±10% envelope), not an imminent power loss.")
356371
else:
357372
tail = ""
358373

tests/e2e/groups/single-ups.sh

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -173,11 +173,15 @@ sleep 2
173173
# Run briefly to detect brownout
174174
timeout 8 eneru run --config $E2E_DIR/config-e2e-dry-run.yaml 2>&1 | tee /tmp/test6.log || true
175175

176-
# Verify brownout was detected (should log a voltage warning)
177-
if grep -q -i "voltage\|brownout" /tmp/test6.log; then
178-
echo "PASS (6a): Voltage event detected"
176+
# Verify brownout was specifically detected -- not just any voltage
177+
# log line. The startup `Voltage Monitoring Active` line would match
178+
# `voltage` and let a regression slip through; require the actual
179+
# BROWNOUT_DETECTED event marker.
180+
if grep -q "BROWNOUT_DETECTED" /tmp/test6.log; then
181+
echo "PASS (6a): BROWNOUT_DETECTED event fired"
179182
else
180-
echo "Note: Voltage event may not have been logged in short window"
183+
echo "FAIL (6a): brownout scenario did not produce BROWNOUT_DETECTED log"
184+
exit 1
181185
fi
182186

183187
# rc9: startup log line should expose BOTH the grid-quality warning
@@ -204,11 +208,12 @@ fi
204208
# rc9: the BROWNOUT detail must include the % deviation framing.
205209
# Notification dispatch is gated by hysteresis (default 30s) so it
206210
# may not fire in our 8s window, but the immediate BROWNOUT_DETECTED
207-
# log row carries the same detail string.
211+
# log row carries the same detail string -- so this MUST be present.
208212
if grep -q "below.*nominal" /tmp/test6.log; then
209213
echo "PASS (6d): brownout log carries 'below nominal' framing"
210214
else
211-
echo "Note (6d): 'below nominal' framing absent in test window"
215+
echo "FAIL (6d): brownout log missing rc9 '<X>% below <Y>V nominal' framing"
216+
exit 1
212217
fi
213218
)
214219

tests/test_voltage.py

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,26 @@ def test_garbage_transfer_falls_back_to_ten_percent(self):
370370
assert _derive_warning_low(120, 250) == 108.0 # 250 way off |250-108|=142 > 30
371371
assert _derive_warning_high(120, 50) == 132.0 # 50 way off |50-132|=82 > 30
372372

373+
@pytest.mark.unit
374+
def test_low_transfer_at_or_above_nominal_is_rejected(self):
375+
# Regression for CodeRabbit major: NUT reporting low_transfer
376+
# >= nominal is nonsense (UPS would switch on perfectly normal
377+
# mains). Without the guard, low_transfer=250 on 230V passes
378+
# the ±25% sanity (|250-207|=43 ≤ 57.5) and would yield
379+
# warning_low=255, making 230V mains read as a brownout.
380+
assert _derive_warning_low(230, 250) == 207.0 # rejected, fall back to ±10%
381+
assert _derive_warning_low(230, 230) == 207.0 # equal also rejected
382+
assert _derive_warning_low(230, 231) == 207.0 # just-above also rejected
383+
384+
@pytest.mark.unit
385+
def test_high_transfer_at_or_below_nominal_is_rejected(self):
386+
# Symmetric guard: high_transfer <= nominal is nonsense.
387+
# 200 on 230V would otherwise yield warning_high = 195V,
388+
# below the 207V low warning -- impossible band.
389+
assert _derive_warning_high(230, 200) == 253.0 # rejected
390+
assert _derive_warning_high(230, 230) == 253.0 # equal also rejected
391+
assert _derive_warning_high(230, 229) == 253.0 # just-below also rejected
392+
373393
@pytest.mark.unit
374394
def test_state_records_ups_transfer_points(self):
375395
h = _TestHost(ups_vars={
@@ -564,13 +584,23 @@ def test_message_omits_ups_context_when_transfer_unknown(self):
564584
assert "13.0% below" in body
565585

566586
@pytest.mark.unit
567-
def test_mild_overvoltage_includes_en50160_hint(self):
587+
def test_mild_overvoltage_explains_en50160_envelope_correctly(self):
588+
# Regression for CodeRabbit major: the previous "EN 50160
589+
# considers up to that level acceptable" wording was wrong --
590+
# it implied EN 50160 accepts the UPS switch threshold (e.g.,
591+
# 280V), but EN 50160 actually caps at nominal × 1.1 (253V on
592+
# 230V). The corrected message frames the warning as outside
593+
# the EN 50160 ±10% envelope without putting words into the
594+
# standard's mouth about higher voltages.
568595
h = _TestHost(ups_vars={
569596
"input.voltage.nominal": "230",
570597
"input.transfer.high": "280",
571598
}, hysteresis=0)
572599
h._initialize_voltage_thresholds()
573600
h._check_voltage_issues("OL", "256") # 11.3% above, mild
574601
body, _ = h.notifications[0]
575-
assert "EN 50160 considers up to that level acceptable" in body
602+
# Corrected wording: warning is OUTSIDE the EN 50160 envelope.
603+
assert "outside the EN 50160" in body
576604
assert "UPS will not switch to battery until 280.0V" in body
605+
# The buggy wording must NOT appear.
606+
assert "EN 50160 considers up to that level acceptable" not in body

0 commit comments

Comments
 (0)