micro-manager / mmCoreAndDevices

Micro-Manager's device control layer, written in C++
39 stars 101 forks source link

`setConfig` should always trigger an `onConfigGroupChanged` or a new signal should be created #25

Open ianhi opened 3 years ago

ianhi commented 3 years ago

Problem

I would expect the MMEventCallback's onConfigGropChanged method to be always be called whenever MMCore:SetConfig has been called. However, this is not the case as the only place that callback is called from is here:

https://github.com/micro-manager/mmCoreAndDevices/blob/0e5ba3c139ad5fab40aeec58bdd57c91f3c9c474/MMCore/CoreCallback.cpp#L496-L506

This means that if you follow the demo config and have a group of a channel with only a filter cube in it then any callbacks you have set up will not fire.

Here is an example using pymmcore:

# from pymmcore_plus import CMMCorePlus
import pymmcore

# core = CMMCorePlus()
core = pymmcore.CMMCore()

mm_path = .... #"/usr/local/lib/micro-manager"
core.setDeviceAdapterSearchPaths([mm_path])

cb = pymmcore.MMEventCallback()
core.registerCallback(cb)

core.loadSystemConfiguration('demo_config.cfg')

# switch twice to make sure we get at least one switch
core.setConfig('Channel', "DAPI")
core.setConfig('Channel', "FITC")

# you will not see any printouts relating to config changes

A use case for the callback always firing is in napari-micromanager where the core state may also be updated by a different python process and it's important for the napari gui to stay up to date.

Proposed Solution(s)

Either of or some combination of:

  1. Remove the check of < 1 to allow for notifying when the group only has one property.
  2. emit this signal in the SetConfig method and then silence/remove the subsequent emission from SetProperty
  3. Add a new signal onConfigSet which fires

For my money the best option is combine 1 and 3. The onSet is different from onChanged in that the former would only be sent by SetConfig and the latter can be triggered by any property change so it makes sense to have both. As for option 1: it seems that this check was added to support micromanager, but is there any reason that that logic couldn't live in micromanager?

marktsuchida commented 3 years ago

Related: https://forum.image.sc/t/micromanager-events-core-events-not-coming-through/53014/6

I think adding a new event with a well-defined behavior is doable. Changing the behavior of the existing onConfigGroupChanged is tricky -- even though it doesn't fully make sense, there is complicated GUI code that depends on that behavior.

ianhi commented 3 years ago

I think adding a new event with a well-defined behavior is doable. Changing the behavior of the existing onConfigGroupChanged is tricky -- even though it doesn't fully make sense, there is complicated GUI code that depends on that behavior.

Sounds good to me. How about mirror the signature of onConfigGroupChanged and having: onConfigSet(char* groupName, char* configName) ? This would be called directly from SetConfig and nowhere else.

I'm happy to open a PR if that sounds good.

(also cc @tlambert03 as this would affect both pymmcore-plus and napari-micromanger)

nicost commented 3 years ago

Not so sure if I am a fan of adding a second, almost similar callback is a great idea. IT may be better to change the GUI code to work with the "correct" behavior, rather than introduce yet another way of doing things. However, the main problem may be that a configuration group could be changed by an action started by the user in Micro-Manager, or by the state of one of the hardware components changing (for instance by pressing a button on the microscope), and that specific hardware component calling back to the core to tell it about its state change. Problem is that it is not known which hardware components do callback, and which ones do not, hence it is hard to avoid callbacks being fired more often than needed.

It may be best to test what happens (at least with the demo configuration) when the onConfigGroupChanged callback is always called (possibly sometimes twice). There may need to be a mechanism to stop the code from going out to the hardware to figure out the new state (as the GUI code does not do this when an acquisition is running).

marktsuchida commented 3 years ago

I didn't mean that we should maintain two variants in the long term (though I should have been clearer). We could add a new function, with a different name, perhaps initially documented as experimental, and iterate over it until we are happy with its interface (because we certainly want to minimize the number of times we make these changes). Then we can update our GUI code to use the new function, and deprecate the existing onConfigGroupChanged (and possibly eventually remove it).

That way, third-party code that depends on the current behavior will not be affected, and its developers will get notified (by deprecation warnings and later compile errors) of the need to review/change.

If we change the behavior of onConfigGroupChanged, it will likely induce infinite loops in code that depends on the current behavior. And there is no way to "test" this, because we don't have access to all third party code that may use this function.

ianhi commented 3 years ago

Ah so part of the issue is that I was misunderstanding the intended purpose of the onConfigGroupChanged. Is there documentation somewhere of what these events are meant to represent? My best understanding was based on the name which I took to mean that a new config group had been assigned. Based on

However, the main problem may be that a configuration group could be changed by an action started by the user in Micro-Manager, or by the state of one of the hardware components changing (for instance by pressing a button on the microscope)

I see now how it's really reporting that one of the properties in that config group has been changed.


Anecdotally I have experienced switching a config group in MM, having the hardware swap and then have the group preset go blank. I suppose that whats happening there is that getConfigFromCache is returning an empty string?

marktsuchida commented 3 years ago

The preset goes blank when the current state of device properties doesn't match any of the presets in the group (which is not in itself an error). This can happen even right after setting a preset, with some devices that change property values in interdependent ways. I suppose it could also have something to do with the cache, as you suggest. It's hard to say more without the specific details.

nicost commented 3 years ago

I have seen this happen on a Nikon Ti. It is confusing to the users. Earlier versions of MM (2.0 beta from 2016 or so) did not have this behavior. I mean to get back to it, but these things are very difficult to trouble shoot since stepping through a debugger will result in different behavior. It may be that the first event triggers a system cache update, that puts an intermediate state into the cache, and while that is going on, the event that a certain device reaches its destination is ignored. Just speculating, plus I need to get back to that somehow.