fix: handle illumination sources without a hardware source code#518
Conversation
- get_illumination_source_code() returns -1 when channel has no source code (e.g. software-only illumination not using the controller) - _is_led_matrix() now checks 0 <= source < 10, so -1 correctly evaluates to False instead of being treated as LED matrix Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent software-only illumination channels (with no microcontroller “source code”) from being treated as LED-matrix sources by introducing a -1 sentinel value and tightening the LED-matrix detection logic in LiveController.
Changes:
- Update
AcquisitionChannel.get_illumination_source_code()documentation and return value when the illumination channel cannot be resolved. - Update
LiveController._is_led_matrix()to treat only source codes in the0–9range as LED matrix sources (excluding-1).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
software/control/models/acquisition_config.py |
Adjusts illumination source code resolution to use -1 when the primary illumination channel cannot be found. |
software/control/core/live_controller.py |
Narrows LED-matrix detection to 0 <= source < 10 to ensure -1 is not misclassified as LED matrix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| ill_channel_name = self.primary_illumination_channel | ||
| if not ill_channel_name: | ||
| return 0 |
There was a problem hiding this comment.
get_illumination_source_code() docstring says it returns -1 when not found, but the if not ill_channel_name: branch still returns 0. Since 0 is a valid LED-matrix source code, this can incorrectly classify “no primary channel configured” as LED matrix. Consider returning -1 here (or otherwise making the sentinel consistent).
| return 0 | |
| return -1 |
| ill_channel = illumination_config.get_channel_by_name(ill_channel_name) | ||
| if not ill_channel: | ||
| return 0 | ||
| return -1 | ||
| return illumination_config.get_source_code(ill_channel) |
There was a problem hiding this comment.
This change doesn’t yet handle the case described in the PR summary (“channel exists but has no hardware source code”). IlluminationChannelConfig.get_source_code() falls back to 0 when controller_port_mapping has no entry, so software-only/N/A channels will still appear as source code 0. Consider detecting a missing mapping (and no legacy source_code/led_matrix_pattern) and returning -1 in that case.
| def get_illumination_source_code(self, illumination_config: "IlluminationChannelConfig") -> int: | ||
| """Get the illumination source code for the primary illumination channel. | ||
|
|
||
| Args: | ||
| illumination_config: The machine's illumination channel configuration. | ||
|
|
||
| Returns: | ||
| Source code (int) for the primary illumination channel, or 0 if not found. | ||
| Source code (int) for the primary illumination channel, or -1 if not found. | ||
| """ | ||
| ill_channel_name = self.primary_illumination_channel | ||
| if not ill_channel_name: | ||
| return 0 | ||
| ill_channel = illumination_config.get_channel_by_name(ill_channel_name) | ||
| if not ill_channel: | ||
| return 0 | ||
| return -1 | ||
| return illumination_config.get_source_code(ill_channel) |
There was a problem hiding this comment.
Add/adjust tests for the new -1 sentinel behavior (e.g., missing primary channel, missing channel by name, and existing channel with no controller_port_mapping entry) to ensure software-only channels don’t trigger LED-matrix logic and that 0–9 source codes still do.
| @@ -85,7 +85,7 @@ def _get_illumination_wavelength(self) -> Optional[int]: | |||
|
|
|||
| def _is_led_matrix(self) -> bool: | |||
| """Check if current configuration is LED matrix (source code < 10).""" | |||
There was a problem hiding this comment.
The docstring says “source code < 10”, but the logic now specifically treats 0–9 as LED matrix and excludes -1. Update the docstring to match the actual range and sentinel behavior to avoid confusion for future changes.
| """Check if current configuration is LED matrix (source code < 10).""" | |
| """Return True if the current configuration uses LED-matrix illumination. | |
| LED-matrix sources are encoded with integer IDs in the range 0–9 inclusive. | |
| Negative values (e.g. -1) are reserved for “no source” / invalid and are | |
| not treated as LED matrix. | |
| """ |
| def _is_led_matrix(self) -> bool: | ||
| """Check if current configuration is LED matrix (source code < 10).""" | ||
| return self._get_illumination_source() < 10 | ||
| return 0 <= self._get_illumination_source() < 10 |
There was a problem hiding this comment.
_get_illumination_source() returns 0 when currentConfiguration or ill_config is missing, and 0 is treated as LED matrix here. If the intent is “unknown/unconfigured source => not LED matrix”, consider having _get_illumination_source() return -1 for those cases (or make _is_led_matrix() return False when config is unavailable).
| return 0 <= self._get_illumination_source() < 10 | |
| # Treat missing configuration as "not LED matrix" instead of assuming source code 0. | |
| if not self.currentConfiguration: | |
| return False | |
| ill_config = self._get_illumination_config() | |
| if not ill_config: | |
| return False | |
| source_code = self.currentConfiguration.get_illumination_source_code(ill_config) | |
| return 0 <= source_code < 10 |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
get_illumination_source_code()now returns-1(instead of0) when a channel exists but has no hardware source code (e.g. software-only illumination sources not using the microcontroller)_is_led_matrix()now checks0 <= source < 10so that-1correctly evaluates toFalseinstead of being misidentified as LED matrixTest plan
🤖 Generated with Claude Code