Dynamic range: selectable fusion / sensor HDR / raw / tone-map methods - #18
Merged
Conversation
adaptive_timelapse.hdr set libcamera's HdrMode control, which the Pi 4's imx708 never acts on -- its on-chip HDR is switched through V4L2 before the camera opens. The block was dormant on every camera in the fleet. In its place, adaptive_timelapse.dynamic_range selects one of four opt-in methods (fusion, sensor_hdr, raw, tone_map -- implemented in the following commits). The DynamicRange facade mirrors build_overlay's seam: the daemon builds one instance, asks it for the post-process chain and per-frame capture behaviour, and `method: off` provably reproduces the old pipeline (the chain IS build_overlay's callable). Construction never raises: unknown methods and missing optional dependencies (OpenCV, rawpy -- absent in CI and on lean installs) degrade to `off` with one warning, and label() reports the degraded reality so trial records stay honest. Every sidecar now records dr_method. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011uhRMmJgKzCJad6PNUihq3
📝 WalkthroughWalkthroughDynamic-range capture now supports fusion, sensor HDR, raw development, tone mapping, and DNG sidecars. The daemon integrates these methods into camera setup, capture, post-processing, metadata, and shutdown. Legacy HDR controls are removed. ChangesDynamic range processing
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Daemon
participant DynamicRange
participant ImageCapture
participant PostProcessor
participant Database
Daemon->>DynamicRange: prepare method and camera
DynamicRange->>ImageCapture: capture ordinary frame or bracket
ImageCapture-->>DynamicRange: JPEG, optional DNG, and metadata
DynamicRange->>PostProcessor: develop, upscale, tone-map, and overlay
PostProcessor-->>Daemon: processed frame and dr_method
Daemon->>Database: store capture metadata
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@raspilapse/dynrange/__init__.py`:
- Around line 102-118: Implement dynamic-range method dispatch in
DynamicRange.pre_open() and build_post_process(), invoking the appropriate
camera-setup and post-processing handlers for fusion, sensor_hdr, raw, and
tone_map while retaining the overlay-only behavior for off. In
raspilapse/daemon.py lines 1159-1164, apply self._dr.pre_open(decision.mode)
before camera initialization; in raspilapse/daemon.py lines 715-724, record the
selected method only after its handler succeeds, or record off after fallback.
Update docs/CONFIG-REFERENCE.yml lines 336-344 to list only implemented methods,
and add mocked coverage in tests/test_dynrange_config.py lines 65-72 verifying
each enabled method changes the intended capture or processing path.
- Around line 66-67: Validate the value assigned to block from
adaptive.get("dynamic_range") before calling .get() in the surrounding
configuration flow. If it is not a mapping, log exactly one warning and replace
it with an empty mapping so method resolves to "off"; preserve the existing
mapping and missing-value behavior.
🪄 Autofix
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: e65ca0b0-c36b-4b57-9cd8-919a2a1ba807
📒 Files selected for processing (8)
docs/CONFIG-REFERENCE.ymlraspilapse/camera/capture.pyraspilapse/camera/exposure.pyraspilapse/daemon.pyraspilapse/dynrange/__init__.pytests/replay/extract_sequences.pytests/test_dynrange_config.pytests/test_exposure.py
💤 Files with no reviewable changes (2)
- raspilapse/camera/exposure.py
- tests/test_exposure.py
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The gentlest of the dynamic-range methods: redistribute what the
captured JPEG already holds. CLAHE runs on the L channel only (chroma
untouched, no colour shifts) at fixed gentle parameters, blended into
the original at tone_map.strength -- the one knob. It combines with any
capture method; `method: tone_map` is sugar for off + enabled.
Timelapse frames must not flicker, so the night guard is a smooth fade
(full strength above mean L 45, zero below 35) rather than an on/off
skip that would visibly toggle as a twilight scene hovers around the
line -- lifting a 20-second exposure's shadows amplifies noise into
something worse than the crushed shadows were.
The write is atomic with the overlay's own 0644 dance, and every
failure path leaves the original frame untouched: a failed polish costs
the polish, never the photo. The overlay gains a {dr_method}
placeholder so trial frames are labelled on the image itself; it
defaults to "off" for callers that don't inject it (test shots, older
sidecars).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011uhRMmJgKzCJad6PNUihq3
Each slot captures the commanded base exposure plus up to two brackets (2 EV under for the sky, 2 EV over for the shadows) and saves their Mertens fusion -- per-pixel well-exposedness weights through a pyramid blend, so the result reads as one well-graded photograph, not "HDR". No alignment: the mount is fixed, and scene motion degrades to soft ghosting that playback never shows. The timelapse constraint shapes the maths. The bracket spread is a continuous, monotonic function of the base exposure (log ramp from 0.05s to single_shot_above_s), so consecutive frames cannot jump between looks, and by the time night exposures arrive the plan has already collapsed to the plain single-shot path -- the day-night transition has no seam because no fusion code runs at all. A slot budget guard drops brackets (over first: blown highlights are unrecoverable, dark shadows are merely dark) sooner than overrun the capture grid. The base shot comes first and keeps its metadata and lores metrics, so metering, the ladder, lux and the golden replays keep describing the exposure that was actually decided. Brackets wait for their controls to land by watching the sensor's reported ExposureTime (within 10% -- whole-line quantisation means exact never happens), capped at ten discards; the measured settle cost feeds back into the budget guard. While fusion is active, highlight_protection defaults off: the under bracket buys ~2 stops of highlight headroom against the ~0.5 the protection floor could, without darkening the base frame. An explicit enabled: true still wins. capture_bracketed stays free of cv2 and Pillow -- the fuse function is injected and returns encoded bytes. OpenCV and rawpy join requirements.txt (CI-only, apt on a Pi) so the pixel paths are tested in CI rather than skipped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011uhRMmJgKzCJad6PNUihq3
The sensor merges multiple exposures itself and hands the ISP one pre-merged stream -- zero CPU cost. The trade is resolution: merging happens in a binned mode capped at 2304x1296, so pre_open requests that size (asking the vc4 ISP to upscale a sensor mode is at best undefined) and a post-process stage upscales the saved frame back to the configured size before the overlay is drawn. Everything downstream -- the scale-filter-less daily video, the keogram, status.jpg -- only ever sees one frame size. On the Pi 4 the switch is the V4L2 wide_dynamic_range sensor control, set while the camera is closed; the per-frame reopen makes that free. day_only (default) keeps HDR out of transition and night: merged exposures cannot be long ones, and leaving the mode on would silently cap the ladder's lengthening dusk commands and fight the metering loop -- the same fight that made HDR nights noisy for anyone who tried the mode years ago. The WB reference shot captures at full resolution, so it drops WDR first; daemon shutdown clears the flag, which outlives the process and would otherwise cap the next run whatever its method. A camera without the control (any non-imx708) degrades to off with one warning at construction, and the label reports the reality. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011uhRMmJgKzCJad6PNUihq3
The two sensor_hdr parse tests passed on a camera-equipped Pi and failed in CI: unmocked, DynamicRange's construction probed the real /dev/v4l-subdev* nodes, so the test's outcome depended on the host's hardware. Tests must never touch a real sensor -- least of all on a Pi where the daemon owns it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011uhRMmJgKzCJad6PNUihq3
The raw method develops the sensor's 10-bit DNG (rawpy, with the camera's own embedded WB and colour matrices, no_auto_bright so the develop is deterministic and cannot flicker) instead of keeping the ISP's 8-bit JPEG -- about two extra stops of shadow latitude. The ISP JPEG is captured in the same request and is the built-in fallback: night frames and long exposures skip the develop (12 MP costs real seconds a night slot does not have), any failure keeps the JPEG, and the temporary DNG never survives the frame except by promotion. Developing runs as the first post-process stage -- the only moment after the DNG exists and before the overlay is burned in. The DNG sidecar is independent of the method: every Nth frame's negative is kept beside its JPEG for hand-developing in a desktop raw workflow, pruned oldest-first past max_files so the collection plateaus (~3 GB at the defaults). Ships disabled. Inert under sensor_hdr, whose merged binned mode has no true negative. Schema v7 adds a dr_method column so trial periods compare with a WHERE clause instead of cross-referencing config-change times; old rows stay NULL rather than claiming a fabricated "off". The migration-6 test now asserts the chain reaches SCHEMA_VERSION, not the literal 6 it pinned. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011uhRMmJgKzCJad6PNUihq3
The trial tool the whole feature was built around: one auto-exposure metering pass picks the base settings, then each method captures the identical scene with identical manual settings into one directory, labelled by method, with a timing and brightness summary at the end. It doubles as the measurement harness for the numbers the daemon's budget guards assume -- bracket settle frames, fusion time, develop time -- and refuses to run while raspilapse.service owns the camera, with the exact stop/start commands in the message. Docs polish alongside: the README gains a dynamic-range section, the example config points at the block and the tool, and the CHANGELOG tells the release's story. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011uhRMmJgKzCJad6PNUihq3
CodeRabbit caught the gap: `dynamic_range: fusion` -- a natural typo
for `method: fusion` -- arrived as a string and crashed construction
with AttributeError, which is exactly what the never-raises contract
forbids. The same latent hazard sat in every `or {}` sub-block read
(tone_map, fusion, sensor_hdr, dng_sidecar, camera.resolution, output).
One _mapping() helper now guards them all: a scalar or list where a
mapping belongs logs one warning naming the offending value and is
ignored. A parametrised test pins each shape.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011uhRMmJgKzCJad6PNUihq3
Owner
Author
Hardware validation on sigerfjordcam-2 (Pi 4B + Camera Module 3, night of 2026-08-09)Full suite on-device with real OpenCV/rawpy: 1281 passed (the 11 pixel-path tests that skip in-CI-less-world run here).
Measured numbers vs. the guards' assumptions: bracket settle 7 frames (seed was 8), fusion total ~14 s of a 30 s slot (guard assumed ≲15), raw develop ~4–5 s (estimate was 15 — comfortably conservative). Live daemon, ~10 min per method ( Open risks from the design, all retired: subdev released on The camera stays on 🤖 Generated with Claude Code |
A day of live tuning on two cameras taught things the docs should say: ev_spread is the shadow dial and it is continuous (2.0 near-black foliage, 3.0 dramatic, 2.7 right, on the scene that drove the tuning); TROUBLESHOOTING and EXPOSURE now route "shadows crushed against a bright sky" to dynamic_range instead of leaving underexpose-harder as the only advice; and drtest warns that sequential method shots in fast-changing light are not a comparison -- learned from fog burning off between frames. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011uhRMmJgKzCJad6PNUihq3
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
raspilapse/dynrange/__init__.py (1)
103-165: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNumeric conversions can raise and break the never-raises contract.
_mappingprotects against wrong container shapes only. The scalar conversions below still raise on malformed YAML. Examples:brackets: threeraisesValueErrorat Line 107,interval: "30s"raisesValueErrorat Line 112, andquality: nullraisesTypeErrorat Line 160.DynamicRangeis constructed inAdaptiveTimelapse.__init__(raspilapse/daemon.py Line 157), so the daemon fails to start instead of degrading tooff.Add a numeric coercion helper that warns and returns the default.
🛡️ Proposed fix
+def _number(value, default, name: str, cast=float): + """A numeric config value, or the default with one warning.""" + if value is None: + return default + try: + return cast(value) + except (TypeError, ValueError): + logger.warning(f"{name} must be a number, not {value!r}; using {default}") + return defaultThen use it for each scalar, for example:
- strength = tone_map.get("strength", 0.5) - self._tone_map_strength = min(max(float(strength), 0.0), 1.0) + strength = _number(tone_map.get("strength"), 0.5, "dynamic_range.tone_map.strength") + self._tone_map_strength = min(max(strength, 0.0), 1.0) @@ - self._fusion_brackets = min(max(int(fusion_cfg.get("brackets", 3)), 2), 3) + brackets = _number(fusion_cfg.get("brackets"), 3, "dynamic_range.fusion.brackets", int) + self._fusion_brackets = min(max(brackets, 2), 3)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raspilapse/dynrange/__init__.py` around lines 103 - 165, Introduce a numeric coercion helper near the DynamicRange configuration parsing that catches invalid or null scalar conversions, logs a warning, and returns the supplied default. Replace the direct numeric conversions in the shown initialization—tone-map strength, fusion brackets/EV spread/single-shot threshold, adaptive interval, resolution dimensions, quality, sidecar frequency, and retention—with this helper while preserving each existing default and subsequent clamping behavior so malformed YAML cannot break DynamicRange construction.
🧹 Nitpick comments (8)
tests/test_database.py (1)
966-1000: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
CaptureDatabase.SCHEMA_VERSIONfor the final version assertion.The v6 fixture applies all migrations. Replace the literal
7at Line 997 so later schema changes do not break this test.Proposed fix
- assert version == 7 + assert version == CaptureDatabase.SCHEMA_VERSION🤖 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 966 - 1000, Update the final schema version assertion in test_migration_7_adds_dr_method_to_an_existing_database to compare against CaptureDatabase.SCHEMA_VERSION instead of the literal 7, while leaving the migration and column assertions unchanged.raspilapse/dynrange/__init__.py (1)
360-366: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe chain discards stage results when the overlay is active.
processedaccumulates the stage results. Ifoverlayis notNone, Line 365 returns the overlay result and dropsprocessed. A failed develop or a failed tone map then reports success toImageCapture, so the "Post-processing returned nothing" warning never appears for stage failures. The overlay is enabled in most deployments, so the stage return values are effectively unused.Combine both results.
♻️ Proposed change
if overlay is not None: - return overlay(image_path, metadata, mode, output_path=output_path) + return bool(overlay(image_path, metadata, mode, output_path=output_path)) and processed return processed🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raspilapse/dynrange/__init__.py` around lines 360 - 366, Update chain to combine the accumulated processed result with the overlay result when overlay is configured, while preserving processed as the result when no overlay exists. Ensure any failed stage causes the final chain result to remain false even if the overlay succeeds.raspilapse/dynrange/sidecar.py (1)
37-43: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe prune walks the whole image tree.
rglob("*.dng")enumerates every entry underoutput_directory, not only the.dngfiles. A production tree holds one JPEG and one metadata JSON per frame, so it can reach hundreds of thousands of entries after some months.prune_sidecarsruns inside the capture slot, on the keeper frame, after the DNG is promoted. The walk plus onestat()per negative can therefore consume a measurable part of the slot on a Pi with a slow SD card.Two options: keep an in-memory list of promoted negatives for the process lifetime, or restrict the sweep to the most recent date subdirectories.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raspilapse/dynrange/sidecar.py` around lines 37 - 43, The prune_sidecars scan using root.rglob("*.dng") traverses the entire output tree during the capture slot; replace it with a bounded approach that avoids walking all historical entries, preferably by tracking promoted negatives in memory for the process lifetime or restricting the scan to recent date subdirectories. Preserve the existing mtime ordering, max_files retention, OSError handling, and return behavior around negatives and doomed.tests/test_dynrange_sensor_hdr.py (1)
173-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused caplog wiring.
The test attaches and detaches
caplog.handleron thedynrangelogger, but it never asserts oncaplog. The two assertions only check the degraded method and label. Either assert the expected warning or drop the handler code.♻️ Proposed cleanup
def test_missing_subdev_degrades_to_off(self, monkeypatch, caplog): - import logging - - logger = logging.getLogger("dynrange") - logger.addHandler(caplog.handler) - try: - dr = make_dr(monkeypatch, subdev=None) - finally: - logger.removeHandler(caplog.handler) + dr = make_dr(monkeypatch, subdev=None) assert dr.method == "off" assert dr.label() == "off"🤖 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_dynrange_sensor_hdr.py` around lines 173 - 183, Remove the unused logging import, logger setup, handler attachment/removal, and try/finally block from test_missing_subdev_degrades_to_off; keep the make_dr call and its method and label assertions unchanged.tests/test_metering.py (1)
264-285: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a case for a null
dynamic_rangeblock.
metering.pyline 136 usesor {}to survivedynamic_range: None, which is what a baredynamic_range:line in YAML produces.tests/test_dynrange_config.pycovers that shape for the facade. No test pins it for the meter, so a future simplification of the guard would pass CI and then raiseAttributeErrorat startup.♻️ Proposed test
def test_default_stays_on_without_fusion(self): assert Meter(config())._p95_enabled is True assert Meter(config(dynamic_range={"method": "tone_map"}))._p95_enabled is True + + def test_null_dynamic_range_block_is_tolerated(self): + """A bare `dynamic_range:` line in YAML arrives as None.""" + assert Meter(config(dynamic_range=None))._p95_enabled is True🤖 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_metering.py` around lines 264 - 285, Add a test to TestFusionRelaxesHighlightProtection covering config(dynamic_range=None), and assert Meter initializes successfully with _p95_enabled remaining True. This should pin the null dynamic_range handling in Meter without changing the existing fusion or explicit-setting cases.tests/test_dynrange_tonemap.py (1)
120-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePermission assertions mask the bits they check. Both tests use
st_mode & 0o644 == 0o644, which confirms the read bits are present but accepts extra bits such as group-write or world-write. The stated intent is an exact0o644result afteros.replace.
tests/test_dynrange_tonemap.py#L120-L126: change the assertion toimage.stat().st_mode & 0o777 == 0o644.tests/test_dynrange_sensor_hdr.py#L139: change the assertion tosmall.stat().st_mode & 0o777 == 0o644.🤖 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_dynrange_tonemap.py` around lines 120 - 126, Update the permission assertions in test_result_is_world_readable in tests/test_dynrange_tonemap.py:120-126 and the corresponding assertion in tests/test_dynrange_sensor_hdr.py:139 to mask st_mode with 0o777 before comparing to 0o644, ensuring the file permissions match exactly.raspilapse/dynrange/sensor_hdr.py (2)
105-107: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard against a zero or missing dimension.
hdr_main_sizedivides bywidthandheight. A configuration with a zero dimension raisesZeroDivisionErrorinsidepre_open, which the daemon calls for every frame. The rest of this pipeline degrades instead of raising. A minimum-value guard keeps that property.♻️ Proposed guard
width, height = configured + if width <= 0 or height <= 0: + return HDR_MODE_SIZE scale = min(HDR_MODE_SIZE[0] / width, HDR_MODE_SIZE[1] / height, 1.0) return (int(width * scale) // 2 * 2, int(height * scale) // 2 * 2)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raspilapse/dynrange/sensor_hdr.py` around lines 105 - 107, Update the size calculation in hdr_main_size to guard width and height against missing or zero values before computing scale, substituting the pipeline’s minimum valid dimension behavior so pre_open does not raise ZeroDivisionError. Preserve the existing HDR scaling and even-dimension rounding for valid configurations.
132-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Image.Resampling.LANCZOSinstead ofImage.LANCZOS. Pillow 11 retainsImage.LANCZOSas a deprecated alias.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@raspilapse/dynrange/sensor_hdr.py` at line 132, Update the resize call to use Image.Resampling.LANCZOS instead of the deprecated Image.LANCZOS alias, preserving the existing target size and resize behavior.
🤖 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 `@raspilapse/camera/capture.py`:
- Around line 660-664: Update capture_bracketed after the exposure loop to
restore the base exposure from bracket_exposures_us[0] via update_controls,
ensuring restoration also occurs when bracket capture fails by placing it in the
appropriate cleanup/finally path. Preserve the existing frame collection and
settle-frame behavior.
In `@raspilapse/camera/metering.py`:
- Around line 135-137: Update the initialization around _p95_enabled to use the
resolved DynamicRange method rather than re-parsing raw adaptive configuration,
and pass that resolved method into Meter. Validate dynamic_range and
highlight_protection as mappings before accessing .get(), preserving safe
behavior for null, scalar, or list values and ensuring unavailable fusion
resolves consistently to off.
In `@raspilapse/cli/drtest.py`:
- Around line 296-302: Update the CLI flow around print_summary and the final
return so an empty rows result returns a nonzero exit status, while successful
captures retain the existing zero status and summary behavior. Add a test
covering all run_method calls returning None, including the expected failure
exit code.
- Around line 167-185: Set method_config["output"]["directory"] to str(outdir)
before calling DynamicRange.from_config(method_config), so DynamicRange captures
the requested output directory for sidecars. Keep the existing CameraConfig
output-directory assignment unchanged for image capture.
- Around line 74-77: Update the dynamic-range configuration assignment in the
CLI flow to preserve the existing mapping and override only method and
tone_map.enabled, retaining configured fusion brackets, EV spread, sensor-HDR,
and tone-map strength. Add a regression test using non-default fusion and
tone-map settings to verify those values survive the override.
In `@raspilapse/daemon.py`:
- Around line 1252-1255: Guard the self._dr.shutdown() call in the finally block
so permission or I/O failures during shutdown do not escape run(). Catch
shutdown exceptions and preserve best-effort cleanup, allowing the existing
final stop log to execute.
In `@tests/test_database.py`:
- Around line 949-951: Remove the dr_method column from the v5 fixture DDL
construction used by the legacy database test, while preserving it in
CAPTURES_DDL for migration 7 to add. Ensure the fixture remains a valid v5
schema so the test can migrate through CaptureDatabase.SCHEMA_VERSION without a
duplicate-column error.
---
Outside diff comments:
In `@raspilapse/dynrange/__init__.py`:
- Around line 103-165: Introduce a numeric coercion helper near the DynamicRange
configuration parsing that catches invalid or null scalar conversions, logs a
warning, and returns the supplied default. Replace the direct numeric
conversions in the shown initialization—tone-map strength, fusion brackets/EV
spread/single-shot threshold, adaptive interval, resolution dimensions, quality,
sidecar frequency, and retention—with this helper while preserving each existing
default and subsequent clamping behavior so malformed YAML cannot break
DynamicRange construction.
---
Nitpick comments:
In `@raspilapse/dynrange/__init__.py`:
- Around line 360-366: Update chain to combine the accumulated processed result
with the overlay result when overlay is configured, while preserving processed
as the result when no overlay exists. Ensure any failed stage causes the final
chain result to remain false even if the overlay succeeds.
In `@raspilapse/dynrange/sensor_hdr.py`:
- Around line 105-107: Update the size calculation in hdr_main_size to guard
width and height against missing or zero values before computing scale,
substituting the pipeline’s minimum valid dimension behavior so pre_open does
not raise ZeroDivisionError. Preserve the existing HDR scaling and
even-dimension rounding for valid configurations.
- Line 132: Update the resize call to use Image.Resampling.LANCZOS instead of
the deprecated Image.LANCZOS alias, preserving the existing target size and
resize behavior.
In `@raspilapse/dynrange/sidecar.py`:
- Around line 37-43: The prune_sidecars scan using root.rglob("*.dng") traverses
the entire output tree during the capture slot; replace it with a bounded
approach that avoids walking all historical entries, preferably by tracking
promoted negatives in memory for the process lifetime or restricting the scan to
recent date subdirectories. Preserve the existing mtime ordering, max_files
retention, OSError handling, and return behavior around negatives and doomed.
In `@tests/test_database.py`:
- Around line 966-1000: Update the final schema version assertion in
test_migration_7_adds_dr_method_to_an_existing_database to compare against
CaptureDatabase.SCHEMA_VERSION instead of the literal 7, while leaving the
migration and column assertions unchanged.
In `@tests/test_dynrange_sensor_hdr.py`:
- Around line 173-183: Remove the unused logging import, logger setup, handler
attachment/removal, and try/finally block from
test_missing_subdev_degrades_to_off; keep the make_dr call and its method and
label assertions unchanged.
In `@tests/test_dynrange_tonemap.py`:
- Around line 120-126: Update the permission assertions in
test_result_is_world_readable in tests/test_dynrange_tonemap.py:120-126 and the
corresponding assertion in tests/test_dynrange_sensor_hdr.py:139 to mask st_mode
with 0o777 before comparing to 0o644, ensuring the file permissions match
exactly.
In `@tests/test_metering.py`:
- Around line 264-285: Add a test to TestFusionRelaxesHighlightProtection
covering config(dynamic_range=None), and assert Meter initializes successfully
with _p95_enabled remaining True. This should pin the null dynamic_range
handling in Meter without changing the existing fusion or explicit-setting
cases.
🪄 Autofix
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: 32b4896f-2e74-4ea2-a741-5e392db67aed
📒 Files selected for processing (30)
CHANGELOG.mdREADME.mdconfig/config.example.ymldocs/CONFIG-REFERENCE.ymldocs/EXPOSURE.mddocs/TROUBLESHOOTING.mdpyproject.tomlraspilapse/camera/capture.pyraspilapse/camera/metering.pyraspilapse/cli/drtest.pyraspilapse/daemon.pyraspilapse/dynrange/__init__.pyraspilapse/dynrange/fusion.pyraspilapse/dynrange/raw_develop.pyraspilapse/dynrange/sensor_hdr.pyraspilapse/dynrange/sidecar.pyraspilapse/dynrange/tonemap.pyraspilapse/overlay/render.pyraspilapse/storage/database.pyrequirements.txttests/test_database.pytests/test_drtest_cli.pytests/test_dynrange_config.pytests/test_dynrange_fusion.pytests/test_dynrange_raw.pytests/test_dynrange_sensor_hdr.pytests/test_dynrange_sidecar.pytests/test_dynrange_tonemap.pytests/test_metering.pytests/test_overlay_fields.py
Comment on lines
+660
to
+664
| for exposure_us in bracket_exposures_us[1:]: | ||
| self.update_controls({"ExposureTime": int(exposure_us)}) | ||
| frame, discarded = self._capture_at_exposure(int(exposure_us), settle_frames_max) | ||
| frames.append(frame) | ||
| self.last_settle_frames.append(discarded) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find callers of capture_bracketed and check whether any reuses one open camera.
rg -n -C 10 'capture_bracketed' --type=py
# Inspect the comparison CLI's camera lifecycle.
fd -i 'drtest' --type f --exec rg -n -C 5 'initialize_camera|capture\(|capture_bracketed|close\(' {}Repository: ekstremedia/raspilapse
Length of output: 314
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(capture\.py|.*drtest.*)$|raspilapse'
printf '%s\n' '--- capture_bracketed definition and callers ---'
rg -n -C 12 'capture_bracketed|def capture|initialize_camera|close\(' --glob '*.py' .
printf '%s\n' '--- camera lifecycle and drtest-related files ---'
fd -i 'drtest|capture|camera' --type f . | while IFS= read -r file; do
if rg -q 'capture_bracketed|initialize_camera|close\(|capture\(' "$file"; then
printf '\n### %s\n' "$file"
rg -n -C 8 'capture_bracketed|initialize_camera|close\(|capture\(' "$file"
fi
doneRepository: ekstremedia/raspilapse
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- capture_bracketed references ---'
rg -n -C 6 'capture_bracketed' raspilapse tests --glob '*.py'
printf '%s\n' '--- capture.py camera state and bracket implementation ---'
rg -n '^(class | def | def )|ExposureTime|update_controls|_capture_at_exposure' raspilapse/camera/capture.py | tail -n 140
printf '%s\n' '--- drtest implementation ---'
cat -n raspilapse/cli/drtest.py
printf '%s\n' '--- tests that exercise bracket state or drtest lifecycle ---'
rg -n -C 12 'capture_bracketed|ExposureTime|initialize_camera|close\(' tests/test_dynrange_fusion.py tests/test_drtest_cli.pyRepository: ekstremedia/raspilapse
Length of output: 46096
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- bracket implementation ---'
sed -n '400,435p;470,625p;625,705p' raspilapse/camera/capture.py
printf '%s\n' '--- all ImageCapture lifecycle call sites ---'
rg -n -C 8 'ImageCapture\(|initialize_camera\(|\.capture\(|capture_frame\(|\.close\(' raspilapse --glob '*.py'
printf '%s\n' '--- drtest test assertions about camera instances ---'
rg -n -C 12 'run_method|meter_scene|ImageCapture|initialize_camera|capture_bracketed|close' tests/test_drtest_cli.pyRepository: ekstremedia/raspilapse
Length of output: 48604
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import ast
from pathlib import Path
capture_path = Path("raspilapse/camera/capture.py")
capture_tree = ast.parse(capture_path.read_text())
bracket = next(
node for node in ast.walk(capture_tree)
if isinstance(node, ast.FunctionDef) and node.name == "capture_bracketed"
)
loop = next(node for node in ast.walk(bracket) if isinstance(node, ast.For))
loop_calls = [
ast.unparse(node)
for node in ast.walk(loop)
if isinstance(node, ast.Call)
]
after_loop = [
ast.unparse(node)
for node in bracket.body[bracket.body.index(loop) + 1:]
if isinstance(node, ast.Expr) and isinstance(node.value, ast.Call)
]
print("capture_bracketed loop calls:")
print("\n".join(loop_calls))
print("direct call expressions after bracket loop:")
print("\n".join(after_loop) or "<none>")
drtest_tree = ast.parse(Path("raspilapse/cli/drtest.py").read_text())
run_method = next(
node for node in ast.walk(drtest_tree)
if isinstance(node, ast.FunctionDef) and node.name == "run_method"
)
lifecycle = [
ast.unparse(node)
for node in ast.walk(run_method)
if isinstance(node, ast.Call)
and isinstance(node.func, ast.Attribute)
and node.func.attr in {"initialize_camera", "close", "capture_frame"}
]
print("run_method lifecycle calls:")
print("\n".join(lifecycle))
PYRepository: ekstremedia/raspilapse
Length of output: 296
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import ast
from pathlib import Path
def calls_in(node):
return [
(child.lineno, ast.unparse(child))
for child in ast.walk(node)
if isinstance(child, ast.Call)
]
capture_tree = ast.parse(Path("raspilapse/camera/capture.py").read_text())
bracket = next(
node for node in ast.walk(capture_tree)
if isinstance(node, ast.FunctionDef) and node.name == "capture_bracketed"
)
loops = [
node for node in ast.walk(bracket)
if isinstance(node, ast.For)
]
print("capture_bracketed loops:")
for loop in loops:
print(f"line {loop.lineno}:")
for line, call in calls_in(loop):
print(f" line {line}: {call}")
print("capture_bracketed calls after the final loop:")
final_loop_end = max(loop.end_lineno for loop in loops)
for line, call in calls_in(bracket):
if line > final_loop_end:
print(f" line {line}: {call}")
drtest_tree = ast.parse(Path("raspilapse/cli/drtest.py").read_text())
run_method = next(
node for node in ast.walk(drtest_tree)
if isinstance(node, ast.FunctionDef) and node.name == "run_method"
)
print("run_method lifecycle calls:")
for line, call in calls_in(run_method):
if any(name in call for name in ("initialize_camera", "capture_frame", ".close(")):
print(f" line {line}: {call}")
PYRepository: ekstremedia/raspilapse
Length of output: 1627
Restore the base exposure after capture_bracketed. When an ImageCapture instance remains open, the next capture() uses the last bracket exposure because it does not reset controls. Restore bracket_exposures_us[0] after the loop, including the failure path. raspilapse-drtest closes each per-method camera, so it is not affected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@raspilapse/camera/capture.py` around lines 660 - 664, Update
capture_bracketed after the exposure loop to restore the base exposure from
bracket_exposures_us[0] via update_controls, ensuring restoration also occurs
when bracket capture fails by placing it in the appropriate cleanup/finally
path. Preserve the existing frame collection and settle-frame behavior.
Comment on lines
135
to
+137
| protection = adaptive.get("highlight_protection", {}) | ||
| self._p95_enabled = protection.get("enabled", True) | ||
| fusion_active = (adaptive.get("dynamic_range", {}) or {}).get("method") == "fusion" | ||
| self._p95_enabled = protection.get("enabled", not fusion_active) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where Meter is constructed and what configuration it receives.
rg -n -C5 'Meter\(' raspilapse/ --type=py -g '!**/tests/**'
# Check whether the resolved dynamic-range label is available at that point.
rg -n -C3 'def label|_dr\b|DynamicRange' raspilapse/daemon.py raspilapse/camera/exposure.pyRepository: ekstremedia/raspilapse
Length of output: 4642
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- metering.py ---'
sed -n '1,180p' raspilapse/camera/metering.py
printf '%s\n' '--- dynrange.py symbols and implementation ---'
ast-grep outline raspilapse/dynrange.py
sed -n '1,260p' raspilapse/dynrange.py
printf '%s\n' '--- exposure.py constructor context ---'
sed -n '1,115p' raspilapse/camera/exposure.py
printf '%s\n' '--- relevant tests ---'
sed -n '1,110p' tests/test_dynrange_config.py
sed -n '240,295p' tests/test_dynrange_config.py
printf '%s\n' '--- all DynamicRange and Meter references ---'
rg -n -C3 'DynamicRange\.from_config|\.label\(\)|Meter\(|highlight_protection|dynamic_range' raspilapse tests --type pyRepository: ekstremedia/raspilapse
Length of output: 7775
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- dynamic-range files ---'
git ls-files | rg '(^|/)(dynrange|dynamic.*range|.*range.*)\.py$|test_dynrange_config\.py$'
printf '%s\n' '--- DynamicRange definitions and imports ---'
rg -n -C5 'class DynamicRange|def from_config|def label|python3-opencv|OpenCV|cv2|DynamicRange' . --glob '*.py'
printf '%s\n' '--- meter configuration readers ---'
rg -n -C4 'adaptive|get\("highlight_protection"|get\("dynamic_range"|Meter\(' raspilapse --glob '*.py'Repository: ekstremedia/raspilapse
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- DynamicRange configuration seam ---'
sed -n '35,205p' raspilapse/dynrange/__init__.py
printf '%s\n' '--- daemon initialization and controller construction ---'
rg -n -C8 'ExposureController|self\._dr|DynamicRange\.from_config' raspilapse/daemon.py
printf '%s\n' '--- exposure controller initialization ---'
sed -n '50,95p' raspilapse/camera/exposure.py
printf '%s\n' '--- configuration merge behavior ---'
rg -n -C5 'def merge_defaults|merge_defaults|adaptive_timelapse' raspilapse/config.py raspilapse --glob '*.py' | head -160
printf '%s\n' '--- focused tests for meter and dynamic-range wiring ---'
rg -n -C5 'Meter|p95_enabled|highlight protection|highlight_protection|fusion_active|DynamicRange' tests --glob '*.py' | head -240Repository: ekstremedia/raspilapse
Length of output: 40549
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
metering = Path("raspilapse/camera/metering.py").read_text()
daemon = Path("raspilapse/daemon.py").read_text()
exposure = Path("raspilapse/camera/exposure.py").read_text()
assert 'fusion_active = (adaptive.get("dynamic_range", {}) or {}).get("method") == "fusion"' in metering
assert "self.meter = Meter(config)" in exposure
assert "self.exposure = ExposureController(self.config)" in daemon
assert "self._dr = DynamicRange.from_config(self.config)" in daemon
def meter_p95_enabled(adaptive):
protection = adaptive.get("highlight_protection", {})
fusion_active = (adaptive.get("dynamic_range", {}) or {}).get("method") == "fusion"
return protection.get("enabled", not fusion_active)
def resolved_label(adaptive, cv2_available):
block = adaptive.get("dynamic_range")
block = block if isinstance(block, dict) else {}
method = str(block.get("method", "off")).lower()
if method not in ("off", "fusion", "sensor_hdr", "raw", "tone_map"):
method = "off"
if method == "tone_map":
method = "off"
if method == "fusion" and not cv2_available:
method = "off"
return method
cases = [
("scalar dynamic_range", {"dynamic_range": "fusion"}),
("list dynamic_range", {"dynamic_range": ["fusion"]}),
("null highlight_protection", {"highlight_protection": None}),
("scalar highlight_protection", {"highlight_protection": "enabled"}),
("list highlight_protection", {"highlight_protection": []}),
]
for name, adaptive in cases:
try:
meter_p95_enabled(adaptive)
except AttributeError:
print(f"{name}: AttributeError")
else:
print(f"{name}: no exception")
adaptive = {"dynamic_range": {"method": "fusion"}}
print("fusion with cv2:", resolved_label(adaptive, True),
"meter p95:", meter_p95_enabled(adaptive))
print("fusion without cv2:", resolved_label(adaptive, False),
"meter p95:", meter_p95_enabled(adaptive))
assert resolved_label(adaptive, False) == "off"
assert meter_p95_enabled(adaptive) is False
PYRepository: ekstremedia/raspilapse
Length of output: 442
Share the resolved DynamicRange state with Meter.
Meter re-parses the raw configuration before DynamicRange resolves it.
- A scalar or list
dynamic_rangevalue raisesAttributeErrorinMeter. - A null, scalar, or list
highlight_protectionvalue also raisesAttributeError. - If
fusionlackscv2,DynamicRangeresolves tooff, butMeterdisables highlight protection because it still reads the raw method. Single-shot capture then runs without fusion or highlight protection.
Pass the resolved method to Meter. At minimum, validate both sub-blocks as mappings before calling .get().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@raspilapse/camera/metering.py` around lines 135 - 137, Update the
initialization around _p95_enabled to use the resolved DynamicRange method
rather than re-parsing raw adaptive configuration, and pass that resolved method
into Meter. Validate dynamic_range and highlight_protection as mappings before
accessing .get(), preserving safe behavior for null, scalar, or list values and
ensuring unavailable fusion resolves consistently to off.
Comment on lines
+74
to
+77
| adaptive["dynamic_range"] = { | ||
| "method": method, | ||
| "tone_map": {"enabled": tone_map}, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the configured dynamic-range options.
Lines 74-77 replace the complete dynamic_range mapping. This discards configured fusion brackets, EV spread, sensor-HDR options, and tone-map strength. The comparison can then use settings that differ from the configured camera pipeline.
Keep the existing mapping. Override only method and tone_map.enabled. Add a regression test with non-default fusion and tone-map values.
Proposed fix
- adaptive["dynamic_range"] = {
- "method": method,
- "tone_map": {"enabled": tone_map},
- }
+ dynamic_range = adaptive.setdefault("dynamic_range", {})
+ dynamic_range["method"] = method
+ dynamic_range.setdefault("tone_map", {})["enabled"] = tone_map📝 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.
Suggested change
| adaptive["dynamic_range"] = { | |
| "method": method, | |
| "tone_map": {"enabled": tone_map}, | |
| } | |
| dynamic_range = adaptive.setdefault("dynamic_range", {}) | |
| dynamic_range["method"] = method | |
| dynamic_range.setdefault("tone_map", {})["enabled"] = tone_map |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@raspilapse/cli/drtest.py` around lines 74 - 77, Update the dynamic-range
configuration assignment in the CLI flow to preserve the existing mapping and
override only method and tone_map.enabled, retaining configured fusion brackets,
EV spread, sensor-HDR, and tone-map strength. Add a regression test using
non-default fusion and tone-map settings to verify those values survive the
override.
Comment on lines
+167
to
+185
| method_config = build_method_config(base_config, token) | ||
| dr = DynamicRange.from_config(method_config) | ||
| if dr.label() == "off" and token != "off": | ||
| print(f" {token}: not available on this camera (see the warning above), skipping") | ||
| return None | ||
|
|
||
| mode = light_mode_for(settings["ExposureTime"]) | ||
|
|
||
| # A private CameraConfig pointed at the outdir with a label filename, so | ||
| # every method lands exactly where the comparison wants it. | ||
| camera_config = CameraConfig(config_path) | ||
| camera_config.config["output"]["directory"] = str(outdir) | ||
| camera_config.config["output"]["organize_by_date"] = False | ||
| camera_config.config["output"]["filename_pattern"] = f"{stamp}_{token.replace('+', '_')}.jpg" | ||
| camera_config.config["system"][ | ||
| "metadata_filename" | ||
| ] = f"{stamp}_{token.replace('+', '_')}_metadata.json" | ||
|
|
||
| capture = ImageCapture(camera_config, post_process=dr.build_post_process(method_config)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Set the dynamic-range output directory before construction.
DynamicRange.from_config(method_config) reads output.directory before Lines 178-183 change the separate CameraConfig. If DNG sidecars are enabled, DynamicRange retains the original output directory and writes sidecars outside --outdir.
Update method_config["output"]["directory"] before constructing DynamicRange.
Proposed fix
method_config = build_method_config(base_config, token)
+ method_config["output"]["directory"] = str(outdir)
dr = DynamicRange.from_config(method_config)📝 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.
Suggested change
| method_config = build_method_config(base_config, token) | |
| dr = DynamicRange.from_config(method_config) | |
| if dr.label() == "off" and token != "off": | |
| print(f" {token}: not available on this camera (see the warning above), skipping") | |
| return None | |
| mode = light_mode_for(settings["ExposureTime"]) | |
| # A private CameraConfig pointed at the outdir with a label filename, so | |
| # every method lands exactly where the comparison wants it. | |
| camera_config = CameraConfig(config_path) | |
| camera_config.config["output"]["directory"] = str(outdir) | |
| camera_config.config["output"]["organize_by_date"] = False | |
| camera_config.config["output"]["filename_pattern"] = f"{stamp}_{token.replace('+', '_')}.jpg" | |
| camera_config.config["system"][ | |
| "metadata_filename" | |
| ] = f"{stamp}_{token.replace('+', '_')}_metadata.json" | |
| capture = ImageCapture(camera_config, post_process=dr.build_post_process(method_config)) | |
| method_config = build_method_config(base_config, token) | |
| method_config["output"]["directory"] = str(outdir) | |
| dr = DynamicRange.from_config(method_config) | |
| if dr.label() == "off" and token != "off": | |
| print(f" {token}: not available on this camera (see the warning above), skipping") | |
| return None | |
| mode = light_mode_for(settings["ExposureTime"]) | |
| # A private CameraConfig pointed at the outdir with a label filename, so | |
| # every method lands exactly where the comparison wants it. | |
| camera_config = CameraConfig(config_path) | |
| camera_config.config["output"]["directory"] = str(outdir) | |
| camera_config.config["output"]["organize_by_date"] = False | |
| camera_config.config["output"]["filename_pattern"] = f"{stamp}_{token.replace('+', '_')}.jpg" | |
| camera_config.config["system"][ | |
| "metadata_filename" | |
| ] = f"{stamp}_{token.replace('+', '_')}_metadata.json" | |
| capture = ImageCapture(camera_config, post_process=dr.build_post_process(method_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 `@raspilapse/cli/drtest.py` around lines 167 - 185, Set
method_config["output"]["directory"] to str(outdir) before calling
DynamicRange.from_config(method_config), so DynamicRange captures the requested
output directory for sidecars. Keep the existing CameraConfig output-directory
assignment unchanged for image capture.
Comment on lines
+296
to
+302
| if rows: | ||
| print_summary(rows) | ||
| if args.repeat > 1 and round_index < args.repeat - 1: | ||
| time.sleep(2) | ||
|
|
||
| print("\nRemember: sudo systemctl start raspilapse") | ||
| return 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return failure when no frame was captured.
If every run_method call returns None, the CLI prints no summary and exits with status 0. This reports success after capture failures such as an output write error.
Return a nonzero status when rows is empty. Add a test for this path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@raspilapse/cli/drtest.py` around lines 296 - 302, Update the CLI flow around
print_summary and the final return so an empty rows result returns a nonzero
exit status, while successful captures retain the existing zero status and
summary behavior. Add a test covering all run_method calls returning None,
including the expected failure exit code.
Comment on lines
+1252
to
+1255
| # Leave the sensor as the plain pipeline expects to find it | ||
| # (sensor_hdr's wide_dynamic_range flag outlives the process). | ||
| self._dr.shutdown() | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard self._dr.shutdown() in the finally block.
shutdown() writes the sensor's wide_dynamic_range control through sensor_hdr.set_wdr. That touches a V4L2 subdevice, which can fail with a permission or I/O error. An exception here escapes run() from the finally block and skips the final stop log. Wrap the call so shutdown stays best-effort.
🛡️ Proposed fix
- self._dr.shutdown()
+ try:
+ self._dr.shutdown()
+ except Exception as e:
+ logger.warning(f"Dynamic-range shutdown failed: {e}")📝 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.
Suggested change
| # Leave the sensor as the plain pipeline expects to find it | |
| # (sensor_hdr's wide_dynamic_range flag outlives the process). | |
| self._dr.shutdown() | |
| # Leave the sensor as the plain pipeline expects to find it | |
| # (sensor_hdr's wide_dynamic_range flag outlives the process). | |
| try: | |
| self._dr.shutdown() | |
| except Exception as e: | |
| logger.warning(f"Dynamic-range shutdown failed: {e}") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@raspilapse/daemon.py` around lines 1252 - 1255, Guard the self._dr.shutdown()
call in the finally block so permission or I/O failures during shutdown do not
escape run(). Catch shutdown exceptions and preserve best-effort cleanup,
allowing the existing final stop log to execute.
Comment on lines
+949
to
+951
| # The chain runs to the current version, not just to 6 -- a legacy | ||
| # database picks up every later migration in the same open. | ||
| assert version == CaptureDatabase.SCHEMA_VERSION |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove dr_method from the v5 fixture DDL.
Line 949 now runs migration 7 for this fixture. CAPTURES_DDL already includes dr_method, so migration 7 fails with a duplicate-column error. Remove dr_method when constructing the v5 schema.
Proposed fix
for column in (
"system_mem_used_mb INTEGER",
"system_mem_percent REAL",
"system_disk_free_gb REAL",
"system_disk_percent REAL",
"system_uptime_s INTEGER",
"process_rss_mb INTEGER",
"network_up INTEGER",
"network_signal_dbm INTEGER",
):
ddl = ddl.replace(f" {column},\n", "")
+ ddl = ddl.replace(" dr_method TEXT,\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_database.py` around lines 949 - 951, Remove the dr_method column
from the v5 fixture DDL construction used by the legacy database test, while
preserving it in CAPTURES_DDL for migration 7 to add. Ensure the fixture remains
a valid v5 schema so the test can migrate through CaptureDatabase.SCHEMA_VERSION
without a duplicate-column error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
Daytime frames trade crushed shadows for protected highlights: the metering loop deliberately underexposes (
highlight_protectionscales the target down to 0.70) because one exposure cannot hold both ends of a high-contrast sky. This PR gives the pipeline more dynamic range than a single exposure carries, as a selectable, opt-in method so each approach can be trialed on a real camera before any default changes:fusionsensor_hdrrawtone_maptone_map.enabled)Also included: a
dr_methodoverlay placeholder + DB column so trial frames are labeled, an opt-in DNG sidecar (occasional raw negatives, retention-capped, ships disabled), and araspilapse-drtestCLI that captures the same scene with every method back-to-back for side-by-side comparison.Design constraints
offis provably the old pipeline — the post-process chain is literallybuild_overlay's callable, and golden replay tests are untouched.python3-opencv,python3-rawpy), absent in CI; all imports are lazy, and a method whose dependency is missing logs one warning and runs asoff.adaptive_timelapse.hdrblock is removed — it set libcamera'sHdrModecontrol, which the Pi 4's sensor never acts on (dormant on every camera).Commits land incrementally
This PR is pushed commit-by-commit (config seam → tone_map → fusion → sensor_hdr → raw+sidecar → drtest CLI) so review can start early; the checklist above describes the finished state.
🤖 Generated with Claude Code
https://claude.ai/code/session_011uhRMmJgKzCJad6PNUihq3
Summary by CodeRabbit
New Features
raspilapse-drtestcomparison tool for evaluating capture methods.Bug Fixes
Documentation