Reflector focus - Focus maintenance device based on a camera, shutter and stage #803

nicost · 2025-12-18T18:53:48Z

This was previously developed as part of the Utilties device adapter. Split out to make things more maintainable.

…in a now defunct branch).

…where to avoid future confusion.

Copilot

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.

DeviceAdapters/ReflectionFocus/ReflectionFocusStage.cpp

DeviceAdapters/ReflectionFocus/ReflectionFocus.cpp

DeviceAdapters/ReflectionFocus/Makefile.am

DeviceAdapters/ReflectionFocus/ReflectionFocus.cpp

DeviceAdapters/ReflectionFocus/Makefile.am

DeviceAdapters/ReflectionFocus/ReflectionFocus.cpp

…check during calibration).

Copilot

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.

Copilot · 2025-12-27T00:12:55Z

DeviceAdapters/ReflectionFocus/ReflectionFocus.cpp

+   // Snap image with shutter open
+   camera->SnapImage();
+   ImgBuffer lightImage;
+   ret = GetImageFromBuffer(lightImage);


The return value 'ret' from GetImageFromBuffer is not checked. If the function fails, lightImage may be in an invalid state, but the code continues to use it for subtraction. Check the return value and handle errors appropriately.

Copilot · 2025-12-27T00:12:55Z

DeviceAdapters/ReflectionFocus/ReflectionFocus.cpp

+      ret = GetImageFromBuffer(darkImageLoop);
+
+      // Open shutter and take light image
+      shutter->SetOpen(true);
+      CDeviceUtils::SleepMs(10);
+      camera->SnapImage();
+      ImgBuffer lightImageLoop;
+      ret = GetImageFromBuffer(lightImageLoop);


The return value 'ret' from GetImageFromBuffer is not checked in the calibration loop. If any image capture fails, the calibration data will be corrupted but the code continues. Check the return value and handle errors appropriately.

Copilot · 2025-12-27T00:12:56Z

DeviceAdapters/ReflectionFocus/ReflectionFocus.cpp

+      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);
+               }
+            }
+         }


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.

Copilot · 2025-12-27T00:12:56Z

DeviceAdapters/ReflectionFocus/license.txt

@@ -0,0 +1,30 @@
+Copyright (c) 2008-2025, Regents of the University of California


The description field in license.txt says "Reflector focus" but the actual device implementation is called "ReflectionFocus" (without the "or"). For consistency, the license description should match the actual device name.

Copilot · 2025-12-27T00:12:56Z

DeviceAdapters/ReflectionFocus/ReflectionFocus.cpp

+            if (calibrationMap_.find(deviceSettings_) == calibrationMap_.end())
+            {
+               continuousFocusLocked_ = false;
+               UpdateStatus("Out of Range");
+               CDeviceUtils::SleepMs(50);
+               continue;
+            }
+            cal = calibrationMap_[deviceSettings_];


The calibrationMap_ is accessed without mutex protection in the continuous focus thread (line 846) while it could potentially be modified in other threads through property handlers (e.g., OnDeviceSettings loads calibration data at line 456-495). This creates a race condition. Consider protecting calibrationMap_ access with a mutex or making a local copy under lock protection.

Copilot · 2025-12-27T00:13:00Z

DeviceAdapters/ReflectionFocus/ReflectionFocus.cpp

+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;
+}


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.

Copilot · 2025-12-27T00:13:01Z

DeviceAdapters/ReflectionFocus/ReflectionFocus.cpp

+#include <direct.h>   // For _mkdir
+#include <io.h>       // For _access
+#endif


Windows-specific headers direct.h and io.h are included but only used for _mkdir and _access in the GetCalibrationFilePath function. However, these headers are included at the top level even when not on Windows. Consider moving these includes inside the #ifdef _WIN32 block or at least near where they're used to make the platform-specific dependencies clearer.

Copilot · 2025-12-27T00:13:01Z

DeviceAdapters/ReflectionFocus/ReflectionFocus.cpp

+
+         // Step 3: Calculate Z correction
+         double diffZ = CalculateTargetZDiff(cal, lastSpotX_, lastSpotY_);
+         diffZ += offset_;


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.

Copilot · 2025-12-27T00:13:01Z

DeviceAdapters/ReflectionFocus/ReflectionFocus.h

+  int GetLimits(double& min, double& max);
+
+  int IsStageSequenceable(bool& isSequenceable) const {isSequenceable = false; return DEVICE_OK;}
+  bool IsContinuousFocusDrive() const {return true;}


The method IsContinuousFocusDrive returns true, but according to the Micro-Manager device API, this method indicates whether the stage is specifically designed for continuous focus drive functionality (like piezo stages). ReflectionFocusStage is a virtual stage wrapper around an autofocus offset, not a physical continuous focus drive. This should likely return false unless there's a specific reason this wrapper needs to be marked as a continuous focus drive.

Suggested change

bool IsContinuousFocusDrive() const {return true;}

bool IsContinuousFocusDrive() const {return false;}

Copilot · 2025-12-27T00:13:01Z

DeviceAdapters/ReflectionFocus/ReflectionFocus.cpp

+   // Snap image with shutter closed
+   camera->SnapImage();
+   ImgBuffer darkImage;
+   int ret = GetImageFromBuffer(darkImage);


The return value 'ret' from GetImageFromBuffer is not checked. If the function fails, darkImage may be in an invalid state, but the code continues to use it. Check the return value and handle errors appropriately.

nicost added 2 commits December 18, 2025 10:30

Reflection Autofocus device. Previously experimentally in Utilities (…

c6b6bc9

…in a now defunct branch).

ReflectionFocus: changed name to ReflectionFocus from Autofocus every…

bef5473

…where to avoid future confusion.

nicost requested a review from Copilot December 18, 2025 21:50

Copilot started reviewing on behalf of nicost December 18, 2025 21:51 View session

nicost mentioned this pull request Dec 18, 2025

Utilties-Hardware Autofocus #800

Closed

Copilot AI reviewed Dec 18, 2025

View reviewed changes

nicost added 2 commits December 26, 2025 15:52

ReflectionFocus: fix typos and punctuation.

38fa2cb

ReflectionFocus: implement most CoPilot suggestions (except for more …

54b0540

…check during calibration).

nicost requested a review from Copilot December 27, 2025 00:07

Copilot started reviewing on behalf of nicost December 27, 2025 00:07 View session

Copilot AI reviewed Dec 27, 2025

View reviewed changes

		@@ -0,0 +1,30 @@
		Copyright (c) 2008-2025, Regents of the University of California

	bool IsContinuousFocusDrive() const {return true;}
	bool IsContinuousFocusDrive() const {return false;}

Reflector focus - Focus maintenance device based on a camera, shutter and stage #803

Are you sure you want to change the base?

Reflector focus - Focus maintenance device based on a camera, shutter and stage #803

Uh oh!

Conversation

nicost commented Dec 18, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant