-
Notifications
You must be signed in to change notification settings - Fork 130
Add getBinning/setBinning methods to MMCore #811
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Implements getting and setting camera binning values (1-100) via MM::g_Keyword_Binning property. Auto-handles integer and NxN formats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds new methods to MMCore for getting and setting camera binning values, providing a more convenient API similar to the existing exposure methods. The implementation handles binning values from 1-100 and automatically adapts to either integer format ("2") or NxN format ("2x2") based on the camera's current property format.
- Adds four new methods:
getBinning(),getBinning(const char* label),setBinning(int binning), andsetBinning(const char* label, int binning) - Implements automatic format detection and conversion for binning property values
- Increments the minor version from 11.11.0 to 11.12.0
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| MMCore/MMCore.h | Declares four new public methods for getting and setting camera binning |
| MMCore/MMCore.cpp | Implements the binning methods with format detection logic and updates API version to 11.12.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Checks if the property exists and throws an error if not. Handles both 1 and 1x1 formats. Throws error if property returns empty value, if the range is outside 1-100.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MMCore/MMCore.cpp
Outdated
| // atoi() naturally handles both formats - it parses until it hits non-digit | ||
| // "2" -> 2, "2x2" -> 2, "4x4" -> 4 | ||
| int binning = atoi(binningValue.c_str()); |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using atoi() for parsing is unsafe as it returns 0 for invalid input and doesn't provide error detection. For example, if the binning property returns "0", "abc", or other invalid values, atoi() will return 0, which will then fail the range check. However, if it returns something like "0x2", atoi() will parse it as 0, potentially missing a valid "2" value. Consider using std::stoi() with exception handling or checking if the first character is a digit before calling atoi() to ensure robust parsing.
| // atoi() naturally handles both formats - it parses until it hits non-digit | |
| // "2" -> 2, "2x2" -> 2, "4x4" -> 4 | |
| int binning = atoi(binningValue.c_str()); | |
| // Use std::stoi to parse leading digits and detect invalid formats | |
| // "2" -> 2, "2x2" -> 2, "4x4" -> 4 | |
| int binning = 0; | |
| try | |
| { | |
| std::size_t parsedChars = 0; | |
| binning = std::stoi(binningValue, &parsedChars, 10); | |
| if (parsedChars == 0) | |
| { | |
| throw CMMError("Invalid binning value (no leading digits): " + binningValue); | |
| } | |
| } | |
| catch (const std::exception&) | |
| { | |
| throw CMMError("Invalid binning value: " + binningValue); | |
| } |
MMCore/MMCore.cpp
Outdated
| if (!currentValue.empty() && currentValue.find('x') != std::string::npos) | ||
| { | ||
| useNxNFormat = true; | ||
| } |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The format detection logic only checks for the presence of 'x' in the current value. This could incorrectly detect formats if the camera returns values like "max", "mixed", or other strings containing 'x'. Consider using a more robust check, such as a regex pattern to match "NxN" format (e.g., checking for digits before and after 'x'), or checking for a pattern like std::isdigit() before 'x' and after 'x'.
MMCore/MMCore.cpp
Outdated
| int CMMCore::getBinning() MMCORE_LEGACY_THROW(CMMError) | ||
| { | ||
| std::shared_ptr<mmi::CameraInstance> camera = currentCameraDevice_.lock(); | ||
| if (!camera) | ||
| { | ||
| throw CMMError(getCoreErrorText(MMERR_CameraNotAvailable).c_str(), MMERR_CameraNotAvailable); | ||
| } | ||
| else | ||
| { | ||
| std::string cameraName; | ||
| { | ||
| mmi::DeviceModuleLockGuard guard(camera); | ||
| cameraName = camera->GetLabel(); | ||
| } | ||
| return getBinning(cameraName.c_str()); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns the current binning setting of the specified camera. | ||
| * Binning values in "NxN" format (e.g., "2x2") are parsed to return the integer factor (e.g., 2). | ||
| * @param label the camera device label | ||
| * @return the binning factor (1-100) | ||
| */ | ||
| int CMMCore::getBinning(const char* label) MMCORE_LEGACY_THROW(CMMError) | ||
| { | ||
| std::shared_ptr<mmi::CameraInstance> pCamera = | ||
| deviceManager_->GetDeviceOfType<mmi::CameraInstance>(label); | ||
|
|
||
| mmi::DeviceModuleLockGuard guard(pCamera); | ||
|
|
||
| if (!pCamera->HasProperty(MM::g_Keyword_Binning)) | ||
| { | ||
| throw CMMError("Camera does not support binning property"); | ||
| } | ||
|
|
||
| std::string binningValue = pCamera->GetProperty(MM::g_Keyword_Binning); | ||
|
|
||
| // Parse the binning value - handle both integer ("2") and NxN ("2x2") formats | ||
| if (binningValue.empty()) | ||
| { | ||
| throw CMMError("Binning property returned empty value"); | ||
| } | ||
|
|
||
| // atoi() naturally handles both formats - it parses until it hits non-digit | ||
| // "2" -> 2, "2x2" -> 2, "4x4" -> 4 | ||
| int binning = atoi(binningValue.c_str()); | ||
|
|
||
| if (binning < 1 || binning > 100) | ||
| { | ||
| throw CMMError("Binning value out of valid range (1-100): " + binningValue); | ||
| } | ||
|
|
||
| return binning; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the binning setting of the camera. | ||
| * Automatically handles both integer ("2") and "NxN" ("2x2") property formats. | ||
| * @param binning the binning factor (1-100) | ||
| */ | ||
| void CMMCore::setBinning(int binning) MMCORE_LEGACY_THROW(CMMError) | ||
| { | ||
| if (binning < 1 || binning > 100) | ||
| { | ||
| throw CMMError("Binning must be between 1 and 100"); | ||
| } | ||
|
|
||
| std::shared_ptr<mmi::CameraInstance> camera = currentCameraDevice_.lock(); | ||
| if (!camera) | ||
| { | ||
| throw CMMError(getCoreErrorText(MMERR_CameraNotAvailable).c_str(), MMERR_CameraNotAvailable); | ||
| } | ||
| else | ||
| { | ||
| std::string cameraName; | ||
| { | ||
| mmi::DeviceModuleLockGuard guard(camera); | ||
| cameraName = camera->GetLabel(); | ||
| } | ||
| setBinning(cameraName.c_str(), binning); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Sets the binning setting of the specified camera. | ||
| * Automatically handles both integer ("2") and "NxN" ("2x2") property formats. | ||
| * @param label the camera device label | ||
| * @param binning the binning factor (1-100) | ||
| */ | ||
| void CMMCore::setBinning(const char* label, int binning) MMCORE_LEGACY_THROW(CMMError) | ||
| { | ||
| if (binning < 1 || binning > 100) | ||
| { | ||
| throw CMMError("Binning must be between 1 and 100"); | ||
| } | ||
|
|
||
| std::shared_ptr<mmi::CameraInstance> pCamera = | ||
| deviceManager_->GetDeviceOfType<mmi::CameraInstance>(label); | ||
|
|
||
| { | ||
| mmi::DeviceModuleLockGuard guard(pCamera); | ||
| LOG_DEBUG(coreLogger_) << "Will set camera " << label << | ||
| " binning to " << binning; | ||
|
|
||
| if (!pCamera->HasProperty(MM::g_Keyword_Binning)) | ||
| { | ||
| throw CMMError("Camera does not support binning property"); | ||
| } | ||
|
|
||
| // Try to determine the format by checking the current property value | ||
| std::string currentValue; | ||
| bool useNxNFormat = false; | ||
|
|
||
| try | ||
| { | ||
| currentValue = pCamera->GetProperty(MM::g_Keyword_Binning); | ||
| // If current value contains 'x' and looks like NxN format, use that | ||
| if (!currentValue.empty() && currentValue.find('x') != std::string::npos) | ||
| { | ||
| useNxNFormat = true; | ||
| } | ||
| } | ||
| catch (...) | ||
| { | ||
| // If we can't get current value, we'll try integer format first | ||
| } | ||
|
|
||
| // Build the property value string | ||
| std::string binningValue; | ||
| if (useNxNFormat) | ||
| { | ||
| // NxN format: "2x2", "4x4", etc. | ||
| std::string binStr = CDeviceUtils::ConvertToString(binning); | ||
| binningValue = binStr + "x" + binStr; | ||
| } | ||
| else | ||
| { | ||
| // Integer format: "2", "4", etc. | ||
| binningValue = CDeviceUtils::ConvertToString(binning); | ||
| } | ||
|
|
||
| // Try to set the property with detected format | ||
| try | ||
| { | ||
| pCamera->SetProperty(MM::g_Keyword_Binning, binningValue); | ||
| } | ||
| catch (const CMMError&) | ||
| { | ||
| // If it failed and we haven't tried the other format yet, try it | ||
| if (useNxNFormat) | ||
| { | ||
| // Tried NxN, now try integer | ||
| binningValue = CDeviceUtils::ConvertToString(binning); | ||
| } | ||
| else | ||
| { | ||
| // Tried integer, now try NxN | ||
| std::string binStr = CDeviceUtils::ConvertToString(binning); | ||
| binningValue = binStr + "x" + binStr; | ||
| } | ||
|
|
||
| // Try with alternate format (this will throw if it still fails) | ||
| pCamera->SetProperty(MM::g_Keyword_Binning, binningValue); | ||
| } | ||
|
|
||
| { | ||
| MMThreadGuard scg(stateCacheLock_); | ||
| stateCache_.addSetting(PropertySetting(label, MM::g_Keyword_Binning, binningValue.c_str())); | ||
| } | ||
| } | ||
|
|
||
| LOG_DEBUG(coreLogger_) << "Did set camera " << label << | ||
| " binning to " << binning; | ||
| } |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new getBinning and setBinning methods lack test coverage. Given that the codebase includes unit tests (e.g., PixelSize-Tests.cpp tests binning-related functionality), these new API methods should have corresponding tests to verify:
- Parsing of integer format ("2")
- Parsing of NxN format ("2x2")
- Setting binning in both formats
- Error handling for cameras without binning support
- Error handling for invalid binning values
- The fallback mechanism when one format fails
MMCore/MMCore.cpp
Outdated
| } | ||
| catch (...) | ||
| { | ||
| // If we can't get current value, we'll try integer format first |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The catch block catches all exceptions (catch (...)) but provides no logging or indication of what went wrong. This makes debugging difficult if GetProperty fails. Consider either catching specific exceptions and logging them at DEBUG level, or at minimum adding a debug log message indicating that the current value couldn't be retrieved and the default format will be tried.
| // If we can't get current value, we'll try integer format first | |
| // If we can't get current value, we'll try integer format first | |
| LOG_DEBUG(coreLogger_) << "Could not retrieve current binning value for camera " | |
| << label << "; will try integer format first."; |
|
When "x" is detected, make sure the last digit is the same as the first one (i.e. take binning detected by atoi and ensure the part after the "x" is the same). |
n and m are equal.
|
That concern was addressed in the latest commit. @marktsuchida this is ready to be merged as far as I am concerned (other than adding tests, but not sure how to do that). |
X and Y binning are reported and IMO should both be parsed. There are cameras which support setting asymmetric binnings. (Except it is forbidden to have non-square pixel sizes) |
|
@ReneGreiner, I am not sure I understand what you would like to see happen. Currently, Micro-manager support only one number for binning, and that same number applies to rows and columns. We are trying to unify the binning interface in the API to enforce this, rather than rely on however the device adapter implements the property Binning. Most camera device adapter supply either a single number in the property, or two numbers separated by an x. We want to accept both forms and be sure that in the latter case both numbers are the same, i.e. assymetric binning will result in an error. I believe you mean the same thing, but I am not quite sure. Current code works as we would like it to work, are there any issues? |
Just wanted to understand that API. It's a bit confusing that the device has to parse both values even that micro manager just supports squared binning. So I wondered, is there the possibility to provide asymmetric binning (like 2x1). But after I posted it, I also found out, it's not possible. |
marktsuchida
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API and the basic mechanism looks good. Some comments on the details.
MMCore/MMCore.cpp
Outdated
| try | ||
| { | ||
| currentValue = pCamera->GetProperty(MM::g_Keyword_Binning); | ||
| // If current value contains 'x' and looks like NxN format, use that | ||
| size_t xPos = currentValue.find('x'); | ||
| if (!currentValue.empty() && xPos != std::string::npos) | ||
| { | ||
| // Validate that the current value has symmetric binning | ||
| int binX = atoi(currentValue.c_str()); | ||
| int binY = atoi(currentValue.c_str() + xPos + 1); | ||
| if (binX != binY) | ||
| { | ||
| throw CMMError("Asymmetric binning not supported: " + currentValue); | ||
| } | ||
| useNxNFormat = true; | ||
| } | ||
| } | ||
| catch (...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
We can check the property type (Integer/String) instead of the current value (error if it's Float). Since we do not support string values like "1" or "4", just knowing the property type is enough to determine the only possible correct value to set. Then we either succeed or get an error.
-
Is it necessary to check for equal H and V binning when we're going to overwrite the current value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure that all Binning Properties of type String will actually return "2x2", rather than "2". Do we want to disallow returning a number as type String?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented in the latest commit. I am still a bit unsure why we can rely on Property Types. I went through all camera device adapters, and mandating that a String Binning property needs to have the format 1x1 will break the DahengGalaxy and some instances of the PVCAM device adapter. I believe that I could fix the Dahenggalaxy, but not the PVCAM device adapter. See: #818
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not aware that PVCAM did that; relying on type alone was not a good idea then. But when it is a string you can still query the allowed values to see whether "N" is accepted or "NxN" is required. Still no need to look at the previous value.
As for PVCAM's MxN format (with M ≠ N), I don't immediately see a reason to disallow it. setBinning() need not care if such options exist (it will only support setting square binnings). getBinning() will be an error if M ≠ N but will always work if the previous setting was done by setBinning(). Hopefully the camera will start out at 1x1, so you only get an error if you use the property to choose a non-square option.
Users wanting non-square binning can use the property -- MMStudio won't work nicely with these (at least pixel calibration will be broken) but still useful for scripted acquisitions if you need these. If feels unfortunate to remove functionality from the PVCAM adapter just because we don't have an official way to support it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Querying allowed values will only work if the device adapter actually sets allowed values. That is not a guarantee, whereas it is a guarantee that it has a value, so querying for the actual value seems safer to me than querying for the first allowed value. We could query in a try catch for an allowed value, then for the actual value if it fails, but why so complex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that hard after all. Committed.
MMCore/MMCore.cpp
Outdated
|
|
||
| // Parse the binning value - handle both integer ("2") and NxN ("2x2") formats | ||
| int binning; | ||
| size_t xPos = binningValue.find('x'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would first check the property type. If it is Integer, just return its value. Only when it is String we should expect the NxN format. Otherwise we'd allow Binning to be a String property with values like "1" or "4", which we do not want to allow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we not want to allow that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented, but see comment above. My main worry is that we will now break the PVCAM device adapter. That may be worth it, but we should first check in with them.
work with the new get and set Binning functions in the core.
These will now use numbers for properties of type Integer. String Properties can be either N or NxN. Tested with DemoCamera and DahengGalaxy.
|
I believe this address all issues. I am not sure that I have a PVCAM camera available to test (which seems to be the most divergent). This is ready to be merged as far as I am concerned. @marktsuchida ? |
marktsuchida
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good!
It just occurred to me that we need a third function that returns the allowed binning values. Otherwise these can't be used, say, in MMStudio's Binning dropdown. I'd be okay with adding that either before or after merging this PR.
MMCore/MMCore.cpp
Outdated
| if (propType == MM::Float) | ||
| { | ||
| throw CMMError("Binning property has Float type, which is not supported"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I think better to add this as else clause to the if statement below branching for Integer vs String.
MMCore/MMCore.cpp
Outdated
| if (propType == MM::Float) | ||
| { | ||
| currentValue = pCamera->GetProperty(MM::g_Keyword_Binning); | ||
| // If current value contains 'x' and looks like NxN format, use that | ||
| size_t xPos = currentValue.find('x'); | ||
| if (!currentValue.empty() && xPos != std::string::npos) | ||
| { | ||
| // Validate that the current value has symmetric binning | ||
| int binX = atoi(currentValue.c_str()); | ||
| int binY = atoi(currentValue.c_str() + xPos + 1); | ||
| if (binX != binY) | ||
| { | ||
| throw CMMError("Asymmetric binning not supported: " + currentValue); | ||
| } | ||
| useNxNFormat = true; | ||
| } | ||
| } | ||
| catch (...) | ||
| { | ||
| // If we can't get current value, we'll try integer format first | ||
| throw CMMError("Binning property has Float type, which is not supported"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick (repeated): I think better to add this as else clause to the if statement below branching for Integer vs String.
getAllowedBinningValues needs to be vector<long> or swig will not convert this to a LongVector. For symmetry, change the other functions from int to long as well.
|
Note the change in type from int to long. Because getAllowedBinningValues() needs to return vector (so that Swig returns a LongVector) it seemed more logical for all the binning functions to use long rather than int. |
- No need to check for Float type - Remove unreachable throw statement
|
Thanks, I will just push 2 commits to
Ready to merge if checks pass. |
Implements getting and setting camera binning values (1-100) via
MM::g_Keyword_Binning property. Auto-handles integer and NxN formats.
Addresses parts of #38.
Closes #818