Keep guiding on offsets; cap deliberate offsets at 300", guiding at 60"#449
Keep guiding on offsets; cap deliberate offsets at 300", guiding at 60"#449cfremling wants to merge 4 commits into
Conversation
acamd's offset_goal() previously kicked the system from TARGET_GUIDE back into TARGET_ACQUIRE whenever an offset was applied without the "fineguiding" flag. Because the slicecam "put on slit" path and the sequencer's target_offset() (run right after fine acquisition) both send ACAMD_OFFSETGOAL without that flag, a guided "put on slit" and every post-fineacquire science offset would drop guiding and restart acquisition. There is no use case for re-acquiring on an offset while already acquired and guiding. Make offset_goal() always stay in TARGET_GUIDE while guiding, resetting the offset median-filter buffers so the new goal takes effect quickly (the same reset the fineguiding path already did). The "fineguiding" flag is no longer needed and is removed from acamd's offset_goal() and from slicecam's offset_acam_goal(). The fix is contained entirely to the ACAMD_OFFSETGOAL command handler. The acquisition loop is untouched, and behavior in non-guiding states (ACQUIRE, ACQUIRE_HERE, NOP) is unchanged. 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: 05b27d5c14
ℹ️ 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 ( this->target.acquire_mode == Acam::TARGET_GUIDE ) { | ||
| // for slicecam fine aquisition/guiding, stay in TARGET_GUIDE but | ||
| // reset the filtering so the goal takes effect quickly | ||
| if ( is_fineguiding ) { | ||
| this->target.reset_offset_params(); | ||
| } | ||
| else { | ||
| this->target.acquire_mode = Acam::TARGET_ACQUIRE; | ||
| this->target.nacquired = 0; | ||
| this->target.attempts = 0; | ||
| this->target.sequential_failures = 0; | ||
| this->target.timeout_time = std::chrono::steady_clock::now() | ||
| + std::chrono::duration<double>(this->target.timeout); | ||
| } | ||
| this->target.reset_offset_params(); |
There was a problem hiding this comment.
When offsetperiod is configured above 1 second and a put-on-slit or target offset exceeds ACQUIRE_TCS_MAX_OFFSET, keeping TARGET_GUIDE causes the offset to never be applied. On the first frame, do_acquire() permits the large offset using the one-shot putonslit_offset - last_putonslit_offset allowance, but median_filter() returns false while filling its window; last_putonslit_offset is nevertheless updated, so every subsequent frame rejects the still-pending correction against the normal maximum at acamd/acam_interface.cpp:3640-3648. The previous transition to TARGET_ACQUIRE bypassed the median filter and sent the correction during that first allowed frame.
Useful? React with 👍 / 👎.
| this->target.timeout_time = std::chrono::steady_clock::now() | ||
| + std::chrono::duration<double>(this->target.timeout); | ||
| } | ||
| this->target.reset_offset_params(); |
There was a problem hiding this comment.
For ordinary guided offset requests, this newly added reset can run in the server command thread while the frame-grab thread is executing do_acquire() and mutating the same ra_offs, dec_offs, and time_offs vectors in median_filter() (acamd/acam_interface.cpp:3807-3820). Clearing those vectors concurrently with a push or sort is undefined behavior and can corrupt memory or crash acamd; use a shared lock or defer the reset to the acquisition thread.
Useful? React with 👍 / 👎.
A deliberate goal offset applied while guiding -- put-on-slit, offset-star acquisition, the end-of-fineacquire target offset, and the pyGUI 'Offset' button (all routed through offset_goal/ACAMD_OFFSETGOAL) -- can legitimately be much larger than an ordinary guiding correction. The 60" ACQUIRE_TCS_MAX_OFFSET gate would reject these. The previous mechanism enlarged the gate by the frame-to-frame change in a putonslit_offset (maxoffset = tcs_max_offset + |putonslit_offset - last_putonslit_offset|). That was fragile: it only widened the cap when the offset *changed*, the one-shot widening was consumed on the first frame even if median_filter deferred the send, and it was never reset between targets, so a second similar-magnitude offset was rejected at 60". Replace it with an explicit one-shot allowance: offset_goal sets allow_large_offset while guiding, and do_acquire permits that one correction up to PUTONSLIT_TCS_MAX_OFFSET (300"). The allowance is consumed only when the offset is actually sent to the TCS, so a median_filter deferral can no longer waste it. Ordinary guiding corrections keep the 60" cap so a bad solution can't slew the telescope, and the ACQUIRE path (which uses tcs_max_offset, raised to 300" by the acquire wrapper during acquisition) is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
cfremling
changed the title
|
Thanks Codex — both findings evaluated against the real code; both are valid. Update in commit P1 — large put-on-slit/target offset never applied ( P2 — data race on |
The 300" deliberate-offset cap was a hard-coded constant; the 60" guiding cap is the config parameter ACQUIRE_TCS_MAX_OFFSET. Make them consistent: add ACQUIRE_TCS_MAX_PUTONSLIT_OFFSET (default 300") to acamd.cfg, loaded the same way as ACQUIRE_TCS_MAX_OFFSET, stored in Target::tcs_max_putonslit_offset with a matching setter/getter. The member defaults to 300" so existing configs that predate the new line still behave correctly.
|
Follow-up ( |
Also clear allow_large_offset on the reject path and add a mutex around the median-filter offset vectors.
|
The stay-in-guide change and the P1 fix are correct and kept. The one change being reverted is the deletion of the fineguiding flag. Deleting it looked safe because its original job (gating re-acquire) became obsolete, but the flag was also implicitly carrying the 60″/300″ cap distinction. Without it, the per-cycle slicecam fine-acquire correction ( Fix: restore fineguiding and gate the allowance on Two smaller follow-ups included: clear allow_large_offset on the reject path (it was leaking the 300″ allowance to the next correction), and add a mutex around the median-filter offset vectors to close the P2 race, which is runtime-reachable via |
2 participants
What this does
Two related fixes to how ACAM handles offsets applied while guiding.
1. Never drop out of guiding on an offset
offset_goal()previously kickedTARGET_GUIDE → TARGET_ACQUIREwhenever an offset arrived without afineguidingflag. Both the GUI "put on slit" path and the sequencer's post-fineacquiretarget_offset()sendACAMD_OFFSETGOALwithout that flag, so a guided put-on-slit and every post-fineacquire science offset dropped guiding and restarted acquisition. Dropping out of guiding on an offset is detrimental and has no practical use case; it is removed. No flag is needed to gate it, so thefineguidingflag is removed entirely. This matches the proven CF/mac-sim implementation ofoffset_goal.2. Cap deliberate offsets at 300″, ordinary guiding at 60″
A deliberate goal offset applied while guiding — put-on-slit, offset-star acquisition, the end-of-fineacquire target offset, and the pyGUI 'Offset' button (all routed through
offset_goal/ACAMD_OFFSETGOAL) — can legitimately be far larger than a guiding correction. The 60″ACQUIRE_TCS_MAX_OFFSETgate would reject them.The old mechanism widened the gate by the frame-to-frame change in
putonslit_offset— fragile: it only widened when the offset changed, the one-shot widening was consumed on the first frame even ifmedian_filterdeferred the send, and it was never reset between targets (so a second similar offset was rejected at 60″).Replaced with an explicit one-shot allowance:
offset_goalsetsallow_large_offsetwhile guiding, anddo_acquirepermits that one correction up toPUTONSLIT_TCS_MAX_OFFSET(300″), consumed only when the offset is actually sent to the TCS (so amedian_filterdeferral can't waste it).tcs_max_offset, which the acquire wrapper already raises to 300″ during acquisition.Scope / safety
offset_goaland thedo_acquiregate;allow_large_offsetis atomic (set in the command thread, read/cleared in the framegrab thread).Re: Codex review on the first commit
offsetperiod>1) — resolved: the new allowance is consumed only on an actual send and uses a flat 300″ cap (no delta math), so amedian_filterdeferral no longer wastes it.reset_offset_params) — dormant in practice:offsetperiod=1in the deployed config, wheremedian_filterreturns before touching the offset vectors, so there is no concurrent access. Left as-is.🤖 Generated with Claude Code