Restructure into a package, and replace the three exposure modes with one ladder - #13
Conversation
The refactor ahead moves every module in the project. This is what will
show whether moving them changed any exposure decision.
Recorded light goes in -- lux, sun elevation, brightness metrics, camera
metadata -- and every settings dict the controller produces comes out,
compared against output recorded from the code as it stands now. The
harness drives the controller through the same call order as the capture
loop, so a divergence means the reorganisation changed behaviour.
Twelve sequences. Six are real, pulled from the capture database: the
dawn and dusk mode boundaries, a converged day, a bright polar night, the
darkest frames on record, and a midsummer noon against the top stop. Six
are synthetic, and exist because mutating the controller's constants one
at a time showed the recorded ones were not enough -- the feedback ratio
clamps, the underexposure thresholds and the clipped-pixel thresholds all
survived being changed.
mutation_check.py is that experiment, kept: it breaks one constant at a
time and asserts the golden tests notice. All 32 mutations are caught.
Every sequence in the synthetic set exists because a mutation survived
until it was added, and three needed the masking diagnosed first --
saturated exposure, an entering-night branch that overrides the ramp
speeds, and an EMA that only lands on a threshold exactly once.
Two findings fell out of that work, both left alone for now:
- min_scale below 0.70 is dead configuration. highlight_factor's last
segment reaches 0.70 at p95 255, the highest an 8-bit frame can have,
before the floor is ever consulted.
- lux is identical across consecutive daylight frames in the database,
because the test shot is pinned at 0.2s and saturates in daylight.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
Every module carried a block like
try:
from src.logging_config import get_logger
except ImportError:
from logging_config import get_logger
because the systemd units ran the modules as scripts, so sys.path[0] was
src/ and `src.x` was not importable. Seventeen of these across twelve
files, and the cost was not only the noise: mypy had to disable no-redef
project-wide because it read every fallback arm as a redefinition, ruff
had to exempt src/ from E402 because the imports followed sys.path edits,
and a CI job existed purely to prove the flat half still worked.
src/ becomes raspilapse/, grouped by what each part talks to -- camera/,
overlay/, video/, storage/, cli/ -- so it is visible at a glance which
parts you can do without. There is one import path per module now, and
all seventeen blocks are gone, along with four stray sys.path bootstraps.
Verification that this changed no behaviour: the golden replay tests pass
unchanged. Same exposure decisions, frame for frame, across all twelve
recorded and synthetic sequences.
Dropped with the idiom:
- mypy's no-redef suppression. Confirmed to leave no no-redef errors
behind; ~60 unrelated pre-existing errors remain, mostly Any leaking
out of config.get(), so mypy stays advisory until those are dealt
with separately.
- ruff's E402 exemption for application code.
- the compatibility-check CI job, replaced by one that installs the
package and checks every module imports and every console script
resolves.
Also fixed: raspilapse/__init__.py declared __version__ = "0.1.0" while
__version__.py, pyproject and the CHANGELOG all said 1.4.0. It now
re-exports the real one.
The units run `python3 -m raspilapse.cli.x` from WorkingDirectory rather
than a path into src/. Console scripts are declared and work after
`pip install -e .`, but nothing requires them: a virtualenv is usually
the wrong move on Raspberry Pi OS, because picamera2 comes from apt and
a venv's own numpy shadows the one it was built against. Verified on the
live camera -- units reinstalled, service restarted, capture continuous.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
…ree packages
The install line asked for seven apt packages and a pip invocation before
you could take a photo. Two of them were redundant: python3-picamera2
depends on python3-numpy and python3-pil, so naming them separately only
made the command look bigger than it is.
sudo apt install -y python3-picamera2 python3-yaml ffmpeg
is now the whole thing. astral, requests, requests-toolbelt and matplotlib
each buy exactly one feature and are installed separately if wanted.
This is not what the plan said to do. The plan was to drop numpy by
rewriting the lores metering onto a stdlib histogram, on the premise that
numpy was a barrier to getting started. It is not -- apt pulls it in with
picamera2 either way -- so that rewrite would have touched the
exposure-critical percentile code for no gain to anyone installing this.
The barrier was the install line itself.
What actually needed doing:
- requests was a hard module-level import in storage/upload.py, so a
camera that never uploads still needed an HTTP client to render its
daily video. Now optional, with a sentinel exception class so the
handler still parses, and a clear error naming the apt package.
- install.sh treated every module as required and failed the check if
astral was missing. Required and optional are now separate, and the
optional ones report what each would buy.
Also fixed, found while removing the last sys.path bootstraps: daily.py
shelled out to os.path.join(project_root, "src", "make_timelapse.py"),
assembled from separate arguments, which is why the previous commit's
grep for "src/" did not find it. The 05:00 video job would have failed
with a file-not-found. Its test asserted only that "make_timelapse.py"
appeared in the command, which stayed true after the path stopped
existing; it now resolves the target with find_spec, and was confirmed to
fail against the broken version.
New tests: test_optional_dependencies.py hides each optional module from
the import machinery -- patching the attribute to None cannot catch a
top-level import, because by then it has already succeeded -- plus a
guard asserting the hiding itself works, so the suite cannot pass
vacuously.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
`overlay.enabled: false` was supposed to mean "do not draw text on my frames". It also silently switched off all database logging. ImageOverlay.__init__ returned early when disabled, before assigning self.weather. The capture loop read capture.overlay.weather to fill the database's weather columns, so with the overlay off that raised AttributeError into an `except Exception` that logged at DEBUG -- a level nobody runs in production. Every capture went unrecorded and nothing said so. Five hasattr(self, "ships") guards elsewhere in the class were the same bug worked around rather than fixed; they are gone with it. The underlying mistake was treating weather as an overlay feature. It is a data source: the database has columns for it whether or not anything is drawn. The daemon now owns a WeatherData directly. The weather cache is process-wide and keyed by endpoint, so the daemon's instance and the overlay's share it -- two instances, one HTTP request. Pillow was also a hard requirement of taking a photo, because camera/capture.py imported the renderer at module level. ImageCapture now takes a post_process callable and has no idea what an overlay is; overlay.build_overlay returns one only when the overlay is switched on, importing Pillow only in that case. Verified by hiding PIL from the import machinery: the capture path imports clean. Also fixed here, and caused by the package move: the ship icon. The path was Path(__file__).parent.parent / "icons", which reached the repo root from src/ but lands in raspilapse/ from raspilapse/overlay/. Guarded by an exists() check, so the icon just quietly stopped loading. It uses PROJECT_ROOT now, and logs a warning when the file is missing rather than saying nothing. Verified on the live camera: service restarted, capture continuous, the overlay renders with weather, exposure, tide curve and aurora intact, and weather is landing in the database again. Each fix was confirmed by reverting it and watching the tests fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
…to 69 config.example.yml was the starting point and the schema at once, so it had to spell out every setting the code might read. Six hundred and eighty-one lines is a lot to face before taking a first photograph, and copying it wholesale is how a config ends up pinning values its owner never chose. The code indexed 35 config paths without a fallback -- config["camera"] ["resolution"], config["adaptive_timelapse"]["interval"] and so on -- and those are what forced the file to be complete. They now have defaults in raspilapse/config.py, merged underneath whatever the user wrote, so a config file only has to say what it wants to change. An eight-line file works. The values are exactly what the old example shipped, so nothing changes for an existing config. Keys already read with .get(key, fallback) are deliberately not duplicated into the table -- that fallback is the default, and a second copy is a second thing to keep in step. config/config.example.yml 69 lines: what to set, and what is optional docs/CONFIG-REFERENCE.yml the former example, unchanged, as reference The schema-drift tests move to the reference and keep their guarantees: a documented key nothing reads still fails, and so does a key read into an attribute nothing loads. Two new ones matter more: test_every_hard_indexed_key_has_a_default walks the AST for config subscripts with no fallback and requires each to be in DEFAULTS. This is the invariant that lets the example stay short -- adding config["new_section"] to the code fails here rather than on someone's camera. Confirmed by adding such a subscript and watching it fail. test_the_hard_index_scan_finds_something, because a scan that silently matched nothing would make the test above pass for the wrong reason. Verified on the live camera: installer check passes, service restarted, capture continuous at 30s with weather still recording. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
run() was the first thing anyone reads to understand what this project
does, and it was 287 lines at five levels of indentation. Six locals were
threaded through it, initialised to None at the top so that a later
diagnostics call would not NameError when a branch skipped them.
It is 97 lines now, and the body reads as what it does:
decision = self._meter() measure light, pick mode, get settings
capture_frame(...) take the frame
self._observe(...) feed brightness back to the controller
self._record(...) diagnostics, WB reference, database row
The six locals became a Decision dataclass, so a frame's decision travels
as one value rather than as a convention about which variables happen to
be set. Nothing in daemon.py is over 100 lines any more.
Two things dropped out of the split rather than being changed on purpose:
- The inline transition-position arithmetic was a copy of
ExposureController.transition_position, including the clamp. It now
calls the method, which additionally guards the None and
divide-by-zero cases the copy did not.
- store_capture failures logged at DEBUG. That is the level that let a
disabled overlay silently lose every database row for as long as it
did. Now a warning.
Behaviour is unchanged: the golden replay tests pass, and the AST check
that pins the sun-elevation evaluation order still fires -- it walks the
whole module, so it followed the code from run() into _meter(). Confirmed
by reintroducing the inline form and watching it fail.
Verified on the live camera: service restarted, capture continuous, and
the per-frame diagnostics block still written in full.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
Mode selection compared an uncalibrated lux figure against absolute
thresholds -- night: 3, day: 80 -- that had to be retuned for every
camera and every site, and were overridden at high latitude by sun
elevation because they could not survive being moved. The camera did not
work out of the box anywhere but here.
A camera has two ways to gather more light and they are not
interchangeable: a longer shutter costs time, more gain costs noise. So
the order is forced, and that single rule is the whole of what the three
modes were. _settings_day pinned gain at its floor; _settings_transition
did the same until the shutter reached 80% of its ceiling;
_settings_night opened the shutter fully and then raised gain. Three
regions of one curve, with different log strings.
required = last * (target / measured) ** damping
shutter, gain = allocate(required, ceiling, max_gain)
Deleted: determine_mode, apply_hysteresis (nothing discrete is left to
flip between), _is_polar_day and the civil-twilight override, the hybrid
brightness override, the three settings builders, the separate gain and
exposure interpolators, the entering-night coordinated ramp, and the
night gain-reduction path. Config keys light_thresholds, reference_lux,
hysteresis_frames, gain_transition_speed, civil_twilight_threshold and
smooth_wb_in_day_mode went with them -- the schema-drift test found each
one and refused to let the reference keep documenting them.
Mode survives as a label derived from the settings, so it cannot
disagree with what the camera is doing. The old one could: 368k frames
were labelled "day", and among them were frames at a 20-second exposure
and gain 5.5, because the polar override set the label while the shutter
was wide open.
Astral is demoted, not removed. Sun elevation is still recorded with
every frame and still feeds graph_solar_patterns.py; nothing decides
from it. Without astral installed the column is NULL and the camera
behaves identically, which is what "location does not matter" has to
mean.
Evidence this is better, not merely different -- tests/replay/compare.py,
which runs both controllers against the same recorded light with the loop
closed, across 3400 frames of this camera's history:
brightness error 35.0 -> 25.6
flicker, stops 0.077 -> 0.031
settled 38% -> 61%
Closing that loop mattered. The first version replayed recorded
brightness, which is a property of the exposure the old controller
happened to be using -- so the new one lowered exposure, the measurement
did not respond, and it lowered again into a runaway that cannot happen
on a camera. It reported the ladder as 13 stops darker. The replay
harness derives scene luminance instead and shows each controller the
brightness its own choice would have produced.
Two bugs that only that measurement found:
- a near-black frame held the exposure where it was, because the guard
treated any brightness under 1.0 as a missing measurement. Under the
old night mode it never showed; on the ladder it left a simulated
polar night stuck at the cold-start 20 ms for 300 frames.
- a uniform rate limit made daylight sluggish. The old code applied no
interpolation in day mode at all and 8% per frame in night, so the
rate now scales with ladder position between those two ends.
Golden files re-recorded, which the tests exist to make deliberate. The
mutation set was rebuilt for the new code: 33 mutations, all caught.
Three constants are documented as absent from it rather than silently
dropped -- two proven unreachable, one needing four simultaneous
conditions and pinned in test_metering.py instead. Fixing that last one
exposed a unit test of my own that covered nothing, asserting a value on
the wrong side of both thresholds.
Verified on the live camera: restarted, converged in three frames, stable
at 210us against a highlight-protected target, overlay and diagnostics
intact.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
Every capture cost two camera cycles: close the running camera, open a second one for a fixed-settings metering shot, close that, open again with the real settings. Thirty times a minute, for the life of the installation. It cost that because the shot did two jobs. Neither needs its own frame any more, or needs one as often: Lux is now measured from the frame the camera was taking anyway. Same arithmetic, from that frame's own brightness and the settings the sensor actually used. Since the ladder landed, nothing decides from lux -- it is a number for the overlay and the graphs -- so paying two camera restarts a frame for it had stopped making sense. White balance still needs the ISP, because the metering shot is the only frame taken with AWB enabled and there is no other way to learn what the scene's white is. But colour changes when light changes, so that is the trigger now: movement along the exposure ladder since the last reading, with a one-hour floor for drift the ladder does not see. Measured over 20 frames on the camera: 40 camera initialisations before, 22 after. Two things this turned up, both measured rather than assumed: Settings cannot be pushed to a running camera. set_controls looks like it should work; on this hardware a commanded exposure took eight frames to appear in the returned metadata, and four consecutive captures after a change all came back carrying the value from two commands earlier. At a 20-second night exposure that is 160 seconds of wrong frames against a teardown costing two. Keeping the camera open between frames is therefore not available, and the loop still tears it down -- once per frame instead of twice. Skipping even that teardown when the settings barely move was built, measured, and removed. The sensor quantises exposure to whole lines -- it delivers 210us for a commanded 217us -- so command and delivery differ by more than any useful tolerance on every frame and the branch never fired. It is gone rather than left in place looking useful. The lux column changes scale here, deliberately. It used to be measured from a shot pinned at 0.2s, which saturates in daylight: 368 thousand rows of this database carry the identical value 887.190349001447. Continuity with a constant is not worth keeping. Calibrated so full daylight reads around 20,000 and a 20-second night frame reads 0.03, which is roughly what a light meter would say. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe project is migrated from ChangesPackage migration and exposure-control overhaul
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
tests/replay/compare.py-307-319 (1)
307-319: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
statistics.fmeanraises on an empty list when every sequence scores NaN.
brightness_errorandsettled_fractionreturn NaN whenever a sequence has no frames pastWARMUP_FRAMES(20).synthetic_threshold_edge_dayandsynthetic_threshold_edge_nightare exactly 20 frames, socompare.py synthetic_threshold_edge_dayleavesold_values/new_valuesempty and line 316 dies withStatisticsErrorinstead of printing a summary.🐛 Proposed guard
old_values = [r[old_key] for r in results if not math.isnan(r[old_key])] new_values = [r[new_key] for r in results if not math.isnan(r[new_key])] + if not old_values or not new_values: + print(f" {label:18} no comparable frames") + continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/replay/compare.py` around lines 307 - 319, Guard the summary calculation in the comparison logic around old_values, new_values, and the statistics.fmean calls so all-NaN result sets do not raise StatisticsError. Preserve the existing averages and win-count output for non-empty values, while printing an appropriate empty-result summary when no valid scores remain.tests/replay/compare.py-92-94 (1)
92-94: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTruly black frames are discarded as "missing metadata".
not brightnessrejectsmean_brightness == 0.0, which is a measurement, not an absence — the same distinction_required_exposureinraspilapse/camera/exposure.py(lines 277-284) was deliberately changed to make. These frames are then excluded from theusablecount at line 223 and from both controllers' closed loops, so the darkest frames ofdeep_darkandcrashing_lightare dropped from exactly the comparison used to justify the ladder.🐛 Proposed fix
- if not brightness or not exposure_us: + if brightness is None or not exposure_us: out.append(None) continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/replay/compare.py` around lines 92 - 94, Update the metadata presence check near the replay comparison flow so a brightness value of 0.0 is accepted as a valid measurement; only treat brightness as missing when it is None (while preserving the equivalent exposure validation). Ensure truly black frames continue through usable counting and both controller comparison loops.tests/replay/compare.py-128-186 (1)
128-186: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMirror the DAY→manual seeding in
simulate_legacy. The harness seedsseed_from_metadata(...)on the first transition out of DAY, but this path only callsreset_seed_state()on DAY entry. That leaves the legacy replay missing the same handover setup and can skew the comparison.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/replay/compare.py` around lines 128 - 186, Update simulate_legacy to mirror the DAY-to-manual handover behavior by invoking seed_from_metadata(...) on the first transition out of DAY, in addition to the existing reset_seed_state() call on DAY entry. Track the prior mode and ensure this seeding occurs only when transitioning from modes.DAY to a non-DAY mode, using the available frame/config metadata required by the controller.raspilapse/cli/apply_overlay.py-27-39 (1)
27-39: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPoint examples at the actual module.
raspilapse.cli.overlayis not the overlay CLI module; these commands fail before parsing arguments. Useraspilapse.cli.apply_overlayconsistently.Proposed fix
- python3 -m raspilapse.cli.overlay test_photos/kringelen_2025_11_05_10_30_45.jpg + python3 -m raspilapse.cli.apply_overlay test_photos/kringelen_2025_11_05_10_30_45.jpg ... - python3 -m raspilapse.cli.overlay test_photos/*.jpg + python3 -m raspilapse.cli.apply_overlay test_photos/*.jpg🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raspilapse/cli/apply_overlay.py` around lines 27 - 39, Update all overlay CLI examples in the documentation block to invoke the actual raspilapse.cli.apply_overlay module instead of raspilapse.cli.overlay, preserving the existing arguments and command examples.raspilapse/daemon.py-785-794 (1)
785-794: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
not brightnessconflates a genuinely black frame with a missing measurement.
mean_brightness == 0.0is a real reading in deep dark, but it returnsNone, so the row stores no lux at all instead of ~0. Test explicitly forNoneon the inputs and keep the arithmetic guard for the divisors.🐛 Proposed fix
- if not brightness or not exposure_us or not gain: + if brightness is None or exposure_us is None or gain is None: return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raspilapse/daemon.py` around lines 785 - 794, Update the lux calculation around the settings lookup to treat brightness, exposure_us, and gain as missing only when they are None, allowing a valid mean_brightness of 0.0 to produce a near-zero lux value. Preserve the existing positive-value guards for exposure_us and gain before performing the arithmetic.docs/OVERLAY.md-143-148 (1)
143-148: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the packaged overlay module name.
raspilapse.cli.overlayis not the documented entry-point module; the package exposesraspilapse.cli.apply_overlay. Both examples fail as written. Usepython3 -m raspilapse.cli.apply_overlay ...orraspilapse-overlay ....🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/OVERLAY.md` around lines 143 - 148, Update both command examples in the overlay documentation to invoke the packaged entry point raspilapse.cli.apply_overlay, or consistently use the raspilapse-overlay executable, so the documented commands run successfully.docs/CONFIG-REFERENCE.yml-170-170 (1)
170-170: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale
src/paths and pre-migration script names.Line 611's
python3 src/database.py --vacuumis now unrunnable after the package move, and the section headers still nameauto_timelapse.py/daily_timelapse.py.📝 Proposed fix
-# Adaptive Timelapse Settings (for auto_timelapse.py) +# Adaptive Timelapse Settings (for raspilapse.cli.capture)-# Video Upload Settings (for daily_timelapse.py) +# Video Upload Settings (for raspilapse.cli.daily)# Pruning runs from raspilapse-cleanup.timer. Space is only handed back to - # the filesystem by an explicit `python3 src/database.py --vacuum`, which is - # slow and needs free disk equal to the database size. + # the filesystem by an explicit `python3 -m raspilapse.cli.db --vacuum`, + # which is slow and needs free disk equal to the database size.Also applies to: 555-555, 610-612
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/CONFIG-REFERENCE.yml` at line 170, Update the configuration reference’s adaptive timelapse section and related entries to use the current post-migration script names and paths, replacing auto_timelapse.py, daily_timelapse.py, and the stale src/database.py vacuum command. Ensure the documented vacuum command is runnable from the current package layout while preserving the existing configuration guidance.docs/CONFIG-REFERENCE.yml-1-8 (1)
1-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHeader still frames this as a copy-me template.
Every doc that points here calls it reference-only (
config/README.md: "Reference, not a template"), andconfig/config.example.ymlis the file to copy. Telling readers tocp config/config.example.yml config/config.ymlfrom inside the reference — while the surrounding text says "This is the default configuration template" — undoes the split this PR is making.📝 Proposed fix
-# Raspilapse Configuration File (Example/Template) -# -# IMPORTANT: This is the default configuration template. -# Copy this file to config.yml and customize it: -# cp config/config.example.yml config/config.yml -# -# Your config.yml will NOT be tracked by git, so you can safely -# customize it with your personal settings (API keys, paths, etc.) +# Raspilapse configuration reference. +# +# Every setting there is, with its default. This is documentation, not a +# template -- do not copy it wholesale. Start from +# config/config.example.yml and copy in only the blocks you want to change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/CONFIG-REFERENCE.yml` around lines 1 - 8, Update the header in CONFIG-REFERENCE.yml to describe the file as reference-only rather than a copyable template, and remove or revise the copy command so it points readers to config/config.example.yml as the file to copy. Preserve the guidance about customizing the untracked config.yml.docs/INSTALL.md-53-53 (1)
53-53: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win"polar-day override" contradicts the new exposure model.
README.mdlines 161-163 state that without astral the camera behaves identically and nothing decides from sun elevation. Astral now buys only the recorded elevation column and the graph script.Note the same stale claim sits in the unchanged prose at lines 102-104 ("sun elevation decides when day begins and ends"), which is worth correcting in the same pass.
📝 Proposed fix
-| `pip3 install --break-system-packages 'astral>=3.2'` | sun elevation recorded with each frame, and the polar-day override | +| `pip3 install --break-system-packages 'astral>=3.2'` | sun elevation recorded with each frame, for the database and the solar graphs |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/INSTALL.md` at line 53, Update the Astral dependency description in the installation table to mention only recorded sun elevation and graph-script support, removing the polar-day override claim. Also revise the related prose around the sun-elevation day-boundary behavior so it matches the documented exposure model: Astral must not be described as deciding when day begins or ends.tests/test_overlay_optional.py-28-35 (1)
28-35: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDisable weather explicitly in these fixtures.
disabled_configomits aweathersection entirely, soget_weather_data()depends onWeatherData's internal default to avoid a real HTTP fetch. The daemon test below already pins{"weather": {"enabled": False}}— do the same here so the test can never reach the network.🛡️ Proposed fix
`@pytest.fixture` def disabled_config(): - return {"overlay": {"enabled": False}} + return {"overlay": {"enabled": False}, "weather": {"enabled": False}} `@pytest.fixture` def enabled_config(): - return {"overlay": {"enabled": True, "font": {"family": "default"}}} + return { + "overlay": {"enabled": True, "font": {"family": "default"}}, + "weather": {"enabled": False}, + }Also applies to: 50-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_overlay_optional.py` around lines 28 - 35, Update the disabled_config and enabled_config fixtures to explicitly include weather enabled=False, ensuring overlay tests cannot trigger real weather requests while preserving their existing overlay settings.tests/replay/harness.py-124-139 (1)
124-139: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
not brightnessswallows a legitimate zero.A frame that genuinely measured
mean_brightness == 0.0(deep-dark sequences) is treated as "no metadata" and never fed toobserve_frame, so the controller silently skips the closed loop for exactly the frames where correction matters most. Test forNoneinstead.🐛 Proposed fix
- if not brightness or not exposure_us: + if brightness is None or not exposure_us: return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/replay/harness.py` around lines 124 - 139, Update scene_luminance to distinguish a missing mean_brightness from a valid zero value: check brightness explicitly against None rather than using a falsy check. Preserve the existing handling for absent exposure_us and continue returning zero luminance when brightness is 0.0 and the exposure product is positive.tests/test_config_example.py-41-45 (1)
41-45: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDrop
ALLOWED_UNDOCUMENTEDor wire it into the coverage test. It isn’t referenced anywhere intests/test_config_example.py, andraspilapse.storage.database.retention_daysdoesn’t match the config-path checks used here (database.retention_days).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_config_example.py` around lines 41 - 45, Remove the unused ALLOWED_UNDOCUMENTED constant from tests/test_config_example.py, or integrate it into the coverage test using the normalized database.retention_days path expected by the existing checks.
🧹 Nitpick comments (10)
raspilapse/camera/exposure.py (1)
200-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
smooth_luxcan returnNone, but is annotated-> float.When
raw_luxisNoneand nothing has been smoothed yet, line 208 returnsNone. Callers (replay harness, capture loop) record this value; the annotation should reflect it.♻️ Proposed signature fix
- def smooth_lux(self, raw_lux: float) -> float: + def smooth_lux(self, raw_lux: Optional[float]) -> Optional[float]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raspilapse/camera/exposure.py` around lines 200 - 214, Update the smooth_lux method’s return annotation to allow None, matching its existing behavior when raw_lux and _smoothed_lux are both unset. Leave the smoothing logic and returned values unchanged.raspilapse/camera/metering.py (2)
324-417: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThresholds are hardcoded while the rates they select are configurable.
Lines 338-339 and 388 hold the trigger/release points as literals, yet
_fast_down,_critical_up, etc. all come fromtransition_mode. Anyone tuning a camera can change how fast it recovers but not when it decides to. Worth lifting into the same config section, or at least into module constants next toBrightnessZones.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raspilapse/camera/metering.py` around lines 324 - 417, Move the exposure trigger and release thresholds used by _check_overexposure and _check_underexposure out of their local literals into the existing transition-mode configuration, or shared module-level constants beside BrightnessZones. Update both methods to consume those centralized values while preserving the current two-tier severity and hysteresis behavior.
202-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_dynamic_targetis annotated-> intbut can return a float.Line 218 returns
self._base_target + self._overcast_boostunrounded; both come from config and may be floats. Only the interpolated branch rounds. This flows intodiagnostics()["target_brightness"], so the golden fixtures would change shape for a float-configured boost.♻️ Proposed fix
if std <= self._contrast_low: - return min(self._base_target + self._overcast_boost, self._max_target) + return min(int(round(self._base_target + self._overcast_boost)), self._max_target)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raspilapse/camera/metering.py` around lines 202 - 222, Update _dynamic_target so the low-contrast branch returning self._base_target plus self._overcast_boost rounds the boosted value to an integer before applying the max-target clamp, matching the interpolated branch and preserving the declared int return type.tests/replay/synthetic_sequences.py (2)
155-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese fixture descriptions document a controller that no longer exists.
determine_mode, the lux thresholds (night: 3,day: 80) and the hybrid override are all removed by this PR —raspilapse/camera/exposure.pyderives the mode from the settings instead. The sequences remain useful as regression inputs, butsynthetic_threshold_edge_*andsynthetic_hybrid_overridenow claim to exercise branches that are gone, and the embeddeddescriptionstrings ship into the fixture JSON.Worth a sentence in each saying they are retained as legacy-boundary regression inputs rather than tests of a live threshold.
Also applies to: 354-369
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/replay/synthetic_sequences.py` around lines 155 - 172, Update the descriptions for synthetic_threshold_edge_night, synthetic_threshold_edge_day, and synthetic_hybrid_override to state that these sequences are retained as legacy-boundary regression inputs, not coverage of the removed threshold or hybrid controller branches. Keep the fixture data unchanged and ensure the revised descriptions do not claim to test determine_mode, lux thresholds, or hybrid override behavior.
20-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo
sys.pathinserts and two import styles for sibling modules.Line 24 exists only so line 27 can import
extract_sequencesbare, while line 29 imports its sibling through the package. Dropping the first insert and usingfrom tests.replay.extract_sequences import OUT_DIR, REPLAY_CONFIGmakes both imports resolve the same way.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/replay/synthetic_sequences.py` around lines 20 - 29, Remove the sys.path insertion targeting the tests/replay directory and update the extract_sequences import to use the tests.replay.extract_sequences package path, keeping the repository-root path insertion and dump_frames import unchanged.raspilapse/camera/capture.py (1)
488-497: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unreachable
elifbranch.request.get_metadata()already yields a dict on this path, so the fallback warning never fires.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raspilapse/camera/capture.py` around lines 488 - 497, Remove the unreachable elif branch following the post_process invocation in the capture flow. Keep the existing post-processing condition, success debug log, and failure warning unchanged, since metadata_dict is always a dictionary on this path.tests/replay/mutation_check.py (1)
261-284: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused temp-directory backup.
shutil.copy2copies each target intotmp(Lines 262-263), but nothing ever reads fromtmpafterward — restoration relies entirely on the in-memoryoriginalsdict (Lines 280, 282-283). The temporary directory and its copies are dead weight and could mislead a future reader into thinking they provide a recovery path (e.g., in case ofkill -9mid-run) when they don't.Consider dropping the
tempfile.TemporaryDirectory()block entirely, sinceoriginalsalready provides restoration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/replay/mutation_check.py` around lines 261 - 284, Remove the unused tempfile.TemporaryDirectory block and its shutil.copy2 backup loop from the mutation workflow. Keep the existing originals-based restoration in the try/finally around the MUTATIONS loop, including restoring each target from originals after each mutation and during final cleanup.README.md (1)
133-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced blocks (markdownlint MD040).
Both blocks are unlabeled;
textis enough to satisfy the linter.📝 Proposed fix
-``` +```text required = current * (target / measured) ** damping-``` +```text raspilapse/Also applies to: 199-222
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 133 - 137, Add the text language identifier to every unlabeled fenced code block in README.md, including the blocks containing the exposure formula and raspilapse/ directory example, while preserving their contents.Source: Linters/SAST tools
tests/replay/harness.py (1)
37-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the pre-migration import fallbacks.
src.exposure/ bareexposureno longer exist after the package move, so these branches can only mask a genuineImportErrorfromraspilapse.camera.exposure(e.g. a broken transitive import) by re-raising a confusing one from a path nobody ships. A plain import keeps failures legible.♻️ Proposed simplification
-def load_controller() -> Callable[..., Any]: - """Return the ExposureController class from wherever it currently lives.""" - try: - from raspilapse.camera.exposure import ExposureController # noqa: F401 - - return ExposureController - except ImportError: - pass - try: - from src.exposure import ExposureController - - return ExposureController - except ImportError: - from exposure import ExposureController - - return ExposureController - - -def load_modes() -> Any: - """Return the LightMode constants from wherever they currently live.""" - try: - from raspilapse.camera.exposure import LightMode # noqa: F401 - - return LightMode - except ImportError: - pass - try: - from src.exposure import LightMode - - return LightMode - except ImportError: - from exposure import LightMode - - return LightMode +def load_controller() -> Callable[..., Any]: + """Return the ExposureController class.""" + from raspilapse.camera.exposure import ExposureController + + return ExposureController + + +def load_modes() -> Any: + """Return the LightMode constants.""" + from raspilapse.camera.exposure import LightMode + + return LightMode🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/replay/harness.py` around lines 37 - 70, Remove the legacy src.exposure and bare exposure fallback imports from load_controller and load_modes. Import ExposureController and LightMode directly from raspilapse.camera.exposure so genuine import failures propagate without being masked.tests/test_daily_timelapse.py (1)
315-318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the claimed behavior instead of just commenting it.
The comment says subprocess should not be called with
--only-upload, but nothing asserts it —mock_runcould be invoked and the test would still pass.✅ Suggested addition
with patch("raspilapse.video.daily.subprocess.run") as mock_run: mock_run.return_value = MagicMock(returncode=0) main() - # Should not have called subprocess since --only-upload was used + # Should not have called subprocess since --only-upload was used + mock_run.assert_not_called()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_daily_timelapse.py` around lines 315 - 318, Update the test around main() and the patched mock_run to assert that subprocess.run was not called with the --only-upload argument, while preserving the existing successful return-code setup and test flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/CONFIG-REFERENCE.yml`:
- Around line 10-12: The documentation must stop describing sun elevation as
controlling exposure decisions. In docs/CONFIG-REFERENCE.yml lines 10-12, remove
the Polar Day/Night and Civil Twilight Override descriptions and state that
location is recorded per frame and read only by the solar graph script; in
docs/INSTALL.md line 53, remove “and the polar-day override,” leaving only the
recorded sun elevation description.
In `@raspilapse/cli/status.py`:
- Around line 202-215: Update StatusDisplay._load_config() to use
raspilapse.config.load_config() or apply merge_defaults() before the status
rendering reads adaptive["night_mode"]. Preserve the existing user-facing error
handling, and ensure configurations omitting adaptive_timelapse.night_mode
receive defaults so the status command continues printing instead of raising
KeyError.
In `@raspilapse/config.py`:
- Around line 89-97: Update merge_defaults to deep-copy all default values when
initializing merged, including nested dictionaries and lists, so returned
configurations cannot alias DEFAULTS. Preserve the existing recursive merge
behavior for user-provided config values while ensuring untouched nested
defaults remain independently mutable per load.
In `@raspilapse/daemon.py`:
- Around line 949-957: Wrap the camera reinitialization block in the inner
tolerant error-handling path used for frame failures, including both
ImageCapture construction and initialize_camera. On initialization errors, log
the failure, perform the existing retry sleep, and continue the capture loop so
transient libcamera errors do not reach the outer handler and terminate the
daemon.
- Around line 706-722: Update _take_reference_shot so the exception path records
the failed reference attempt by setting _reference_frame and _reference_position
to the current frame count and exposure ladder position before returning. Keep
the existing warning and successful metadata updates unchanged, ensuring
_wants_reference_shot() waits until REFERENCE_MAX_INTERVAL_FRAMES before
retrying.
In `@requirements.txt`:
- Line 18: Update the Pillow dependency declaration from Pillow>=10.0.0 to a
minimum version of Pillow>=10.0.1, ensuring the overlay and keogram
image-decoding paths use a patched release.
In `@tests/replay/golden/night_underexposure_edge.json`:
- Line 9: Update the replay fixture or replay update ordering so
diagnostics.applied_exposure_ms, diagnostics.applied_exposure_s, and
diagnostics.applied_gain match the frame settings ExposureTime and AnalogueGain
on the same record. Regenerate the night_underexposure_edge fixture if the
implementation already applies settings correctly; otherwise adjust the replay
state update to record applied values after the settings are applied.
In `@tests/replay/golden/very_bright_night.json`:
- Line 9: Update the replay flow in tests/replay/harness.py so DAY→TRANSITION
diagnostics are captured immediately after decide() returns, before
seed_from_metadata(...) can overwrite _shutter, _gain, and _position. Ensure the
recorded diagnostics reflect the frame’s actual settings, then re-record the
affected golden files.
In `@tests/replay/sequences/stable_day.json`:
- Around line 3-5: Regenerate the stable_day fixture from a genuinely stable
scene so its brightness remains near the stated baseline throughout the replay,
or rename the fixture and update its expected convergence/flicker assertions to
reflect the observed variation. Preserve the stable baseline semantics before
using stable_day for controller-drift validation.
In `@tests/replay/sequences/synthetic_clipping_sweep.json`:
- Around line 4-5: Regenerate the synthetic clipping sweep sequence so frames
8–179 contain non-zero clipped-pixel values spanning below, at, and above the 3%
and 5% thresholds, with corresponding percentile_95 values that exercise the
described controller-rate changes. Update the sequence’s golden output to match
the regenerated replay results while preserving the existing sweep progression
and configuration.
---
Minor comments:
In `@docs/CONFIG-REFERENCE.yml`:
- Line 170: Update the configuration reference’s adaptive timelapse section and
related entries to use the current post-migration script names and paths,
replacing auto_timelapse.py, daily_timelapse.py, and the stale src/database.py
vacuum command. Ensure the documented vacuum command is runnable from the
current package layout while preserving the existing configuration guidance.
- Around line 1-8: Update the header in CONFIG-REFERENCE.yml to describe the
file as reference-only rather than a copyable template, and remove or revise the
copy command so it points readers to config/config.example.yml as the file to
copy. Preserve the guidance about customizing the untracked config.yml.
In `@docs/INSTALL.md`:
- Line 53: Update the Astral dependency description in the installation table to
mention only recorded sun elevation and graph-script support, removing the
polar-day override claim. Also revise the related prose around the sun-elevation
day-boundary behavior so it matches the documented exposure model: Astral must
not be described as deciding when day begins or ends.
In `@docs/OVERLAY.md`:
- Around line 143-148: Update both command examples in the overlay documentation
to invoke the packaged entry point raspilapse.cli.apply_overlay, or consistently
use the raspilapse-overlay executable, so the documented commands run
successfully.
In `@raspilapse/cli/apply_overlay.py`:
- Around line 27-39: Update all overlay CLI examples in the documentation block
to invoke the actual raspilapse.cli.apply_overlay module instead of
raspilapse.cli.overlay, preserving the existing arguments and command examples.
In `@raspilapse/daemon.py`:
- Around line 785-794: Update the lux calculation around the settings lookup to
treat brightness, exposure_us, and gain as missing only when they are None,
allowing a valid mean_brightness of 0.0 to produce a near-zero lux value.
Preserve the existing positive-value guards for exposure_us and gain before
performing the arithmetic.
In `@tests/replay/compare.py`:
- Around line 307-319: Guard the summary calculation in the comparison logic
around old_values, new_values, and the statistics.fmean calls so all-NaN result
sets do not raise StatisticsError. Preserve the existing averages and win-count
output for non-empty values, while printing an appropriate empty-result summary
when no valid scores remain.
- Around line 92-94: Update the metadata presence check near the replay
comparison flow so a brightness value of 0.0 is accepted as a valid measurement;
only treat brightness as missing when it is None (while preserving the
equivalent exposure validation). Ensure truly black frames continue through
usable counting and both controller comparison loops.
- Around line 128-186: Update simulate_legacy to mirror the DAY-to-manual
handover behavior by invoking seed_from_metadata(...) on the first transition
out of DAY, in addition to the existing reset_seed_state() call on DAY entry.
Track the prior mode and ensure this seeding occurs only when transitioning from
modes.DAY to a non-DAY mode, using the available frame/config metadata required
by the controller.
In `@tests/replay/harness.py`:
- Around line 124-139: Update scene_luminance to distinguish a missing
mean_brightness from a valid zero value: check brightness explicitly against
None rather than using a falsy check. Preserve the existing handling for absent
exposure_us and continue returning zero luminance when brightness is 0.0 and the
exposure product is positive.
In `@tests/test_config_example.py`:
- Around line 41-45: Remove the unused ALLOWED_UNDOCUMENTED constant from
tests/test_config_example.py, or integrate it into the coverage test using the
normalized database.retention_days path expected by the existing checks.
In `@tests/test_overlay_optional.py`:
- Around line 28-35: Update the disabled_config and enabled_config fixtures to
explicitly include weather enabled=False, ensuring overlay tests cannot trigger
real weather requests while preserving their existing overlay settings.
---
Nitpick comments:
In `@raspilapse/camera/capture.py`:
- Around line 488-497: Remove the unreachable elif branch following the
post_process invocation in the capture flow. Keep the existing post-processing
condition, success debug log, and failure warning unchanged, since metadata_dict
is always a dictionary on this path.
In `@raspilapse/camera/exposure.py`:
- Around line 200-214: Update the smooth_lux method’s return annotation to allow
None, matching its existing behavior when raw_lux and _smoothed_lux are both
unset. Leave the smoothing logic and returned values unchanged.
In `@raspilapse/camera/metering.py`:
- Around line 324-417: Move the exposure trigger and release thresholds used by
_check_overexposure and _check_underexposure out of their local literals into
the existing transition-mode configuration, or shared module-level constants
beside BrightnessZones. Update both methods to consume those centralized values
while preserving the current two-tier severity and hysteresis behavior.
- Around line 202-222: Update _dynamic_target so the low-contrast branch
returning self._base_target plus self._overcast_boost rounds the boosted value
to an integer before applying the max-target clamp, matching the interpolated
branch and preserving the declared int return type.
In `@README.md`:
- Around line 133-137: Add the text language identifier to every unlabeled
fenced code block in README.md, including the blocks containing the exposure
formula and raspilapse/ directory example, while preserving their contents.
In `@tests/replay/harness.py`:
- Around line 37-70: Remove the legacy src.exposure and bare exposure fallback
imports from load_controller and load_modes. Import ExposureController and
LightMode directly from raspilapse.camera.exposure so genuine import failures
propagate without being masked.
In `@tests/replay/mutation_check.py`:
- Around line 261-284: Remove the unused tempfile.TemporaryDirectory block and
its shutil.copy2 backup loop from the mutation workflow. Keep the existing
originals-based restoration in the try/finally around the MUTATIONS loop,
including restoring each target from originals after each mutation and during
final cleanup.
In `@tests/replay/synthetic_sequences.py`:
- Around line 155-172: Update the descriptions for
synthetic_threshold_edge_night, synthetic_threshold_edge_day, and
synthetic_hybrid_override to state that these sequences are retained as
legacy-boundary regression inputs, not coverage of the removed threshold or
hybrid controller branches. Keep the fixture data unchanged and ensure the
revised descriptions do not claim to test determine_mode, lux thresholds, or
hybrid override behavior.
- Around line 20-29: Remove the sys.path insertion targeting the tests/replay
directory and update the extract_sequences import to use the
tests.replay.extract_sequences package path, keeping the repository-root path
insertion and dump_frames import unchanged.
In `@tests/test_daily_timelapse.py`:
- Around line 315-318: Update the test around main() and the patched mock_run to
assert that subprocess.run was not called with the --only-upload argument, while
preserving the existing successful return-code setup and test flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57b761d0-69f6-405e-bb17-59d5a4c2d214
📒 Files selected for processing (135)
.github/PULL_REQUEST_TEMPLATE.md.github/workflows/tests.ymlCONTRIBUTING.mdMakefileREADME.mdconfig/README.mdconfig/config.example.ymldocs/CONFIG-REFERENCE.ymldocs/EXPOSURE.mddocs/INSTALL.mddocs/OVERLAY.mddocs/TIMELAPSE_VIDEO.mddocs/TROUBLESHOOTING.mdpyproject.tomlraspilapse/__init__.pyraspilapse/__version__.pyraspilapse/camera/__init__.pyraspilapse/camera/capture.pyraspilapse/camera/exposure.pyraspilapse/camera/ladder.pyraspilapse/camera/metering.pyraspilapse/cli/__init__.pyraspilapse/cli/apply_overlay.pyraspilapse/cli/capture.pyraspilapse/cli/daily.pyraspilapse/cli/db.pyraspilapse/cli/retry_uploads.pyraspilapse/cli/snapshot.pyraspilapse/cli/status.pyraspilapse/cli/timelapse.pyraspilapse/config.pyraspilapse/console.pyraspilapse/daemon.pyraspilapse/logging_setup.pyraspilapse/overlay/__init__.pyraspilapse/overlay/layout.pyraspilapse/overlay/render.pyraspilapse/overlay/sources/__init__.pyraspilapse/overlay/sources/json_sources.pyraspilapse/overlay/sources/weather.pyraspilapse/storage/__init__.pyraspilapse/storage/database.pyraspilapse/storage/upload.pyraspilapse/system.pyraspilapse/video/__init__.pyraspilapse/video/daily.pyraspilapse/video/keogram.pyraspilapse/video/timelapse.pyrequirements.txtscripts/db_graphs.pyscripts/db_stats.pyscripts/graph_solar_patterns.pyscripts/install.shsrc/__init__.pysrc/exposure.pysystemd/raspilapse-cleanup.service.insystemd/raspilapse-daily-video.service.insystemd/raspilapse-upload-retry.service.insystemd/raspilapse.service.intests/conftest.pytests/replay/__init__.pytests/replay/compare.pytests/replay/extract_sequences.pytests/replay/golden/blown_highlights.jsontests/replay/golden/bright_night.jsontests/replay/golden/crashing_light.jsontests/replay/golden/dawn_transition.jsontests/replay/golden/deep_dark.jsontests/replay/golden/dusk_transition.jsontests/replay/golden/night_underexposure_edge.jsontests/replay/golden/stable_day.jsontests/replay/golden/synthetic_clamped_highlights.jsontests/replay/golden/synthetic_clipped_pixels.jsontests/replay/golden/synthetic_clipping_sweep.jsontests/replay/golden/synthetic_extreme_gain.jsontests/replay/golden/synthetic_hybrid_override.jsontests/replay/golden/synthetic_low_night_gain.jsontests/replay/golden/synthetic_night_brightness_sweep.jsontests/replay/golden/synthetic_starved_light.jsontests/replay/golden/synthetic_threshold_edge_day.jsontests/replay/golden/synthetic_threshold_edge_night.jsontests/replay/golden/synthetic_underexposure_release.jsontests/replay/golden/very_bright_night.jsontests/replay/harness.pytests/replay/mutation_check.pytests/replay/record_golden.pytests/replay/sequences/blown_highlights.jsontests/replay/sequences/bright_night.jsontests/replay/sequences/crashing_light.jsontests/replay/sequences/dawn_transition.jsontests/replay/sequences/deep_dark.jsontests/replay/sequences/dusk_transition.jsontests/replay/sequences/night_underexposure_edge.jsontests/replay/sequences/stable_day.jsontests/replay/sequences/synthetic_clamped_highlights.jsontests/replay/sequences/synthetic_clipped_pixels.jsontests/replay/sequences/synthetic_clipping_sweep.jsontests/replay/sequences/synthetic_extreme_gain.jsontests/replay/sequences/synthetic_hybrid_override.jsontests/replay/sequences/synthetic_low_night_gain.jsontests/replay/sequences/synthetic_night_brightness_sweep.jsontests/replay/sequences/synthetic_starved_light.jsontests/replay/sequences/synthetic_threshold_edge_day.jsontests/replay/sequences/synthetic_threshold_edge_night.jsontests/replay/sequences/synthetic_underexposure_release.jsontests/replay/sequences/very_bright_night.jsontests/replay/synthetic_sequences.pytests/test_apply_overlay.pytests/test_auto_timelapse.pytests/test_capture_image.pytests/test_colors.pytests/test_config_example.pytests/test_config_utils.pytests/test_create_keogram.pytests/test_daily_timelapse.pytests/test_database.pytests/test_exposure.pytests/test_ladder.pytests/test_logging_config.pytests/test_make_timelapse.pytests/test_make_timelapse_daily.pytests/test_metering.pytests/test_optional_dependencies.pytests/test_overlay.pytests/test_overlay_draw.pytests/test_overlay_optional.pytests/test_overlay_simplified.pytests/test_overlay_sources.pytests/test_replay_golden.pytests/test_retry_uploads.pytests/test_status.pytests/test_system_monitor.pytests/test_upload_service.pytests/test_version.pytests/test_weather.py
💤 Files with no reviewable changes (2)
- src/init.py
- src/exposure.py
| {"diagnostics":{"applied_exposure_ms":52.78,"applied_exposure_s":0.05278,"applied_gain":1.0,"base_target_brightness":120,"ladder_position":0.4478,"last_brightness":0.04,"last_p95":0.08,"mode":"day","overcast_boost_active":true,"required_exposure":0.05278,"target_brightness":135,"target_exposure_ms":52.78,"target_exposure_s":0.05278,"target_gain":1.0},"ladder_position":0.447835,"measured_brightness":0.043,"mode":"day","settings":{"AeEnable":0,"AnalogueGain":1.0,"AwbEnable":0,"ColourGains":[2.314075,1.71655],"ExposureTime":52780},"smoothed_lux":1.328223}, | ||
| {"diagnostics":{"applied_exposure_ms":85.74,"applied_exposure_s":0.085742,"applied_gain":1.0,"base_target_brightness":120,"ladder_position":0.4825,"last_brightness":0.07,"last_p95":0.13,"mode":"day","overcast_boost_active":true,"required_exposure":0.085742,"target_brightness":135,"target_exposure_ms":85.74,"target_exposure_s":0.085742,"target_gain":1.0},"ladder_position":0.482498,"measured_brightness":0.07,"mode":"day","settings":{"AeEnable":0,"AnalogueGain":1.0,"AwbEnable":0,"ColourGains":[2.241464,1.762067],"ExposureTime":85741},"smoothed_lux":1.331788}, | ||
| {"diagnostics":{"applied_exposure_ms":139.29,"applied_exposure_s":0.139288,"applied_gain":1.0,"base_target_brightness":120,"ladder_position":0.5172,"last_brightness":0.12,"last_p95":0.22,"mode":"day","overcast_boost_active":true,"required_exposure":0.139288,"target_brightness":135,"target_exposure_ms":139.29,"target_exposure_s":0.139288,"target_gain":1.0},"ladder_position":0.517161,"measured_brightness":0.115,"mode":"day","settings":{"AeEnable":0,"AnalogueGain":1.0,"AwbEnable":0,"ColourGains":[2.179744,1.800757],"ExposureTime":139288},"smoothed_lux":1.320299}, | ||
| {"diagnostics":{"applied_exposure_ms":19999.99,"applied_exposure_s":19.999994,"applied_gain":5.988,"base_target_brightness":120,"ladder_position":0.9999,"last_brightness":0.19,"last_p95":0.36,"mode":"transition","overcast_boost_active":true,"required_exposure":0.226274,"target_brightness":135,"target_exposure_ms":226.27,"target_exposure_s":0.226274,"target_gain":1.0},"ladder_position":0.999861,"measured_brightness":0.187,"mode":"transition","settings":{"AeEnable":0,"AnalogueGain":1.0,"AwbEnable":0,"ColourGains":[2.127283,1.833644],"ExposureTime":226274},"smoothed_lux":1.312226}, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep diagnostics.applied_* consistent with the frame settings.
Line 9 records ExposureTime: 226274 and gain 1.0, but diagnostics.applied_* reports 20,000 ms and gain 5.988. This stale/misaligned state weakens replay validation; regenerate the fixture or fix the replay update ordering.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/replay/golden/night_underexposure_edge.json` at line 9, Update the
replay fixture or replay update ordering so diagnostics.applied_exposure_ms,
diagnostics.applied_exposure_s, and diagnostics.applied_gain match the frame
settings ExposureTime and AnalogueGain on the same record. Regenerate the
night_underexposure_edge fixture if the implementation already applies settings
correctly; otherwise adjust the replay state update to record applied values
after the settings are applied.
| "description": "converged daylight, as a baseline that should barely move", | ||
| "source": "captures 2026-04-26T11:00:00 .. 2026-04-26T12:00:00", | ||
| "config": {"adaptive_timelapse": {"brightness_damping": 0.5, "brightness_target": {"base": 120, "contrast_threshold_high": 40, "contrast_threshold_low": 25, "max_target": 140, "overcast_boost": 15}, "day_mode": {"analogue_gain": 1, "awb_enable": true, "exposure_time": 0.01, "fixed_colour_gains": [2.5, 1.6]}, "diagnostics": {"enabled": true}, "enabled": true, "hdr": {"day_mode": "SingleExposure", "enabled": false, "night_mode": "Off"}, "highlight_protection": {"apply_in_night": false, "critical_p95": 240, "enabled": true, "min_scale": 0.7, "safe_p95": 200, "slew": 0.25, "warning_p95": 220}, "interval": 30, "light_thresholds": {"day": 80, "night": 3}, "night_mode": {"analogue_gain": 6, "awb_enable": false, "colour_gains": [1.83, 2.02], "max_exposure_time": 20}, "num_frames": 0, "reference_lux": 3.8, "test_shot": {"analogue_gain": 1, "enabled": true, "exposure_time": 0.2, "frequency": 1}, "transition_mode": {"brightness_feedback_enabled": true, "critical_rampdown_speed": 0.7, "ev_safety_clamp_enabled": true, "exposure_transition_speed": 0.08, "fast_rampdown_speed": 0.2, "fast_rampup_speed": 0.2, "gain_transition_speed": 0.1, "hysteresis_frames": 3, "lux_change_threshold": 3, "lux_smoothing_factor": 0.3, "sequential_ramping": true, "smooth_exposure_in_day_mode": true, "smooth_transition": true, "smooth_wb_in_day_mode": true, "target_brightness": 120, "wb_transition_speed": 0.15}}, "location": {"civil_twilight_threshold": -6, "latitude": 68.7, "longitude": 15.4, "timezone": "Europe/Oslo"}}, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make stable_day actually stable before using it as a baseline.
The fixture varies substantially despite claiming it should “barely move”: mean brightness rises from 121.2 at Line 12 to 173.97 at Line 35, then ends around 155 at Line 126. This can mask exposure-controller drift and invalidate convergence/flicker assertions. Regenerate it from a stable scene, or rename it and adjust the expected behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/replay/sequences/stable_day.json` around lines 3 - 5, Regenerate the
stable_day fixture from a genuinely stable scene so its brightness remains near
the stated baseline throughout the replay, or rename the fixture and update its
expected convergence/flicker assertions to reflect the observed variation.
Preserve the stable baseline semantics before using stable_day for
controller-drift validation.
| "description": "a dark scene with a bright light source in it, alternating between two levels while its highlights spread wider and wider -- walking the clipped-pixel fraction through the 3% and 5% thresholds at a point on the ladder, and in a state of the loop, where the resulting rate change is visible", | ||
| "config": {"adaptive_timelapse": {"brightness_damping": 0.5, "brightness_target": {"base": 120, "contrast_threshold_high": 40, "contrast_threshold_low": 25, "max_target": 140, "overcast_boost": 15}, "day_mode": {"analogue_gain": 1, "awb_enable": true, "exposure_time": 0.01, "fixed_colour_gains": [2.5, 1.6]}, "diagnostics": {"enabled": true}, "enabled": true, "hdr": {"day_mode": "SingleExposure", "enabled": false, "night_mode": "Off"}, "highlight_protection": {"apply_in_night": false, "critical_p95": 240, "enabled": true, "min_scale": 0.7, "safe_p95": 200, "slew": 0.25, "warning_p95": 220}, "interval": 30, "light_thresholds": {"day": 80, "night": 3}, "night_mode": {"analogue_gain": 6, "awb_enable": false, "colour_gains": [1.83, 2.02], "max_exposure_time": 20}, "num_frames": 0, "reference_lux": 3.8, "test_shot": {"analogue_gain": 1, "enabled": true, "exposure_time": 0.2, "frequency": 1}, "transition_mode": {"brightness_feedback_enabled": true, "critical_rampdown_speed": 0.7, "ev_safety_clamp_enabled": true, "exposure_transition_speed": 0.08, "fast_rampdown_speed": 0.2, "fast_rampup_speed": 0.2, "gain_transition_speed": 0.1, "hysteresis_frames": 3, "lux_change_threshold": 3, "lux_smoothing_factor": 0.3, "sequential_ramping": true, "smooth_exposure_in_day_mode": true, "smooth_transition": true, "smooth_wb_in_day_mode": true, "target_brightness": 120, "wb_transition_speed": 0.15}}, "location": {"civil_twilight_threshold": -6, "latitude": 68.7, "longitude": 15.4, "timezone": "Europe/Oslo"}}, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Populate the clipping sweep with non-zero clipped pixels.
Despite the description, every frame from Line 8 through Line 179 has overexposed_percent: 0.0 and percentile_95: 200.0. The replay therefore cannot cross the 3%/5% thresholds or validate the resulting controller-rate change. Regenerate the sequence with values below, at, and above both thresholds, then update its golden output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/replay/sequences/synthetic_clipping_sweep.json` around lines 4 - 5,
Regenerate the synthetic clipping sweep sequence so frames 8–179 contain
non-zero clipped-pixel values spanning below, at, and above the 3% and 5%
thresholds, with corresponding percentile_95 values that exercise the described
controller-rate changes. Update the sequence’s golden output to match the
regenerated replay results while preserving the existing sweep progression and
configuration.
…ller ones CodeRabbit's best catch is a genuine defect, not a test-only one. On any frame crossing out of the bright end, the diagnostics written into the metadata JSON described the handover seed rather than the exposure the frame was taken with: `_decide()` calls `decide()`, then `_seed_across_mode_change()` runs `seed_from_metadata`, which overwrites the controller's shutter, gain and ladder position -- and `_record` read the diagnostics after that. A frame carrying settings of gain 1.0 reported an applied gain of 5.9. The golden files had it baked in too, so a later fix to the ordering would have read as a behavioural regression against the baseline. The Decision now carries a snapshot taken the instant decide() returns, and the replay harness does the same. A new test pins it, and checks both the committed goldens and a fresh replay -- checking only the files caught a bad re-recording but not the harness bug that produced it, since reintroducing the fault leaves the files on disk untouched. Confirmed by reintroducing it. Also genuinely broken: raspilapse-status died with `Error: 'night_mode'` against the very config.example.yml the README tells people to copy. It read raw YAML where every other entry point merges the defaults. Mine, from the ladder commit, which added that lookup. merge_defaults copied one level deep, so an untouched nested default aliased the entry in DEFAULTS and a caller mutating its own config would change what every later load in the process saw. The docstring promised a new dict. A failing reference shot left its position unrecorded, so _wants_reference_shot stayed true and the loop tore the live camera down and failed to open a second one on every frame, forever. Camera initialisation sat outside the tolerant try, so a device still briefly busy after the previous teardown ended the daemon rather than costing a frame. That matters more now the camera is opened once per frame. `not brightness` conflated a frame measuring 0.0 with a missing measurement, in three places -- the same distinction _required_exposure was deliberately changed to make, applied inconsistently. The darkest frames were being dropped from the comparison used to judge the ladder. Docs pointed at `raspilapse.cli.overlay`, which does not exist; the module is `apply_overlay` and the console script is `raspilapse-overlay`. CONFIG-REFERENCE.yml still read as a copy-me template, still described the polar-day override and civil-twilight threshold, and still told people to run `python3 src/database.py --vacuum`. INSTALL.md still said sun elevation decides when day begins and ends. Pillow floor raised to 10.0.1 -- 10.0.0 is affected by advisories fixed there, and this project decodes images in the overlay and keogram paths. Skipped, with reasons: the metering thresholds are hardcoded while the rates they select are configurable, which is a design opinion rather than a defect and would add config surface to settings the mutation check already pins. The clipping-sweep fixture's zero overexposed_percent is not a gap -- the harness computes the clipped fraction from each frame's spread and never reads that field; the description now says so, and the mutation check proves the sweep crosses both thresholds. Verified on the live camera: restarted, capturing, and the status command now works against the shipped example. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
… missing a line CodeRabbit's pre-merge check puts docstring coverage at 79.63% against an 80% threshold. Six of the twenty-eight it counted were genuinely undocumented and say something a reader cannot get from the name: Meter.__init__ -- which config section it reads and what for Meter.set_dark_end -- why the caller has to tell it, and what it changes the four CLI main() entry points -- one line each The rest are left alone. They are property getters called `brightness`, `p95`, `overexposed`, `last_mode` -- a docstring on those restates the name and makes the file longer without making it clearer, which is the opposite of what this branch is for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
|
Three inline comments are still showing as open. One is fixed and just not
That it works is checked rather than asserted: The sequence description now explains where the clipped fraction comes |
3.8 MB of JSON against 1.1 MB of Python is a ratio that needs justifying, and nothing in the repository justified it. tests/replay/README.md now does: what a sequence is, what a golden is, why some are recorded from the database and some invented, and why there are seventeen. Measuring that turned up three that no longer earn their place. synthetic_threshold_edge_day and synthetic_threshold_edge_night pinned determine_mode's `<` against `<=`; synthetic_hybrid_override drove the hybrid brightness override. The ladder has neither a threshold to sit on the edge of nor an override to trigger, so all three had been catching mutations by coincidence since it landed -- which is not a reason to keep a fixture. Removing them leaves all 33 mutations caught, and promotes synthetic_clipping_sweep to sole detector of the clipped-pixel warning, which is the fixture built for it. The README carries the coverage measurement rather than an assurance: which sequence detects how many of the 33 mutations, and which four are the sole detector of something. That is the table to consult before deleting one of these files, and it is why the remaining overlap is deliberate -- six sequences would cover the mutation check on their own, but compare.py needs a realistic spread of light to say whether an exposure change is an improvement, and a thin sample would make the next one much harder to justify. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@requirements.txt`:
- Line 18: Update the Pillow dependency constraint in requirements.txt from
10.0.1 to a project-compatible patched minimum at or above 12.2.0, then validate
the resulting dependency set across all supported targets with python -m
pip_audit -r requirements.txt.
In `@tests/replay/compare.py`:
- Around line 92-95: Update the scene-luminance eligibility checks in
tests/replay/compare.py lines 92-95 and tests/replay/harness.py lines 118-121 to
reject brightness values at or above SATURATED, along with existing unusable
values, before calculating luminance. Apply the identical rule in both helpers
so comparisons and replay goldens use the same scene model; preserve valid zero
brightness measurements.
In `@tests/replay/README.md`:
- Line 110: Update the fenced code block in the replay README to specify the
text language by changing its opening fence to use text, while leaving the block
contents unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77d2f765-2d77-4aca-8dc2-6e8b49926f39
📒 Files selected for processing (47)
CONTRIBUTING.mdREADME.mddocs/CONFIG-REFERENCE.ymldocs/INSTALL.mddocs/OVERLAY.mdpyproject.tomlraspilapse/camera/exposure.pyraspilapse/camera/metering.pyraspilapse/cli/apply_overlay.pyraspilapse/cli/retry_uploads.pyraspilapse/cli/status.pyraspilapse/config.pyraspilapse/daemon.pyraspilapse/video/daily.pyraspilapse/video/keogram.pyraspilapse/video/timelapse.pyrequirements.txttests/replay/README.mdtests/replay/compare.pytests/replay/extract_sequences.pytests/replay/golden/blown_highlights.jsontests/replay/golden/bright_night.jsontests/replay/golden/crashing_light.jsontests/replay/golden/dawn_transition.jsontests/replay/golden/deep_dark.jsontests/replay/golden/dusk_transition.jsontests/replay/golden/night_underexposure_edge.jsontests/replay/golden/stable_day.jsontests/replay/golden/synthetic_clamped_highlights.jsontests/replay/golden/synthetic_clipped_pixels.jsontests/replay/golden/synthetic_clipping_sweep.jsontests/replay/golden/synthetic_extreme_gain.jsontests/replay/golden/synthetic_low_night_gain.jsontests/replay/golden/synthetic_night_brightness_sweep.jsontests/replay/golden/synthetic_starved_light.jsontests/replay/golden/synthetic_underexposure_release.jsontests/replay/golden/very_bright_night.jsontests/replay/harness.pytests/replay/mutation_check.pytests/replay/sequences/stable_day.jsontests/replay/sequences/synthetic_clipping_sweep.jsontests/replay/synthetic_sequences.pytests/test_auto_timelapse.pytests/test_config_example.pytests/test_daily_timelapse.pytests/test_overlay_optional.pytests/test_replay_golden.py
💤 Files with no reviewable changes (1)
- tests/test_config_example.py
🚧 Files skipped from review as they are similar to previous changes (38)
- tests/replay/golden/deep_dark.json
- tests/replay/golden/synthetic_starved_light.json
- tests/replay/golden/synthetic_clipped_pixels.json
- tests/replay/golden/synthetic_extreme_gain.json
- tests/replay/golden/bright_night.json
- tests/test_overlay_optional.py
- tests/replay/golden/synthetic_low_night_gain.json
- tests/replay/golden/synthetic_clipping_sweep.json
- tests/replay/golden/dusk_transition.json
- raspilapse/cli/apply_overlay.py
- tests/replay/sequences/stable_day.json
- tests/replay/golden/blown_highlights.json
- tests/replay/sequences/synthetic_clipping_sweep.json
- raspilapse/video/timelapse.py
- raspilapse/video/daily.py
- tests/replay/mutation_check.py
- README.md
- CONTRIBUTING.md
- tests/replay/golden/night_underexposure_edge.json
- tests/replay/golden/synthetic_clamped_highlights.json
- pyproject.toml
- docs/OVERLAY.md
- tests/replay/golden/very_bright_night.json
- tests/replay/golden/synthetic_night_brightness_sweep.json
- tests/test_daily_timelapse.py
- tests/replay/golden/crashing_light.json
- tests/replay/extract_sequences.py
- tests/test_replay_golden.py
- raspilapse/config.py
- raspilapse/cli/retry_uploads.py
- raspilapse/video/keogram.py
- docs/INSTALL.md
- raspilapse/camera/exposure.py
- raspilapse/camera/metering.py
- tests/test_auto_timelapse.py
- docs/CONFIG-REFERENCE.yml
- tests/replay/golden/dawn_transition.json
- raspilapse/daemon.py
| # Image analysis (capture pipeline) and graph generation (scripts/db_graphs.py) | ||
| # Overlay rendering and keograms; also the lores brightness metering the | ||
| # exposure loop closes on. Both are pulled in by python3-picamera2. | ||
| Pillow>=10.0.1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
Raise the Pillow security floor beyond 10.0.1.
Pillow 10.0.1 remains affected by advisories such as PYSEC-2026-165, fixed in 12.2.0; PyPI lists 12.3.0 as the current release. Because this project uses Pillow for overlay/keogram image processing, this constraint still permits vulnerable installations. (osv.dev)
Use a project-compatible patched floor and validate all supported targets:
python -m pip_audit -r requirements.txt🧰 Tools
🪛 OSV Scanner (2.4.0)
[CRITICAL] 18-18: pillow 10.0.1: undefined
(PYSEC-2026-165)
[CRITICAL] 18-18: pillow 10.0.1: Pillow buffer overflow vulnerability
(PYSEC-2026-1793)
[CRITICAL] 18-18: pillow 10.0.1: undefined
(PYSEC-2026-2253)
[CRITICAL] 18-18: pillow 10.0.1: undefined
(PYSEC-2026-2254)
[CRITICAL] 18-18: pillow 10.0.1: undefined
(PYSEC-2026-2255)
[CRITICAL] 18-18: pillow 10.0.1: undefined
(PYSEC-2026-2256)
[CRITICAL] 18-18: pillow 10.0.1: undefined
(PYSEC-2026-2257)
[CRITICAL] 18-18: pillow 10.0.1: Pillow has a PDF Parsing Trailer Infinite Loop (DoS)
(PYSEC-2026-2874)
[CRITICAL] 18-18: pillow 10.0.1: undefined
(PYSEC-2026-3451)
[CRITICAL] 18-18: pillow 10.0.1: undefined
(PYSEC-2026-3453)
[CRITICAL] 18-18: pillow 10.0.1: undefined
(PYSEC-2026-3454)
[CRITICAL] 18-18: pillow 10.0.1: Pillow: Out-of-bounds read via attacker-controlled row stride on Pillow's mmap path (McIdas AREA files)
(PYSEC-2026-3493)
[CRITICAL] 18-18: pillow 10.0.1: Pillow TGA RLE encoder can serialize up to ~57 KB of adjacent heap data into generated images
(PYSEC-2026-3494)
[CRITICAL] 18-18: pillow 10.0.1: Pillow: Decompression Bomb DoS via PdfParser.PdfStream.decode()
(PYSEC-2026-3495)
[CRITICAL] 18-18: pillow 10.0.1: Pillow JPEG2000 tiled decode retains a growing scratch buffer and can be used for denial of service
(PYSEC-2026-3496)
[CRITICAL] 18-18: pillow 10.0.1: Arbitrary Code Execution in Pillow
(PYSEC-2026-457)
[CRITICAL] 18-18: pillow 10.0.1: Arbitrary Code Execution in Pillow
[CRITICAL] 18-18: pillow 10.0.1: Pillow buffer overflow vulnerability
[CRITICAL] 18-18: pillow 10.0.1: Pillow BdfFontFile: Image.new() called without _decompression_bomb_check() — bomb protection bypass via font loading
[CRITICAL] 18-18: pillow 10.0.1: Pillow: WindowsViewer.get_command() OS command injection via unescaped shell path
[CRITICAL] 18-18: pillow 10.0.1: Pillow: FontFile.compile(): Image.new() called without _decompression_bomb_check()
[CRITICAL] 18-18: pillow 10.0.1: Pillow: Out-of-bounds read via attacker-controlled row stride on Pillow's mmap path (McIdas AREA files)
[CRITICAL] 18-18: pillow 10.0.1: Pillow: Heap out-of-bounds write Image.paste() / Image.crop() via signed coordinate overflow
[CRITICAL] 18-18: pillow 10.0.1: Pillow PcfFontFile._load_bitmaps(): Image.frombytes() called without _decompression_bomb_check() — bomb protection bypass via PCF font loading
[CRITICAL] 18-18: pillow 10.0.1: Pillow: Controlled heap out-of-bounds write in Pillow ImageCmsTransform.apply() via output mode mismatch
[CRITICAL] 18-18: pillow 10.0.1: Pillow TGA RLE encoder can serialize up to ~57 KB of adjacent heap data into generated images
[CRITICAL] 18-18: pillow 10.0.1: Pillow: Decompression Bomb DoS via PdfParser.PdfStream.decode()
[CRITICAL] 18-18: pillow 10.0.1: Pillow GdImageFile._open(): image dimensions accepted without _decompression_bomb_check()
[CRITICAL] 18-18: pillow 10.0.1: Pillow has a PDF Parsing Trailer Infinite Loop (DoS)
[CRITICAL] 18-18: pillow 10.0.1: Pillow JPEG2000 tiled decode retains a growing scratch buffer and can be used for denial of service
[CRITICAL] 18-18: pillow 10.0.1: Pillow has an integer overflow when processing fonts
[CRITICAL] 18-18: pillow 10.0.1: Pillow: Heap out-of-bounds write in ImageFilter.RankFilter via integer overflow in ImagingExpand
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@requirements.txt` at line 18, Update the Pillow dependency constraint in
requirements.txt from 10.0.1 to a project-compatible patched minimum at or above
12.2.0, then validate the resulting dependency set across all supported targets
with python -m pip_audit -r requirements.txt.
Source: Linters/SAST tools
scene_luminance's docstring claimed it returned None for a saturated frame, "since a clipped measurement puts only a lower bound on the light". The code never checked. At the sensor ceiling the reading bounds the scene from below rather than measuring it, so dividing understates the light -- in the very comparison that judged the exposure ladder. Both copies now reject it, so the goldens and the comparison share one scene model. No frame in the current fixtures is affected: nothing recorded reaches 254. The guard is right anyway, and the docstring is no longer a lie. Pillow raised to 11.0.0, not the 12.2.0 the scanner asked for. Pillow 12 requires Python 3.10; this package supports 3.9 because Raspberry Pi OS Bullseye ships it and CI builds against it, so 12.x would fail the build outright. 11.x is the ceiling until 3.9 is dropped, which is a bigger decision than a dependency floor. Worth noting either way that a Pi installs neither: apt's python3-pil is 9.4.0 and arrives with picamera2. This floor is for pip installs -- CI and development checkouts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/replay/README.md (1)
141-144: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the fixture format accurately.
These files are JSON documents with one frame object per line inside the
framesarray—not one standalone JSON object per line (JSONL). Clarify this so readers do not try to parse each line independently.Proposed fix
-The files are one JSON object per line on purpose: +The files place one frame object per line inside a JSON document on purpose:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/replay/README.md` around lines 141 - 144, Update the fixture-format description in the README to state that each file is a single JSON document whose frames array contains one frame object per line, rather than standalone JSONL objects. Preserve the explanation about readable diffs and large-file-hook limits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@requirements.txt`:
- Around line 18-26: Update the Pillow requirement from >=11.0.0 to >=11.3.0,
revise the adjacent compatibility comments to state that 11.3.0 is the Python
3.9-compatible security floor and Pillow 12 remains unavailable until Python 3.9
support is dropped, then run pip_audit against requirements.txt across supported
environments.
---
Outside diff comments:
In `@tests/replay/README.md`:
- Around line 141-144: Update the fixture-format description in the README to
state that each file is a single JSON document whose frames array contains one
frame object per line, rather than standalone JSONL objects. Preserve the
explanation about readable diffs and large-file-hook limits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72803dbb-b4ca-4d8b-b6ef-a8f2e48607ee
📒 Files selected for processing (5)
pyproject.tomlrequirements.txttests/replay/README.mdtests/replay/compare.pytests/replay/harness.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/replay/compare.py
- tests/replay/harness.py
- pyproject.toml
apply_overlay was 469 lines, of which 213 drew the two right-hand sections of the top bar. They move to overlay/widgets.py behind a contract the bar can compose: draw yourself against the right edge inset by what is already there, return the width you used. The five _format_* helpers move to overlay/formats.py, where the fixed-width reasoning is written down once instead of being repeated per formatter. render.py 1186 -> 910, apply_overlay 469 -> 283. Verified by rendering rather than by the tests passing. The overlay's product is an image, and the 120 overlay tests check formatters and data preparation, not pixels -- so a golden-image check pins time, weather, tide, aurora and system metrics, renders four cases, and hashes them. Original and refactored agree on every pixel of all four. The check was then mutated to confirm it can fail: divider alpha off by one, wave resolution, wave width, section gap, marker position, marker colour and tide event ordering are all caught. Two survived at first, and both were the check's fault rather than the code's -- the tide fixture had its high before its low, so the chronological branch and an always-high-first mutant emitted the same string, and a fourth case was added for it. The other survivor is real and documented at AURORA_WIDEST: only the wider line of each template pair sets the section width, so shortening the narrower one is genuinely inert. Also from review, both correct: Pillow floor 11.0.0 -> 11.3.0. 11.3.0 is the last 11.x release and carries every fix available to Python 3.9; the earlier floor admitted 11.0.0 and its advisories for no reason, since 3.9 support only rules out 12.x. tests/replay/README.md called the fixtures one JSON object per line, which reads as JSONL. They are one JSON document with one frame object per line inside the frames array, and must be read with json.load. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
apply_overlay was still 283 lines after the widgets moved out: font loading, the whole top-bar layout, the corner-box layout and the save, in one function under one try. Those become _load_sized_fonts, _draw_top_bar and _draw_corner_box, leaving apply_overlay at 65 lines that read as what it does -- open, size the fonts, prepare the data, pick a layout, save. The extraction introduced one bug and the tests caught it. _draw_corner_box returned None where the caller expected "did I draw anything", so every non-top-bar preset skipped the save and returned the untouched image. The pixel check was green throughout, because at that point it only rendered top-bar frames. Two corner-box cases were added for exactly that reason, and the restored bug is now caught by both. The check itself moves to tests/overlay_render_check.py, next to the replay harness and for the same reason: the overlay's product is an image and none of the 122 overlay tests looks at a pixel. It freezes the clock, weather, tide, aurora and system metrics, generates its input image rather than reading one, and prints a hash per case to diff across a change. It is a tool rather than a pytest test, and the docstring says why: text lands on different pixels under a different Pillow, FreeType or DejaVu build, so a committed hash would fail where nothing is wrong and teach people to re-record it. Against itself on one machine it is exact. Verified across the whole refactor -- ec7767b's 1186-line render.py and this one produce byte-identical output on all six cases. Mutation-checked: a missing return, a one-pixel line offset, a one-pixel text offset and a width/height swap are all caught, and a no-op control survives. raspilapse/overlay/render.py 1186 -> 934, largest function 469 -> 207 (_prepare_overlay_data, next). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
The last long function in overlay/render.py, at 207 lines, was six unrelated things sharing one dict: camera settings, host metrics, weather, ships, tide. Each becomes a helper returning its own fields, and _prepare_overlay_data is the six lines that merge them. The tide block wrote its nine placeholder values out twice, identically -- once for "disabled" and once for "enabled but no reading". Those are the same case and now share a branch. The source is still only queried when enabled, which is not cosmetic: get_widget_data reads a cache file and may refresh it over the network, and doing that 2,880 times a day for output that is discarded is worth avoiding. There is now a test that it is not called. The refactor turned up a coverage hole rather than a bug. Dropping the ship fields entirely, dropping the system fields entirely, changing the disabled-tide placeholders, and querying the tide source while disabled all passed the full overlay suite -- 122 tests, none of which looked at these keys. The contract they leave unguarded is that every placeholder exists on every frame: a missing one raises KeyError inside apply_overlay, which catches it and returns None, so the failure is a single un-overlaid frame in the middle of a timelapse rather than anything that announces itself. tests/test_overlay_fields.py covers it: 41 tests over the tide, ship and system groups, plus a parametrised check that 33 placeholders survive with every source switched off and every network call failing. Six of the seven mutations above are now caught there, and the seventh (dropping weather) was already caught by the existing suite. overlay/render.py 1186 -> 962, largest function 469 -> 154 (_draw_top_bar). Pixel-identical to ec7767b on all six render-check cases; 1037 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
…r sees Review read synthetic_clipping_sweep's stored `overexposed_percent: 0.0` on all 172 frames and concluded the clipping path was unreachable, so the fixture could not do the job its description claims. Reasonable from the file; wrong about the harness. The replay is closed-loop. It recovers scene luminance from the recorded brightness and exposure, then shows the controller the brightness its own choices would produce -- harness.observe derives overexposed_percent and underexposed_percent from the simulated mean and the carried-over spread, and never passes the recorded ones to the code under test. Measured over a replay: clipping ranges to 26.8%, 129 of 172 frames sit past the 5% warning threshold, and breaking that threshold on purpose is still detected by this sequence and no other. So no fixture change, but the confusion is the README's fault and this is the second reader it has caught. It now says which stored fields are inputs to the scene model and which are ignored, and names this sequence as the example. Verified by re-running the mutation: `clipped_warning 5 -> 6` is detected by synthetic_clipping_sweep alone, as the coverage table claims. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
|
Re-checked the four threads still showing as unresolved. All four are 1. 2. 3. And re-running the mutation directly: The file is genuinely misleading though, and this is the second reader it has caught, so 4. Pillow floor. Fixed in f1e1ad0, and the thread text is stale (it refers to 10.0.1; the floor was 11.0.0 by then). Now All 6 CI checks pass; 1037 tests. |
1 participant
Eight commits, each verified against the live camera at Kringelen, which
captured continuously throughout.
Three goals: make the code readable, make the optional parts genuinely
optional, and make the exposure work anywhere rather than only at 68°N.
Making it work anywhere
Mode selection is gone. It compared an uncalibrated lux figure against
absolute thresholds —
night: 3,day: 80— that had to be retuned forevery camera and every site, and were overridden at high latitude by a
sun-elevation check because the thresholds could not survive being moved.
In its place, one ladder. A longer shutter costs time; more gain costs
noise, so the order is forced:
That turned out to be the simplification rather than a trade against
it.
_settings_dayand_settings_transitionwere the same functionwith different log strings, and
_settings_nightdiffered only in whereit started.
exposure.pywent from 1418 lines to 440.modesurvives as a label derived from the settings, so it can no longerdisagree with what the camera is doing. The old one could: 368k frames
were labelled "day", and among them were frames at a 20-second exposure
and gain 5.5, because the polar override set the label while the shutter
was wide open.
astral is demoted, not removed. It was used, but as a patch rather
than a feature — its docstring claimed it preserved AWB through twilight,
and day mode had set
AwbEnable = 0for a long time. Sun elevation isstill recorded with every frame and still feeds
graph_solar_patterns.py.Nothing decides from it, and without astral installed the column is NULL
and the camera behaves identically.
Is the new exposure better, or just different?
tests/replay/compare.pyruns both controllers against the same recordedlight with the loop closed, over 3400 frames of this camera's history:
Closing that loop mattered, and getting it wrong first was instructive.
Replaying recorded brightness is open-loop: the measurement is a
property of the exposure the old controller happened to be using, so the
new one lowers exposure, nothing responds, and it lowers again into a
runaway that cannot happen on a camera. It reported the ladder as 13
stops darker. Deriving scene luminance and showing each controller the
brightness its own choice would have produced fixed it — and immediately
found two real bugs: a near-black frame held the exposure where it was,
and a uniform rate limit made daylight sluggish.
Making the optional parts optional
overlay.enabled: falsewas silently disabling all database logging.ImageOverlay.__init__returned before assigningself.weather, and thecapture loop read
capture.overlay.weatherinside anexcept Exceptionthat logged at DEBUG. Weather is a data source the daemon owns now, and
capture.pyno longer imports the renderer at all — so Pillow is out ofthe capture path entirely.
The install is three apt packages:
It used to be seven plus a pip line. Two of those were redundant —
python3-picamera2depends on numpy and Pillow — and the rest each buyone feature and now degrade cleanly when absent.
Readability
src/is araspilapsepackage groupedby what each part talks to. That also removed mypy's project-wide
no-redefsuppression and ruff'sE402exemption.config.example.yml: 681 lines → 69. Everything the code indexeswithout a fallback has a default in
raspilapse/config.py; the fullannotated schema moved to
docs/CONFIG-REFERENCE.yml. An eight-lineconfig works.
run(): 287 lines → 97, decomposed into named steps.off every frame.
Bugs fixed
Each confirmed by reverting it and watching a test fail.
overlay.enabled: falsesilently disabled all database logging.exists()check swallowed it.daily.pyshelledout to a path assembled from separate arguments, so grepping for
src/missed it. Its test asserted only that"make_timelapse.py"appeared in the command, which stayed true after the file stopped
existing.
at the delivered ones.
Two things built, measured, and thrown away
The plan said keep the camera open between frames. It cannot be done: a
commanded exposure took eight frames to reach the metadata, with four
consecutive captures returning a value from two commands earlier. At a
20-second night exposure that is 160 seconds of wrong frames.
Then a "skip the teardown when settings barely move" path — the sensor
quantises 217 µs to 210 µs, so command and delivery always differ by more
than any useful tolerance and the branch never fired. Both are gone
rather than left in looking useful.
Reported, not fixed
highlight_protection.min_scalebelow 0.70 is dead configuration: thecurve reaches 0.70 at p95 255, the highest an 8-bit frame has.
commands 100 µs at gain 1.0 — the bottom of the ladder — and the sensor
delivers 210 µs at gain 1.12, leaving frames about a stop overexposed.
This predates the branch: 13 July had 893 frames pinned at minimum
exposure averaging brightness 153. It is optics, not software.
Testing
831 → 985 tests. The golden-master harness in
tests/replay/recordswhat the controller decides for twelve sequences — six pulled from the
capture database, six synthetic — and
mutation_check.pybreaks oneconstant at a time to prove those tests can fail. All 33 mutations are
caught, and three further constants are documented as absent with
reasons rather than quietly dropped.
Golden files were re-recorded once, for the ladder, which is what those
tests exist to make deliberate.
Not done
overlay/render.pyis still 1186 lines with a 469-lineapply_overlay.It is the last structural item;
exposure.pyno longer needs one.🤖 Generated with Claude Code
https://claude.ai/code/session_0123LLDR874Fyp1gbjAVbR2a
Summary by CodeRabbit