Skip to content

main_hcs: start converting code reachable via main_hcs.py to use native python logging instead of print - #2

Merged
ianohara merged 5 commits into
ian-logging-toolsfrom
ian-main-hcs-logging
Oct 25, 2024
Merged

main_hcs: start converting code reachable via main_hcs.py to use native python logging instead of print#2
ianohara merged 5 commits into
ian-logging-toolsfrom
ian-main-hcs-logging

Conversation

@ianohara

@ianohara ianohara commented Oct 24, 2024

Copy link
Copy Markdown
Collaborator

This will be done in a bunch of pull requests so they are manageable, but the goal is to convert all our print to Logger.* calls. This will allow us to send logs to multiple places easily (eg: stdout and a file), change log levels depending on destination (eg: only error to stdout, all to file), and generally make logging more useful in the repo.

Tested by: Running python main_hcs.py --simulation and making sure it runs as expected.

@ianohara
ianohara merged commit ebe78e2 into ian-logging-tools Oct 25, 2024
ianohara added a commit that referenced this pull request Oct 25, 2024
@ianohara
main_hcs: start converting code reachable via main_hcs.py to use nati…
8839798 
…ve python logging instead of print (#2)

This will be done in a bunch of pull requests so they are manageable,
but the goal is to convert all our `print` to `Logger.*` calls. This
will allow us to send logs to multiple places easily (eg: stdout and a
file), change log levels depending on destination (eg: only error to
stdout, all to file), and generally make logging more useful in the
repo.

Tested by: Running `python main_hcs.py --simulation` and making sure it
runs as expected.

---------

Co-authored-by: Ian OHara <ianohara+git@gmail.com>
hongquanli added a commit that referenced this pull request Dec 30, 2025
@hongquanli @claude
fix: Address review comments #2, #3, #5
e04deb3 
- Add comment clarifying SET_LIM uses limit codes instead of axis IDs
- Add comment noting MERGIN typo is intentional to match firmware
- Replace defensive padding/truncation with assertion for MSG_LENGTH

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
hongquanli added a commit that referenced this pull request Dec 30, 2025
@hongquanli @claude
fix: Address review comments #2, #3, #4
cd43686 
- Add byte count breakdown comment for struct format ">BBiiiiBi"
- Update platformio.ini to use +<src/utils/*.cpp> pattern for future modules
- Skip axis validation if valid_axes is empty (missing firmware constants)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@hongquanli hongquanli mentioned this pull request Dec 31, 2025
Merged
hongquanli added a commit that referenced this pull request Jan 28, 2026
@hongquanli @claude
fix: Add port validation and consistent wait behavior
117c04c 
Address code review issues #1 and #2:

1. Add port index validation (0-15) to Microcontroller methods:
   - set_port_intensity, turn_on_port, turn_off_port, set_port_illumination
   - Raises ValueError for out-of-range ports, TypeError for non-integers

2. Add wait_till_operation_is_completed() to IlluminationController.set_port_intensity()
   - Makes behavior consistent with turn_on_port() and turn_off_port()
   - Prevents race conditions when setting intensity before turning on

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
hongquanli added a commit that referenced this pull request Mar 24, 2026
@hongquanli @claude
fix: address PR review comments on thread safety tests
6e13063 
- Use SCREW_PITCH_W_MM and config bounds for step_size calculation
  instead of hardcoded 1.0/8 (comment #1)
- Add assert not t.is_alive() after join(timeout=...) to fail
  deterministically on deadlock (comment #2)
- Assert call_order sequence is non-interleaving, accounting for the
  offset move within _home_wheel_unlocked (comment #3)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Alpaca233 added a commit that referenced this pull request Apr 27, 2026
@Alpaca233 @claude
fix(plate-mode): clear plate canvas when slot dims or well coverage c…
424e136 
…hanges

Bug: clicking Start Acquisition a second time made the plate view show wells
at a different grid position than before. Root cause: the channel layer's
canvas was allocated by _create_channel_layer on the first tile of run #1
and reused as-is for run #2, but a new run can produce a different
well_slot_shape (max-extent over scanned wells) or scan a different subset
of wells — both shift where new tiles compute their slot offsets, while the
canvas geometry from run #1 has not moved.

Fix: setPlateLayout now wipes existing channel canvases to fresh zero-filled
plate-sized arrays whenever the new slot dimensions don't match the current
canvas, OR the set of wells in the new run differs from the previous one.
For re-runs of the *same* wells with the *same* extent, the canvas is
preserved (existing tiles stay visible until overwritten by new ones).

Plumbing changes:
- PlateViewInit gains a `well_ids: List[str]` field.
- multi_point_worker._emit_plate_layout populates it from
  scan_region_fov_coords_mm.
- gui_hcs.MultiPointController.plate_view_init Qt signal carries the new list.
- UnifiedMosaicWidget.setPlateLayout accepts well_ids, compares against the
  cached _plate_well_ids frozenset, and zeroes affected layers when the
  comparison shows coverage changed (or dims changed).
- Plate boundary layer is dropped so it gets redrawn at the new slot scale.

Tests: 1132 passed, 5 skipped, 1 xfailed. Smoke test clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
hongquanli added a commit that referenced this pull request May 18, 2026
@hongquanli
address copilot review feedback: ack-before-retry, helper split, hear…
8fc1e78 
…tbeat-skip, etc.

Addresses six issues from the copilot-pull-request-reviewer bot review:

#1 (cephla.py): Call acknowledge_aborted_command() after catching
CommandAborted (and after the inner resend failure if it was also a
CommandAborted), so the next send_command doesn't log the spurious
"Last command aborted and not cleared before new command sent!"
warning. The inner ack is gated on isinstance(e2, CommandAborted) to
avoid the "ack with nothing to ack" path on TimeoutError.

#2 (cephla.py): Drop the redundant `target_usteps = ...` recompute
after _home_wheel. config and target_pos haven't changed and
_target_pos_to_usteps doesn't depend on current_pos.

#3 (cephla.py): Fix _home_wheel docstring — wheel is driven to
config.min_index (typically slot 1), not "slot 0".

#5 (firmware/serial_communication.cpp): Skip the
`mcu_cmd_execution_status = COMPLETED_WITHOUT_ERRORS` reset when
processing a HEARTBEAT. The keepalive has no result to report, and
resetting would clobber a pending CMD_EXECUTION_ERROR from the
previous command if the broadcast hasn't fired yet. Eliminates the
narrow race where heartbeat traffic interleaves a failure broadcast.

#6 (firmware/stage_commands.cpp): Split mark_move_failed() into two
helpers — mark_move_failed() (for paths that already set
mcu_cmd_execution_in_progress = true) and report_move_error() (for
early-return paths that didn't). The !enabled branch in
dispatch_filterwheel_move now uses report_move_error() so it doesn't
spuriously unwind in_progress for an unrelated motion in flight on
another axis. Invariant: only the function that claimed in_progress
gets to clear it.

#7 (test_filter_wheel.py): Add `getattr(mc, move_rel_attr).assert_not_called()`
to both parametrized CommandAborted/TimeoutError tests, so the
absolute-MOVETO recovery path is enforced — fall-back to relative
MOVE would now be caught.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ianohara