Reflector focus - Focus maintenance device based on a camera, shutter and stage#803
Reflector focus - Focus maintenance device based on a camera, shutter and stage#803nicost merged 10 commits intomicro-manager:mainfrom
Conversation
…in a now defunct branch).
…where to avoid future confusion.
There was a problem hiding this comment.
Pull request overview
This PR extracts a ReflectionFocus autofocus device adapter from the Utilities device adapter to improve maintainability. The adapter implements hardware-based reflection spot tracking for autofocus using a camera, shutter, and Z-stage. It provides two devices: the main ReflectionFocus autofocus device and a ReflectionFocus-Stage that exposes the autofocus offset as a standard Z-stage interface.
Key changes:
- New standalone ReflectionFocus device adapter with autofocus and stage wrapper devices
- Calibration system supporting multiple device settings with persistence to JSON
- Continuous focus thread for real-time focus maintenance
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| micromanager.sln | Adds ReflectionFocus project to Visual Studio solution |
| DeviceAdapters/configure.ac | Registers ReflectionFocus in autotools build system |
| DeviceAdapters/ReflectionFocus/license.txt | BSD license for the new adapter |
| DeviceAdapters/ReflectionFocus/ReflectionFocusStage.cpp | Stage wrapper that exposes autofocus offset as Z-stage |
| DeviceAdapters/ReflectionFocus/ReflectionFocusModule.cpp | Module initialization and device factory |
| DeviceAdapters/ReflectionFocus/ReflectionFocus.vcxproj.filters | Visual Studio project filters |
| DeviceAdapters/ReflectionFocus/ReflectionFocus.vcxproj | Visual Studio project configuration |
| DeviceAdapters/ReflectionFocus/ReflectionFocus.h | Header with class declarations and error codes |
| DeviceAdapters/ReflectionFocus/ReflectionFocus.cpp | Main implementation with image analysis and calibration |
| DeviceAdapters/ReflectionFocus/Makefile.am | Unix build configuration (contains critical errors) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 24 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else if ((pos = line.find("\"description\"")) != std::string::npos) | ||
| { | ||
| size_t colonPos = line.find(":", pos); | ||
| if (colonPos != std::string::npos) | ||
| { | ||
| size_t quotePos1 = line.find("\"", colonPos + 1); | ||
| if (quotePos1 != std::string::npos) | ||
| { | ||
| size_t quotePos2 = line.find("\"", quotePos1 + 1); | ||
| if (quotePos2 != std::string::npos) | ||
| { | ||
| currentCal.description = line.substr(quotePos1 + 1, quotePos2 - quotePos1 - 1); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The JSON parsing for string fields (spotSelection and description) doesn't handle escaped quotes within the string value. If the description contains escaped quotes (\"), the parsing will incorrectly find the first quote character as the end of the string. This could cause data corruption when loading calibration data. Consider improving the JSON parsing to handle escaped characters properly.
| double ReflectionFocus::CalculateTargetZDiff(const CalibrationData& cal, double spotX, double spotY) | ||
| { | ||
| const double MIN_SLOPE = 1e-6; // Threshold for effectively zero slope | ||
| const double INVALID_Z = -1.0e10; // Sentinel value | ||
|
|
||
| bool xValid = fabs(cal.slopeX) > MIN_SLOPE; | ||
| bool yValid = fabs(cal.slopeY) > MIN_SLOPE; | ||
|
|
||
| if (!xValid && cal.dominantAxis == 'X') | ||
| return INVALID_Z; // slope too small - unusable calibration | ||
| if (!yValid && cal.dominantAxis == 'Y') | ||
| return INVALID_Z; // slope too small - unusable calibration | ||
|
|
||
| if (cal.dominantAxis == 'X') | ||
| return -(spotX - cal.refSpotX) / cal.slopeX; | ||
| else if (cal.dominantAxis == 'Y') | ||
| return -(spotY - cal.refSpotY) / cal.slopeY; | ||
|
|
||
| // error | ||
| return INVALID_Z; | ||
| } |
There was a problem hiding this comment.
The function returns INVALID_Z (-1.0e10) in error cases, but the caller in FullFocus and ContinuousFocusThread uses this value directly without checking if it's valid. This could cause the stage to attempt to move to an extremely negative position. Add validation in the calling code to check for this sentinel value and handle it as an error.
|
|
||
| // Step 3: Calculate Z correction | ||
| double diffZ = CalculateTargetZDiff(cal, lastSpotX_, lastSpotY_); | ||
| diffZ += offset_; |
There was a problem hiding this comment.
The offset_ member variable is accessed without mutex protection. It's read in the continuous focus thread (line 858) and can be modified via SetOffset (line 656) which could be called from another thread. While offset_ is a double (which might be atomic on some platforms), it's not guaranteed to be thread-safe across all platforms. Consider using std::atomic<double> or protecting access with a mutex.
…sure that light and dark image have the same dimensions.
This was previously developed as part of the Utilties device adapter. Split out to make things more maintainable.