Skip to content

Commit 557b259

Browse files
widgetiiclaude
andauthored
power.routeros: power_on must promote saved 'off' to 'forced-on' (#104)
## Why \`RouterOSController\` saves the port's previous \`poe-out\` mode on \`power_off\` so \`power_on\` can put it back where it was — that preserves "auto-on" vs "forced-on" distinctions correctly. But if the port was already \"off\" when \`power_off\` ran (recovering a parked camera; bench is dark on a fresh shell), the saved \"previous\" mode is literally \"off\", and \`power_on\` then \"restored\" the port to off. Cycling a powered-down port left it powered down forever, which silently broke every recovery flow that started from off. Surfaced while testing #103 — every fresh script that started with \`power_off → power_on\` left the camera unpowered, until I switched to \`_set_poe(port, 'forced-on')\` directly as a workaround. ## What \`power_on\` must always result in a powered port. If the saved mode is \"off\", promote to \"forced-on\" and log the promotion (visible at \`-v\`): \`\`\`python async def power_on(self, port: str) -> None: restore_mode = self._saved_poe_out.pop(port, \"forced-on\") if restore_mode == \"off\": logger.info(\"PoE ON: %s on %s (saved state was 'off' — promoting to 'forced-on')\", port, self._host) restore_mode = \"forced-on\" else: logger.info(\"PoE ON: %s on %s (restoring %s)\", port, self._host, restore_mode) await self._set_poe(port, restore_mode) \`\`\` ## Test plan - [x] \`uv run pytest tests/ -x --ignore=tests/fuzz\` — **527 passed, 2 skipped** (5 new tests) - [x] \`uv run ruff check\` + \`mypy\` on changed files — clean - [x] Verified on real hardware (MikroTik 10.216.128.2 / ether8, currently powered down): \`\`\` before: ether8 poe-out='off' power_off: saved='off' power_on: saved=None, port now 'forced-on' \`\`\` 5 new tests in \`tests/test_power.py::TestRouterOSPowerOnOff\` covering forced-on round-trip, auto-on round-trip (preserved — not blindly clobbered to forced-on), **off→on promotion (the bug)**, no-prior-off default, and double-power_off-doesn't-clobber-saved-state edge cases. They use a \`_PoeStateRouterOS\` subclass that stubs the two network primitives so we can exercise the save/restore state machine without a real switch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Dmitry Ilyin <widgetii@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 4193805 commit 557b259

2 files changed

Lines changed: 89 additions & 1 deletion

File tree

src/defib/power/routeros.py

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -365,7 +365,21 @@ async def power_off(self, port: str) -> None:
365365

366366
async def power_on(self, port: str) -> None:
367367
restore_mode = self._saved_poe_out.pop(port, "forced-on")
368-
logger.info("PoE ON: %s on %s (restoring %s)", port, self._host, restore_mode)
368+
# power_off saves whatever the port's poe-out was BEFORE we turned
369+
# it off. If the port was already off (e.g. recovering a camera
370+
# that's been parked), the saved "previous" state is "off" — and
371+
# restoring it would defeat the purpose of power_on entirely.
372+
# Promote to forced-on so the port actually comes up.
373+
if restore_mode == "off":
374+
logger.info(
375+
"PoE ON: %s on %s (saved state was 'off' — promoting to "
376+
"'forced-on')", port, self._host,
377+
)
378+
restore_mode = "forced-on"
379+
else:
380+
logger.info(
381+
"PoE ON: %s on %s (restoring %s)", port, self._host, restore_mode,
382+
)
369383
await self._set_poe(port, restore_mode)
370384

371385
async def power_cycle(self, port: str, off_duration: float = 5.0) -> None:

tests/test_power.py

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,3 +187,77 @@ def test_custom_values(self, monkeypatch: pytest.MonkeyPatch) -> None:
187187
assert ctrl._username == "root"
188188
assert ctrl._password == "secret"
189189
assert ctrl._api_port == 9999
190+
191+
192+
# ---------------------------------------------------------------------------
193+
# RouterOS power_off / power_on save+restore semantics
194+
#
195+
# RouterOSController records the previous poe-out mode on power_off so
196+
# power_on can put the port back to its prior state ("forced-on" / "auto-on" /
197+
# "auto-off") rather than blindly hard-coding one mode. The tricky case is
198+
# when the port was already "off" on entry — see test_power_on_promotes_off
199+
# below; that was a real bug that left ports stuck off.
200+
# ---------------------------------------------------------------------------
201+
202+
203+
class _PoeStateRouterOS(RouterOSController):
204+
"""RouterOSController with the two network-touching primitives stubbed
205+
out so the save/restore state machine can be tested without a switch."""
206+
207+
def __init__(self, initial_poe_out: str) -> None:
208+
super().__init__(host="test", username="u", password="p")
209+
self._poe_state: dict[str, str] = {"ether3": initial_poe_out}
210+
self.set_calls: list[tuple[str, str]] = []
211+
212+
async def _get_poe_out(self, interface_name: str) -> str: # type: ignore[override]
213+
return self._poe_state.get(interface_name, "auto-on")
214+
215+
async def _set_poe(self, interface_name: str, poe_out: str) -> None: # type: ignore[override]
216+
self.set_calls.append((interface_name, poe_out))
217+
self._poe_state[interface_name] = poe_out
218+
219+
220+
class TestRouterOSPowerOnOff:
221+
async def test_power_off_then_on_restores_forced_on(self) -> None:
222+
ctrl = _PoeStateRouterOS("forced-on")
223+
await ctrl.power_off("ether3")
224+
await ctrl.power_on("ether3")
225+
assert ctrl._poe_state["ether3"] == "forced-on"
226+
# Saved state cleared on restore
227+
assert "ether3" not in ctrl._saved_poe_out
228+
229+
async def test_power_off_then_on_restores_auto_on(self) -> None:
230+
# PoE mode can legitimately be "auto-on" (negotiate with device);
231+
# power_on should preserve that, not blindly set "forced-on".
232+
ctrl = _PoeStateRouterOS("auto-on")
233+
await ctrl.power_off("ether3")
234+
await ctrl.power_on("ether3")
235+
assert ctrl._poe_state["ether3"] == "auto-on"
236+
237+
async def test_power_on_promotes_off(self) -> None:
238+
# Regression: with no fix, this left the port at "off" — because
239+
# power_off saved "off" as the previous state, and power_on then
240+
# "restored" to "off". power_on must always result in a powered
241+
# port, so a saved "off" gets promoted to "forced-on".
242+
ctrl = _PoeStateRouterOS("off")
243+
await ctrl.power_off("ether3")
244+
await ctrl.power_on("ether3")
245+
assert ctrl._poe_state["ether3"] == "forced-on"
246+
247+
async def test_power_on_without_prior_off_uses_forced_on(self) -> None:
248+
# No saved state means power_on has nothing to restore — default
249+
# to forced-on.
250+
ctrl = _PoeStateRouterOS("auto-off")
251+
await ctrl.power_on("ether3")
252+
assert ctrl._poe_state["ether3"] == "forced-on"
253+
254+
async def test_double_power_off_does_not_clobber_saved_state(self) -> None:
255+
# Calling power_off twice in a row must not overwrite the saved
256+
# state with "off" — otherwise power_on would have nothing useful
257+
# to restore on the chip that was once "forced-on".
258+
ctrl = _PoeStateRouterOS("forced-on")
259+
await ctrl.power_off("ether3")
260+
await ctrl.power_off("ether3")
261+
assert ctrl._saved_poe_out["ether3"] == "forced-on"
262+
await ctrl.power_on("ether3")
263+
assert ctrl._poe_state["ether3"] == "forced-on"

0 commit comments

Comments
 (0)