Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
6 tasks
Collaborator
Author
|
Not yet tested on sky (the dome had closed). The 18 acquisitions of that night are the measurement this PR is based on. |
cfremling
force-pushed
the
fix/acam-acquire-frame-after-start
branch
from
October 29, 2026 14:10
d119269 to
d6e30b0
Compare
cfremling
changed the base branch from
main
to
feat/fineacquire-wait-for-move
October 29, 2026 14:10
The framegrab runs through the slew and do_acquire() is called with the first frame to finish after the acquire command, which can have been exposed entirely while the telescope was still settling. On 2026-10-29, 4 of 18 acquisitions used such a frame (its solve landed 1.2-2.6 s after ontarget: a 2 s exposure started 1.5-3 s before it) and overshot the first correction by 46-107%, costing 1-3 extra frames each; every acquisition whose first frame began at or after ontarget corrected to within 35% (median 5%) in one move. Target::acquire() stamps the start; do_acquire() skips a frame whose readout time (mjd0) is less than one frame cycle after it. Costs one frame cycle only when the in-flight frame straddles the command. Co-Authored-By: Claude Opus 5.5 <[email protected]>
cfremling
force-pushed
the
fix/acam-acquire-frame-after-start
branch
from
October 29, 2026 14:25
d6e30b0 to
6071413
Compare
cfremling
changed the base branch from
feat/fineacquire-wait-for-move
to
main
October 29, 2026 14:25
This branch has not been deployed
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
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.
The ACAM framegrab runs through the slew, and
do_acquire()is called with the first frame to finish after theacquirecommand. That frame can have been exposed entirely while the telescope was still settling: with a 2 s exposure and ~3.1 s cycle, a frame finishing 1–3 s after the command began 1.5–3 s before it.Measured on 2026-10-29 (18 acquisitions, auto-ontarget, acquisition starting at ontarget): 4 acquisitions used such a frame (first solve 1.2–2.6 s after ontarget) and overshot the first correction by 46–107% (18.5" → 19.7" in the opposite direction on one target), costing 1–3 extra frames each; every acquisition whose first frame began at or after ontarget corrected to within 35% (median 5%) in one move. Frames exposed during the slew that fail to solve already cost one frame harmlessly.
Target::acquire()stamps the start of an acquisition;do_acquire()does not use a frame whose readout time (camera_info.mjd0) is less than one frame cycle (kintime) after it. Costs one frame cycle only when the in-flight frame straddles the command, instead of a fixed delay on every target.Not yet tested on sky.
Applies to main on its own, and merges cleanly before or after #503.
🤖 Generated with Claude Code