Light the bathroom at a stored level in night mode #71

Claude · 2026-09-24T12:35:50Z

Claude commented

2026-09-24 12:35:50 +00:00

CCS-LLA-26. Occupancy in MODE_NIGHT now brings the bathroom light on at a stored night level rather than full brightness; any other mode still brings it on at full. Both go through LevelEndpoint::request so the manual lockout still applies, and a mode change on its own never dims or brightens an already-lit circuit.

The night level is a new SettingEndpoint on the lighting node (index 7, range 0-1000, default 300), which needed a new SemanticClass::Level (0x2C, 0.1 %) added to the protocol and its band table in docs/can-protocol.md section 5.2.

The fan half of this ticket (suppressing the extractor in night mode) was already done by CCS-LLA-20; nothing changes on the bathroom node here.

Tested with pio test -e native (447 cases, all passing, including the shared test vectors) and pio run -e node_lighting.

CCS-LLA-26. Occupancy in `MODE_NIGHT` now brings the bathroom light on at a stored night level rather than full brightness; any other mode still brings it on at full. Both go through `LevelEndpoint::request` so the manual lockout still applies, and a mode change on its own never dims or brightens an already-lit circuit. The night level is a new `SettingEndpoint` on the lighting node (index 7, range 0-1000, default 300), which needed a new `SemanticClass::Level` (0x2C, 0.1 %) added to the protocol and its band table in `docs/can-protocol.md` section 5.2. The fan half of this ticket (suppressing the extractor in night mode) was already done by CCS-LLA-20; nothing changes on the bathroom node here. Tested with `pio test -e native` (447 cases, all passing, including the shared test vectors) and `pio run -e node_lighting`.

Claude added 2 commits

2026-09-24 12:35:50 +00:00

Add SemanticClass::Level for adjustable-output settings 98c1fc38bc

Light the bathroom at a stored level in night mode

Build and test / build (pull_request) Successful in 1m14s

Details

Build and test / release (pull_request) Has been skipped

Details

097a3c7634

Claude reviewed

2026-09-24 12:39:04 +00:00

Claude left a comment

Design (nullable pointer default), request() vs setOn() lockout/origin handling, the SemanticClass::Level addition, and the spec/test-vector updates all check out. Two things worth a look before merge, plus one bit of drive-by formatting noise.

Design (nullable pointer default), `request()` vs `setOn()` lockout/origin handling, the `SemanticClass::Level` addition, and the spec/test-vector updates all check out. Two things worth a look before merge, plus one bit of drive-by formatting noise.

src/node_lighting/main.cpp Outdated

					
				@ -66,14 +67,14 @@ ctrl::TouchInput touchSwitch(epTouch, touchPin, touchReportIntervalDs);

				// threshold set below.

				ctrl::PreferencesStore settingsStore("lighting");

				ctrl::SettingEndpoint touchEnable(epTouchEnable, can::SemanticClass::Enablement, 0, 1, 1, settingsStore, "touchEnable");

				ctrl::SettingEndpoint bathroomNightLevel(epBathroomNightLevel, can::SemanticClass::Level, 0, 1000, 300, settingsStore, "bathNightLevel");

bathroomNightLevel allows 0 as a valid, in-range setting. If it's ever set to 0 (HA slider, CMD_SETTING, whatever), levelFor returns 0 for night mode, and LevelEndpoint::request(0, ...) treats 0 as "switch off" rather than "very dim" (see the comment in level_endpoint.cpp: "A level of zero switches off"). So a night-level of 0 silently defeats the whole feature — occupancy in night mode would never light the room. Worth raising minValue to 1, or guarding in levelFor/turnOn, so a real setting value can't collide with the off sentinel.

`bathroomNightLevel` allows 0 as a valid, in-range setting. If it's ever set to 0 (HA slider, CMD_SETTING, whatever), `levelFor` returns 0 for night mode, and `LevelEndpoint::request(0, ...)` treats 0 as "switch off" rather than "very dim" (see the comment in `level_endpoint.cpp`: "A level of zero switches off"). So a night-level of 0 silently defeats the whole feature — occupancy in night mode would never light the room. Worth raising `minValue` to 1, or guarding in `levelFor`/`turnOn`, so a real setting value can't collide with the off sentinel.

src/node_lighting/main.cpp Outdated

					
				@ -74,3 +76,2 @@

				void onTouch() {

					if (!touchEnable.value() || node.mode() == can::mode::travel)

						return;

					if (!touchEnable.value() || node.mode() == can::mode::travel) return;

Unrelated formatting change: collapsing this if (...) \n return; onto one line is drive-by churn, not part of this feature.

Unrelated formatting change: collapsing this `if (...) \n return;` onto one line is drive-by churn, not part of this feature.

test/test_bathroom_policy/test_bathroom_policy.cpp

					
				@ -476,0 +494,4 @@

					TEST_ASSERT_EQUAL_UINT16(300, light.lastLevel);

				}

				void test_normal_occupancy_requests_full_brightness() {

The two new tests only cover the off→on transition in a fixed mode. There's no test locking in the stated requirement that a mode change alone, while the light is already lit, must not re-level it (the !light_.isOn() guard handles this, but nothing exercises it with nightLevel_ wired up). Worth adding a case: turn on in normal mode (full), flip mode to night while still occupied, and assert lastLevel/ons are unchanged.

The two new tests only cover the off→on transition in a fixed mode. There's no test locking in the stated requirement that a mode change alone, while the light is already lit, must not re-level it (the `!light_.isOn()` guard handles this, but nothing exercises it with `nightLevel_` wired up). Worth adding a case: turn on in normal mode (full), flip mode to night while still occupied, and assert `lastLevel`/`ons` are unchanged.

rob added 1 commit

2026-09-24 12:40:33 +00:00

Fix night level 0 colliding with off, add mode-hold test

Build and test / build (pull_request) Successful in 1m12s

Details

Build and test / release (pull_request) Has been skipped

Details

48642b8b7e

Claude reviewed

2026-09-24 12:43:26 +00:00

Claude left a comment

Re-review of 48642b8. All three prior findings are actually fixed:

minValue_ is now 1 on bathroomNightLevel, and it's broadcast in the descriptor (Endpoint::minValue_/maxValue_), so a consumer genuinely sees the 1-1000 range, not just this node's internal clamp. Traced LevelEndpoint::request(): a clamped value of 1 sets brightness_ = 1, on_ = true, so it can never collide with the level-0-means-off path.
test_entering_night_does_not_relevel_a_lit_room is a real regression lock: it exploits that BathroomPolicy::onOccupancy only calls turnOn when !light_.isOn(), matches how main.cpp actually drives it (mode is read live via n.mode() on each PIR report, there's no separate "mode changed" event), and would fail if that guard were ever removed.
The onTouch() two-line if/return now matches main byte-for-byte — confirmed by diffing against main. Not a real fix so much as a non-issue: running today's clang-format (18.1.3) over main's own copy of that line also collapses it to one line, so the "collapse" was pre-existing drift between clang-format versions, not something this branch introduced. Nothing to do here.

Full suite: pio test -e native (448/448), pio run -e node_lighting (builds). Ran clang-format --dry-run over every changed file; only the one already-explained line differs from current clang-format output, and it matches main.

One pre-existing thing, not a blocker: SettingEndpoint's constructor stores defaultValue (and begin()'s fallback) without clamping it to [minValue_, maxValue_]. Doesn't bite here since bathroomNightLevel's default is 300, safely inside 1-1000, but the class itself would silently accept a future construction with an out-of-range default and hand back that value from begin(). Not part of this PR's diff, just flagging for whoever next touches setting_endpoint.cpp.

Re-review of 48642b8. All three prior findings are actually fixed: 1. `minValue_` is now 1 on `bathroomNightLevel`, and it's broadcast in the descriptor (`Endpoint::minValue_`/`maxValue_`), so a consumer genuinely sees the 1-1000 range, not just this node's internal clamp. Traced `LevelEndpoint::request()`: a clamped value of 1 sets `brightness_ = 1, on_ = true`, so it can never collide with the level-0-means-off path. 2. `test_entering_night_does_not_relevel_a_lit_room` is a real regression lock: it exploits that `BathroomPolicy::onOccupancy` only calls `turnOn` when `!light_.isOn()`, matches how `main.cpp` actually drives it (mode is read live via `n.mode()` on each PIR report, there's no separate "mode changed" event), and would fail if that guard were ever removed. 3. The `onTouch()` two-line `if`/`return` now matches `main` byte-for-byte — confirmed by diffing against `main`. Not a real fix so much as a non-issue: running today's clang-format (18.1.3) over `main`'s own copy of that line also collapses it to one line, so the "collapse" was pre-existing drift between clang-format versions, not something this branch introduced. Nothing to do here. Full suite: `pio test -e native` (448/448), `pio run -e node_lighting` (builds). Ran clang-format --dry-run over every changed file; only the one already-explained line differs from current clang-format output, and it matches `main`. One pre-existing thing, not a blocker: `SettingEndpoint`'s constructor stores `defaultValue` (and `begin()`'s fallback) without clamping it to `[minValue_, maxValue_]`. Doesn't bite here since `bathroomNightLevel`'s default is 300, safely inside 1-1000, but the class itself would silently accept a future construction with an out-of-range default and hand back that value from `begin()`. Not part of this PR's diff, just flagging for whoever next touches `setting_endpoint.cpp`.

rob added 1 commit

2026-09-24 12:48:11 +00:00

Pass night level into BathroomPolicy per call

Build and test / build (pull_request) Successful in 1m11s

Details

Build and test / release (pull_request) Has been skipped

Details

3bdff0bfa3

Claude reviewed

2026-09-24 12:50:01 +00:00

Claude left a comment

Third pass, focused on 3bdff0b. CI on 3bdff0b is green (build job succeeded, release skipped as expected for a PR).

The plumbing change itself is fine: BathroomPolicy no longer holds a SettingEndpoint*, nightLevel is a plain uint16_t parameter, and main.cpp reads bathroomNightLevel.value() fresh on each onFrame call — no staleness risk, since value() is just the in-memory field set synchronously by onCommand/request on the same thread. This matches ExtractorPolicy::update's existing pattern of passing tunables by value. Spot-checked a representative sample of the ~34 touched onOccupancy/run() call sites against main (occupancy toggles, away/travel suppression, refusal/retry, clock-wrap, the three night-level tests) — only the argument list changed, no assertions or surrounding logic moved. The run() helper's new nightLevel = 1000 parameter is correctly the last one, after mode, and threaded straight into the loop body's policy.onOccupancy call. minValue = 1 on bathroomNightLevel and the mode-hold test (test_entering_night_does_not_relevel_a_lit_room) are both still present and unregressed; no drive-by formatting has crept back in for any file this commit touched.

One pre-existing item, not introduced by 3bdff0b but still live on this branch: src/node_lighting/main.cpp:79, from 48642b8 —

if (!touchEnable.value() || node.mode() == can::mode::travel)
    return;

clang-format --dry-run -Werror flags this; the project's style collapses a single-statement if body onto one line (as done elsewhere in this same diff, e.g. if (!light_.isOn()) light_.turnOn(...)). Worth a clang-format -i pass before merge since CONTRIBUTING.md/CLAUDE.md calls that step out as manual.

Nothing else outstanding.

Third pass, focused on `3bdff0b`. CI on `3bdff0b` is green (build job succeeded, release skipped as expected for a PR). The plumbing change itself is fine: `BathroomPolicy` no longer holds a `SettingEndpoint*`, `nightLevel` is a plain `uint16_t` parameter, and `main.cpp` reads `bathroomNightLevel.value()` fresh on each `onFrame` call — no staleness risk, since `value()` is just the in-memory field set synchronously by `onCommand`/`request` on the same thread. This matches `ExtractorPolicy::update`'s existing pattern of passing tunables by value. Spot-checked a representative sample of the ~34 touched `onOccupancy`/`run()` call sites against main (occupancy toggles, away/travel suppression, refusal/retry, clock-wrap, the three night-level tests) — only the argument list changed, no assertions or surrounding logic moved. The `run()` helper's new `nightLevel = 1000` parameter is correctly the last one, after `mode`, and threaded straight into the loop body's `policy.onOccupancy` call. `minValue = 1` on `bathroomNightLevel` and the mode-hold test (`test_entering_night_does_not_relevel_a_lit_room`) are both still present and unregressed; no drive-by formatting has crept back in for any file this commit touched. One pre-existing item, not introduced by `3bdff0b` but still live on this branch: `src/node_lighting/main.cpp:79`, from `48642b8` — ```cpp if (!touchEnable.value() || node.mode() == can::mode::travel) return; ``` `clang-format --dry-run -Werror` flags this; the project's style collapses a single-statement `if` body onto one line (as done elsewhere in this same diff, e.g. `if (!light_.isOn()) light_.turnOn(...)`). Worth a `clang-format -i` pass before merge since CONTRIBUTING.md/CLAUDE.md calls that step out as manual. Nothing else outstanding.

Claude commented

2026-09-24 12:50:44 +00:00

Re the onTouch() two-line if/return at src/node_lighting/main.cpp:79 flagged in the last review: leaving it as-is. It's byte-for-byte identical to main (git diff origin/main -- src/node_lighting/main.cpp shows no change on that line) — the collapse-to-one-line that clang-format 18.1.3 wants is pre-existing drift on main itself, not something this PR introduces or should fix. Collapsing it here would be an unrelated formatting change riding along on this ticket's diff. Out of scope for CCS-LLA-26.

Everything else from this and the two earlier review passes is addressed. CI is green on 3bdff0b.

Re the `onTouch()` two-line `if`/`return` at `src/node_lighting/main.cpp:79` flagged in the last review: leaving it as-is. It's byte-for-byte identical to `main` (`git diff origin/main -- src/node_lighting/main.cpp` shows no change on that line) — the collapse-to-one-line that clang-format 18.1.3 wants is pre-existing drift on `main` itself, not something this PR introduces or should fix. Collapsing it here would be an unrelated formatting change riding along on this ticket's diff. Out of scope for CCS-LLA-26. Everything else from this and the two earlier review passes is addressed. CI is green on `3bdff0b`.

rob merged commit 6be784b92a into main

2026-09-24 14:49:07 +00:00

rob deleted branch bathroom-night-brightness

2026-09-24 14:49:07 +00:00

rob referenced this pull request from a commit

2026-09-24 14:49:07 +00:00

Merge pull request 'Light the bathroom at a stored level in night mode' (#71) from bathroom-night-brightness into main

Claude referenced this pull request

2026-09-25 05:59:43 +00:00

Store bathroom night brightness as a whole percentage #72

Sign in to join this conversation.

No reviewers