Tecan spark by xbtu2 · Pull Request #798 · PyLabRobot/pylabrobot

xbtu2 · 2025-12-21T01:33:06Z

tecan spark WIP. luminescence coming up soon. i had a hard time to get io.usb to work with this, so i'm directly using libusb for now

hazlamshamin · 2025-12-21T05:56:39Z

CRAZY COOOOL! this is VERY complete

Copilot

Pull request overview

This PR adds support for the Tecan Spark plate reader by implementing a comprehensive async backend with USB communication, packet parsing, and data processing capabilities. The implementation includes controls for various instrument subsystems and processors for absorbance and fluorescence measurements.

Key Changes

New async USB reader with packet parsing for Tecan Spark devices
Absorbance and fluorescence data processors with real-world test coverage
Comprehensive control modules for plate transport, optics, sensors, measurements, and system configuration
Utility function for calculating non-overlapping well rectangles

Reviewed changes

Copilot reviewed 23 out of 24 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pylabrobot/plate_reading/utils.py	Adds helper to find non-overlapping rectangles covering plate wells
pylabrobot/plate_reading/tecan/spark20m/spark_reader_async.py	Implements async USB communication layer for Spark devices
pylabrobot/plate_reading/tecan/spark20m/spark_packet_parser.py	Parses binary measurement data packets from the device
pylabrobot/plate_reading/tecan/spark20m/spark_processor.py	Processes raw measurement data into absorbance/fluorescence values
pylabrobot/plate_reading/tecan/spark20m/spark_backend.py	Main backend integrating all components for plate reading operations
pylabrobot/plate_reading/tecan/spark20m/controls/*.py	Control modules for device subsystems (optics, sensors, movement, etc.)
pylabrobot/plate_reading/tecan/spark20m/*_tests.py	Unit tests for reader, processor, and backend components

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pylabrobot/plate_reading/tecan/spark20m/controls/system_control.py

pylabrobot/plate_reading/tecan/spark20m/spark_reader_async.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

rickwierenga · 2026-01-21T00:30:42Z

is this ready for review?

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

- Remove AutoReaderProxy, integrate reading into send_command - Remove reading context manager (no longer needed) - Rename control classes to follow PEP8 (BaseControl, PlateControl, etc.) - BaseControl now takes send_command directly instead of reader - Make init_read and get_response private methods - Add io.binary.Reader utility class Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

rickwierenga · 2026-01-31T18:49:05Z

Code review changes

Simplified the Spark control architecture:

Removed AutoReaderProxy - This was a magic proxy that wrapped every async method call with a reading context. Instead, the reading logic is now integrated directly into send_command(), making the flow more explicit and easier to follow.
Removed reading() context manager - No longer needed since send_command() handles read setup internally.
Renamed control classes to follow PEP8 - baseControl → BaseControl, plateControl → PlateControl, measurement_control → MeasurementControl, etc.
Simplified BaseControl - Now takes send_command function directly instead of the entire reader object, since that's all it uses.
Made internal methods private - init_read() → _init_read(), get_response() → _get_response() since they're only used internally by send_command().
Added io.binary.Reader - Utility class for binary parsing, used to replace the custom _read_u8, _read_u16, etc. helper functions in the packet parser.

# Conflicts: # pylabrobot/io/binary.py # pylabrobot/io/usb.py

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Replace custom byte parsing helpers (_read_bytes, _read_u8, _read_u16, _read_u32, _read_string) with the Reader class from pylabrobot.io.binary. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Use pytest.approx() instead of exact equality for float comparisons in test_process_real_data tests. Compare row by row since pytest.approx does not support nested data structures. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Add max_workers parameter to USB class constructor with default of 1 to maintain backward compatibility. SparkReaderAsync passes max_workers=16 for its specific needs. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

The PR fixed the timeout conversion for the main write but missed the zero-length packet write. PyUSB expects timeout in milliseconds, so timeout must be multiplied by 1000. Also add max_workers parameter to USBValidator for signature compatibility. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Replace manual calculation of well spacing with the new item_dx and item_dy properties from ItemizedResource. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

pylabrobot/plate_reading/tecan/spark20m/spark_reader_async.py

rickwierenga · 2026-02-01T04:09:26Z

instead of returning False on error, can we raise the error in send command? this is a more pythonic way of dealing with errors

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

- Remove unnecessary .keys() calls in dictionary iteration - Use None instead of -1 as sentinel value with is/is not checks - Remove unnecessary list() conversions when iterating bytes - Use statistics.mean() for average temperature calculation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Methods like set_camera_aoi, set_bpp, set_cam_gain were duplicates of set_camera_area_of_interest, set_camera_bits_per_pixel, set_camera_gain. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

The parameter is actually used in scan_plate_range call. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

DARK_BLOCK_INDEX, REFERENCE_BLOCK_INDEX, and UINT16_MAX make the calibration block indices and ADC resolution self-documenting. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

rickwierenga · 2026-02-05T00:00:55Z

get_empty_position_wavelength_limit_lower and get_empty_position_wavelength_limit_upper have identical implementations:

https://github.com/PyLabRobot/pylabrobot/blob/tecan_spark/pylabrobot/plate_reading/tecan/spark20m/controls/optics_control.py#L152-L172

Both send ?CONFIG FILTER=EMISSION TYPE=EMPTY. Should one of them include a LIMIT=LOWER or LIMIT=UPPER parameter?

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Cancel response_task in finally block if an exception occurs before it completes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

TemperatureDevice, TemperatureState, and ChillerState were defined in both files. Keep the ones in spark_enums (PascalCase) and import. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Log when all ratios are NaN or when avg_ratio is non-positive, instead of silently returning NaN. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Helps debug malformed packets from USB corruption or firmware issues. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

rickwierenga · 2026-02-05T00:37:56Z

FluorescenceCarrier has duplicate enum values

class FluorescenceCarrier(Enum):
    MONOCHROMATOR_EXCITATION = "MONO"
    MONOCHROMATOR_EMISSION = "MONO"  # Same value as above

Both MONOCHROMATOR_EXCITATION and MONOCHROMATOR_EMISSION map to "MONO". This will cause issues with reverse lookups (e.g., FluorescenceCarrier("MONO") will always return MONOCHROMATOR_EXCITATION).

If this is intentional because the hardware uses the same identifier for both, consider documenting why. Otherwise, one of these values may be incorrect.

xbtu2 · 2026-02-05T02:34:57Z

FluorescenceCarrier has duplicate enum values
class FluorescenceCarrier(Enum):
    MONOCHROMATOR_EXCITATION = "MONO"
    MONOCHROMATOR_EMISSION = "MONO"  # Same value as above
Both MONOCHROMATOR_EXCITATION and MONOCHROMATOR_EMISSION map to "MONO". This will cause issues with reverse lookups (e.g., FluorescenceCarrier("MONO") will always return MONOCHROMATOR_EXCITATION).

If this is intentional because the hardware uses the same identifier for both, consider documenting why. Otherwise, one of these values may be incorrect.

merged two enums to MONOCHROMATOR. I don't see the need to distinguish them

xbtu2 · 2026-02-05T02:35:30Z

get_empty_position_wavelength_limit_lower and get_empty_position_wavelength_limit_upper have identical implementations:

https://github.com/PyLabRobot/pylabrobot/blob/tecan_spark/pylabrobot/plate_reading/tecan/spark20m/controls/optics_control.py#L152-L172

Both send ?CONFIG FILTER=EMISSION TYPE=EMPTY. Should one of them include a LIMIT=LOWER or LIMIT=UPPER parameter?

fixed with correct command strings

The cancel() call only requests cancellation - the task continues running until the next await point. This change awaits the task after cancellation to ensure proper cleanup and prevent resource leaks. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

The previous implementation used an attempt counter (default 10000) which could spin for 100+ seconds with 10ms sleeps. Now uses time.monotonic() to enforce a wall-clock deadline (default 60 seconds), making the timeout behavior predictable and bounded. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Use _safe_div() for the (raw_ref_signal - ref_dark) denominator to return NaN instead of raising ZeroDivisionError when reference signal equals reference dark value. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

MirrorCarrier, LaserPowerState, and LightingState were already defined in spark_enums.py. Import them instead of redefining. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

…trol These enums are already defined in spark_enums.py. Import them instead. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

- Add explicit None check for inner_mult_index in _parse_nested_mult call - Import ScanDirection from spark_enums instead of re-exporting through measurement_control Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

rickwierenga · 2026-02-05T20:00:56Z

is it a problem that DataControl is not complete yet? I'm fine either way

xbtu2 · 2026-02-06T23:20:05Z

how does it look now?

xbtu2 added 5 commits December 20, 2025 12:23

tecan spark backend

b132be7

use enum for endpoints

fbec36f

add time

16edc58

format

ad9843d

fix typo

a799855

BioCam requested a review from Copilot December 21, 2025 07:33

Copilot AI reviewed Dec 21, 2025

View reviewed changes

pylabrobot/plate_reading/tecan/spark20m/controls/system_control.py Outdated Show resolved Hide resolved

pylabrobot/plate_reading/tecan/spark20m/spark_reader_async.py Outdated Show resolved Hide resolved

xbtu2 and others added 6 commits December 21, 2025 11:24

Update pylabrobot/plate_reading/tecan/spark20m/spark_reader_async.py

4190426

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

remove unused method

c937c24

use plr usb io

7560ed3

Merge branch 'main' into tecan_spark

1e2c30a

fix test

0c99649

Merge branch 'main' into tecan_spark

eaf2f3b

rickwierenga and others added 2 commits January 31, 2026 10:18

Use io.binary.Reader in spark_packet_parser

24ec263

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Merge remote-tracking branch 'origin/main' into tecan_spark

b56ffdd

# Conflicts: # pylabrobot/io/binary.py # pylabrobot/io/usb.py

rickwierenga force-pushed the tecan_spark branch from fc9cf36 to b56ffdd Compare January 31, 2026 19:20

rickwierenga and others added 7 commits January 31, 2026 11:27

Rename control attributes to include _control suffix

aaa6fae

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Refactor spark_packet_parser.py to use binary.py Reader

9747549

Replace custom byte parsing helpers (_read_bytes, _read_u8, _read_u16, _read_u32, _read_string) with the Reader class from pylabrobot.io.binary. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Make USB max_workers configurable per-instance

2e1db3c

Add max_workers parameter to USB class constructor with default of 1 to maintain backward compatibility. SparkReaderAsync passes max_workers=16 for its specific needs. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Merge remote-tracking branch 'origin/main' into tecan_spark

31949fa

Use item_dx and item_dy properties in SparkBackend

847d895

Replace manual calculation of well spacing with the new item_dx and item_dy properties from ItemizedResource. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

rickwierenga reviewed Jan 31, 2026

View reviewed changes

pylabrobot/plate_reading/tecan/spark20m/spark_reader_async.py Outdated Show resolved Hide resolved

fix send_commend and tests

e95d88d

rickwierenga and others added 7 commits February 4, 2026 15:23

Use statistics.mean() and remove unnecessary * 1.0

7cefe4e

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Use reversed(range()) instead of range(n-1, -1, -1)

e5de32f

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Remove 30 duplicate abbreviated methods from CameraControl

a0d970f

Methods like set_camera_aoi, set_bpp, set_cam_gain were duplicates of set_camera_area_of_interest, set_camera_bits_per_pixel, set_camera_gain. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Make _format_scan_range a staticmethod

8e55bab

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Remove misleading '# Unused' comment on focal_height parameter

217887d

The parameter is actually used in scan_plate_range call. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Replace magic numbers with named constants in spark_processor

461c0ed

DARK_BLOCK_INDEX, REFERENCE_BLOCK_INDEX, and UINT16_MAX make the calibration block indices and ADC resolution self-documenting. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

rickwierenga and others added 6 commits February 4, 2026 16:25

Add division by zero protection in fluorescence K calculation

8181847

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Fix potential task leak in send_command

e4d0322

Cancel response_task in finally block if an exception occurs before it completes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Remove duplicate enums from sensor_control, import from spark_enums

b6a779d

TemperatureDevice, TemperatureState, and ChillerState were defined in both files. Keep the ones in spark_enums (PascalCase) and import. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Add device context to background read error messages

39a7e8b

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Add warnings when absorbance ratios are invalid

19c6463

Log when all ratios are NaN or when avg_ratio is non-positive, instead of silently returning NaN. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Include hex dump in packet parse error messages

cc2fab3

Helps debug malformed packets from USB corruption or firmware issues. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

fix command strings and enums

97750cb

xbtu2 and others added 7 commits February 4, 2026 20:19

Merge branch 'main' into tecan_spark

09d4549

Protect fluorescence RFU calculation from division by zero

ab79ec1

Use _safe_div() for the (raw_ref_signal - ref_dark) denominator to return NaN instead of raising ZeroDivisionError when reference signal equals reference dark value. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Remove duplicate enum definitions from optics_control.py

707f5b1

MirrorCarrier, LaserPowerState, and LightingState were already defined in spark_enums.py. Import them instead of redefining. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Remove duplicate ScanDirection and ScanDarkState from measurement_con…

f78e07d

…trol These enums are already defined in spark_enums.py. Import them instead. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Fix type annotation issues for mypy --strict

6d74c4d

- Add explicit None check for inner_mult_index in _parse_nested_mult call - Import ScanDirection from spark_enums instead of re-exporting through measurement_control Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

xbtu2 added 2 commits February 6, 2026 14:44

refactor io/usb.py

6179ef9

fix duplicate commands

8c147f4

fix import order

77927c9

Conversation

xbtu2 commented Dec 21, 2025

Uh oh!

hazlamshamin commented Dec 21, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

rickwierenga commented Jan 21, 2026

Uh oh!

rickwierenga commented Jan 31, 2026

Code review changes

Uh oh!

Uh oh!

rickwierenga commented Feb 1, 2026

Uh oh!

rickwierenga commented Feb 5, 2026

Uh oh!

rickwierenga commented Feb 5, 2026

FluorescenceCarrier has duplicate enum values

Uh oh!

xbtu2 commented Feb 5, 2026

FluorescenceCarrier has duplicate enum values

Uh oh!

xbtu2 commented Feb 5, 2026

Uh oh!

rickwierenga commented Feb 5, 2026

Uh oh!

xbtu2 commented Feb 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants