Repository navigation
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: f56675fd0e
ℹ️ 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 ( !tracking || !( sep_as < this->tcs_auto_ontarget_sep ) ) { consecutive=0; continue; } | ||
|
|
||
| if ( ++consecutive < 2 ) continue; // debounce: two reads in a row |
There was a problem hiding this comment.
When the new target is within tcs_auto_ontarget_sep of the previous field and the TCS does not drop ?ONTARGET or leave tracking, the previous settled state already satisfies every predicate here. The watcher therefore reaches two consecutive samples and calls ontarget() even though it has never observed the commanded move begin or settle—the exact small-nudge case that is supposed to remain operator-controlled. Require a post-command disqualifying transition before incrementing this counter, or otherwise exclude slews below the threshold.
Useful? React with 👍 / 👎.
| this->broadcast.notice( function, message.str() ); | ||
| this->ontarget(); |
There was a problem hiding this comment.
If an abort sets cancel_flag after the loop condition was checked—for example while the second ?ONTARGET request is in flight—this path still calls ontarget(), whose implementation clears cancel_flag. The waiting sequence can then observe is_ontarget rather than the abort and continue into acquisition or exposure. Recheck auto_ontarget_active and cancel_flag immediately before acknowledging, and ensure the automatic path cannot clear a concurrently issued cancellation.
Useful? React with 👍 / 👎.
| double tgt_ra_h = radec_to_decimal( this->target.ra_hms ); // decimal hours | ||
| double tgt_dec_d = radec_to_decimal( this->target.dec_dms ); // decimal degrees |
There was a problem hiding this comment.
For targets using a focal-plane point mode such as SLIT or ACAM, these database coordinates are transformed by compute_offset(pointmode, "SCOPE", ...), and move_to_target() sends the resulting ra_out/dec_out to the TCS. REQPOS telemetry describes that scope position, so comparing it with the untransformed input can leave the measured separation larger than the default 5 arcsec and prevent auto-ontarget from ever succeeding. Capture and compare against the commanded scope coordinates instead.
Useful? React with 👍 / 👎.
| double dra_as = ( this->tcsinfo.ra_h - tgt_ra_h ) * 15.0 | ||
| * cos( tgt_dec_d * M_PI / 180.0 ) * 3600.0; |
There was a problem hiding this comment.
For a target near 00:00 whose measured position falls just across the 24-hour boundary, subtracting the hour values directly produces a delta close to 24 hours rather than a sub-arcsecond delta. The separation test consequently never passes even when the telescope is on target, leaving these observations dependent on the manual acknowledgment. Wrap the RA difference into the shortest signed interval before converting it to arcseconds.
Useful? React with 👍 / 👎.
9a91093 to
0451b1f
Compare
|
All four review findings addressed in the amended commit:
🤖 Generated with Claude Code |
0451b1f to
9f73642
Compare
?ONTARGET was invisible through tcsd because send_command translates
every non-whitelisted reply as a status code: the flag IS the reply, so
0 collided with TCS_SUCCESS ('success') and 1 fell through to
'tcs_undefined' ERROR. Whitelist it for verbatim passthrough.
sequencerd: config TCS_AUTO_ONTARGET (default no) spawns a watcher for
exactly the lifetime of move_to_target's TCSOP wait. It calls
ontarget() -- the operator's own path -- when, on two consecutive 1 Hz
reads: ?ONTARGET==1 AND motion==tracking AND |telescope-target| <
TCS_AUTO_ONTARGET_SEP arcsec. All three are required: shadow data
(UT 2026-08-25, 64 slews) shows the flag is position-agnostic, the
separation cannot see sub-threshold hops, and motion says nothing
about settle. Operator ack trailed the flag by median +9.7 s / p90
+18 s and never once preceded it. The operator's press is unchanged;
whichever arrives first wins.
9f73642 to
4b2bfcf
Compare
…ply flexure at exposure start and between sub-exposures Two behaviour changes for the 2026-10-21 test, both in the sequence loop. 1. Early slew. When the last sub-exposure of a target begins reading out, the next pending row is looked at (TargetInfo::peek_next, a read that writes no TargetInfo field and publishes nothing) and its COORDS + ring angle are sent to the TCS exactly as move_to_target would send them. The target on sky is untouched: its TargetInfo keeps its identity for the TARGET_COMPLETE/insert_completed writes, no TARGETINFO is published until the loop takes the next row itself, and camerad has already taken every telescope-derived header value (telemetry at exposure start, AIRMASS1/MJD1 at shutter close) before the first PIXELCOUNT that triggers this. move_to_target() consumes the early slew only when the row it is given matches it exactly (OBSERVATION_ID, name, RA, DEC, slit angle) and then runs only the ontarget wait; anything else -- reordered queue, edited row, retry at an offset star -- slews normally. Not sent in single-step, single-target, TCS-disconnected, no-pending, calibration, no-coords, nexp-0, ACAM-pointmode or repeat-pointing cases. The record is cleared on every sequence_start entry and exit. Readout starts are counted per sub-exposure: a camerad status with SHUTTEROPEN=true marks a sub-exposure and the first async PIXELCOUNT after it marks its readout. Open reports only (a paused/resumed exposure opens twice, reads out once); PIXELCOUNT never precedes a shutter close; a lost status publish can only lose an early slew, never issue one early. #483's auto-ontarget watcher is started with the early COORDS and carries a launch generation so a watcher for coordinates the loop then did not take retires when a new one starts. is_ontarget is cleared right before every COORDS send so only an acknowledgment of that move satisfies its wait. 2. Flexure timing. flexure_set leaves the pre-slew worker set (where flexured compensated for the PREVIOUS target's pointing, ~4 min before the shutter opened on 2026-10-19) and runs after acquisition and the target offset, alongside the slit move and offset settle, joined before the expose command; failure semantics unchanged. For NEXP>1 the compensation is re-issued during each non-final readout (flexure_reapply, never fatal), so every sub-exposure starts from the telescope position at that moment. Config templates: ACQUIRE_OFFSET_THRESHOLD=1.0 (acamd) and ACQUIRE_TIMEOUT=150 (sequencerd) are the values running on the summit since October as hand edits of the generated .cfg files; written into the .cfg.in so a rebuild from this branch keeps them. Not compiler-checked (per CF; done on the test night). Co-Authored-By: Claude Fable 5.1 <[email protected]>
…out review fixes
NEXP>1 stop and abort (CF: "fix abort / stop for exposure sequences with
NEXP > 1"; see the 2026-08-13 power-cycle night):
- sequencerd stop_exposure (seq stop, and abort's first step) now reaches
camerad for the whole series. It was gated on SEQ_WAIT_EXPOSE, which only
covers sub-exposure 1; it now also runs under SEQ_WAIT_READOUT.
- camerad stop ends an NEXP>1 series after the frame in progress whatever
that frame is doing (nexp=1): integrating with >= 5 s left it is also cut
short as before; with < 5 s left, reading out, or between sub-exposures,
it finishes and nothing follows ("series_ends", NO_ERROR). A single
exposure behaves as before.
- camerad abort no longer starts the next sub-exposure: do_abort sets
nexp=0 before releasing exposure_pending, which dothread_monitor_exposure_
pending used to read as a finished frame and continue the series.
- camerad do_expose stores nexp only after its "exposure already pending"
refusal, and not at all for the series' own continuation call, so a stray
expose cannot change a running series and a continuation cannot undo a
stop that lowered nexp.
- An abort during the readout wait no longer marks an unfinished series
complete: unless every sub-exposure's integration had ended, the target is
set unassigned, exactly like an abort during the exposure wait.
Auto-ontarget (#483) also requires the cass ring at the commanded angle (CF:
"both slew and cass motion must be done before ontarget"):
|TCS cass angle - RINGGO angle| < TCS_AUTO_ONTARGET_CASS (0.05 deg) on the
same two consecutive reads. On 2026-10-19 all 73 settled rings were within
0.022 deg of the command (median 0.001). ACAM's goal is computed from the
cass angle at the acquire, ~8.5" per degree of ring still to go.
Review fixes to the pick at readout start (9110904):
- camerad waits up to 2 s for a finishing FITS write before refusing
`bin` or `buffer` (wait_writes_done): READY precedes the end of the last
write by 3-29 ms, so a camera_set right after READY -- the 1x1 case where
the next target waits at the gate -- was refused and the target stranded.
- camera_set runs again in parallel (a channel power-up takes ~7.5 s),
behind the readout gate, joined just before the exposure.
- A row that waits for the gate (calibration, no coordinates, ACAM) is set
active and published first, so a calibration rebuild cannot delete it
from under the sequencer.
- A calibration is never completed early: no pick when the current row is
a calibration, so calibration blocks run exactly as before.
- A PIXELCOUNT from the second half of a readout (the tail of the previous
target's readout) cannot end the next target's EXPOSE wait.
slicecamd: every fine-acquire lock writes the same "fine acquisition
converged: offset dRA=..." line as before, with how it locked appended
(single frame after a correction; the budget's final correction). The
published lock flag never changed.
Not compiler-checked (per CF; done on the test night).
Co-Authored-By: Claude Opus 5.5 <[email protected]>
This branch has not been deployed
1 participant
Reference implementation from on-sky measurement — offered as the working example for the preferred sequencerd implementation; adapt freely.
What the operator's press costs, measured
Every science slew ends with sequencerd blocking on the TCS operator's
manual "ontarget" press (
move_to_target, SEQ_WAIT_TCSOP). A shadow rig ran?ONTARGETat 1 Hz through a whole night (UT 2026-08-25, 64 slews, 59 witha clean 0→1 transition) alongside the operator's presses:
median +9.7 s, p90 +18 s, max +31.5 s — and never once preceded it
(0 of 59);
coordinates to median 0.4″;
To get this flag through tcsd
?ONTARGET's reply IS the flag (0/1), butTCS::Interface::send_commandtranslates every reply that is not on its information-command whitelist
through
parse_reply_code:0collides withTCS_SUCCESSand renders"success";
1is no valid status code and renders "tcs_undefined" ERROR.The code here properly exposes the flag status.
The change (120 lines, all opt-in)
tcsd — add
?ONTARGETtosend_command's verbatim whitelist(tcs_interface.cpp, 1 line + comment).
tcs native '?ONTARGET'nowreturns
0/1.sequencerd — config
TCS_AUTO_ONTARGET(defaultno: deployedbehaviour is byte-identical until someone flips it). When enabled,
move_to_targetspawnsdothread_auto_ontargetfor exactly the lifetimeof the TCSOP wait (an
auto_ontarget_activeatomic brackets it). Thewatcher acknowledges through the same condition variable the operator's
press uses — whichever arrives first wins — when two consecutive 1 Hz
reads satisfy all three of:
A wrong acknowledgment is deliberately treated as self-limiting: ACAM
cannot solve a field that is not there, and the acquisition retry
machinery handles the failure — so these conditions exist to make wrong
acks rare, not impossible, and no operator press is expected in this
mode. The comparison is against the SCOPE-frame coordinates
move_to_targetactually commanded (the database coordinates differ bythe pointmode focal-plane offset), with the RA delta wrapped to the
shortest arc. The acknowledgment deliberately does not go through
ontarget(), which clearscancel_flag: a concurrent cancel alwayswins against the automatic path (re-checked at the last instant, and
never cleared by it).
All three are required because each alone is measured to fail:
previous field (observed at 25–57° separation);
still in progress (the offset-star expose race);
On nearby offset-star acquisitions (~2.7′) all three signals transition cleanly
(flag drops to 0 for ~10 s, motion reads "offsetting"); for nudges below
a few arcsec nothing transitions and the watcher simply never fires —
the operator (or the existing settle logic) covers that case unchanged.
The debounce covers two observed settle bounces (1→0→1) from the logged night.
Config template
sequencerd.cfg.in: the two keys, default off.What does NOT change
TCS_AUTO_ONTARGET=no(the default): nothing. The watcher is neverspawned; the whitelist line only affects the reply text of a query
nothing currently issues.
the same
ontarget()and either satisfies the wait.cancel_flag, onis_ontarget, andwhen the wait closes (
auto_ontarget_activefalse) — it cannot outlivethe wait or fire into a later phase.
Validation plan before enabling
The shadow rig keeps running nightly; each night appends ~60 more
slews of (flag, motion, separation, operator) tuples. Enable on-sky only
after N nights show zero would-have-fired-early cases against the operator
baseline. First enable with the operator still watching: the broadcast
notice ("TCS reports on-target … sending ontarget") makes every automatic
ack visible in the sequencer log and GUI.
🤖 Generated with Claude Code