[feat] Plugin to monitor and control the camera cooling#3367
Open
pieleric wants to merge 1 commit intodelmic:masterfrom
Open
[feat] Plugin to monitor and control the camera cooling#3367pieleric wants to merge 1 commit intodelmic:masterfrom
pieleric wants to merge 1 commit intodelmic:masterfrom
Conversation
📝 Walkthrough🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@plugins/camera_temperature.py`:
- Around line 55-72: The metadata-default block accesses
self._camera.targetTemperature.value before verifying the VA exists, causing an
AttributeError; move the entire block that reads metadata, checks
MD_TARGET_COOLING_TEMP, reads target_temp and calls updateMetadata (using
MD_TARGET_COOLING_TEMP, COOLING_OFF_TEMP, self._camera.getMetadata(),
self._camera.targetTemperature.value, and self._camera.updateMetadata()) to
after the two model.hasVA(self._camera, "temperature") and
model.hasVA(self._camera, "targetTemperature") guards so targetTemperature is
only accessed once its VA presence is confirmed.
- Around line 197-206: The VA callback _on_temperature_update can run after the
dialog is destroyed because call_in_wx_main uses wx.CallAfter; add a simple
guard: introduce an _is_closed boolean on the dialog (default False), set
_is_closed = True in _on_close before calling Destroy(), and at the start of
_on_temperature_update return immediately if self._is_closed (or if
self._temp_display is None) to avoid touching destroyed widgets; keep using
call_in_wx_main but ensure the early-return guard prevents access to
_temp_display after close.
🧹 Nitpick comments (1)
plugins/camera_temperature.py (1)
229-255: FIXME on line 242: target cooling temp initialization.The comment acknowledges the issue — the previous target should be captured at init time rather than relying solely on metadata that may not exist yet. This is partially addressed by the metadata initialization in
__init__of the plugin, but if the plugin starts with cooling already off (target ≥ 25°C), no value gets stored, andDEFAULT_COOLING_TEMPis used as the fallback, which may not match the user's preference.Would you like me to open an issue to track resolving this FIXME, or generate a fix that captures and stores the initial target temperature more robustly?
There was a problem hiding this comment.
Pull request overview
Adds a new GUI plugin that exposes a dedicated menu entry to monitor and control camera cooling via the camera’s temperature / targetTemperature vigilant attributes.
Changes:
- Introduces
CameraTemperaturePluginto add the menu entryAcquisition/Camera temperature.... - Implements a non-modal
wx.Dialogthat subscribes to live temperature/target updates and allows toggling cooling + setting a target temperature. - Persists/restores the last cooling target temperature via camera metadata.
pieleric
force-pushed
the
feat-plugin-to-monitor-and-control-the-camera-cooling
branch
from
5ca23a8 to
e1838bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@plugins/camera_temperature.py`:
- Around line 278-289: Remove the dialog-owned clearing of the plugin reference
from TemperatureControlDialog._on_close (delete the line that sets self._dlg =
None) and move plugin cleanup into CameraTemperaturePlugin: when creating the
dialog, bind the dialog close/destroy event to a new plugin method (e.g.,
CameraTemperaturePlugin._on_dialog_closed) that sets
CameraTemperaturePlugin._dlg = None and lets the event continue, and update
CameraTemperaturePlugin._on_menu_item to defensively check the dialog before
calling IsShown (e.g., ensure self._dlg is not None and hasattr(self._dlg,
"IsShown") or wrap IsShown in a try/except) so the plugin never calls IsShown on
a destroyed wx.Dialog.
🧹 Nitpick comments (1)
plugins/camera_temperature.py (1)
104-110: Docstring is missing thetemp_cooling_offparameter.The
__init__docstring documentsparentandcamerabut omits thetemp_cooling_offparameter.📝 Proposed fix
""" Initialize the temperature control dialog. :param parent: Parent window :param camera: Camera component with temperature and targetTemperature VAs + :param temp_cooling_off: Temperature value that indicates cooling is off """
Provides an extra menu entry "Acquisition/Camera temperature..." which opens a small window to monitor and control the camera cooling.
pieleric
force-pushed
the
feat-plugin-to-monitor-and-control-the-camera-cooling
branch
from
e1838bb to
92eb884
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/camera_temperature.py (2)
104-110: Docstring missingtemp_cooling_offparameter.♻️ Suggested fix
""" Initialize the temperature control dialog. :param parent: Parent window :param camera: Camera component with temperature and targetTemperature VAs + :param temp_cooling_off: Temperature value (°C) that indicates cooling is off """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/camera_temperature.py` around lines 104 - 110, Docstring for __init__ in plugins/camera_temperature.py is missing the temp_cooling_off parameter; update the method docstring for def __init__(self, parent: wx.Window, camera: model.HwComponent, temp_cooling_off: float) -> None to include a :param temp_cooling_off: line describing that value (e.g., temperature threshold or value used to disable cooling) and its units/type, so callers and maintainers understand the purpose of temp_cooling_off.
46-47: Missing type hint formain_appand missing docstring for__init__.Per coding guidelines, all function parameters should have type hints and all functions should have docstrings.
♻️ Suggested fix
- def __init__(self, microscope: model.Microscope, main_app) -> None: + def __init__(self, microscope: model.Microscope, main_app: "wx.App") -> None: + """ + Initialize the camera temperature plugin. + + :param microscope: The microscope model. + :param main_app: The main GUI application. + """ super().__init__(microscope, main_app)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/camera_temperature.py` around lines 46 - 47, Add a type hint for the main_app parameter and a short docstring to the __init__ method: update the signature of __init__(self, microscope: model.Microscope, main_app) -> None to include the concrete type for main_app (e.g., main_app: MainApp or the appropriate class/interface used across the project) and add a one- or two-line docstring describing what the constructor does and its parameters; modify the method that calls super().__init__(microscope, main_app) to match the new signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/camera_temperature.py`:
- Around line 245-252: The restored previous_target (from MD_TARGET_COOLING_TEMP
or DEFAULT_COOLING_TEMP) must be clipped to the camera's allowed
targetTemperature.range before assigning to self._camera.targetTemperature.value
and _target_spin.SetValue; fetch the allowed min/max from
self._camera.targetTemperature.range (or equivalent attributes), clamp
previous_target into [min, max], then use the clamped value for
self._camera.targetTemperature.value, _target_spin.SetValue, and the logging
call so no out-of-range value can raise an exception.
---
Duplicate comments:
In `@plugins/camera_temperature.py`:
- Around line 84-96: The plugin retains a reference to a destroyed wx.Dialog
which makes calling self._dlg.IsShown() unsafe; update _on_menu_item to first
check that self._dlg is not destroyed by using self._dlg.IsBeingDeleted() (or
guard with hasattr/validity checks) before calling IsShown(), and ensure the
dialog clears the plugin reference when closed by binding the dialog's
destroy/close event (e.g., in TemperatureControlDialog or when creating it) to a
handler that sets self._dlg = None (or uses EVT_WINDOW_DESTROY/EVT_CLOSE to call
a plugin method like _on_close to clear the reference) so subsequent menu clicks
never call methods on a destroyed native window.
- Around line 203-212: The wx.CallAfter-backed callback _on_temperature_update
can run after the dialog/widgets are destroyed; add a simple guard at the top of
_on_temperature_update (and mirror it in _on_target_temperature_update) to
return early if the dialog or target widget no longer exists. For example, check
that self still exists and that the target widget attribute (self._temp_display
and the target widget used in _on_target_temperature_update) is present/non-None
and not destroyed (e.g., if not getattr(self, "_temp_display", None) or
getattr(self, "_temp_display", "destroyed") == "destroyed": return) before
calling SetLabel; this prevents accessing destroyed widgets when call_in_wx_main
(wx.CallAfter) invokes the callback. Ensure the same pattern is applied to the
_on_target_temperature_update handler.
---
Nitpick comments:
In `@plugins/camera_temperature.py`:
- Around line 104-110: Docstring for __init__ in plugins/camera_temperature.py
is missing the temp_cooling_off parameter; update the method docstring for def
__init__(self, parent: wx.Window, camera: model.HwComponent, temp_cooling_off:
float) -> None to include a :param temp_cooling_off: line describing that value
(e.g., temperature threshold or value used to disable cooling) and its
units/type, so callers and maintainers understand the purpose of
temp_cooling_off.
- Around line 46-47: Add a type hint for the main_app parameter and a short
docstring to the __init__ method: update the signature of __init__(self,
microscope: model.Microscope, main_app) -> None to include the concrete type for
main_app (e.g., main_app: MainApp or the appropriate class/interface used across
the project) and add a one- or two-line docstring describing what the
constructor does and its parameters; modify the method that calls
super().__init__(microscope, main_app) to match the new signature.
Comment on lines
+245
to
+252
| if is_cooling: | ||
| # Turn cooling on: restore previous target or use default | ||
| metadata = self._camera.getMetadata() | ||
| previous_target = metadata.get(MD_TARGET_COOLING_TEMP, DEFAULT_COOLING_TEMP) | ||
| self._camera.targetTemperature.value = previous_target | ||
| self._target_spin.SetValue(previous_target) | ||
| self._target_spin.Enable(True) | ||
| logging.info("Camera cooling enabled, target temperature: %.1f °C", previous_target) |
There was a problem hiding this comment.
Restored target temperature may be out of VA range.
previous_target (from metadata or DEFAULT_COOLING_TEMP = -75) is assigned directly to the VA without clipping. If the stored value or default falls outside the camera's targetTemperature.range, this will raise an exception.
🐛 Proposed fix
previous_target = metadata.get(MD_TARGET_COOLING_TEMP, DEFAULT_COOLING_TEMP)
+ previous_target = self._camera.targetTemperature.clip(previous_target)
self._camera.targetTemperature.value = previous_target🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/camera_temperature.py` around lines 245 - 252, The restored
previous_target (from MD_TARGET_COOLING_TEMP or DEFAULT_COOLING_TEMP) must be
clipped to the camera's allowed targetTemperature.range before assigning to
self._camera.targetTemperature.value and _target_spin.SetValue; fetch the
allowed min/max from self._camera.targetTemperature.range (or equivalent
attributes), clamp previous_target into [min, max], then use the clamped value
for self._camera.targetTemperature.value, _target_spin.SetValue, and the logging
call so no out-of-range value can raise an exception.
| # Main sizer | ||
| main_sizer = wx.BoxSizer(wx.VERTICAL) | ||
|
|
||
| # Current temperature display |
Contributor
There was a problem hiding this comment.
You can create all widgets easily using the
odemis/src/odemis/gui/comp/settings.py
Line 52 in 4180acf
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Provides an extra menu entry "Acquisition/Camera temperature..." which opens a
small window to monitor and control the camera cooling.