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

Fix behavior of Core "Initialize" vs device initialization state #397

Open
5 tasks
marktsuchida opened this issue Nov 1, 2023 · 4 comments
Open
5 tasks
Assignees

Comments

@marktsuchida
Copy link
Member

marktsuchida commented Nov 1, 2023

Issues reported by @tlambert03 in #384 (comment).

  • Setting Core-Initialize to 1 when any (non-Core) device has already been initialized throws an exception -> need to fix this by telling initializeAllDevices() to skip over devices that are not in the Uninitialized state document that this is expected behavior
  • When setting Core-Initialize results in an error, we should avoid changing its value (for what is's worth; see below)
  • getDeviceInitializationState('Core') fails -> it should always return InitializedSuccessfully
  • We should also check that the load/unload/initialize functions do something reasonable when called on Core

Initialize is a pseudo-property that does not encode any real state (other than the last written value) but simply performs actions when being set (1 = initializeAllDevices(); 0 = unloadAllDevices(); the actions are not even symmetric!). Not sure why we even have this, other than as a messy hack to be able to unload/initialize all devices in a single command in config files (it probably would have been better if we just had dedicated config file commands for the two actions).

Note that reading the value of Initialize will only read out what was last set, regardless of whether direct calls to initializeAllDevices() or unloadAllDevices() were made in the interim. So the read value is not particularly useful.

Also note that the Initialize property has nothing to do with the initialization state of the Core device itself. The Core device is always loaded and always initialized successfully and cannot be unloaded. (Things wouldn't work otherwise.)

  • Put the above notes somewhere as documentation for the Initialize property
@marktsuchida marktsuchida self-assigned this Nov 1, 2023
@marktsuchida
Copy link
Member Author

Or should it remain an error to try to "initialize all devices" when not all devices are Uninitialized? That also seems reasonable behavior, given how it is actually used. Then we simply document this also for the Initialize property.

@tlambert03
Copy link
Contributor

tlambert03 commented Nov 1, 2023

Initialize is a pseudo-property that does not encode any real state (other than the last written value) but simply performs actions when being set (1 = initializeAllDevices(); 0 = unloadAllDevices(); the actions are not even symmetric!). Not sure why we even have this, other than as a messy hack to be able to unload/initialize all devices in a single command in config files (it probably would have been better if we just had dedicated config file commands for the two actions).

yeah makes sense, I understand now. Also makes sense to probably just stick with it now, no sense in trying to change the config file command format

Note that reading the value of Initialize will only read out what was last set, regardless of whether direct calls to initializeAllDevices() or unloadAllDevices() were made in the interim. So the read value is not particularly useful.

yep, i see. one likely wouldn't ever run into this except in the MMStudio config wizard, or in the pymmcore-widgets config wizard. (and even then, only in tests when asserting that the state of the microscope model and the MMCore object are indeed the same). Something I'm more than happy to just special case in my tests.

Or should it remain an error to try to "initialize all devices" when not all devices are Uninitialized? That also seems reasonable behavior, given how it is actually used. Then we simply document this also for the Initialize property.

given my current understanding. I think it's probably reasonable enough just to make a note something along the lines of "think of setProperty('Core', 'Initialize', 1) as a command alias for initializeAllDevices() ... and discourage its use in anything but config files (or in declarative models of the core state). I bet it would be more problematic to try to change the semantics of what initializeAllDevices actually means.

@marktsuchida
Copy link
Member Author

Yeah, keeping the behavior seems best; edited the original post accordingly.

@marktsuchida
Copy link
Member Author

Currently these result in errors, which is good:
initializeDevice("Core") -> No device with label "Core"
unloadDevice("Core") -> No device with label "Core"

These are okay but maybe we'll special case them so that
initializeDevice("Core") -> Device already initialized
unloadDevice("Core") -> The Core device may not be unloaded

(loadDevice() is not relevant here because there is no way to call it to load "Core")

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