Align slicecam fine-acquisition with the proven auto-acq-clean engine#451
Align slicecam fine-acquisition with the proven auto-acq-clean engine#451cfremling wants to merge 1 commit into
Conversation
main's slicecam fine-acquisition is a C++ reimplementation (fineacquire/ do_fineacquire) of the proven embedded-C engine on feature/acq-automation-clean (ngps_acq.c). The reimplementation diverged in several ways that caused wrong/ overshooting moves and different convergence. This aligns the behavior with the known-good engine (Option C: targeted parameter + logic alignment; the larger adaptive faint/bright exposure engine is out of scope here). - gain 0.7 -> 1.0 (FINE_ACQUIRE_GAIN). A fine-tune step applies the full measured offset; deliberately under-correcting a known offset is never right. - Refuse to move on a noisy solution. Previously, at max_samples the move was commanded regardless of scatter; now (matching the reference) a correction is only sent when the per-axis MAD scatter is within prec, otherwise the batch is discarded and re-gathered. - prec 0.1 -> 0.4 arcsec and now configurable (FINE_ACQUIRE_PREC), matching the reference's --prec-arcsec 0.4. With the old hardcoded 0.1 the scatter gate was rarely satisfied, so the loop fell through to the move-anyway path. - Post-move settle delay: wait FINE_ACQUIRE_SETTLE_SEC (default 4s) after a TCS move before evaluating the next frame. Time-based, independent of frame cadence, matching the reference's --settle-sec/--cadence-sec 4. A pure frame count (settle_count) does not guarantee the move + readout has completed. - Reject duplicate frames. A commanded framegrab does not guarantee the image refreshed; processing the same frame twice double-counts samples and can command a second move from a pre-move image (overshoot). Skip a frame whose cheap subsample signature matches the previous one. - centroid sigma 2.0 -> 1.2 px, matching the reference's --centroid-sigma 1.2. Offset sign/convention verified to already match the reference: both measure (star - goal) with cos(dec) and tcs_sign=+1, and both apply via ACAMD_OFFSETGOAL(star - goal) (the reference reaches the identical value through put_on_slit/solve_offset, since solve_offset(slit, slit+delta) = delta). No sign change was needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abf9384fe6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const size_t step = img_data.size() > 4096 ? img_data.size() / 4096 : 1; | ||
| for ( size_t i = 0; i < img_data.size(); i += step ) { | ||
| sig = ( sig ^ static_cast<uint64_t>( static_cast<int64_t>( img_data[i] ) ) ) * 1099511628211ULL; | ||
| } | ||
| if ( sig == this->fineacquire_state.last_frame_sig ) return; // identical frame; wait for a fresh one |
There was a problem hiding this comment.
When frames are larger than 4096 pixels, this duplicate check hashes only a fixed stride of pixels. For a 1024x1024 frame that samples columns 0/256/512/768, while the configured fine-acquire target is around x=150 (FINE_ACQUIRE_AIMPOINT/ROI), so in emulated or low-noise averaged runs a fresh frame where only the star region changes can produce the same signature and be skipped indefinitely; do_fineacquire() then stops accumulating samples after the first matching signature. Include the centroid ROI/full frame or a real frame counter/timestamp in the signature so legitimate new frames cannot be mistaken for stale ones.
Useful? React with 👍 / 👎.
|
Closing — not ready for a real PR yet. Keeping the work on the branch while we evaluate the centroiding method (2D Moffat ground-truth) on real SCAM data and address review feedback. |
1 participant
Why behavior differs from
feature/acq-automation-cleanmain's slicecam fine-acquisition is not the same code as the known-good branch — it is a from-scratch C++ rewrite (
fineacquire/do_fineacquire) of the proven embedded-C engineslicecamd/ngps_acq.c(which main deleted). The rewrite diverged in several ways that produce wrong/overshooting moves and different convergence.This PR is the targeted alignment (Option C) — parameter + logic fixes to make the behavior match the reference. The reference's larger adaptive faint/bright exposure engine (EWMA modes, frame-averaging) is out of scope here.
Changes
max_samples, moves anyway regardless of scatterprec; else discard & re-gather0.1″hardcoded0.4″, configurable viaFINE_ACQUIRE_PREC0.71.0— apply the full measured offset (never under-correct a known offset)FINE_ACQUIRE_SETTLE_SEC(4 s) after a TCS move, time-based2.0px1.2px (--centroid-sigma 1.2)The overshoot symptom
The most likely contributors to "moves wrong / overshoots" were (1) re-processing a duplicate frame (the framegrab file may not have refreshed) → a second move from a pre-move image, and (2) moving on a high-scatter median. Both are now prevented; the 4 s post-move settle further ensures the move + readout completes before the next measurement.
Sign / convention — verified, no change needed
Both branches measure
(star − goal)withcos(dec)andtcs_sign = +1, and both apply viaACAMD_OFFSETGOAL(star − goal). The reference reaches the identical value throughput_on_slit→solve_offset(withslit = goal,cross = slit + (star−goal) = star, andsolve_offset(slit, cross) = (cross − slit) = (star − goal)). So main's direct application has the same sign and magnitude; the only nuance is main's flatcos(dec)vs the reference's spherical solver — negligible at arcsec scale.Notes
FINE_ACQUIRE_PREC=0.4andFINE_ACQUIRE_SETTLE_SEC=4added toslicecamd.cfg.in; members default to those values so pre-existing configs still behave correctly.gain = 1.0, the two-tiergain/gain_largeis now effectively a single full-gain step.🤖 Generated with Claude Code