Skip to content
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

Save status variables during runtime #107

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

TobiasSpohn
Copy link
Contributor

Description

This pull request adds a button in the main GUI to manually save the status variables during runtime of qudi. Furthermore, an option to enable automatic saving is implemented, which will periodically save the status variables with an interval, set by the user. By default the automatic dumping is turned off.

Motivation and Context

Qudi saves status variables of modules during deactivation of said module. When qudi crashes or a module is not correctly deactivated it may lead to status variables not being saved. To tackle this inconvenience a manual and automatic method of saving status variables during runtime is implemented.

How Has This Been Tested?

Tested on confocal setup using the iqo-modules scanning toolchain and performing a 2000 px x 2000 px scan and performing pulsed measurements using the pulsed toolchain.

Types of changes

  • Bug fix
  • New feature
  • Breaking change (Causes existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • I have documented my changes in /docs/changelog.md.
  • My change requires additional/updated documentation.
  • I have updated the documentation accordingly.
  • I have added/updated the config example for any module docstrings as necessary.
  • I have checked that the change does not contain obvious errors
    (syntax, indentation, mutable default values, etc.).
  • I have tested my changes using 'Load all modules' on the default dummy configuration.
  • All changed Jupyter notebooks have been stripped of their output cells.

@timoML timoML self-requested a review August 19, 2024 09:52
with self._lock:
for _, module in self.modules.items():
if module.is_active:
module.instance._dump_status_variables()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Evil access on private method. On the first glance, we might elevate dump_status_variables() to a public function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method has been elevated to a public function.

self.automated_status_variable_dumping_timer.stop()
try:
self.automated_status_variable_dumping_timer.timeout.disconnect()
except RuntimeError:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the origin of the RuntimeError?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the timer was already destroyed (or never activated), calling the disconnect function will result in a RuntimError. To not have this throw an Error during e.g. shutdown of Qudi, just do nothing.

"""
self._automated_status_variable_dumping_timer_interval = interval

def automated_status_variable_dumping_timer_interval_slot(self, interval):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a setter and a slot?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The slot is called by the signal from the GUI, as signals can't call setter methods. The setter method is there for a consistent cli property behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the slot method to only pass the variable on to the setter. The setter now handles the actual setting of the timer.

self.automated_status_variable_dumping_timer.start()

@property
def automated_status_variable_dumping_timer_interval(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Units of all qudi variables should be SI (preferred) or part of the variable name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the modulemanager only handles the interval in seconds.

return self._automated_status_variable_dumping_timer_interval

@automated_status_variable_dumping_timer_interval.setter
def automated_status_variable_dumping_timer_interval(self, interval):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be some value checking. From non-gui, interval=0 will probably cause issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I introduced an Exception that is thrown if the interval is <= 0.

@TobiasSpohn
Copy link
Contributor Author

I addressed the comments and merged main into this branch . Please check if other changes should be made.

@TobiasSpohn
Copy link
Contributor Author

On a sidenote I discovered that when using ModuleManager.automated_status_variable_dumping_timer_interval or ModuleManager.toggle_automated_status_variable_dumping from the ipython terminal, QT Warnings are raised:
QObject::killTimer: Timers cannot be stopped from another thread and QObject::startTimer: Timers cannot be started from another thread. This is due to the functions being called from the ipython terminal thread and not the main thread. This prohibits the use of CLI to control the automatic status variable dumping for now. Should I add a thread change to the functions or should we wait for the modulemanager overhaul?

@Neverhorst
Copy link
Member

Hi @TobiasSpohn ,
as discussed with you earlier, I cherry-picked the GUI changes into the upcoming major release in develop branch (see commits db3af14 and fbd0bd8)
The rest of this PR will be discarded during release should we merge it into main now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants