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

onSystemConfigurationLoaded upon failed loading of config file causes problems #594

Open
marktsuchida opened this issue Mar 12, 2025 · 1 comment

Comments

@marktsuchida
Copy link
Member

Currently, CMMCore::loadSystemConfiguration() calls the event callback onSystemConfigurationLoaded even if there was an error while executing the config file. And it does so before proceeding to unload the devices that were loaded up to the point of the error.

This causes problems because a handler for the event has no way of knowing whether the loading was successful or not. If the loading was unsuccessful, there may be uninitialized devices that cannot be safely accessed. (We ran into this scenario while testing pymmcore-gui on real hardware; it caused a segfault.)

There are several potential ways to fix this, but here is what I propose:

  • Upon successful completion of loading, emit onSystemConfigurationLoaded (no change)
  • Upon error during loading, do not emit onSystemConfigurationLoaded
  • Upon unloading all devices (including as a consequence of failed config loading) emit onSystemConfigurationLoaded (new)

These changes will effectively make the event mean "the current config has finished changing", which is probably how applications typically interpret it. The "no config" state is treated as a config in its own right (MMStudio doen't have a direct GUI way to reach that state, but a BeanShell command can cause it, and the GUI should reflect it).

A call to loadSystemConfiguration will still always end with firing onSystemConfigurationLoaded, but only after unloading the partially loaded devices.


There is just one problem with this idea: valid config files always contain the line Property,Core,Initialize,0 at the beginning, which causes all devices to be unloaded. Issuing onSystemConfigurationLoaded in this instance would be theoretically correct, but would break MMStudio, because removing the XY and Z stages will cause it to clear the position list, etc., which it will not restore even if the stages reappear.

To maintain backward compatibility, we should mask these events during the config loading itself, so that executing the lines of the config file never fire the loaded/willUnload events.


Additional ideas:

  • We could also add a new onSystemConfigurationWillUnload event, so that the app can bracket the time period during which the config is being changed (something that might be reflected in a GUI). Both loadSystemConfiguration and unloadAllDevices (and also setProperty("Core", "Initialize", 0) from outside config loading) would start by issuing this event.

  • It may be useful if the event(s) conveyed the path of the config file, and empty string when unloading all devices. Changing onSystemConfigurationLoaded would be breaking, so this should be done by deprecating that event and adding a new one (perhaps onSystemConfigurationDidLoad, which pairs with WillUnload)

  • In the longer term, it will be good to issue more fine-grained events per-device: didLoad/willInitialize/didInitialize/willUnload (Add callback for device (un)loaded #149).


Cc: @gselzer

@tlambert03
Copy link
Contributor

thanks for the writeup @marktsuchida, and thanks again for testing pymmcore-gui. Makes sense to me. I'm fine with all of the proposed fixes...

  • simply changing the behavior of onSystemConfigurationLoaded to also include unloadAllDevices (with a block at Property,Core,Initialize,0).
  • adding new did/will events as needed for finer grained control

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

No branches or pull requests

2 participants