micro-manager / mmCoreAndDevices

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

Discussion of callback system in MMCore #164

Open ianhi opened 2 years ago

ianhi commented 2 years ago

There has been some scattered discussion of whether new callbacks should added in a python wrapper or directly to the C++ core. I'd like to suggest using this issue as a place to centralize that and work out the future of potential new callbacks in MMCore.

My use case is that I want to interact with microscopes in the following way:

As far as I can tell this mixed methods approach means that in order to keep both the GUI and script up to date on the current state of harware/core state there will need to be a decent number of new signals introduced.

In a dream world it would be easy to add these to the C++ core as it would benefit all downstream users and be standardized. However, this brings a higher development cost than adding them to Python. That development cost is surmountable, but I think that @marktsuchida raised some important concerns in https://github.com/micro-manager/mmCoreAndDevices/issues/150#issuecomment-1045134968 and https://github.com/micro-manager/mmCoreAndDevices/issues/150#issuecomment-1045265839

Before I go off and try to start implementing signals somewhere it would be nice to work out what the best approach would be. To that end I think some of the key questions are:

  1. Do we try to put new callbacks in here - or should they go into python bindings/other wrappers?
    • If they go on the python side where do they actually live?
  2. If they live in C++ should the approach to callbacks be changed at all or is some re-architecturing in order?
    • Consider this quote from #150:

      "Although it would be nice if MMCore worked more like a general event dispatch mechanism, with more consistent behavior, unfortunately that work has not been done, and I'm not sure it would be best to try to accomplish that within its current (one might say broken) architecture."

  3. What callbacks should added/what is the standard for addition of a callback?

Downstream users (that I'm aware of): Python: @tlambert03 @fdrgsp @ianhi @oeway Python/Java: @henrypinkard Java: All already watching this repo I believe :) C/C++: @dpshepherd (I think c++?), @tractatus

dpshepherd commented 2 years ago

I'm adding @ptbrown1729 and @AlexCoul to this conversation, they are the main group members in my lab working on control code. Primarily, I believe we land into the Python user group, although we do some C++ and Python/Java work.

We are moving to unify our code into a single Napari-focused use case: One GUI widget per scope control module, where each widget interacts with external hardware and DAQ cards in a manner that often does not fit the "standard" micro-manager model. @ptbrown1729 has a fork of your napari-micromanager project that does this for setup and operation for a DAQ as master dual mode microscope.

tractatus commented 2 years ago

I agree with all of the above and can add that I think it will be difficult in the end to patch up some of the design limitations in MMCore from the higher-level Python/Java end with the wish to keep development cost surmountable. For example, being able to support simultaneous acquisition on two or more cameras/sensors will be pretty key for a lot of future technologies but currently, MMCore requires some hacks at the C++ level to overcome this.

I'm sort of in a daily discussion with myself if what I'm doing makes sense at all. Especially with all the closed drivers and everything making smooth builds and maintenance wishful thinking for a distant future, versus just designing software for single hardware implementation and doing that very very well and high-performance level. But that is a discussion for another day.

I do however strongly believe in what this thread tries to accomplish.

ianhi commented 2 years ago

For example, being able to support simultaneous acquisition on two or more cameras/sensors will be pretty key for a lot of future technologies but currently, MMCore requires some hacks at the C++ level to overcome this.

I'm sort of in a daily discussion with myself if what I'm doing makes sense at all. Especially with all the closed drivers and everything making smooth builds and maintenance wishful thinking for a distant future, versus just designing software for single hardware implementation and doing that very very well and high-performance level. But that is a discussion for another day.

I'm curious about both those hacks and that daily discussion. Perhaps open a new issue for those?

Also as for the smooth builds (https://github.com/micro-manager/mmCoreAndDevices/pull/86#issuecomment-984033653) a question for @marktsuchida is are there ways we could be help move that forward? I think several people on this thread have an interest in improving the situation and I for one would be happy to try to help make it happen.

marktsuchida commented 2 years ago

I can't say that I don't feel the same sentiments as @tractatus pretty often.

Focusing on notification/callback stuff for now (please feel free to create other issues -- an issue for multiple cameras appears to be missing even though we are well aware of the problems), I think there are some things that can be done in C++ that are worthwhile and may prolong the useful life of MMCore.

I feel that the devil is in the details, though. Apart from the ones already discussed, what sorts of callbacks are you envisioning? It would help to hear what notification capabilities you feel are lacking, even if you're not sure what the concrete callback signals should be. (I can think of too many.)

If it is only going to be a few more that are similar to the ones discussed, it's not hard to just tack on.

Somewhat more can be achieved after what I would consider reasonable effort by refactoring the internals of MMCore to make it more modular but preserving its API (basically, take logic out of CMMCore and put into modular components such as DeviceInstance (and new ones); CMMCore will then be a thin(ner) wrapper). Some of this work is needed to address other unrelated long-standing issues as well (error handling and multiple cameras among them).

At some point, though, adding notifications to the existing behavior may no longer be sufficient for correct concurrency management. That is the point where things really start to get hard.

None of our existing notifications (e.g. property changed) guarantee any specific concurrency behavior. They are fire-and-forget (nothing is guaranteed to wait for them to return), and are not guaranteed to be issued from any particular thread, although it wouldn't be hard to make them always fire from a dedicated thread.

It just occurred to me that the just-proposed device-will-become-unavailable (#163) departs from such behavior, because it needs to guarantee that the device will stay available while the callback runs. This makes me wonder if we should already start organizing these into two separate categories (potentially-asynchronous notification vs synchronous callback). The decision might be better informed if we think about other notifications/callbacks we want to add soon.

ianhi commented 2 years ago

Apart from the ones already discussed, what sorts of callbacks are you envisioning? It would help to hear what notification capabilities you feel are lacking, even if you're not sure what the concrete callback signals should be. (I can think of too many.)

So when looking at #163 I noticed that for each callback I added there was already a coreLogger call happening. I think these logs may be a good proxy for places where callbacks are useful. Here is a partial list of places I looked at. Most of them looked as though they could be conceivably useful to someone sometime.

Core logger that could be callbacks **all the default role assignments** https://github.com/micro-manager/mmCoreAndDevices/blob/ab2fc602e8a1bc9a8ae8c5fa997c0708d72e14af/MMCore/MMCore.cpp#L726 **`onSystemReset`** https://github.com/micro-manager/mmCoreAndDevices/blob/ab2fc602e8a1bc9a8ae8c5fa997c0708d72e14af/MMCore/MMCore.cpp#L872 **device initliazation** a callback per device and one for the final log statemnt: https://github.com/micro-manager/mmCoreAndDevices/blob/ab2fc602e8a1bc9a8ae8c5fa997c0708d72e14af/MMCore/MMCore.cpp#L897-L906 **system cache updates** https://github.com/micro-manager/mmCoreAndDevices/blob/ab2fc602e8a1bc9a8ae8c5fa997c0708d72e14af/MMCore/MMCore.cpp#L965-L974 **waiting for a device** Particularly useful in any multi threaded downstream applications https://github.com/micro-manager/mmCoreAndDevices/blob/ab2fc602e8a1bc9a8ae8c5fa997c0708d72e14af/MMCore/MMCore.cpp#L1203-L1230 **setPosition** Also for all variants. I'm less convinced of the need here as there is already `onStagePositionChanged` but it may also be nice to notify of intent https://github.com/micro-manager/mmCoreAndDevices/blob/ab2fc602e8a1bc9a8ae8c5fa997c0708d72e14af/MMCore/MMCore.cpp#L1334-L1341 **stopping stage** https://github.com/micro-manager/mmCoreAndDevices/blob/ab2fc602e8a1bc9a8ae8c5fa997c0708d72e14af/MMCore/MMCore.cpp#L1607-L1617 **homing stages** In particular if a downstream library intiates a `home` it would be nice to have some way (probably ideally `await`able) to know that it finished. https://github.com/micro-manager/mmCoreAndDevices/blob/ab2fc602e8a1bc9a8ae8c5fa997c0708d72e14af/MMCore/MMCore.cpp#L1661-L1679 **setting origins** I can definitely see this class of callback becoming useful at some point. https://github.com/micro-manager/mmCoreAndDevices/blob/ab2fc602e8a1bc9a8ae8c5fa997c0708d72e14af/MMCore/MMCore.cpp#L1705-L1718 **snapImage** I added this pymmcore-plus already because it was useful, but ideally it would live in core. See discussion in https://github.com/micro-manager/mmCoreAndDevices/issues/150 https://github.com/micro-manager/mmCoreAndDevices/blob/ab2fc602e8a1bc9a8ae8c5fa997c0708d72e14af/MMCore/MMCore.cpp#L2389-L2397 **imageSynchoList updated** dunno what exactly this is but could see a gui wanting to keep track of this. https://github.com/micro-manager/mmCoreAndDevices/blob/ab2fc602e8a1bc9a8ae8c5fa997c0708d72e14af/MMCore/MMCore.cpp#L2506 **autoshutter set** In the context of mixing a script and GUI this seems to be super important to the GUI. Although I guess this can also be tracked through the property callbacks. https://github.com/micro-manager/mmCoreAndDevices/blob/ab2fc602e8a1bc9a8ae8c5fa997c0708d72e14af/MMCore/MMCore.cpp#L2468-L2490 **sequenceAcquistion** all the life cycle events of sequence acquisition would be good for a GUI to know about. Again example of GUI is open and then user starts something from a script. https://github.com/micro-manager/mmCoreAndDevices/blob/ab2fc602e8a1bc9a8ae8c5fa997c0708d72e14af/MMCore/MMCore.cpp#L2764-L2767 **circular buffer** logs in a few places. Not super convicned that these need callbacks. But listing in the spirit of cataloging logger uses and I lost stream at occurance 73 of 126. Anyone feel free to edit to add more.
marktsuchida commented 2 years ago

I'm not so sure these logging lines are always a good proxy. It is not useful to know that the app set X (technically, the app already knows that); what you want is a notification that X changed, for any reason. (Which is why I mentioned that some refactoring of the code would help -- to provide code paths that are more unified.)

But these broadly seem to break up into two categories: "configuration changes", and "misc device actions that are similar to property changed but not expressed as properties". There's a lot to say about each event, but I don't think it is that hard to implement most of these (quite a bit of work, but not enough to say never). I do think it is a good idea to do some refactoring first, and definitely we should come up with a way to issue notifications with less boilerplate.

For the misc device actions, some warrant a callback from the device adapter, not just from inside MMCore, so that externally initiated actions can be reported where possible.

The point that we need to think carefully about the timing/concurrency implications stands.

For things like home, at least in principle, the willHomeStage event can carry a future so that it can be converted to an awaitable in Python (and Future or CompletionStage in Java). Details of how to make this work with SWIG and all will need to be worked out.

The system state cache is its own problem, because it is impossible to use it both correctly and efficiently. What would be better is a cache that allows invalidation of individual entries (so that they are fetched on the next update) but what we have is one where values are opportunistically updated on setting (which is not always correct) and on device notifications, with no way of telling if any of the entires are up to date. There is also the problem that only property values are cached (pity an everything-is-a-property approach was not taken in early MMDevice design). I slightly hesitate to add notifications that are tied to the current cache implementation, although it might not be that bad since a new implementation will probably need a new API anyway.

Image synchro is an ancient feature that ought to be deprecated and removed entirely.

nicost commented 2 years ago

@tractatus, I opened an issue concerning more native multi-camera support at #168, Very curious about your hacks. Also very motivated to get something done pretty soon.

tractatus commented 2 years ago

@nicost wow that is awesome! Ok I'll try to put together what I have sofar on my limited end of experimenting on a single system and ad to that issue. I know that both me and @kasasxav have a mutual interest in getting this off the ground. Im currently packing up moving from NYC postdoc back to Europe to start faculty position at the end of the month. Hopefully having some time freeing up to dig deeper into this beginning April when dust has settled.