Cleanup: remove the dead ML system, fix three broken services, halve the disk use - #12
Conversation
Three unrelated failures that all surfaced on the same systemd unit. upload_service.py imported requests_toolbelt unconditionally. It is an optional accelerator (it streams the ~300 MB multipart body instead of buffering it), but a missing optional dependency was taking down the whole daily-video run at import time -- /usr/bin/python3, which every unit uses, did not have it. Guard the import and fall back to requests' own multipart encoder, warning once. The fallback must not set Content-Type: requests generates the boundary itself. make_timelapse.py returned 1 when a date had no images, so an empty day (camera off, fresh install) left the unit in `failed`. Return 2 instead and have daily_timelapse.py map it to 0. Exit 1 stays reserved for real errors, and the distinction lives in Python rather than in a blunt SuccessExitStatus= on the unit. Add systemd/journald-raspilapse.conf. journald was running with stock defaults on a 117 GB card, which caps at 4 GB; the journal had reached 3.6 GB. 200 MB is about a month of output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
The queue had 172 rows, every one 'pending' with retry_count=1, dating back to January. None of them could ever succeed: the source videos are deleted long before the queue gives up, and no installer ever installed raspilapse-upload-retry.timer, so nothing had drained the queue at all. Installing that timer as-is would have meant 172 doomed uploads every 30 minutes forever, so fix the queue first: - retry_single_upload() cancels a row whose video file is gone instead of rescheduling it. - get_pending_uploads() returns only 'pending'. 'failed' is terminal -- a row reaches it after the retry schedule is exhausted -- so the automatic pass must not pick it back up. --force opts in explicitly. - retry_uploads.py gains --purge-missing for the existing backlog, and refuses to run at all when video_upload.api_key is empty rather than burning a retry slot and a log line per row to discover that. Ran --purge-missing here: 172 cancelled, queue now empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
docs/MAINTAINER.md printed the live Codecov upload token twice, in a file whose own "Security Notes" section said not to commit secrets. It is gone from HEAD; the token still needs rotating at Codecov, and it remains in history (commit 019b354) because rewriting a public repo's history would break every existing clone and fork. manuals/ was 45 MB of third-party Raspberry Pi PDFs that nothing in the repo referenced -- every clone paid for them. Upstream links belong in the docs instead. ml_state/ml_state.json was force-added past .gitignore, shipping this camera's learned exposure model (lat 68.7) to everyone who cloned. Consolidate contributing docs into a single root CONTRIBUTING.md, which is also where GitHub looks for it. It absorbs BLACK_FORMATTING_GUIDE.md and the release checklist from MAINTAINER.md; the three previously said "run make format" in three different places. Drop the shipped "full version coming soon" placeholder, and stop naming a black version in prose -- requirements-dev.txt is the pin. Also genericise the Netatmo example endpoint, which carried a real hostname and station UUID. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
The repo had four copies of some unit files: the ones in systemd/, plus heredocs inside install.sh and install_cleanup.sh, plus whatever was already deployed. They drifted. The installed daily-video timer still had Requires=, two OnCalendar= lines and Persistent=true -- all three removed from the repo months ago and never redeployed -- which is why the unit fired at boot and failed five seconds later. install.sh also wrote a generated raspilapse.service into the project root, so running the installer left every user with a dirty working tree. Now systemd/ holds *.in templates with @user@/@group@/@PROJECT_DIR@/ @python@, and scripts/install.sh renders them into a temp dir. A template cannot be copied by accident, which is the trap install_daily_video.sh fell into: it cp'd the units verbatim, so anyone not called "pi" living somewhere other than /home/pi got a broken service. ./scripts/install.sh all four units ./scripts/install.sh --only capture just one ./scripts/install.sh --check deps + config, installs nothing ./scripts/install.sh --dry-run print what would be installed ./scripts/install.sh --uninstall ./scripts/install.sh --with-watchdog --check replaces test.sh, which was set -e, blocked on an interactive read, and silently overwrote config.yml from the example. It checks against /usr/bin/python3 specifically, since that is what the units run. Deleted install_cleanup.sh, install_daily_video.sh, uninstall.sh, uninstall_daily_video.sh, test.sh, check_disk_space.sh (duplicated the df check in cleanup_old_images.sh) and check_capture_rate.sh (hand-copied the capture interval from config; nothing installed either). The installer no longer states schedules in prose -- it prints `systemctl list-timers`. Four files claimed 04:00 while the timer said 05:00. check_service.sh becomes raspilapse-watchdog.{service,timer} running as root. From cron it ran as the login user, where polkit denied its `systemctl restart` and its reboot needed root, so it could never recover anything. It now reads output.directory from config instead of hardcoding /var/www/html/images, keeps escalation state in /var/lib/raspilapse so a reboot doesn't reset the counter, and leaves plain process death to Restart=always. Removed Requires= from the cleanup and upload-retry timers too, for the same reason it was removed from daily-video. Added Nice/IOSchedulingClass to the two batch units so a 25-minute ffmpeg run cannot starve a capture loop that ticks every 30 seconds. retry_uploads.py now exits 0 when uploads are unconfigured. It runs every 30 minutes; "not configured" is a setting, not a fault, and exiting 1 painted the unit red forever on any install that doesn't upload. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
Three separate problems in one 162-line module.
Paths were relative. get_logger()'s default config path was
"config/config.yml", which only resolved because the systemd units set
WorkingDirectory. Run any script from anywhere else and the lookup
failed silently, falling back to the built-in defaults -- INFO instead
of WARNING, console handler on. Everything now resolves against the
project root.
Every line was stored twice. The units set StandardOutput=journal and
the logger added a console handler on top, so each line went to both
logs/ and the journal. Combined with journald running at its stock 4 GB
cap, the journal had reached 3.6 GB. logging.console becomes tri-state:
"auto" (the new default) skips the console handler when JOURNAL_STREAM
is set, which systemd sets only when stdout really is wired to the
journal. An explicit true or false is still honoured exactly -- quietly
reinterpreting config is the disease, not the cure.
-c/--config never reached logging. Seven modules call get_logger() at
import time, long before argparse runs. Rather than converting all
seven to lazy proxies, add configure_logging(), which retroactively
reconfigures every logger handed out so far. Entry points call it once
after parse_args().
Also: get_logger() is memoised instead of calling handlers.clear() on
every call, which used to detach handlers other modules were holding;
the config is parsed once per path instead of once per importing
module; and `yaml.safe_load(f) or {}` makes an empty config file work
on purpose rather than by falling through a bare except.
Delete upload_service.py's 27-line shadow get_logger with its own
fallback StreamHandler, and create_keogram.py's bare-logger fallback.
New tests/conftest.py redirects log output to tmp_path. The suite was
writing into the real logs/ -- overlay.log held 5.4 MB of test fixtures
like "Ships file not found: /tmp/nonexistent_ships_test.json", plus two
stray zero-byte files from the logging tests.
Config: console true -> auto, and 10 MB x 5 backups -> 5 MB x 2, which
Update.md claimed had already landed but which only ever reached the
example file.
Note for the docs pass: application logs now live only in
logs/<script>.log. journalctl still shows systemd and libcamera output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
The 300-second cache never took effect. It lived on the WeatherData
instance, and WeatherData is built inside ImageOverlay, which is built
inside ImageCapture, which auto_timelapse constructs twice per capture
cycle -- so every lookup started with an empty cache. The endpoint was
being hit roughly 5,760 times a day instead of 288. Move the cache to
module level, keyed by endpoint. The constructor signature is unchanged.
Failures had no backoff at all: _fetch_weather_data() returned None
without touching the cache timestamp, so the next call went straight
back to the network. During one DNS outage that produced 72,536
identical lines in a single rotated log file. Consecutive failures now
double the retry delay up to max_backoff_seconds (default 900), stale
data keeps being served so the overlay doesn't blink, and an unchanged
error message is logged at most once every ten minutes with a count of
what was suppressed.
Fixed a real parsing bug at the same time: data.get("data", {}) returns
None when the API sends an explicit "data": null, because a default
only applies to a missing key. The next line called .get() on it. That
was 2,204 logged AttributeErrors, and it also silently blanked the
weather overlay whenever the station went offline.
overlay.py reached into twelve private weather._format_* methods and
duplicated the "-" fallback for every field. Replaced with one public
format_fields(). Note that "temperature" is deliberately not one of its
placeholders: in overlay templates that name means the camera sensor
temperature, from capture metadata, and emitting it here would silently
overwrite it with the outdoor reading. format_weather_line(), whose
templates are weather-only, keeps the alias.
conftest resets module-level caches under both `x` and `src.x`. The
dual-import idiom means those are two module objects with separate
state, which is why test_weather passed alone and failed in a group.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
The schema was defined twice. UploadService carried its own copy of the upload_queue DDL, written before migration v4 added the composite retry index, so whichever process opened the file first decided which indexes existed. On this camera that left schema_version pinned at 3 with the v4 index permanently absent. Extract apply_schema(conn) as the single definition and have both callers apply it to their own connection -- which also keeps the in-memory case working, since :memory: is per connection. WAL: the capture loop writes every 30 seconds while db_graphs, db_stats and an external dashboard read the same file. Nothing in the tree had ever issued a PRAGMA, so it was still on the rollback journal, where readers and the writer contend. Paired with synchronous=NORMAL, which is the usual WAL pairing and saves fsyncs on an SD card. Migration v5 drops idx_captures_lux, idx_captures_brightness and idx_captures_mode. Measured with dbstat on 515k rows they cost 26 MB -- a third of all index space -- and three extra B-tree writes per capture, forever. Nothing uses them: get_captures_by_lux_range() has no production caller, nothing filters or sorts on brightness_mean, and idx_captures_mode indexes three distinct values across half a million rows. After migrating and vacuuming, this database went 245 MB -> 213 MB. Retention: database.retention_days, defaulting to 0 (keep everything) so no existing camera loses history merely by pulling this. The example ships 180 days -- a full seasonal cycle, which is what makes the solar-pattern graphs worth having at 68 degrees north. Pruning runs from raspilapse-cleanup.timer as a second ExecStart, so expired images and expired rows go at the same time. Rows deliberately outlive their JPEGs: images go at 7 days, the rows keep the lux, brightness and weather the graphs are built from. VACUUM stays an explicit opt-in flag; it needs free disk equal to the database size and takes minutes. src/database.py grows a real maintenance CLI (--stats, --prune, --dry-run, --vacuum, --retention-days) in place of the __main__ demo that inserted fake rows into an in-memory database. Also: brightness_p25 and brightness_p75 were NULL on every row ever written, because the lores computation emitted percentile_10/90 while store_capture read percentile_25/75. Nothing consumed p10/p90, so the producer now emits what the columns are named. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
Six modules had their own load_config. Three had a byte-identical get_db_path. parse_time_arg and format_duration existed twice with quietly different defaults -- 1h vs 24h, and .1f vs .0f -- so naively merging them would have changed both tools' output. src/config_utils.py is now the single copy, with those differences as explicit parameters. Dead code removed: - daily_timelapse.upload_to_server (80 lines) was a from-scratch reimplementation of upload_service.upload_to_server, using the old non-streaming requests.post that caused the May upload failures. main() has used UploadService since; nothing called this one. Took its 9 tests and two now-unused imports with it. - create_keogram_from_images and create_slitscan_from_images were pure pass-throughs with identical signatures. make_timelapse now calls the real functions. - overlay.ImageOverlay._load_ships read self.barentswatch_enabled and self.ships_file, neither of which __init__ has ever assigned, so calling it would raise AttributeError. Zero callers. - overlay.ShipsData.format_ships_overlay: zero callers. - graph_ml_patterns.load_ml_state: kept "for backwards compatibility with db_graphs.py imports", but db_graphs imports only create_solar_pattern_graph, and the v1 state file it read is gone. make_timelapse also carried its own copy of the Colors class and print helpers instead of importing src/colors.py. Moved its print_subsection into colors.py -- the one function the shared module was missing -- and deleted the local copy. The three date-stamped globbers in daily_timelapse (video, keogram, slitscan) differed only by filename prefix and extension, and the video one was missing the date-confirmation filter the other two had. Collapsed into _find_dated_file plus a pattern builder; the three public names stay so their tests keep working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
1,967 lines plus a 28 KB test file, superseded on every axis: - Its input is deleted by design. It globbed *_metadata.json under output.directory, and cleanup_old_images.sh removes those after 7 days, so it could never look further back than a week. scripts/db_graphs.py reads the database, which here goes back to January. - Output collision: lux_levels.png and overview.png are also written by db_graphs into the same directory, last writer winning. - Its unique outputs have never existed. graphs/ contains only db_graphs and graph_ml_patterns products -- no exposure_time.png, analogue_gain.png, white_balance.png, holy_grail_*.png or timelapse_analysis_*.xlsx has ever been generated there. - Nothing invoked it: no unit, no timer, no script, no cron. - It was the only reason openpyxl was a production dependency, for an Excel export nobody ran. The one capability that did not overlap was white-balance plotting, so port it: create_white_balance_graph() in db_graphs draws colour gains and colour temperature from the colour_gains_r/b and colour_temperature columns, which the database has been recording all along. Verified against the live data -- flat lines at red 2.5 / blue 1.6 / ~6170 K, which is exactly what fixed day-mode colour gains should look like. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
None of it ran. _init_ml_predictor() returned early whenever direct_brightness_control was true, which it has been since January, so _ml_predictor was always None. With ML inert, the "legacy" arms of get_camera_settings were unreachable too, and with those gone the formula functions they called had exactly one live caller left: the metadata diagnostics, which re-ran the entire exposure calculation purely to write numbers into a JSON field. Deleted: ml_exposure.py, ml_exposure_v2.py, bootstrap_ml.py, bootstrap_ml_v2.py, ML.md, ml_state/, four test modules, and from auto_timelapse.py: SustainedDriftCorrector, _init_ml_predictor, get_brightness_adjusted_trust, get_lux_stability_trust, _calculate_target_exposure_from_lux, _calculate_target_gain_from_lux, _calculate_sequential_ramping, _apply_ev_safety_clamp, _get_emergency_brightness_factor, _apply_proactive_exposure_correction, _apply_brightness_feedback, _detect_rapid_lux_change, both LEGACY branches, and eight write-only instance attributes. auto_timelapse.py: 3230 -> 2162 lines. Two traps in there: _apply_brightness_feedback looked like dead ML plumbing -- its return value and _brightness_correction_factor were read by nothing. But its one line `self._last_brightness = actual_brightness` was the only per-frame write of the sole input to the live controller. Deleting it naively would have frozen exposure at the startup seed. run() now writes it explicitly, next to the _last_p95 write it belongs with. The transition branch required `lux is not None` and otherwise fell through to a fallback that hardcoded a 5-second exposure and gain 2.5. lux is None until the first test shot succeeds, so that was reachable in broad daylight on the first frame after a restart. Now handled explicitly with position 0.5. Diagnostics no longer recompute. get_camera_settings records what it decided in _last_decision and the enricher serialises it. That also removes the duplicate [P95-Protect] log lines, which were emitted twice per frame from a calculation whose result never reached the camera. graph_ml_patterns.py is not ML despite the name -- it queries the captures table and draws daily_solar_patterns.png. Renamed to scripts/graph_solar_patterns.py. Same for db_graphs' create_ml_diagnostics_graph, now create_brightness_diagnostics_graph, and its "ML > Formula" axis labels. Verified on hardware: before and after the deletion, gain 1.1228, colour gains 2.5/1.6 and brightness holding 120 against a target of 120. Exposure differs only by the light genuinely fading between the two samples. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
get_p95_highlight_factor() computed a highlight-clipping factor and logged it at WARNING from inside the calculation. It was reachable only from the diagnostics path, so blown-out skies were detected, logged twice per frame, and then not corrected. That one line was 742 of 777 lines in the live log. Split into two pieces. highlight_factor() is now a pure module-level function -- same curve, no logging, no state. _highlight_target_scale() adds the slew limiting, the night exemption and edge-triggered logging, and the controller applies the result. It scales the brightness *target*, not the controller's output. Both formulations settle; I simulated them. The difference is where. Scaling the target leaves the loop's own fixed point intact, so the equilibrium depends only on the highlight_protection settings: mean brightness lands at 118.0 for every damping value from 0.3 to 1.0. Scaling the output makes the loop settle where ratio**damping * scale == 1, which ties the amount of protection to brightness_damping -- equilibrium drifts 116.4 to 118.0 across the same range. Highlight behaviour should not move when an unrelated knob does. Three guards against a noisy sample steering the exposure: an exponential slew on the scale (0.25/frame), an exact 1.0 below safe_p95 so there is a real deadband, and min_scale as a hard floor. Night is off by default. Over 117k night frames here the mean is already 90 against a target of 120 while 11% of frames exceed p95 200 -- streetlamps and the moon, not blown scenes. Protecting those would make aurora frames worse. Verified on hardware. Deployed disabled first: no change. Enabled: p95 209, scale 0.982, effective target 117.8, brightness converging on it, overexposed 0.00%. Zero log lines in the 25 minutes since, against 55 in the hour before. enabled: false reverts to the previous behaviour with no code change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
auto_timelapse.py was one 3,230-line file holding a single class that did config, scheduling, camera I/O, filesystem work, database calls and every exposure decision. After the ML deletion it was 2,246; this takes it to 983 with a 1,423-line exposure.py alongside. ExposureController owns every piece of per-frame exposure state and is the only thing that writes it. It knows nothing about the camera, the filesystem or the database -- the solar position it needs is passed in rather than computed there, which was the only coupling that made the boundary non-obvious. AdaptiveTimelapse holds one, feeds it measurements through observe_frame(), and asks it for settings. get_camera_settings was 283 lines of if/elif; now it dispatches to _settings_night / _settings_day / _settings_transition and applies _apply_wb and _apply_hdr. The three white-balance blocks were near-identical and are now one helper -- the day one had a comment calling AWB "legacy behavior" when it is really an opt-out that trades colour stability for auto white balance. The metadata diagnostics now call exposure.diagnostics() instead of reaching into a dozen private attributes. Startup seeding goes through seed_from_capture(), which applies only the fields the database row actually has, instead of six separate assignments guarded by a shared `seeded` flag that could log a formatted exposure that was never set. Verified on hardware: after the restart, brightness climbed 112 -> 115 -> 118 -> 120 against a target of 120 while exposure lengthened with the fading light. Same gain, same colour gains, same diagnostics keys. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
Regression from the exposure.py extraction. run() still did
self._last_brightness = actual_brightness
self._check_overexposure(brightness_metrics)
but that state had moved to ExposureController. Python does not raise on
the first line -- it just creates a dead attribute on AdaptiveTimelapse.
The second line does raise, into an `except` that logged at DEBUG, which
is invisible at the configured WARNING level. So the controller kept
whatever brightness the startup seeding gave it and drove exposure from
a measurement that was minutes old and getting older.
Every unit test passed. Both halves worked in isolation; only the seam
was broken. What showed it was the camera: brightness climbed 155 -> 194
over four minutes while the loop lengthened exposure, which is the wrong
sign for a loop that is supposed to hold 120.
run() now calls exposure.observe_frame(), the controller's single
per-frame entry point, and the handler around it logs at WARNING --
losing this silently is exactly what went wrong.
Added TestFeedbackWiring. The obvious test (call observe_frame, assert
the state changed) does not catch this, because the broken path is in
run(); I checked, and it passes with the bug reintroduced. The one that
does catch it is a static check that AdaptiveTimelapse never assigns to
a name ExposureController owns. Verified it fails with the bug present
and passes without it.
After the fix, on hardware: brightness 63 -> 80 -> 93 -> 104 -> 111
against a target of 120, exposure tracking up with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
Three separate ways to fail, all fixed together. Black was pinned three ways: requirements-dev said ==24.10.0, pyproject said >=23.0.0, pre-commit said 24.10.0, and this machine had 26.1.0. Black's stable style changes between yearly releases, so >= guarantees that locally-formatted code gets rejected by CI -- which is what three separate docs were dedicated to complaining about. Pinned to 26.1.0 in all three places, and line-length now lives only in pyproject rather than being repeated in Makefile args and pre-commit args. pytest was configured twice, in pytest.ini and pyproject. pytest.ini won, so pyproject's markers were dead and pytest.ini's [coverage:*] sections were in a file coverage does not read. Deleted pytest.ini. The CI lint job could not fail: --exit-zero *and* continue-on-error. Ruff now gates in the test job with neither, and the job is renamed typecheck to reflect that mypy really is advisory. Ruff replaces flake8 and pylint. It found three more test classes shadowing earlier definitions of the same name -- TestControlMapping, TestBrightnessComputation, TestResolutionScaling -- so 15 tests had never run. Renamed; all 15 pass. Also a genuine oddity in db_graphs' smooth_data: it computed a Gaussian sigma from the window size and then ignored it, hardcoding the kernel width via linspace(-2, 2). UP (pyupgrade) is deliberately not enabled: it wants ~200 changes, almost all typing.Dict -> dict, which deserves its own commit. pyproject dependencies were wrong for packaging -- PyYAML and Pillow only, while the code needs numpy, astral and requests. Added those, with requests-toolbelt and matplotlib as extras since they are only needed for uploads and graphs. Version is now dynamic from src/__version__.py instead of a fourth hardcoded copy. Config example: removed the whole `timelapse:` block (interval 3s next to adaptive_timelapse.interval 30s, and no code reads the former), the `graphs:` block (db_graphs takes --output and uses module constants), and six keys nothing reads. Added the five the code needs and the example lacked, so a fresh clone finally runs the same path as a configured camera. New tests/test_config_example.py fails on drift in both directions: documented-but-unread keys, and sections the code reads that the example lacks. Verified against injected drift of each kind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
The documented install path was broken. README installed 3 of the 8
packages the code needs, INSTALL.md installed 6 and missed both requests
and requests-toolbelt, and four separate files pointed at
install_service.sh, which was renamed in v1.0.0. Two more pointed at
src/make_timelapse_daily.py, which has never existed under that name.
The README's config snippet contradicted config.example.yml on nearly
every line, including documenting timelapse.interval -- a key no code
reads, sitting next to the one that matters.
Deleted, in order of how little they belonged in a public repo:
- NEXT_SESSION_CONTEXT.md, an AI session diary citing line numbers as
if they were API
- SETUP_COMPLETE.md, a trip report about one specific Pi ("Memory:
150 MB stable"), linking four files that never existed
- Howto.md and Update.md, single-machine runbooks at the repo root
- MONITORING_SETUP.md, whose "quick reference" installed a cron entry
duplicating the systemd cleanup timer, and put a root-requiring
watchdog in the user crontab where it could not work
- CLAUDE.md, 1,011 lines of Picamera2 vendor notes; linked upstream now
- MAINTENANCE.md, DAILY_VIDEO.md, SLITSCAN_UPGRADE.md, absorbed elsewhere
TRANSITION_SMOOTHING.md and ADAPTIVE_TIMELAPSE_FLOW.md are merged into
one EXPOSURE.md that describes the system as it now is, rather than as a
sequence of fixes to what it used to be. New TROUBLESHOOTING.md absorbs
the troubleshooting sections that were triplicated across README,
INSTALL and SERVICE.
Nothing states a schedule in prose any more. Four files claimed 04:00
while the timer said 05:00; the docs now point at
`systemctl list-timers`.
Version was three different numbers at once: 1.3.2 in CHANGELOG, 1.1.0
in pyproject and __version__, 0.9.0-beta in CITATION.cff. All now 1.4.0
from src/__version__.py, with tests asserting CHANGELOG and CITATION
agree and that pyproject reads it dynamically.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
…odules overlay.py was 1,961 lines with a 500-line apply_overlay(). Now 1,214, with overlay_sources.py (665) and overlay_draw.py (113) beside it. Ships, tide and aurora each had their own copy of the same loader: read a JSON file written by another service, cache it for N seconds, and serve the last good copy if the file is missing or half-written. Byte-identical apart from the config keys, the TTL, the log noun, and whether the payload is wrapped in an envelope. CachedJsonSource is the one copy; the subclasses set four class attributes and, where needed, override _extract. The tide maths, ship formatting and aurora arrows are genuine domain logic and stay with their classes. The three classes are re-exported from overlay.py, so the 113 overlay tests needed no changes at all. overlay_draw.py takes the idioms that apply_overlay repeated: measuring a string with a fallback for fonts that cannot be measured (ten times), formatting a template slot while surviving an unknown placeholder (eight times), and reserving a fixed width from template maxima so a section does not jitter frame to frame. Two shadowing hazards came out of it. _get_position had locals named text_width and text_height, exactly the names of the new helpers, and the tide section had a local text_width used several lines after the call that would have shadowed it. Renamed both; the tests would not have caught either, since the shadowed name still resolved to something plausible. Verified on hardware by comparing the rendered top bar before and after: camera name, localised date, weather, exposure, tide with its wave graphic and aurora all identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 11 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 (13)
📝 WalkthroughWalkthroughRaspilapse 1.4.0 centralizes adaptive exposure, configuration, logging, database maintenance, overlays, uploads, weather caching, systemd installation, and validation. ML exposure modules and obsolete operational assets are removed, while documentation and release metadata are updated. ChangesRaspilapse 1.4.0
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 |
CI caught me doing exactly the thing this branch set out to fix. I pinned black==26.1.0 in three places and then formatted the tree with whatever `black` was on PATH here, which was 24.10.0. The two disagree on six files, so `black --check` failed on 3.11 -- the same locally-formatted-code-rejected-by-CI failure the pin was meant to end. And 26.x requires Python >=3.10, so `pip install -r requirements-dev.txt` could not resolve at all on the 3.9 matrix leg. The README claims 3.9+ and Bullseye ships 3.9, so the matrix is right and the pin was wrong. 25.11.0 is the newest black that supports 3.9. Formatting with it leaves all 49 files unchanged, so this commit is three lines of pin and a comment recording the constraint, rather than a reformat. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
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: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/db_graphs.py (2)
840-850: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLeftover "ML" terminology survives the rename/removal of the ML exposure system.
Per this PR's changelog, the ML exposure system was removed entirely and
create_ml_diagnostics_graphwas renamed tocreate_brightness_diagnostics_graph. But the docstring here (842) still says "Create ML diagnostics graph for monitoring ML-first exposure system", the panel-3 comment/legend still talk about an "ML trust reduction threshold" (991-998), and the figure title still reads"ML Exposure Diagnostics - {time_desc}"(1028-1030) — as does the adjacent unchanged comment on line 1357 ("ML diagnostics graphs (for monitoring ML-first exposure system)"). The same pattern shows up increate_exposure_efficiency_graph: only the two fill labels were updated to "Above/Below reference curve" (1235/1244), while its docstring ("Shows how much ML is deviating from the simple formula", 1130), the "Actual (ML-blended)" legend label (1196), and the "ML Exposure Deviation from Formula" panel-2 title (1256) still reference the removed system.These are user-visible chart titles/labels that will now be actively misleading about a system that no longer exists.
Also applies to: 1028-1030, 1126-1132
🤖 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 `@scripts/db_graphs.py` around lines 840 - 850, Remove the leftover ML terminology from create_brightness_diagnostics_graph, its panel-3 comments/legend, and the adjacent diagnostics comment, replacing it with brightness/exposure terminology consistent with the current system. Update the figure title in create_brightness_diagnostics_graph and the docstring, legend, and panel-2 title in create_exposure_efficiency_graph so all user-visible text describes brightness/exposure behavior rather than ML or ML blending.
219-252: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd a migration or defensive lookup for the new colour columns.
CAPTURES_DDLincludescolour_gains_r/colour_gains_b/colour_temperature, butapply_schema()never backfills them onto an existingcapturestable, so older databases will hitKeyErrorhere and skip rows after partially appending earlier fields.🤖 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 `@scripts/db_graphs.py` around lines 219 - 252, The row-processing logic should remain compatible with existing databases whose captures tables lack the new colour columns. Update apply_schema() to add/backfill colour_gains_r, colour_gains_b, and colour_temperature for existing tables, or update the lookups in the row loop to use safe defaults; ensure missing columns cannot raise KeyError and cause partially appended rows to be skipped.
🟡 Minor comments (16)
src/exposure.py-627-628 (1)
627-628: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
.get(key, default)does not protect against an explicitNonevalue.If
brightness_metricscontainsmean_brightness: None(key present, measurement failed), the default is not applied and the following comparisons raiseTypeErrorinsideobserve_frame. Usemetrics.get("mean_brightness") or 0/ an explicitis Nonecheck.🛡️ Proposed fix
- mean_brightness = brightness_metrics.get("mean_brightness", 0) - overexposed_pct = brightness_metrics.get("overexposed_percent", 0) + mean_brightness = brightness_metrics.get("mean_brightness") + overexposed_pct = brightness_metrics.get("overexposed_percent") + if mean_brightness is None: + return self._overexposure_detected + if overexposed_pct is None: + overexposed_pct = 0Also applies to: 690-690
🤖 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 `@src/exposure.py` around lines 627 - 628, Update the brightness metric reads in observe_frame, including mean_brightness and the corresponding metric at the additional occurrence, so explicit None values resolve to 0 before comparisons; preserve valid nonzero measurements and apply the same handling to overexposed_pct if it can also be None.src/exposure.py-81-90 (1)
81-90: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winEqual thresholds divide by zero.
If a config sets
safe_p95 == warning_p95(orwarning_p95 == critical_p95), these interpolations raiseZeroDivisionErrorinside the capture loop. A cheap guard keeps a bad config from killing captures.🛡️ Proposed guard
- if p95 <= warning: + if p95 <= warning: + if warning <= safe: + return 0.95 # safe -> 1.00, warning -> 0.95 return 1.0 - ((p95 - safe) / (warning - safe)) * 0.05 - if p95 <= critical: + if p95 <= critical: + if critical <= warning: + return 0.85 # warning -> 0.95, critical -> 0.85 return 0.95 - ((p95 - warning) / (critical - warning)) * 0.10🤖 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 `@src/exposure.py` around lines 81 - 90, Update the p95 scoring interpolation around the safe, warning, and critical threshold checks to guard against equal thresholds before dividing. Ensure configurations where safe_p95 equals warning_p95 or warning_p95 equals critical_p95 return a valid score without raising ZeroDivisionError, while preserving normal interpolation for distinct thresholds and the existing floor behavior.src/upload_service.py-544-552 (1)
544-552: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
cancel_uploaddeletes the row, so a transient path problem is unrecoverable.
cancel_upload()is a hardDELETE. Ifvideo_pathsits on a mount that is momentarily unavailable (or the process runs with a different CWD for a relative path), the queue entry is destroyed rather than deferred, and there is no record that the upload was ever due. Marking the row terminal (status='cancelled', whichprune()insrc/database.pyalready expects and cleans up after 90 days) preserves the audit trail and keeps the two modules consistent.🤖 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 `@src/upload_service.py` around lines 544 - 552, Replace the hard-delete call to cancel_upload in the missing-source branch of the upload processing flow with an update that marks the upload row as status='cancelled'. Preserve the existing message, logging, and return behavior, and use the database status update mechanism expected by prune() so the row remains available for cleanup and auditing.src/database.py-941-946 (1)
941-946: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCatch
yaml.YAMLErrortoo.A malformed
config.ymlraises out ofyaml.safe_loadand this exits with a traceback, unlikeretry_uploads.py/daily_timelapse.pywhich print a message and return 1. Since this runs fromraspilapse-cleanup.service, a clean exit keeps the journal readable.🛡️ Proposed fix
try: with open(args.config) as f: config = yaml.safe_load(f) or {} except OSError as e: print(f"Error: could not read {args.config}: {e}") return 1 + except yaml.YAMLError as e: + print(f"Error: invalid YAML in {args.config}: {e}") + return 1🤖 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 `@src/database.py` around lines 941 - 946, Extend the exception handling around config loading in the main flow to catch yaml.YAMLError alongside OSError, so malformed YAML prints the existing read-error message and returns 1 cleanly. Keep the current successful yaml.safe_load behavior unchanged.scripts/install.sh-73-73 (1)
73-73: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
usageprints theset -euo pipefailline.The header comment ends at Line 18; Line 19 is
set -euo pipefail, which has no leading#and so survives the strip and gets printed as part of the help text.🐛 Proposed fix
-usage() { sed -n '2,20p' "${BASH_SOURCE[0]}" | sed 's/^# \{0,1\}//'; } +usage() { sed -n '2,18p' "${BASH_SOURCE[0]}" | sed 's/^# \{0,1\}//'; }🤖 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 `@scripts/install.sh` at line 73, Update usage() so its extracted help-text range stops before the executable set -euo pipefail line, while preserving the existing comment-stripping behavior for the header.src/overlay_sources.py-596-606 (1)
596-606: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winZero levels render as empty strings.
if target_level/if high_level/if low_leveltreat a genuine0.0m reading (chart datum) as "no value", blanking the widget field. Compare againstNone.🔧 Proposed fix
- "target_level_str": f"{int(target_level * 100)}cm" if target_level else "", + "target_level_str": f"{int(target_level * 100)}cm" if target_level is not None else "", ... - "high_level_str": f"{int(high_level * 100)}cm" if high_level else "", + "high_level_str": f"{int(high_level * 100)}cm" if high_level is not None else "", ... - "low_level_str": f"{int(low_level * 100)}cm" if low_level else "", + "low_level_str": f"{int(low_level * 100)}cm" if low_level is not None else "",🤖 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 `@src/overlay_sources.py` around lines 596 - 606, Update the level string expressions in the overlay data construction to check each value against None rather than using truthiness, so genuine 0.0 readings render as “0cm” while missing values remain empty. Apply this to target_level, high_level, and low_level.src/weather.py-155-163 (1)
155-163: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBackoff collapses to zero when
cache_durationis 0.
delay = cache_duration * 2**(failures-1)— a config withcache_duration: 0yieldsnext_attempt_at == now, restoring the every-call network hit and log spam this change exists to prevent. Floor the base.🛡️ Proposed floor on the backoff base
- delay = min(self.cache_duration * (2 ** (entry.failures - 1)), self.max_backoff) + base = max(self.cache_duration, timedelta(seconds=30)) + delay = min(base * (2 ** (entry.failures - 1)), self.max_backoff)🤖 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 `@src/weather.py` around lines 155 - 163, Update _record_failure so the exponential backoff uses a positive minimum base instead of multiplying directly by self.cache_duration. Preserve the existing doubling and max_backoff cap, ensuring cache_duration values of 0 still produce a nonzero next_attempt_at delay.CONTRIBUTING.md-20-28 (1)
20-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win"all three" undercounts
make all's steps.
make allis defined asformat lint check test(4 targets) in the updated Makefile, but this section only listsformat/lint/testand says "which runs all three," omitting thecheck(black--check) step.📝 Proposed fix
```bash make format # black make lint # ruff +make check # black --check make test # pytest-or
make all, which runs all three.
+ormake all, which runs all four.</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@CONTRIBUTING.mdaround lines 20 - 28, Update the “Before you commit” section
in CONTRIBUTING.md to list the make check target between make lint and make
test, and change the make all description from “all three” to “all four” so it
matches the four targets run by make all.</details> <!-- cr-comment:v1:724245e8333caae5e0074864 --> </blockquote></details> <details> <summary>README.md-101-104 (1)</summary><blockquote> `101-104`: _📐 Maintainability & Code Quality_ | _🟡 Minor_ | _⚡ Quick win_ **Add a language to the two fenced blocks (markdownlint MD040).** Both fences are unlabelled; `text` is enough to satisfy the rule. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text mode = f(smoothed lux, sun elevation) night | transition | day exposure = current * (target / measured) ** damping```diff -``` +```text src/ auto_timelapse.py capture loop, scheduling, lifecycleAlso applies to: 147-158
🤖 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 101 - 104, Add the `text` language identifier to both unlabelled fenced code blocks in README.md, including the block containing the mode/exposure formulas and the additional block around the src/auto_timelapse.py listing, without changing their contents.Source: Linters/SAST tools
src/logging_config.py-247-251 (1)
247-251: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
setup_loggerregisters underscript_namebut formats the filename fromself.script_name.With
LoggerConfig(script_name="a").setup_logger("b")the first_applywrites toa.log, while the registry key is"b"— so a laterconfigure_logging()re-applies with"b"and the same logger silently moves tob.log.🐛 Use one name consistently
def setup_logger(self, name: Optional[str] = None) -> logging.Logger: script_name = name or self.script_name - logger = _apply(logging.getLogger(script_name), self.script_name, self.config["logging"]) + logger = _apply(logging.getLogger(script_name), script_name, self.config["logging"]) _registry[script_name] = logger return logger🤖 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 `@src/logging_config.py` around lines 247 - 251, Update setup_logger to use the resolved script_name consistently when applying logging configuration and registering the logger, so setup_logger("b") formats and reconfigures the logger as "b" rather than self.script_name.src/logging_config.py-140-142 (1)
140-142: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
enabled: falsestill lets records reach the root logger.The early return adds a
NullHandlerbut skips thelogger.propagate = Falseat line 173.NullHandlerdoes not stop propagation, so if anything has configured root handlers the "disabled" logger still emits.🐛 Proposed fix
if not settings.get("enabled", True): logger.addHandler(logging.NullHandler()) + logger.propagate = False return logger🤖 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 `@src/logging_config.py` around lines 140 - 142, Update the disabled-logging branch in the logger configuration flow to set logger.propagate = False before returning, matching the propagation behavior of the normal configuration path. Keep the existing NullHandler and early return unchanged otherwise.docs/TROUBLESHOOTING.md-89-89 (1)
89-89: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLabel the sample-output fence.
Add
textto this fence to clear markdownlint MD040.🤖 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/TROUBLESHOOTING.md` at line 89, Label the sample-output code fence in TROUBLESHOOTING.md with the text language identifier by adding `text` to its opening fence, leaving the sample content unchanged.Source: Linters/SAST tools
docs/SERVICE.md-9-9 (1)
9-9: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFinish the centralized-installer documentation migration. The new installer is documented, but both pages retain instructions from the deleted per-service installation flow.
docs/SERVICE.md#L9-L9: update the surrounding service table/timeline to the current four-unit deployment and current timer schedule.docs/USAGE.md#L156-L156: replace the preceding./scripts/install_daily_video.shcommand with the supportedscripts/install.shflow.🤖 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/SERVICE.md` at line 9, Update docs/SERVICE.md around the service table/timeline to describe the current four-unit deployment and timer schedule. In docs/USAGE.md around line 156, replace the obsolete ./scripts/install_daily_video.sh instruction with the supported scripts/install.sh installation flow.docs/EXPOSURE.md-31-31 (1)
31-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd fence languages to satisfy markdownlint.
Label these pseudocode/log blocks as
text(or the applicable language) to clear MD040.Also applies to: 54-54, 88-88, 108-108, 119-119, 151-151
🤖 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/EXPOSURE.md` at line 31, Update the fenced code blocks at the referenced locations in EXPOSURE.md to include an explicit language fence, using text for pseudocode or log content and a more specific applicable language where appropriate, so all blocks satisfy markdownlint MD040.Source: Linters/SAST tools
docs/USAGE.md-122-126 (1)
122-126: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the graph-input description.
db_graphs.pyreads the SQLite database and writes PNG graphs; it does not analyze JSON metadata or export Excel. Update the heading and description to match the replacement commands.🤖 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/USAGE.md` around lines 122 - 126, Update the db_graphs.py usage heading and description to state that it reads the SQLite database and generates PNG graphs. Remove the inaccurate JSON metadata and Excel export claims, and ensure the wording matches the documented replacement commands.docs/INSTALL.md-30-34 (1)
30-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInstall
python3-pipbefore usingpip3. The fresh-image package list doesn’t include it, so step 2 fails on a minimal Raspberry Pi OS install.🤖 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` around lines 30 - 34, Update the package installation commands in the installation instructions to include python3-pip before any documented pip3 usage, ensuring the setup succeeds on a minimal Raspberry Pi OS image.
🧹 Nitpick comments (22)
tests/test_overcast_brightness.py (1)
214-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
test_custom_configasserts the default values.The fixture's
brightness_targetblock is identical to the defaults asserted intest_config_defaults, so this test passes even if config loading is ignored entirely. Use non-default values (e.g.base: 110,overcast_boost: 20) in a dedicated config to make it meaningful.🤖 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_overcast_brightness.py` around lines 214 - 220, Update the dedicated configuration used by test_custom_config to provide non-default brightness_target values, such as base 110 and overcast_boost 20, and adjust the assertions for all configured fields accordingly. Keep test_config_defaults focused on validating the defaults while ensuring test_custom_config verifies that custom values are actually loaded.tests/test_config_example.py (2)
68-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe "is it read?" check is name-only, not path-aware.
Matching just the leaf against every quoted identifier in
src/+scripts/means a key likeenabledorpathpasses regardless of which section it belongs to, so drift in nested blocks slips through. Worth noting in the docstring, or tightening to the full dotted lookup where feasible.🤖 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 68 - 84, Make test_every_documented_key_is_read_somewhere validate configuration keys by their full dotted paths rather than only matching the final leaf name, so identical names in different sections cannot satisfy each other. Update the docstring to document the path-aware behavior, and preserve ALLOWED_UNUSED handling and the existing unread-key assertion.
29-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ALLOWED_UNDOCUMENTEDis never referenced.The docstring promises to catch drift "in both directions", but only top-level sections are checked against the code; no test consumes this allow-list, so
database.retention_daysdocuments an exemption from a check that does not exist. Either wire it into a leaf-level "code reads a key the example omits" assertion or drop the constant.Want me to draft the missing check?
🤖 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 29 - 33, Update the configuration coverage test around ALLOWED_UNDOCUMENTED so it is consumed by a leaf-level assertion detecting keys read by code but omitted from the example, while preserving the documented exemption for database.retention_days; otherwise remove the unused constant if that check is intentionally out of scope.src/exposure.py (2)
557-570: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReturn type is only nominally
int.Branches return raw config values (
_base_target_brightness,base + boost) which areAny/floatdepending on YAML; only the interpolation branch rounds. mypy flags all five returns. Coerce each branch toint(...)for a consistent contract.🤖 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 `@src/exposure.py` around lines 557 - 570, Update the return statements in the brightness calculation method containing the std_brightness threshold branches so every path explicitly coerces its result to int. Apply int conversion to the high-contrast base value, the low-contrast capped boost, and the interpolated capped boost while preserving the existing min, rounding, and threshold behavior.Source: Pipeline failures
111-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate optional state as
Optional[...].
self._smoothed_lux: float = None(and the sibling_last_*fields) are annotated non-optional but initialized toNone; mypy also reportsReturning Anyfromtransition_positionat Line 281 becausethresholdsis untyped. Typing these asOptional[float]and coercing the return tofloatclears part of the failing typecheck job.Also applies to: 273-281
🤖 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 `@src/exposure.py` around lines 111 - 121, Update the nullable state annotations in the exposure class, including _smoothed_lux and sibling _last_* fields, to use Optional with their existing value types; annotate _day_wb_reference and _last_colour_gains as Optional tuples as well. In transition_position, give thresholds an explicit type and ensure its return value is explicitly coerced to float so mypy no longer reports Returning Any.Source: Pipeline failures
tests/test_create_keogram.py (1)
343-349: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClass names still say
FromImages.The wrappers are gone;
TestCreateKeogramFromImages/TestCreateSlitscanFromImages(and the "convenience wrapper" test docstrings) now describe a function that no longer exists. Renaming keeps the suite navigable.Also applies to: 515-521
🤖 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_create_keogram.py` around lines 343 - 349, Rename the test classes TestCreateKeogramFromImages and TestCreateSlitscanFromImages to reflect the current create_keogram and create_slitscan APIs, removing the obsolete “FromImages” wording. Update the affected test docstrings, including “convenience wrapper” descriptions, so they describe the direct functions rather than removed wrappers.tests/test_version.py (1)
78-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParse
pyproject.tomlstructurally instead of matching raw substrings.
Those exact-string checks are whitespace-brittle; since the project supports Python 3.9, usetomlior a smalltomllibfallback if you want to assert the dynamic version config.🤖 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_version.py` around lines 78 - 82, Update test_pyproject_reads_version_dynamically to parse pyproject.toml structurally rather than asserting raw substrings. Use tomli for Python 3.9, or a tomllib import fallback where available, then validate the parsed project dynamic version and tool configuration values.tests/test_database.py (1)
874-918: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
with sqlite3.connect(...)commits but does not close.Both tests leave the setup connection open for the rest of the test, which is exactly the condition under which
apply_schema'sPRAGMA journal_mode=WALbails out (the code comments on that). The assertions here don't depend on WAL, so nothing fails today, but wrapping incontextlib.closingmakes the fixture state unambiguous.♻️ Proposed change
- with sqlite3.connect(path) as conn: + with contextlib.closing(sqlite3.connect(path)) as conn: conn.execute(CAPTURES_DDL)🤖 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_database.py` around lines 874 - 918, Wrap the sqlite3.connect calls in test_migration_5_drops_indexes_on_an_existing_database and test_upload_service_creates_the_v4_index with contextlib.closing so each setup and assertion connection is explicitly closed after use. Preserve the existing commits, queries, and assertions.src/database.py (1)
759-792: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrefer
cursor.rowcountfor the reported delete counts.The counts are gathered by two extra
SELECT COUNT(*)scans before the deletes; on a 500k-row table that doubles the work and the reported number can drift from what was actually deleted (autocommit connection, inserts can land in between).rowcountafter eachDELETEgives the exact figure for free — keep the count queries only for thedry_runpath.♻️ Sketch
- cursor.execute( - "DELETE FROM captures WHERE unix_timestamp < strftime('%s', 'now', ?)", - (cutoff,), - ) + cursor.execute( + "DELETE FROM captures WHERE unix_timestamp < strftime('%s', 'now', ?)", + (cutoff,), + ) + result["captures"] = cursor.rowcount🤖 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 `@src/database.py` around lines 759 - 792, Update the pruning flow to keep the existing COUNT queries only when dry_run is true, then use cursor.rowcount immediately after each DELETE to populate result["captures"] and result["upload_queue"] for actual deletion runs. Preserve the dry-run counts and logging, and ensure reported non-dry-run counts reflect the rows deleted by the corresponding statements.tests/test_graph_solar_patterns.py (2)
220-223: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHard-coded 50 KB PNG floor is brittle.
Rendered size depends on the matplotlib version, DPI and available fonts; a lighter renderer in CI can drop below this without anything being wrong. Asserting the file is non-empty (or has a PNG magic header) tests the same thing without the version coupling.
♻️ Proposed change
assert result is True assert os.path.exists(output_path) - # Check file size is reasonable (should be > 50KB for a graph) - assert os.path.getsize(output_path) > 50000 + assert os.path.getsize(output_path) > 0 + with open(output_path, "rb") as fh: + assert fh.read(8) == b"\x89PNG\r\n\x1a\n"🤖 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_graph_solar_patterns.py` around lines 220 - 223, Replace the hard-coded 50,000-byte size assertion in the graph output test with a renderer-independent validity check: verify the generated file is non-empty, preferably also confirming its PNG signature. Keep the existing result and path existence assertions unchanged.
18-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test reads the developer's real
config/config.yml.
get_db_path()with no argument falls back to loading the on-disk config (src/config_utils.pyLines 61-85), so the assertion silently depends on whateverdatabase.patha local checkout happens to have. Pass an explicit non-existentconfig_pathto actually exercise the default branch.🤖 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_graph_solar_patterns.py` around lines 18 - 24, Update test_returns_default_path_when_no_config to pass an explicit config_path pointing to a non-existent configuration file when calling get_db_path(), ensuring the test exercises the default-path branch without reading the developer’s real config/config.yml. Keep the existing timelapse.db assertion.scripts/install.sh (2)
275-279: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWatchdog units are enumerated twice, and Line 278 will trip the new lint gate.
"${COMPONENT_UNITS[@]}"already expands thewatchdogentry, so appending"${COMPONENT_UNITS[watchdog]}"duplicates it (harmless, but the second pass is dead work). The unquoted$uniton Line 278 is deliberate word-splitting — add an explicit disable so shellcheck stays green in CI.♻️ Proposed cleanup
do_uninstall() { local unit units=() - for unit in "${COMPONENT_UNITS[@]}" "${COMPONENT_UNITS[watchdog]}"; do + for unit in "${COMPONENT_UNITS[@]}"; do + # shellcheck disable=SC2206 # intentional word splitting: values hold multiple units units+=($unit) done🤖 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 `@scripts/install.sh` around lines 275 - 279, Update do_uninstall to iterate only over "${COMPONENT_UNITS[@]}" so the watchdog unit is not appended a second time, and add a narrowly scoped ShellCheck disable for the intentional unquoted $unit expansion in the units+= line.Source: Linters/SAST tools
173-173: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfig path is interpolated into a Python string literal.
$cfg/$PROJECT_DIRare spliced inside'...'in the embedded Python; a path containing a quote or backslash produces a syntax error rather than a clear message. Pass it assys.argvinstead, ascheck_configalready does for the second snippet.🛡️ Proposed fix
- if ! "$PYTHON" -c "import yaml,sys; yaml.safe_load(open('$cfg'))" 2>/dev/null; then + if ! "$PYTHON" -c "import yaml,sys; yaml.safe_load(open(sys.argv[1]))" "$cfg" 2>/dev/null; then- out=$("$PYTHON" -c " -import yaml -cfg = yaml.safe_load(open('$PROJECT_DIR/config/config.yml')) or {} -print((cfg.get('output') or {}).get('directory') or '')" 2>/dev/null) + out=$("$PYTHON" - "$PROJECT_DIR/config/config.yml" <<'PY' 2>/dev/null +import sys, yaml +cfg = yaml.safe_load(open(sys.argv[1])) or {} +print((cfg.get('output') or {}).get('directory') or '') +PY +)Also applies to: 204-207
🤖 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 `@scripts/install.sh` at line 173, Update the config-validation Python invocation in the install script to pass $cfg as a command-line argument and read it via sys.argv, rather than interpolating it inside a Python string literal. Apply the same change to the related second snippet around check_config, preserving the existing YAML loading and validation behavior for paths containing quotes or backslashes.systemd/raspilapse-upload-retry.service.in (1)
7-15: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider an explicit
TimeoutStartSecfor the upload run.
Type=oneshotinheritsDefaultTimeoutStartSec(90s on most systems). A daily video pushed over a slow uplink can exceed that and get SIGTERM'd mid-transfer, leaving the queue entry to churn every 30 minutes.♻️ Proposed change
Type=oneshot +TimeoutStartSec=30min User=`@USER`@🤖 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 `@systemd/raspilapse-upload-retry.service.in` around lines 7 - 15, Add an explicit TimeoutStartSec setting to the raspilapse-upload-retry systemd service so slow uploads are allowed to complete beyond the inherited startup timeout; choose a duration appropriate for daily video transfers and leave the existing ExecStart and logging configuration unchanged.src/daily_timelapse.py (1)
36-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn type should be
Optional[Path].
_find_dated_file(and the threefind_*wrappers) can returnNone, which callers explicitly handle at Line 237/246/251, but the annotations claimPath.🤖 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 `@src/daily_timelapse.py` around lines 36 - 56, Update _find_dated_file and all three find_* wrapper functions to annotate their return type as Optional[Path], matching their existing None results and caller handling. Import Optional if required by the module’s typing conventions; do not change the search behavior.systemd/raspilapse-cleanup.service.in (1)
14-15: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA failing image cleanup silently skips the DB prune.
With
Type=oneshot, systemd aborts the remainingExecStart=lines when one exits non-zero, so a transient failure incleanup_old_images.shalso blocksdatabase.py --prune— and the database keeps growing unnoticed. Prefix the first with-if the two steps are meant to be independent.🔧 Proposed change
-ExecStart=/bin/bash `@PROJECT_DIR`@/scripts/cleanup_old_images.sh +ExecStart=-/bin/bash `@PROJECT_DIR`@/scripts/cleanup_old_images.sh ExecStart=`@PYTHON`@ `@PROJECT_DIR`@/src/database.py --prune🤖 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 `@systemd/raspilapse-cleanup.service.in` around lines 14 - 15, Update the cleanup service’s first ExecStart command for cleanup_old_images.sh to use systemd’s ignored-failure prefix, allowing the subsequent database.py --prune command to run even when image cleanup exits non-zero. Leave the prune command unchanged.src/overlay.py (1)
421-423: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass the already-resolved data explicitly.
When
weather_dataisNone(Line 411 returned nothing and there is no fallback),format_fields(None)callsget_weather_data()a second time. The result is the same dash-filled dict, but the extra call is redundant; considerformat_fields(weather_data or {})to make the "no data" path explicit.🤖 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 `@src/overlay.py` around lines 421 - 423, Update the data preparation in the overlay method around weather_data so format_fields receives the already-resolved value, using an empty mapping when weather_data is None. This prevents format_fields from fetching weather data a second time while preserving the existing dash-filled no-data behavior.systemd/raspilapse-daily-video.service.in (1)
20-21: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
infinitymeans a wedged encode never gets reaped.If ffmpeg hangs, the unit stays activating forever and subsequent timer firings are skipped with no failure signal. A generous bound (e.g.
TimeoutStartSec=4h) preserves the 25-minute happy path while still recovering.🤖 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 `@systemd/raspilapse-daily-video.service.in` around lines 20 - 21, Replace the unbounded TimeoutStartSec=infinity setting in the raspilapse daily video service with a generous finite timeout, such as 4 hours, so normal 25-minute encodes complete while wedged ffmpeg processes are eventually reaped and reported as failures.scripts/db_graphs.py (1)
1039-1090: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
smooth_datareceives NaN-filled lists, which can widen small data gaps into much larger blank stretches.
create_white_balance_graphfeeds[g if g is not None else float("nan") for g in gains_r]straight intosmooth_data(), whose Gaussian convolution propagates NaN to every output sample within the kernel's radius of any missing point.create_brightness_diagnostics_graphavoids this elsewhere in the same file by pre-filtering tovalid_indicesbefore smoothing. Consider the same pattern here for a more robust chart.🤖 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 `@scripts/db_graphs.py` around lines 1039 - 1090, Update create_white_balance_graph to avoid passing NaN-filled gain lists directly to smooth_data; follow the existing valid_indices pattern from create_brightness_diagnostics_graph by smoothing only contiguous valid gain samples while preserving their original timestamps and gaps in the plotted output..pre-commit-config.yaml (1)
18-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRuff version isn't pinned as tightly as black, unlike the precedent set for black in this PR.
blackis pinned to an exact version consistently (rev: 26.1.0here,black==26.1.0in pyproject.toml) specifically to avoid the CI formatting-mismatch bug this PR describes.ruffdoesn't get the same treatment: this hook pinsv0.14.13exactly, but pyproject.toml's dev extra allowsruff>=0.14.0with no upper bound, so a freshpip install -r requirements-dev.txtcan pull a much newer ruff (latest is currently 0.16.x) than what pre-commit enforces. Ruff's rule set and defaults do change across minor releases, so this reopens the same version-drift risk already fixed for black.♻️ Suggested alignment
- "ruff>=0.14.0", + "ruff==0.14.13",in
pyproject.toml'sdevextra, or bump both to the same current pin.🤖 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 @.pre-commit-config.yaml around lines 18 - 22, Align the Ruff version across the pre-commit hook and the pyproject.toml dev extra: update the dev dependency from the open-ended ruff>=0.14.0 constraint to the exact version represented by the hook’s rev v0.14.13, or update both locations to the same newer exact pin. Preserve the existing Ruff hook configuration.src/auto_timelapse.py (1)
31-44: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe fallback branch drops the two re-exports the comment promises.
The
src.exposurebranch re-exportsBrightnessZonesandhighlight_factor; the flat branch imports onlyExposureControllerandLightMode. Per.github/workflows/tests.ymllines 113-116, the flat branch is what systemd actually runs, soauto_timelapse.BrightnessZones/.highlight_factorexist undersrc.-style imports (tests) but not in production.♻️ Keep both branches symmetric
except ImportError: from capture_image import CameraConfig, ImageCapture - from exposure import ExposureController, LightMode + from exposure import ( # noqa: F401 + BrightnessZones, + ExposureController, + LightMode, + highlight_factor, + ) from logging_config import configure_logging, get_logger🤖 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 `@src/auto_timelapse.py` around lines 31 - 44, Make the fallback import branch in auto_timelapse symmetric with the src.exposure branch by importing and exposing BrightnessZones and highlight_factor alongside ExposureController and LightMode. Preserve the existing imports and ensure both flat and src-style execution provide auto_timelapse.BrightnessZones and auto_timelapse.highlight_factor..github/workflows/tests.yml (1)
111-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the import loop fail deterministically.
python -c "import $module" && echo "ok $module"puts the import on the left of an&&list, which is exactly the positionset -eexempts; whether the step fails then depends on bash's handling of the enclosing list and on which module broke. Since this step exists to gate the production flat-import path, it should be unambiguous.♻️ Explicit failure handling
cd src + status=0 for module in logging_config config_utils colors exposure weather database; do - python -c "import $module" && echo "ok $module" + if python -c "import $module"; then + echo "ok $module" + else + echo "FAIL $module" + status=1 + fi done + exit "$status"🤖 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 @.github/workflows/tests.yml around lines 111 - 119, Update the module verification loop in the workflow so each import failure explicitly terminates the step with a nonzero status instead of relying on the current && list behavior. Preserve the existing module list, working directory, success output, and flat-import validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a0f8256-c248-43f2-8782-4398adecdd60
⛔ Files ignored due to path filters (3)
manuals/camera_guide.pdfis excluded by!**/*.pdfmanuals/manual.pdfis excluded by!**/*.pdfmanuals/picamera.pdfis excluded by!**/*.pdf
📒 Files selected for processing (119)
.github/workflows/tests.yml.gitignore.pre-commit-config.yamlCHANGELOG.mdCITATION.cffCONTRIBUTING.mdHowto.mdLICENSEML.mdMakefileREADME.mdUpdate.mdconfig/README.mdconfig/config.example.ymldocs/ADAPTIVE_TIMELAPSE_FLOW.mddocs/BLACK_FORMATTING_GUIDE.mddocs/CLAUDE.mddocs/CONTRIBUTING.mddocs/DAILY_VIDEO.mddocs/EXPOSURE.mddocs/INSTALL.mddocs/MAINTAINER.mddocs/MAINTENANCE.mddocs/MONITORING_SETUP.mddocs/NEXT_SESSION_CONTEXT.mddocs/SERVICE.mddocs/SETUP_COMPLETE.mddocs/SLITSCAN_UPGRADE.mddocs/TIMELAPSE_VIDEO.mddocs/TRANSITION_SMOOTHING.mddocs/TROUBLESHOOTING.mddocs/USAGE.mdml_state/ml_state.jsonpyproject.tomlpytest.inirequirements-dev.txtrequirements.txtscripts/check_capture_rate.shscripts/check_disk_space.shscripts/check_service.shscripts/db_graphs.pyscripts/db_stats.pyscripts/graph_solar_patterns.pyscripts/install.shscripts/install_cleanup.shscripts/install_daily_video.shscripts/test.shscripts/uninstall.shscripts/uninstall_daily_video.shsrc/__version__.pysrc/analyze_timelapse.pysrc/apply_overlay.pysrc/auto_timelapse.pysrc/bootstrap_ml.pysrc/bootstrap_ml_v2.pysrc/capture_image.pysrc/colors.pysrc/config_utils.pysrc/create_keogram.pysrc/daily_timelapse.pysrc/database.pysrc/exposure.pysrc/logging_config.pysrc/make_timelapse.pysrc/ml_exposure.pysrc/ml_exposure_v2.pysrc/overlay.pysrc/overlay_draw.pysrc/overlay_sources.pysrc/retry_uploads.pysrc/status.pysrc/upload_service.pysrc/weather.pysystemd/journald-raspilapse.confsystemd/raspilapse-cleanup.servicesystemd/raspilapse-cleanup.service.insystemd/raspilapse-cleanup.timersystemd/raspilapse-cleanup.timer.insystemd/raspilapse-daily-video.servicesystemd/raspilapse-daily-video.service.insystemd/raspilapse-daily-video.timersystemd/raspilapse-daily-video.timer.insystemd/raspilapse-upload-retry.servicesystemd/raspilapse-upload-retry.service.insystemd/raspilapse-upload-retry.timersystemd/raspilapse-upload-retry.timer.insystemd/raspilapse-watchdog.service.insystemd/raspilapse-watchdog.timer.insystemd/raspilapse.servicesystemd/raspilapse.service.intests/conftest.pytests/test_analyze_timelapse.pytests/test_apply_overlay.pytests/test_auto_timelapse.pytests/test_bootstrap_ml.pytests/test_bootstrap_ml_v2.pytests/test_brightness_zones.pytests/test_capture_image.pytests/test_colors.pytests/test_config_example.pytests/test_create_keogram.pytests/test_daily_timelapse.pytests/test_database.pytests/test_db_graphs.pytests/test_db_stats.pytests/test_graph_solar_patterns.pytests/test_logging_config.pytests/test_make_timelapse.pytests/test_make_timelapse_daily.pytests/test_ml_exposure.pytests/test_ml_exposure_v2.pytests/test_overcast_brightness.pytests/test_overlay.pytests/test_overlay_simplified.pytests/test_status.pytests/test_system_monitor.pytests/test_upload_service.pytests/test_version.pytests/test_weather.py
💤 Files with no reviewable changes (32)
- docs/MAINTAINER.md
- docs/CONTRIBUTING.md
- docs/MONITORING_SETUP.md
- docs/NEXT_SESSION_CONTEXT.md
- scripts/uninstall.sh
- scripts/install_cleanup.sh
- systemd/raspilapse-cleanup.service
- systemd/raspilapse-daily-video.service
- docs/DAILY_VIDEO.md
- pytest.ini
- Update.md
- scripts/test.sh
- ml_state/ml_state.json
- scripts/install_daily_video.sh
- docs/SETUP_COMPLETE.md
- docs/ADAPTIVE_TIMELAPSE_FLOW.md
- Howto.md
- docs/SLITSCAN_UPGRADE.md
- src/bootstrap_ml.py
- docs/TRANSITION_SMOOTHING.md
- ML.md
- scripts/check_disk_space.sh
- systemd/raspilapse-cleanup.timer
- src/analyze_timelapse.py
- docs/CLAUDE.md
- scripts/uninstall_daily_video.sh
- docs/BLACK_FORMATTING_GUIDE.md
- src/ml_exposure.py
- src/bootstrap_ml_v2.py
- docs/MAINTENANCE.md
- scripts/check_capture_rate.sh
- src/ml_exposure_v2.py
| try: | ||
| from src.logging_config import get_logger | ||
| except ImportError: | ||
| from logging_config import get_logger | ||
|
|
||
| logger = get_logger("auto_timelapse") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Typecheck fails on the fallback import.
mypy reports Name "get_logger" already defined at Line 17, and this is currently blocking the Tests / typecheck job.
🔧 Suggested fix
try:
from src.logging_config import get_logger
except ImportError:
- from logging_config import get_logger
+ from logging_config import get_logger # type: ignore[no-redef]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| from src.logging_config import get_logger | |
| except ImportError: | |
| from logging_config import get_logger | |
| logger = get_logger("auto_timelapse") | |
| try: | |
| from src.logging_config import get_logger | |
| except ImportError: | |
| from logging_config import get_logger # type: ignore[no-redef] | |
| logger = get_logger("auto_timelapse") |
🧰 Tools
🪛 GitHub Actions: Tests / 2_typecheck.txt
[error] 17-17: mypy: Name "get_logger" already defined (possibly by an import) [no-redef]
🪛 GitHub Actions: Tests / typecheck
[error] 17-17: mypy: Name "get_logger" already defined (possibly by an import) [no-redef]
🤖 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 `@src/exposure.py` around lines 14 - 19, Update the fallback import around
get_logger in src/exposure.py so mypy sees a single definition rather than two
conditional import declarations; preserve support for both import paths while
using an import structure compatible with static type checking.
Source: Pipeline failures
| try: | ||
| from src.logging_config import get_logger | ||
| except ImportError: | ||
| from logging_config import get_logger | ||
|
|
||
| logger = get_logger("overlay") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
mypy no-redef on the src. / flat fallback import pattern breaks CI in both new modules. The try: from src.logging_config import get_logger / except ImportError: from logging_config import get_logger idiom makes mypy see two bindings of the same name; the typecheck job fails at both sites.
src/overlay_draw.py#L11-L16: add# type: ignore[no-redef]to the fallback import on Line 14.src/overlay_sources.py#L17-L22: add the same# type: ignore[no-redef]to the fallback import on Line 20.
🧰 Tools
🪛 GitHub Actions: Tests / 2_typecheck.txt
[error] 14-14: mypy: Name "get_logger" already defined (possibly by an import) [no-redef]
🪛 GitHub Actions: Tests / typecheck
[error] 14-14: mypy: Name "get_logger" already defined (possibly by an import) [no-redef]
📍 Affects 2 files
src/overlay_draw.py#L11-L16(this comment)src/overlay_sources.py#L17-L22
🤖 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 `@src/overlay_draw.py` around lines 11 - 16, Add the mypy no-redef suppression
to the fallback get_logger import in src/overlay_draw.py lines 11-16 and
src/overlay_sources.py lines 17-22. Apply the same inline type-ignore annotation
to each flat logging_config import while leaving the import behavior unchanged.
Source: Pipeline failures
| class TestNegativeCaching: | ||
| """Failures back off instead of hammering the endpoint every call.""" | ||
|
|
||
| @patch("urllib.request.urlopen") | ||
| def test_failure_suppresses_immediate_refetch(self, mock_urlopen, weather_config): | ||
| mock_urlopen.side_effect = urllib.error.URLError("name resolution failed") | ||
|
|
||
| weather = WeatherData(weather_config) | ||
| for _ in range(5): | ||
| assert weather.get_weather_data() is None | ||
|
|
||
| # One attempt, then backoff. Previously every call hit the network, | ||
| # which is how one log file collected 72,536 identical DNS errors. | ||
| assert mock_urlopen.call_count == 1 | ||
|
|
||
| @patch("urllib.request.urlopen") | ||
| def test_backoff_grows_and_is_capped(self, mock_urlopen, weather_config): | ||
| weather_config["weather"]["cache_duration"] = 100 | ||
| weather_config["weather"]["max_backoff_seconds"] = 250 | ||
| mock_urlopen.side_effect = urllib.error.URLError("down") | ||
|
|
||
| weather = WeatherData(weather_config) | ||
| delays = [] | ||
| for _ in range(4): | ||
| before = datetime.now() | ||
| weather._fetch_weather_data() | ||
| delays.append((weather._entry.next_attempt_at - before).total_seconds()) | ||
|
|
||
| assert round(delays[0]) == 100 | ||
| assert round(delays[1]) == 200 | ||
| assert round(delays[2]) == 250 # capped | ||
| assert round(delays[3]) == 250 | ||
|
|
||
| @patch("urllib.request.urlopen") | ||
| def test_repeated_identical_errors_log_once(self, mock_urlopen, weather_config, weather_logs): | ||
| mock_urlopen.side_effect = urllib.error.URLError("same every time") | ||
| weather = WeatherData(weather_config) | ||
|
|
||
| for _ in range(5): | ||
| weather._fetch_weather_data() | ||
|
|
||
| assert len(weather_logs.records) == 1 | ||
| assert weather._entry.suppressed == 4 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Weather cache state is now process-wide with no test reset, making the new tests order-dependent. _CACHE is keyed by endpoint and every test reuses the single weather_config endpoint, so backoff and error-suppression state leaks between tests.
tests/test_weather.py#L889-L931: add an autouse fixture callingweather.reset_cache()before and after each test.src/weather.py#L43-L48: confirmreset_cache()is actually wired intotests/conftest.py(or the module fixture above); it is currently defined but has no visible caller.
📍 Affects 2 files
tests/test_weather.py#L889-L931(this comment)src/weather.py#L43-L48
🤖 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_weather.py` around lines 889 - 931, The weather cache must be
reset around each test to prevent process-wide state leaking between cases. In
tests/test_weather.py lines 889-931, add an autouse fixture that calls
weather.reset_cache() before and after every test; in src/weather.py lines
43-48, ensure the existing reset_cache() implementation is wired to that fixture
or tests/conftest.py, with no other direct change required there.
CodeRabbit review on #12. Eight findings acted on, three rejected with reasons; verified each against the code rather than applying blind. The real one: _settings_night's gain-reduction branch multiplied self._last_analogue_gain, which seed_from_capture leaves as None when the database row it seeds from has a brightness but no gain. On the first night frame with brightness above 150 and exposure at its floor, that raises TypeError out of get_camera_settings, on the capture path. Falls back to the configured night gain. CachedJsonSource only stamped _cache_time on success, so a missing ships_file re-warned every time the 60-second cache expired -- with the overlay rebuilt twice per capture cycle, exactly the log-flood pattern weather.py was changed to eliminate two commits earlier. Now stamps the attempt and warns once until the file reappears. The watchdog service template still carried [Install], contradicting install.sh's own comment that these services have none "so that enabling them is impossible". Removed. Also: tide levels of exactly 0.0 m (chart datum) rendered as empty strings via a truthiness check; observe_frame's `.get(key, default)` did not cover a key present with an explicit None, which is what a failed measurement writes; the weather backoff collapsed to zero delay if cache_duration was 0; a logger with enabled: false still propagated to root; LoggerConfig.setup_logger registered under one name and formatted the filename from another; usage() printed the `set -euo pipefail` line; db_graphs still had ML in nine chart titles, legends and docstrings after the function itself was renamed; CONTRIBUTING said `make all` runs three targets when it runs four; and neither install doc installed python3-pip, which a minimal Raspberry Pi OS image lacks and step 2 needs. mypy: disable no-redef centrally rather than annotating ~40 sites. The `try: from src.x / except: from x` idiom is deliberate -- the systemd units run the scripts directly -- so flagging it is noise. 120 errors to 70; the rest are pre-existing untyped-dict returns. Rejected, with reasons: - "weather.reset_cache has no caller": it does, from tests/conftest.py, via getattr. The lookup is dynamic, which is presumably why it was missed. - "colour_gains_r/b and colour_temperature need a migration": those columns are in the pre-cleanup schema at lines 216-218. Not new. - "equal p95 thresholds divide by zero": they cannot. The `p95 <= safe` early return covers every p95 that could reach an interpolation with a zero-width band. I wrote the guard, then tested the unguarded function across all 3.5 million threshold/p95 combinations -- zero exceptions -- and reverted it. The test now asserts the invariant instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
|
Worked through the CodeRabbit review in be48d7d. Eight findings fixed, three rejected — recording the rejections here so they don't read as ignored. Fixed, in rough order of severity The Critical one was real:
The watchdog template still carried Plus: tide levels of exactly 0.0 m rendering as empty strings; Rejected
Not acted on
Markdown fence languages: markdownlint isn't in CI, so those are cosmetic. mypy: On the coverage report: project coverage is unchanged, 75% before and after (measured against |
Found with vulture plus an AST pass for config keys read into attributes
that are never loaded.
Two config keys were still documented and still parsed but had stopped
doing anything: brightness_tolerance and brightness_feedback_strength
fed _brightness_tolerance and _brightness_feedback_strength, which died
with _apply_brightness_feedback and which nothing has read since. A user
tuning either of them would have seen no effect at all.
tests/test_config_example.py was added specifically to stop this and did
not catch it. It asserts the key's name appears as a quoted string in
src/, and `transition_config.get("brightness_tolerance", 40)` satisfies
that -- it proves a key is read, not that reading it matters. Added a
second test that walks the AST for `self.x = <cfg>.get("key")` where x
is never loaded. Verified by reintroducing the exact prior state: the
new test fails, the old one still passes.
Also removed:
- capture_image._apply_controls and _save_metadata, zero callers. The
latter's own docstring called it a legacy method; the live path is
_save_metadata_from_dict.
- auto_timelapse._frame_interval, assigned from config and never read;
the loop reads the interval directly.
- ExposureController.last_exposure_time and last_decision properties,
no callers -- the diagnostics path goes through diagnostics().
- db_graphs.add_mode_shading's y_min/y_max parameters. It uses axvspan,
which spans the full axis height, so both were ignored while five call
sites computed them first -- one doing real work to find the smallest
positive lux purely to throw it away.
Behaviour unchanged: gain 1.123 and brightness holding 118 against a
target of 120 across the restart.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
The overlay split extracted seven helpers into overlay_draw.py and only wired in four. Three had no callers at all, which is worse than not extracting them: the duplication they were meant to remove was still there, plus a second copy nobody ran. draw_gradient_bar duplicated ImageOverlay._draw_gradient_bar, which is live. The two differed in arithmetic -- the module version drew 40 bands, the method drew one rectangle per row -- so I matched the module function to the method's fade before making the method delegate. Same pixels. draw_divider replaces the inline vertical rule beside the aurora section, the one site that pattern has. draw_right_aligned is deleted rather than wired. Its two candidate call sites subtract tide_section_width and aurora_section_width before positioning, so it would have needed a signature contorted enough to be worth less than the inline arithmetic it replaced. vulture now reports no dead code across src/ and scripts/ beyond known false positives (sqlite row_factory, context-manager exc_* parameters, matplotlib's required formatter arg). Verified by cropping the rendered top bar before and after: gradient, divider, spacing and all four text slots identical, only the live values differing across the three minutes between frames. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
CONTRIBUTING claimed "one module per src/ module". It wasn't true: exposure.py was the largest and newest module at 1,432 lines with no test_exposure.py, and config_utils, overlay_draw, overlay_sources and retry_uploads had no test file either. Relocation, no new logic: 18 exposure classes (77 tests) move out of test_auto_timelapse.py, and test_brightness_zones.py and test_overcast_brightness.py fold in whole. test_auto_timelapse.py drops from 1,990 to 984 lines and now covers only what AdaptiveTimelapse itself does -- lifecycle, polar awareness, capture flow, main(), and the wiring to the controller. New coverage for the four bare modules, aimed at the failure paths rather than the happy ones: - test_overlay_draw.py: what each helper does when the font cannot be measured, since degrading beats not drawing an overlay. 52% -> 94%. - test_config_utils.py: the parameters that exist because the merged helpers disagreed -- parse_time_arg's per-caller default (1h vs 24h) and format_duration's precision (.1f vs .0f). 91% -> 98%. - test_overlay_sources.py: the CachedJsonSource contract, including serving stale data over blanking the overlay, and warning once rather than every time an absent file's cache expires. - test_retry_uploads.py: the three behaviours this branch introduced -- --purge-missing, --status flagging gone sources, and exiting 0 when uploads are simply not configured. 0% -> 90%. 821 -> 891 tests, project coverage 75.4% -> 76.7%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
Both had drifted past the point where correcting them was better than removing them, and both duplicated README and TROUBLESHOOTING while being the less accurate copy wherever they disagreed. SERVICE.md said "3 systemd services" (there are four, plus two optional watchdog units), gave daily video at 00:04 and cleanup at 01:00 when the timers say 05:00 and 02:00, and told you to `sudo cp raspilapse.service` -- a file that stopped existing when the units became .in templates. It also spent twelve lines teaching `journalctl -u raspilapse` as the way to read the log, which under `console: auto` shows nothing from the application. USAGE.md pointed at the deleted install_daily_video.sh, still had an "Analyze from Metadata Files ... exports to Excel" section describing the deleted analyze_timelapse.py, listed 6 of the 9 graphs db_graphs.py writes, and contained a YAML example with `adaptive_timelapse:` twice at top level -- paste it and the second mapping silently discards `interval`. Kept, by moving: nginx image serving and the capture-rate check into TROUBLESHOOTING, along with the storage arithmetic (6-8 GB/day at 4K and 30s, settling near 50 GB under the 7-day image retention); unit management commands into README, covering all four units rather than three. This removes three cross-document contradictions rather than leaving them to be kept in sync in two more places: where the log lives, when the timers fire, and how the units get installed. README now says outright that `systemctl list-timers` is the authority on the schedule, since four files restating it is how they came to disagree. Also fixed the two remaining broken links -- CHANGELOG to a deleted V1_RELEASE_NOTES.md, and TIMELAPSE_VIDEO to config/config.yml, which from docs/ resolved to docs/config/config.yml. Docs: 11 files -> 10. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
…er existed Both described `overlay.content` as taking `main`, `camera_settings` and `debug` sub-sections, each with `enabled:` and `lines:`. src/overlay.py reads exactly four keys and has only ever read four: line_1_left, line_1_right, line_2_left, line_2_right. The consequence was not cosmetic. Every YAML example in WEATHER.md, and three of the four in OVERLAY.md, produce a blank overlay if pasted -- the keys are silently ignored, so there is no error to follow. Only the last example in OVERLAY.md worked, and it was presented as an alternative style rather than the only one. Both are rewritten around the real four-slot model, and now document placeholders by checking them against what _prepare_overlay_data emits: the camera and capture set, the system set, the weather set, and the tide/ships/aurora widgets from overlay_sources.py. Previously undocumented though present in config.example.yml: overlay.margin, the whole overlay.datetime block, and layout.bottom_padding_multiplier. Dropped section_spacing, removed in the last pass. Leads with top-bar, which is what the example ships; the corner presets are described as the alternative they are. WEATHER.md additionally had the failure behaviour backwards in three places. It said a failed fetch shows "-" to avoid displaying outdated data. The code does the opposite, deliberately: it serves the last good reading, because a value blinking to "-" and back every few minutes is far more distracting in a timelapse than one a few minutes stale. "-" appears only when nothing has ever been fetched. It also predated the exponential backoff and the shared per-endpoint cache -- and the per-instance mental model it described was itself the bug that let one outage write 72,536 identical log lines. Verified after: every config key named in any doc exists in config.example.yml, every placeholder documented is one the code emits, and no doc names a deleted script. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
docs/TIMELAPSE_VIDEO.md documented `video.filename_pattern` as controlling the output name. Nothing reads that key -- make_timelapse.py builds the name itself -- and the same file described the real format correctly 200 lines further down. CRF appeared as 20, 23 and 25 in one document; the example ships 25. `--start`/`--end` defaults were given as 05:00 when argparse says 00:00 and "current time". `-hd` and `-hw` were undocumented. The cron-at-04:00 section is gone: the timer already does this, at 05:00, and that section was one of the four places claiming different times. config/README.md said config.yml is "created automatically during installation". install.sh does the opposite -- it warns that the file is missing and tells you to copy it. It also listed `*.yml.old` as gitignored, which it is not, while its own sharing recipe suggested creating `config/my-setup.yml`, which is also not ignored. The recipe now copies outside the repo and says which fields to strip. docs/EXPOSURE.md claimed every frame's metadata carries a diagnostics block. It is gated on adaptive_timelapse.diagnostics.enabled, which ships false because the brightness analysis costs 100-300 ms per capture. docs/TROUBLESHOOTING.md put the log ceiling at ~100 MB. It is 5 MB x 3 files x 11 loggers, so ~165 MB worst case. CHANGELOG.md had a stale line count, and called highlight protection "new" while its own 1.3.0 entry describes a p95 feature. Added the distinction: the 1.3.0 one scaled exposure, lived in the deleted ML path and never reached the camera; this one scales the target, which is what makes its equilibrium independent of brightness_damping. Verified across every markdown file: all internal links resolve, every path named exists (bar CHANGELOG's historical references and two deliberate placeholders), every command runs, every documented flag is in that script's --help, and every config key named is in config.example.yml. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/overlay_sources.py (1)
287-297: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftNormalize nullable tide measurements before arithmetic.
point.get("level_cm", 0)returnsNonewhen JSON contains"level_cm": null; it only defaults missing keys. Interpolation can then fail at line 296, and the same pattern affects trend calculation, extreme detection, and next-event levels. TreatNonemeasurements as invalid/ missing and skip or fall back consistently before performing arithmetic.🤖 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 `@src/overlay_sources.py` around lines 287 - 297, Normalize nullable level_cm values to a numeric fallback or invalid state before arithmetic, rather than relying on dict.get defaults. Apply this consistently to the interpolation logic near prev_level/next_level and the corresponding trend calculation, extreme detection, and next-event level handling, skipping invalid measurements or using the established fallback behavior before calculations.
🧹 Nitpick comments (9)
tests/test_config_example.py (1)
95-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftPreserve configuration provenance and full key paths.
The detector treats every
.get("key")as a config read, then compares only leaf names. An unrelated mapping or a duplicate key such asenabledcan therefore fail this test. Restrict analysis to config-derived mappings and retain the complete YAML path.Also applies to: 138-141
🤖 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 95 - 108, Update the detector’s `.get` analysis to follow only mappings proven to originate from configuration data, rather than treating every mapping access as a config read. Preserve each setting’s complete YAML path when recording and comparing reads, and use those full paths in `loaded`/`dead` so duplicate leaf names such as “enabled” do not collide. Apply the same provenance and path handling to the related logic around the additional reported lines.tests/test_exposure.py (5)
11-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused imports rather than blanket-
noqathem.
re,MagicMockandpatchhave no usages in this file; the comment "used by relocated tests" no longer holds after the split.♻️ Proposed cleanup
import os -import re # noqa: F401 -- used by relocated tests import sys import tempfile -from unittest.mock import MagicMock, patch # noqa: F401🤖 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_exposure.py` around lines 11 - 15, Remove the unused re, MagicMock, and patch imports from tests/test_exposure.py, including their blanket noqa annotations, while preserving the remaining imports.
1181-1193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an assertion so the test can fail meaningfully. Right now only an exception fails it; asserting the resulting state (e.g.
_last_brightness/_last_p95stayNone) pins the intended contract.🤖 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_exposure.py` around lines 1181 - 1193, Add assertions to test_observe_frame_tolerates_none_measurements verifying that observe_frame leaves the resulting brightness state, including _last_brightness and _last_p95, as None when all measurements are None.
749-766: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing assertion after the sustained-reading loop. The final loop exercises the transition but never checks it happened; the test would pass even if hysteresis never released.
♻️ Proposed fix
for _ in range(3): mode = timelapse.exposure.apply_hysteresis(LightMode.DAY) + assert mode == LightMode.DAY🤖 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_exposure.py` around lines 749 - 766, Add an assertion after the sustained LightMode.DAY loop in test_hysteresis_prevents_mode_flapping, verifying the final mode has transitioned to LightMode.DAY while preserving the existing initial NIGHT assertion and loop.
1471-1528: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueModule-level
timelapsefixture shadows the class-scoped ones.TestHybridModeDetection,TestNightModeGainReductionandTestEnteringNightThrottleeach define their owntimelapsefixture, so the module fixture only serves the classes below it. A distinct name (e.g.brightness_target_timelapse) would remove the ambiguity.🤖 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_exposure.py` around lines 1471 - 1528, The module-level timelapse fixture conflicts with class-scoped fixtures of the same name. Rename this fixture to brightness_target_timelapse and update all tests that use this module-level fixture, leaving the class-scoped timelapse fixtures unchanged.
710-730: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test asserts nothing.
test_metadatais never passed anywhere andassert timelapse is not Nonecannot fail. Either drive the lux path with the metadata or delete the test.🤖 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_exposure.py` around lines 710 - 730, Update test_lux_calculation_with_bright_spot to pass test_metadata through the actual lux-calculation or shot-processing path and assert the resulting behavior against the expected bright-spot handling; remove the test_metadata fixture and test if no callable path can exercise that behavior. Replace the non-failing timelapse existence assertion with a meaningful outcome assertion.tests/test_overlay_draw.py (1)
58-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTautological assertion.
text_height(draw, None) == text_height(draw, None)compares two identical calls, so it cannot demonstrate reference-based sizing. Comparing differentreferencestrings (e.g."Ayg"vs the default) would actually pin the contract.🤖 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_draw.py` around lines 58 - 60, Replace the tautological assertion in test_is_reference_based_not_content_based with a comparison of text_height using distinct reference strings, such as "Ayg" and the default reference, to verify glyph content does not affect the resulting height.tests/test_overlay_sources.py (1)
91-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind to the module's actual logger and use
getMessage(). The hardcoded"overlay"name silently decouples this test fromsrc/overlay_sources.pyif the logger is ever renamed (the assertion would then read zero records and fail confusingly).record.messagealso only exists because the handler formatted the record;getMessage()is unconditional.♻️ Proposed hardening
- import logging + import src.overlay_sources as overlay_sources src = _Source(_cfg(tmp_path / "never.json")) - logger = logging.getLogger("overlay") + logger = overlay_sources.logger logger.addHandler(caplog.handler) @@ - assert len([r for r in caplog.records if "not found" in r.message]) == 1 + assert len([r for r in caplog.records if "not found" in r.getMessage()]) == 1🤖 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_sources.py` around lines 91 - 107, Update test_a_permanently_missing_file_warns_only_once to attach caplog.handler to the logger used by src/overlay_sources.py, deriving it from that module rather than hardcoding "overlay". Change the assertion to inspect each matching record via getMessage() instead of record.message, while preserving the existing single-warning expectation and handler cleanup.tests/test_retry_uploads.py (1)
45-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestore cwd around
_run()main()changes the process working directory, andtests/conftest.pydoesn’t reset it. Wrapmain()in a save/restore here or add an autouse fixture so later tests don’t inherit the changed directory.🤖 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_retry_uploads.py` around lines 45 - 47, Update the test helper _run to save the current working directory before calling main() and restore it afterward with guaranteed cleanup, while preserving the existing sys.argv patch and return behavior so later tests are unaffected.
🤖 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/TIMELAPSE_VIDEO.md`:
- Line 185: Update the CRF references around the documented default and command
example so they consistently use CRF 25, or explicitly identify CRF 20 as a
non-default example. Keep the codec settings documentation aligned across all
referenced sections.
In `@src/overlay_draw.py`:
- Around line 94-96: Update the gradient loop in the overlay drawing function to
use inclusive coordinates for exactly one scanline, drawing each row from y
through y with the right edge at img_width - 1. Add a regression test verifying
the row at bar_height remains transparent while rows within the bar are still
rendered.
In `@src/overlay_sources.py`:
- Around line 73-82: Update the cache-read logic around _cache_time so the retry
TTL is enforced independently of whether _cache is None. During the backoff
window, return the existing stale value, including None, before attempting
another filesystem read; preserve the existing warning behavior and refresh
_cache_time when a read fails.
In `@tests/test_config_utils.py`:
- Around line 90-93: Update test_relative_falls_back_to_the_project_root to
change into tmp_path before calling resolve_config_path, ensuring the
project-root fallback path is exercised. Replace the broad absolute-path and
startswith assertions with an exact equality check against PROJECT_ROOT /
"config/config.yml".
---
Outside diff comments:
In `@src/overlay_sources.py`:
- Around line 287-297: Normalize nullable level_cm values to a numeric fallback
or invalid state before arithmetic, rather than relying on dict.get defaults.
Apply this consistently to the interpolation logic near prev_level/next_level
and the corresponding trend calculation, extreme detection, and next-event level
handling, skipping invalid measurements or using the established fallback
behavior before calculations.
---
Nitpick comments:
In `@tests/test_config_example.py`:
- Around line 95-108: Update the detector’s `.get` analysis to follow only
mappings proven to originate from configuration data, rather than treating every
mapping access as a config read. Preserve each setting’s complete YAML path when
recording and comparing reads, and use those full paths in `loaded`/`dead` so
duplicate leaf names such as “enabled” do not collide. Apply the same provenance
and path handling to the related logic around the additional reported lines.
In `@tests/test_exposure.py`:
- Around line 11-15: Remove the unused re, MagicMock, and patch imports from
tests/test_exposure.py, including their blanket noqa annotations, while
preserving the remaining imports.
- Around line 1181-1193: Add assertions to
test_observe_frame_tolerates_none_measurements verifying that observe_frame
leaves the resulting brightness state, including _last_brightness and _last_p95,
as None when all measurements are None.
- Around line 749-766: Add an assertion after the sustained LightMode.DAY loop
in test_hysteresis_prevents_mode_flapping, verifying the final mode has
transitioned to LightMode.DAY while preserving the existing initial NIGHT
assertion and loop.
- Around line 1471-1528: The module-level timelapse fixture conflicts with
class-scoped fixtures of the same name. Rename this fixture to
brightness_target_timelapse and update all tests that use this module-level
fixture, leaving the class-scoped timelapse fixtures unchanged.
- Around line 710-730: Update test_lux_calculation_with_bright_spot to pass
test_metadata through the actual lux-calculation or shot-processing path and
assert the resulting behavior against the expected bright-spot handling; remove
the test_metadata fixture and test if no callable path can exercise that
behavior. Replace the non-failing timelapse existence assertion with a
meaningful outcome assertion.
In `@tests/test_overlay_draw.py`:
- Around line 58-60: Replace the tautological assertion in
test_is_reference_based_not_content_based with a comparison of text_height using
distinct reference strings, such as "Ayg" and the default reference, to verify
glyph content does not affect the resulting height.
In `@tests/test_overlay_sources.py`:
- Around line 91-107: Update test_a_permanently_missing_file_warns_only_once to
attach caplog.handler to the logger used by src/overlay_sources.py, deriving it
from that module rather than hardcoding "overlay". Change the assertion to
inspect each matching record via getMessage() instead of record.message, while
preserving the existing single-warning expectation and handler cleanup.
In `@tests/test_retry_uploads.py`:
- Around line 45-47: Update the test helper _run to save the current working
directory before calling main() and restore it afterward with guaranteed
cleanup, while preserving the existing sys.argv patch and return behavior so
later tests are unaffected.
🪄 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: 18d910b5-2b99-4425-a7e7-ce98d3b338dc
📒 Files selected for processing (36)
.pre-commit-config.yamlCHANGELOG.mdCONTRIBUTING.mdREADME.mdconfig/README.mdconfig/config.example.ymldocs/EXPOSURE.mddocs/INSTALL.mddocs/OVERLAY.mddocs/SERVICE.mddocs/TIMELAPSE_VIDEO.mddocs/TROUBLESHOOTING.mddocs/USAGE.mddocs/WEATHER.mdpyproject.tomlrequirements-dev.txtscripts/db_graphs.pyscripts/install.shsrc/auto_timelapse.pysrc/capture_image.pysrc/exposure.pysrc/logging_config.pysrc/overlay.pysrc/overlay_draw.pysrc/overlay_sources.pysrc/weather.pysystemd/raspilapse-watchdog.service.intests/test_auto_timelapse.pytests/test_brightness_zones.pytests/test_config_example.pytests/test_config_utils.pytests/test_exposure.pytests/test_overcast_brightness.pytests/test_overlay_draw.pytests/test_overlay_sources.pytests/test_retry_uploads.py
💤 Files with no reviewable changes (6)
- docs/USAGE.md
- docs/SERVICE.md
- src/capture_image.py
- config/config.example.yml
- tests/test_auto_timelapse.py
- src/auto_timelapse.py
🚧 Files skipped from review as they are similar to previous changes (15)
- CONTRIBUTING.md
- .pre-commit-config.yaml
- requirements-dev.txt
- docs/EXPOSURE.md
- docs/INSTALL.md
- docs/TROUBLESHOOTING.md
- CHANGELOG.md
- pyproject.toml
- scripts/install.sh
- README.md
- src/logging_config.py
- src/weather.py
- scripts/db_graphs.py
- src/overlay.py
- src/exposure.py
The overlay bar was darker than its own config said. Each gradient row was
drawn as `[0, y, w, y + 1]` -- both ends inclusive in PIL, so every row was
painted twice and its alpha compounded. `background.color` alpha 70 rendered
at roughly 124. The bar also ran one scanline past `bar_height`. Fixing it
lightens the bar at any given setting, so config.yml here goes 70 -> 124 to
keep this camera's frames looking as they do today; measured mean difference
across the bar is 0.63/255 against a live-value noise floor of 0.11.
A tide point carrying an explicit `"level_cm": null` reached the interpolation
as None and raised TypeError -- `.get("level_cm", 0)` only defaults a *missing*
key. One bad forecast entry took down the whole overlay. Routed the eighteen
call sites through a helper that treats null and absent alike.
The retry interval for a missing ships/tide/aurora file was gated on
`_cache is not None`, which made it dead in exactly the case it was written
for: a file that has never loaded. The stamp two lines below it, and the
comment explaining that stamp, were both inert. Every render paid a stat().
Each of the three is covered by a test verified to fail against the old code.
Five existing tests could not fail: a hysteresis loop with no assertion after
it, a `text_height` self-comparison, a config-path fallback that passed from
either resolution because the repo was cwd, an observe_frame call that only
checked for an exception, and one that built a metadata dict, passed it
nowhere and asserted `timelapse is not None`. Deleted the last -- the lux it
meant to exercise needs a camera, and its neighbour already covers the claim.
Also: `_attributes_never_loaded` collected into a dict keyed by leaf name, so
a second dead `enabled` would evict the first; module-level `timelapse`
fixture renamed out of the way of three class-scoped ones; CRF documented as
25 in two places that still said 20.
Found by CodeRabbit on PR #12.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
determine_mode(lux, self._sun_elevation, self._is_polar_day(lux))
└─ read first ─┘ └─ this is what sets it ─┘
Python evaluates arguments left to right, so the attribute was read before
the call that populates it. On frame 0 it is still None from __init__, and
determine_mode's polar-day branch formats it with `:.1f`. At 68°N that branch
runs all summer, so every single restart raised
TypeError: unsupported format string passed to NoneType.__format__
out of a *log line*, and the except above it swallowed the whole test-shot
block with it: mode selection, hysteresis, transition WB seeding and the
camera settings for that frame. The frame fell back to `last_mode or DAY`.
Ten occurrences in today's log alone, one per restart.
Fixed at both levels: the call that fills the attribute now runs on its own
line first, and the polar log tolerates an absent elevation rather than
trading a mode decision for a formatted number.
The `except` that hid this logged `{e}` with no traceback, across a block
spanning lux, mode, hysteresis, seeding and settings -- the message alone
could not say which of them failed. It now logs exc_info; that is how this
was found after the message had been appearing unexplained all day.
Three tests, each verified against the old code. The ordering one has to be
static: the inline form type-checks, imports and passes all 896 other tests.
Only the shape of the source shows it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
1 participant
A cleanup pass over the whole project. Capture behaviour is unchanged apart
from highlight protection, which is new and can be turned off in config.
Verified continuously on the camera at Kringelen: every stage was deployed and
watched before the next one started.
Three things were broken before this branch
None of them were visible from the logs.
The daily-video service could not start.
upload_service.pyimportedrequests_toolbeltunguarded, and/usr/bin/python3-- which every systemdunit uses -- did not have it. Guarded, with a
requests.postfallback.172 uploads had been stranded since January. Every row pointed at a video
deleted months earlier, and no installer had ever installed the timer meant to
drain them. Installing that timer as-is would have produced 172 doomed attempts
every 30 minutes, forever. Rows whose source is gone are now cancelled;
failedis terminal;--purge-missingclears a backlog.The ML exposure system had not run since January.
direct_brightness_control: truemade_init_ml_predictor()return early. Everything behind it wasunreachable -- except the metadata diagnostics, which re-ran the entire exposure
calculation to fill in a JSON field. That path produced 742 of 777 lines in the
live log, from a calculation whose result never reached the camera.
Also: the installed daily-video timer had drifted from the repo (
Requires=,two
OnCalendar=lines,Persistent=true-- all removed months ago and neverredeployed), which is why it fired at boot and failed.
Numbers
logs/auto_timelapse.pyoverlay.pyThe test count drops because 167 of them tested deleted code. 18 had never
run at all -- four classes were shadowed by a later class of the same name, so
Python rebound the name and the first definition's tests were silently skipped.
The one behaviour change
Highlight protection (
adaptive_timelapse.highlight_protection) lowers thebrightness target when the top of the histogram nears clipping, so bright skies
keep detail instead of blowing out.
It scales the target, not the controller's output. Both settle -- I simulated
them -- but scaling the target leaves the loop's own fixed point intact, so the
equilibrium depends only on the highlight settings: mean brightness lands at
118.0 for every damping value from 0.3 to 1.0. Scaling the output instead ties
the amount of protection to
brightness_damping, drifting 116.4 to 118.0 acrossthe same range.
Off at night by default: across 117k night frames here the mean is already 90
against a target of 120 while 11% exceed p95 200 -- streetlamps and the moon, not
blown scenes.
enabled: falsereverts with no code change.A bug I introduced, and how it was caught
Extracting
exposure.pyleftrun()writingself._last_brightnessonAdaptiveTimelapseafter that state had moved toExposureController. Pythondoes not raise on that -- it creates a dead attribute -- so the controller kept
its startup seed and drove exposure from a measurement that kept getting older.
All 792 tests passed. What showed it was the camera: brightness climbing
155 -> 194 over four minutes while the loop lengthened exposure, which is the
wrong sign for a loop meant to hold 120.
Fixed in 4e4aa6a. The obvious regression test does not catch it -- I checked,
it passes with the bug reintroduced -- because the broken path is in
run().The one that does is a static check that
AdaptiveTimelapsenever assigns to aname
ExposureControllerowns.Removed
ml_exposure.py,ml_exposure_v2.py,bootstrap_ml.py,bootstrap_ml_v2.py,ML.md,ml_state/, four testmodules, and the unreachable branches behind them
analyze_timelapse.py(1,967 lines). It read per-frame metadata JSON thatcleanup deletes after 7 days, so it could never look back further than a week,
while
db_graphs.pyreads months from the database. Its one non-overlappingchart, white balance, is ported
manuals/*.pdf-- 45 MB nothing referenceddocs/MAINTAINER.md. Already regenerated. Itremains in history at 019b354; the old value no longer authorizes anything
ml_state/ml_state.json, which shipped one camera's learned model to everyonewho cloned
test.sh, two orphaned check scripts, twelve docsAdded
scripts/install.shas the single entry point:--only,--check,--dry-run,--uninstall,--with-watchdog. It renderssystemd/*.intemplates rather than copying units that hardcode
piand/home/pidatabase.retention_daysandpython3 src/database.py --prune|--vacuum|--stats.Defaults to 0, keep everything
src/exposure.py,src/capture_cyclehelpers,src/config_utils.py,src/overlay_sources.py,src/overlay_draw.pytests/test_config_example.py, which fails when the example config and thecode drift apart in either direction. Verified against injected drift of both
kinds
Other fixes
logs/and once in the journal.logging.consoleis now tri-state;autoskips the console handler undersystemd. journald capped at 200 MB
-c/--config-- seven modules callget_logger()atimport time, before argparse runs
instance was rebuilt twice per capture cycle, so it never applied. No backoff
either: one outage produced 72,536 identical error lines. Also
data.get("data", {})returningNoneon"data": null, worth 2,204 morebrightness_p25/p75were NULL on every row ever written -- the produceremitted p10/p90
database pinned at v3 with the v4 index missing
three fewer B-tree writes per capture)
mismatch was the cause of the recurring CI formatting failures
--exit-zeroandcontinue-on-errorDocs
20 files to 11. The documented install path was broken: README installed 3 of
the 8 packages the code needs, INSTALL.md installed 6 and missed both
requestsand
requests-toolbelt, and four files pointed atinstall_service.sh, renamedin v1.0.0.
Deleted the AI session diary, the trip report about one specific Pi, two
single-machine runbooks, and a monitoring doc whose "quick reference" installed
a cron entry duplicating the systemd timer. Nothing states a schedule in prose
any more -- four files claimed 04:00 while the timer said 05:00.
Version was three different numbers at once. All now 1.4.0 from
src/__version__.py, with tests asserting CHANGELOG and CITATION agree.Migrating
git pull ./scripts/install.sh --check ./scripts/install.sh # redeploys the corrected units sudo systemctl restart raspilapseThen in
config/config.yml: setlogging.console: auto, optionally addhighlight_protectionanddatabase.retention_days, and removeadaptive_timelapse.ml_exposureanddirect_brightness_control-- both inert.If uploads were configured, clear any stale queue with
python3 src/retry_uploads.py --purge-missing.Before merging
Two things I could not verify:
night boundary is where exposure changes are largest, and where highlight
protection sees p95 climb fastest
was already configured
pre-cleanup-20260726is the rollback tag.🤖 Generated with Claude Code
https://claude.ai/code/session_01DfmEz3MCL16Rp3tsJ7ejgk
Summary by CodeRabbit