[CSDM-1188][feat] Plugin to monitor and control the camera cooling#3367
Open
pieleric wants to merge 1 commit intodelmic:masterfrom
Open
[CSDM-1188][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
📝 WalkthroughWalkthroughAdds a CameraTemperaturePlugin and a TemperatureControlDialog providing an in-GUI camera temperature control. The plugin detects cameras that expose Sequence Diagram(s)sequenceDiagram
participant User as User
participant App as MainApp/Menu
participant Plugin as CameraTemperaturePlugin
participant Dialog as TemperatureControlDialog
participant VA as Camera VAs
User->>App: Select "Acquisition/Camera temperature..."
App->>Plugin: Open dialog request
Plugin->>Dialog: create or focus single instance
Dialog->>VA: subscribe to temperature & targetTemperature
VA-->>Dialog: push current temperature & targetTemperature
Dialog-->>User: update UI (temperature display, spin box, toggle)
User->>Dialog: toggle cooling ON
Dialog->>Dialog: restore previous target or use default
Dialog->>VA: write new targetTemperature
VA-->>Dialog: confirm updated targetTemperature
Dialog-->>User: reflect updated target
User->>Dialog: change target spin box
Dialog->>VA: write targetTemperature (only if cooling ON)
VA-->>Dialog: push updated targetTemperature
Dialog-->>User: update display
User->>Dialog: toggle cooling OFF
Dialog->>Dialog: store current target as previous
Dialog->>VA: write COOLING_OFF_TEMP as targetTemperature
VA-->>Dialog: push updated targetTemperature
Dialog-->>User: reflect cooling OFF state
User->>Dialog: close
Dialog->>VA: unsubscribe from temperature & targetTemperature
Dialog->>Plugin: dialog closed
🚥 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)
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 """
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.
pieleric
changed the title
pieleric
force-pushed
the
feat-plugin-to-monitor-and-control-the-camera-cooling
branch
from
92eb884 to
2b66d34
Compare
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
2b66d34 to
1419570
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
plugins/camera_temperature.py (3)
245-250:⚠️ Potential issue | 🟡 MinorMetadata target temperature may be out of VA range.
When restoring
previous_targetfrom metadata (line 246), the value is not clipped to the camera'stargetTemperature.range. If the stored value falls outside the valid range (e.g., after a camera configuration change), assigning it to the VA on line 250 will raise an exception.The fallback on line 248 correctly clips
DEFAULT_COOLING_TEMP, but the metadata value should also be clipped.🐛 Suggested fix
if is_cooling: # Turn cooling on: restore previous target or use default metadata = self._camera.getMetadata() try: previous_target = metadata[MD_TARGET_COOLING_TEMP] + previous_target = self._camera.targetTemperature.clip(previous_target) except KeyError: previous_target = self._camera.targetTemperature.clip(DEFAULT_COOLING_TEMP) 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 - 250, When restoring previous_target from metadata (MD_TARGET_COOLING_TEMP), ensure the value is clipped to the camera VA range before assignment to avoid range exceptions; replace the current try/except so that on success you call self._camera.targetTemperature.clip(previous_target) (just like the fallback does for DEFAULT_COOLING_TEMP) and then assign the clipped value to self._camera.targetTemperature.value.
200-209:⚠️ Potential issue | 🟡 MinorVA callback may execute after dialog is destroyed.
The
@call_in_wx_maindecorator useswx.CallAfterto queue execution. If the dialog is destroyed between queueing and execution, accessingself._temp_displaywill fail with aRuntimeErroror cause undefined behavior.Add a guard at the start of the callback to check if the dialog is still valid.
🛡️ Suggested fix
`@call_in_wx_main` def _on_temperature_update(self, temperature: float) -> None: """ Callback for temperature VA updates. Updates the current temperature display. :param temperature: Current temperature in °C """ + if not self: + return self._temp_display.SetLabel(f"{temperature:.1f} °C")Apply the same guard to
_on_target_temperature_update:`@call_in_wx_main` def _on_target_temperature_update(self, target_temp: float) -> None: ... + if not self: + return + if self._updating_ui: return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/camera_temperature.py` around lines 200 - 209, The temperature callback can run after the dialog is destroyed; add an early-return guard at the top of _on_temperature_update that verifies the dialog/window is still valid before touching self._temp_display (for example check that the widget exists and the window is not destroyed/hidden, e.g. if not getattr(self, "GetHandle", lambda: None)() or not hasattr(self, "_temp_display"): return), and add the identical guard to _on_target_temperature_update so both VA callbacks skip updating UI when the dialog has been torn down.
89-92:⚠️ Potential issue | 🟡 MinorCalling
IsShown()on a destroyed dialog can crash.After the dialog is closed and destroyed,
self._dlgstill holds a reference to the destroyedwx.Dialogobject. CallingIsShown()on it can raise aRuntimeErroror cause undefined behavior.Consider either clearing the plugin's
_dlgreference when the dialog closes (e.g., by binding towx.EVT_WINDOW_DESTROY), or adding a defensive check.🛡️ Suggested fix: Add defensive check
def _on_menu_item(self) -> None: """ Callback for the menu item. Opens the temperature control dialog if not already open. """ - if self._dlg and self._dlg.IsShown(): + # Check if dialog exists and is still valid (not destroyed) + try: + if self._dlg and self._dlg.IsShown(): + # Dialog already open, just bring it to front + self._dlg.Raise() + return + except RuntimeError: + # Dialog was destroyed, clear reference + self._dlg = None - # Dialog already open, just bring it to front - self._dlg.Raise() - return # Create and show the dialog self._dlg = TemperatureControlDialog(self.main_app.main_frame, self._camera, self._temp_cooling_off) self._dlg.Show()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/camera_temperature.py` around lines 89 - 92, self._dlg may reference a destroyed wx.Dialog, so calling self._dlg.IsShown() can crash; update the code that checks/raises the dialog (the block using self._dlg, IsShown(), and Raise()) to either clear _dlg when the dialog is destroyed (bind a handler to wx.EVT_WINDOW_DESTROY or the dialog's close event and set self._dlg = None) or perform a defensive check before calling IsShown() (verify self._dlg is not None and not destroyed via hasattr/IsBeingDeleted or try/except around IsShown()), ensuring subsequent Raise() is only called on a valid dialog object.
🧹 Nitpick comments (3)
plugins/camera_temperature.py (3)
104-110: Docstring missingtemp_cooling_offparameter documentation.The docstring documents
parentandcamerabut omits thetemp_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 threshold indicating 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, The __init__ docstring for Camera temperature control is missing documentation for the temp_cooling_off parameter; update the __init__ docstring to add a brief description of temp_cooling_off (its type float and what it represents, e.g., the temperature threshold in Celsius at which cooling is turned off) alongside the existing parent and camera descriptions so callers reading plugins/camera_temperature.py (specifically the __init__ method) understand the parameter purpose and units.
46-46: Missing type hint formain_appparameter.The
main_appparameter lacks a type annotation. As per coding guidelines, "Always use type hints for function parameters and return types in Python code."♻️ Suggested fix
- def __init__(self, microscope: model.Microscope, main_app) -> None: + def __init__(self, microscope: model.Microscope, main_app: "odemis.gui.main.OdemisGUIApp") -> None:Alternatively, if the exact type is not importable without circular dependencies, use a string annotation or
typing.Any.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/camera_temperature.py` at line 46, The __init__ method of the CameraTemperature plugin is missing a type annotation for the main_app parameter; update the signature of __init__(self, microscope: model.Microscope, main_app) to add a type for main_app (e.g. use an explicit app class name if available, a forward-reference string, or typing.Any) to satisfy type-hinting guidelines, and import typing.Any if you choose that approach; ensure only the parameter annotation is added (no behavioral changes) in the __init__ method of the class containing this constructor.
30-30: Unused import:utilis imported but never used.The
utilmodule is imported alongsidemodelbut is not referenced anywhere in this file.♻️ Suggested fix
-from odemis import model, util +from odemis import model🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/camera_temperature.py` at line 30, Remove the unused import by deleting `util` from the import statement that currently reads `from odemis import model, util`; keep `model` if it's used elsewhere in this file (e.g., by functions or classes that reference `model`) so the import becomes `from odemis import model` to eliminate the unused `util` symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugins/camera_temperature.py`:
- Around line 245-250: When restoring previous_target from metadata
(MD_TARGET_COOLING_TEMP), ensure the value is clipped to the camera VA range
before assignment to avoid range exceptions; replace the current try/except so
that on success you call self._camera.targetTemperature.clip(previous_target)
(just like the fallback does for DEFAULT_COOLING_TEMP) and then assign the
clipped value to self._camera.targetTemperature.value.
- Around line 200-209: The temperature callback can run after the dialog is
destroyed; add an early-return guard at the top of _on_temperature_update that
verifies the dialog/window is still valid before touching self._temp_display
(for example check that the widget exists and the window is not
destroyed/hidden, e.g. if not getattr(self, "GetHandle", lambda: None)() or not
hasattr(self, "_temp_display"): return), and add the identical guard to
_on_target_temperature_update so both VA callbacks skip updating UI when the
dialog has been torn down.
- Around line 89-92: self._dlg may reference a destroyed wx.Dialog, so calling
self._dlg.IsShown() can crash; update the code that checks/raises the dialog
(the block using self._dlg, IsShown(), and Raise()) to either clear _dlg when
the dialog is destroyed (bind a handler to wx.EVT_WINDOW_DESTROY or the dialog's
close event and set self._dlg = None) or perform a defensive check before
calling IsShown() (verify self._dlg is not None and not destroyed via
hasattr/IsBeingDeleted or try/except around IsShown()), ensuring subsequent
Raise() is only called on a valid dialog object.
---
Nitpick comments:
In `@plugins/camera_temperature.py`:
- Around line 104-110: The __init__ docstring for Camera temperature control is
missing documentation for the temp_cooling_off parameter; update the __init__
docstring to add a brief description of temp_cooling_off (its type float and
what it represents, e.g., the temperature threshold in Celsius at which cooling
is turned off) alongside the existing parent and camera descriptions so callers
reading plugins/camera_temperature.py (specifically the __init__ method)
understand the parameter purpose and units.
- Line 46: The __init__ method of the CameraTemperature plugin is missing a type
annotation for the main_app parameter; update the signature of __init__(self,
microscope: model.Microscope, main_app) to add a type for main_app (e.g. use an
explicit app class name if available, a forward-reference string, or typing.Any)
to satisfy type-hinting guidelines, and import typing.Any if you choose that
approach; ensure only the parameter annotation is added (no behavioral changes)
in the __init__ method of the class containing this constructor.
- Line 30: Remove the unused import by deleting `util` from the import statement
that currently reads `from odemis import model, util`; keep `model` if it's used
elsewhere in this file (e.g., by functions or classes that reference `model`) so
the import becomes `from odemis import model` to eliminate the unused `util`
symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dbce3631-c426-4391-8f08-0c11705712ac
📒 Files selected for processing (1)
plugins/camera_temperature.py
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
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.