slicecamd: anchor the fine-acquire settle to the executed TCS move, not a timer - #479
slicecamd: anchor the fine-acquire settle to the executed TCS move, not a timer#479cfremling wants to merge 1 commit into
Conversation
![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: 837370e902
ℹ️ 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".
| if ( goal_change_at_compute ) { | ||
| this->goalshift_pending.store( false, std::memory_order_release ); | ||
| } |
There was a problem hiding this comment.
If another offsetgoal request arrives after this guide iteration has read acam_goal but before its blocking pt_offset completes, goal_change_at_compute remains true and this store clears the shared flag for the newer, unexecuted goal. In particular, a fine-acquire correction issued while an earlier GUI/sequencer offset is in flight can then observe the resulting pending=false telemetry and resume sampling before its own move occurs, recreating the double-offset/false-lock failure this change is meant to prevent. Associate completion with a monotonically increasing request generation, or otherwise verify that the goal used for this move is still the latest request before clearing it.
Useful? React with 👍 / 👎.
| return ( this->last_acam_pubtime.load(std::memory_order_acquire) > send_time | ||
| && !this->acam_goalshift_pending.load(std::memory_order_acquire) ) | ||
| || !this->is_fineacquire_running.load(std::memory_order_acquire); |
There was a problem hiding this comment.
When framegrab stop, close, or shutdown is requested during this wait, those paths clear should_framegrab_run but do not clear is_fineacquire_running or notify acam_cv, and this predicate ignores the framegrab stop request. Consequently the framegrab thread can remain here for up to the new 60-second timeout, while framegrab("stop") itself times out after roughly five seconds; close() then proceeds to close the cameras even though the framegrab thread has not exited. Include the framegrab stop state in the predicate and notify this condition variable when stopping the loop.
Useful? React with 👍 / 👎.
837370e to
df8e112
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df8e1120d5
ℹ️ 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".
| return ( this->last_acam_pubtime.load(std::memory_order_acquire) > send_time | ||
| && !this->acam_goalshift_pending.load(std::memory_order_acquire) ) |
There was a problem hiding this comment.
If the guide loop publishes goalshift_pending=false after send_time but before the synchronous offsetgoal command sets the flag, that status may reach slicecamd while the command's subsequent pending=true publication is still queued. The predicate then immediately accepts the newer false status when the command returns, so settling and sampling resume before this correction is executed, recreating the stale-sample/double-offset failure. Track a request generation, or require observing this request's pending state before accepting its completion.
Useful? React with 👍 / 👎.
While ACAM guides, a fine-acquire correction only re-points acamd's goal; the guide loop applies it at its next executed correction. The settle was timed from the send, so with short slicecam exposures the next sample median could be built from pre-move frames and the same correction applied twice (UT 2026-08-06, BD+28d4211: exposed 2.6 arcsec off slit). acamd tracks the request: offsetgoal marks the goal change pending; the guide loop clears it when it executes a correction computed from the changed goal (the pending state is captured before the goal is read, so a correction already in flight can never clear it). The flag is published; do_fineacquire waits for pending false on a status newer than its send before settling. A stop interrupts the wait; timeout FINE_ACQUIRE_MOVE_TIMEOUT (default 60 s, 0 disables) proceeds on the old timer. Also fixes the offset_acam_goal log line, which printed degrees labelled arcsec on the guiding path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
df8e112 to
fbf1e77
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbf1e77ec7
ℹ️ 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".
| if ( goal_change_at_compute && | ||
| goal_ra_used == this->acam_goal.ra && | ||
| goal_dec_used == this->acam_goal.dec ) { | ||
| this->goalshift_pending.store( false, std::memory_order_release ); |
There was a problem hiding this comment.
When offsetperiod is configured above 1, median_filter() can replace the correction calculated from goal_ra_used/goal_dec_used with a mature window accumulated for the previous goal. offset_goal() applies the new coordinates and publishes pending=true before it calls reset_offset_params(), so the guide thread can enter the filter during that interval, send the old median, and then clear the flag here merely because the coordinates still match. Slicecamd consequently treats the new goal as physically applied and resumes sampling even though the TCS received a pre-shift correction; reset the filter atomically before exposing the new goal/pending generation, or associate completion with the correction actually sent.
Useful? React with 👍 / 👎.
|
Last Codex point (median filter vs goal registration) deliberately not addressed: the offset median filter is pass-through in practice (tcs_offset_period defaults to 1 and no config key sets it), so the scenario needs a configuration that cannot currently exist. |
1 participant
THIS FIXES A CRITICAL BUG (measured, UT 2026-08-06 07:50, BD+28d4211)
Fine-acquire corrections do not move the telescope:
offset_acam_goalsendsoffsetgoal <dRA> <dDEC> fineguidingto acamd, which only shifts the guideGOAL — the physical
pt_offsethappens at acamd's next guide solve, up toone full guide cycle (ACAM exptime + ~few s) later. slicecamd's post-correction
settle is currently a fixed 3 s sleep anchored at the send (
FINE_ACQUIRE_SETTLE_SEC,slicecam_interface.cpp:576-578), followed by 2 skipped frames and 3 fresh
samples. This works fine most of the time, but very short slicecam exposure times in combination with longer exposure times set on acam WILL lead to a situation where all of the slicecam safety and wait logic
completes before the telescope actually moves. This WILL result in a double offset by fineacquire, which can in a very unfortunate case also lead to a fineacquire 'lock' in between these two guide goal changes. Result is that the star is NOT centered on exposure.
On 2026-08-06 the per-frame log shows this end to end:
offset -0.983478 -2.485729 40(move 1, 5.7 s after the send); DONE 31.893offset -0.902148 -2.568997 40(move 2 — the double application)Prevalence: 1 confirmed double in 109 fine sessions scanned — rare because the
typical guide cadence (~5 s) beats the ~6 s fineacquire re-measure time, but the odds rise
toward certainty in the combination when ACAM exposures lengthen (poor transparency) while bright
stars keep slicecam frames short.
The fix (add a real check for guide goal change instead of the fixed wait in fineacquire)
offsetgoalmarks the goal changepending; the guide loop clears it when it executes a correction computed
from the changed goal (the pending state is captured before the goal is
read, so a correction already in flight can never clear it — and since
every correction is
goal − actual position, executing one computed fromthe new goal lands the telescope at the new goal). The flag is published
in the existing status.
do_fineacquirewaits until acamd reports the goal changeapplied (pending false, on a status newer than the send) before
FINE_ACQUIRE_SETTLE_SEC(now a true post-move settle) and freshsampling. A stop interrupts the wait. Timeout
FINE_ACQUIRE_MOVE_TIMEOUT(default 60 s, cfg.in knob; 0 disables): proceed with a WARNING —
fail-open, never a hang.
free: no sample any decision uses can predate the move it follows.
offset_acam_goal's "requested offsets … arcsec"line printed degrees while guiding (the ×3600 sits in the non-guiding
branch only, slicecam_interface.cpp:2619-2636). Log now prints arcsec on
both paths; nothing sent to acamd changes.
Notes for the reviewer
slicecamd.cfg.in(the deployed .cfg is cmake output andis regenerated on rebuild — the June 22 aimpoint/gain revert).
post-correction sampling gap sits after its move-DONE; the robot's
completion-audit race detector stays armed as the regression alarm either
way.
🤖 Generated with Claude Code