Discussion of callback system in MMCore

[ianhi]

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:

• I open jupyter lab
• From that notebook I launch napari-micromanager
• I do a mix of scripting in the notebook, control of the microscope by hand, and control through the napari GUI

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 #150 (comment) and #150 (comment)

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 Add snap() method #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]

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]

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]

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 (#86 (comment)) 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]

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]

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

mmCoreAndDevices/MMCore/MMCore.cpp

Line 726 in ab2fc60

void CMMCore::assignDefaultRole(boost::shared_ptr<DeviceInstance> pDevice)

onSystemReset

mmCoreAndDevices/MMCore/MMCore.cpp

Line 872 in ab2fc60

LOG_INFO(coreLogger_) << "System reset";

device initliazation

a callback per device and one for the final log statemnt:

mmCoreAndDevices/MMCore/MMCore.cpp

Lines 897 to 906 in ab2fc60

	LOG_INFO(coreLogger_) << "Will initialize device " << devices[i];
	pDevice->Initialize();
	LOG_INFO(coreLogger_) << "Did initialize device " << devices[i];
	assignDefaultRole(pDevice);
	}
	LOG_INFO(coreLogger_) << "Finished initializing " << devices.size() << " devices";
	updateCoreProperties();

system cache updates

mmCoreAndDevices/MMCore/MMCore.cpp

Lines 965 to 974 in ab2fc60

	void CMMCore::updateSystemStateCache()
	{
	LOG_DEBUG(coreLogger_) << "Will update system state cache";
	Configuration wk = getSystemState();
	{
	MMThreadGuard scg(stateCacheLock_);
	stateCache_ = wk;
	}
	LOG_INFO(coreLogger_) << "Did update system state cache";
	}

waiting for a device
Particularly useful in any multi threaded downstream applications

mmCoreAndDevices/MMCore/MMCore.cpp

Lines 1203 to 1230 in ab2fc60

	LOG_DEBUG(coreLogger_) << "Waiting for device " << pDev->GetLabel() << "...";
	MM::TimeoutMs timeout(GetMMTimeNow(),timeoutMs_);
	while (true)
	{
	{
	mm::DeviceModuleLockGuard guard(pDev);
	if (!pDev->Busy())
	{
	break;
	}
	}
	if (timeout.expired(GetMMTimeNow()))
	{
	string label = pDev->GetLabel();
	std::ostringstream mez;
	mez << "wait timed out after " << timeoutMs_ << " ms. ";
	logError(label.c_str(), mez.str().c_str());
	throw CMMError("Wait for device " + ToQuotedString(label) + " timed out after " +
	ToString(timeoutMs_) + "ms",
	MMERR_DevicePollingTimeout);
	}
	sleep(pollingIntervalMs_);
	}
	LOG_DEBUG(coreLogger_) << "Finished waiting for device " << pDev->GetLabel();

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

mmCoreAndDevices/MMCore/MMCore.cpp

Lines 1334 to 1341 in ab2fc60

	void CMMCore::setPosition(const char* label, double position) throw (CMMError)
	{
	boost::shared_ptr<StageInstance> pStage =
	deviceManager_->GetDeviceOfType<StageInstance>(label);
	LOG_DEBUG(coreLogger_) << "Will start absolute move of " << label <<
	" to position " << std::fixed << std::setprecision(5) << position <<
	" um";

stopping stage

mmCoreAndDevices/MMCore/MMCore.cpp

Lines 1607 to 1617 in ab2fc60

	LOG_DEBUG(coreLogger_) << "Will stop stage " << label;
	mm::DeviceModuleLockGuard guard(zStage);
	int ret = zStage->Stop();
	if (ret != DEVICE_OK)
	{
	logError(label, getDeviceErrorText(ret, zStage).c_str());
	throw CMMError(getDeviceErrorText(ret, zStage));
	}
	LOG_DEBUG(coreLogger_) << "Did stop stage " << label;

homing stages

In particular if a downstream library intiates a home it would be nice to have some way (probably ideally awaitable) to know that it finished.

mmCoreAndDevices/MMCore/MMCore.cpp

Lines 1661 to 1679 in ab2fc60

	LOG_DEBUG(coreLogger_) << "Will home stage " << label;
	mm::DeviceModuleLockGuard guard(zStage);
	int ret = zStage->Home();
	if (ret != DEVICE_OK)
	{
	logError(label, getDeviceErrorText(ret, zStage).c_str());
	throw CMMError(getDeviceErrorText(ret, zStage));
	}
	LOG_DEBUG(coreLogger_) << "Did home stage " << label;
	return;
	}
	boost::shared_ptr<XYStageInstance> xyStage =
	boost::dynamic_pointer_cast<XYStageInstance>(stage);
	if (xyStage)
	{
	LOG_DEBUG(coreLogger_) << "Will home xy stage " << label;

setting origins
I can definitely see this class of callback becoming useful at some point.

mmCoreAndDevices/MMCore/MMCore.cpp

Lines 1705 to 1718 in ab2fc60

	void CMMCore::setOriginXY(const char* label) throw (CMMError)
	{
	boost::shared_ptr<XYStageInstance> pXYStage =
	deviceManager_->GetDeviceOfType<XYStageInstance>(label);
	mm::DeviceModuleLockGuard guard(pXYStage);
	int ret = pXYStage->SetOrigin();
	if (ret != DEVICE_OK)
	{
	logError(label, getDeviceErrorText(ret, pXYStage).c_str());
	throw CMMError(getDeviceErrorText(ret, pXYStage).c_str(), MMERR_DEVICE_GENERIC);
	}
	LOG_DEBUG(coreLogger_) << "Zeroed xy stage " << label << " at current position";

snapImage
I added this pymmcore-plus already because it was useful, but ideally it would live in core. See discussion in #150

mmCoreAndDevices/MMCore/MMCore.cpp

Lines 2389 to 2397 in ab2fc60

	LOG_DEBUG(coreLogger_) << "Will snap image from current camera";
	ret = camera->SnapImage();
	if (ret == DEVICE_OK)
	{
	LOG_DEBUG(coreLogger_) << "Did snap image from current camera";
	}
	else
	{
	LOG_ERROR(coreLogger_) << "Failed to snap image from current camera";

imageSynchoList updated
dunno what exactly this is but could see a gui wanting to keep track of this.

mmCoreAndDevices/MMCore/MMCore.cpp

Line 2506 in ab2fc60

LOG_DEBUG(coreLogger_) << "Autoshutter turned " << (state ? "on" : "off");

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.

mmCoreAndDevices/MMCore/MMCore.cpp

Lines 2468 to 2490 in ab2fc60

	LOG_DEBUG(coreLogger_) << "Added " << label << " to image-synchro list";
	}
	/**
	* Removes device from the image-synchro list.
	* @param label the device label
	*/
	void CMMCore::removeImageSynchro(const char* label) throw (CMMError)
	{
	boost::shared_ptr<DeviceInstance> device = deviceManager_->GetDevice(label);
	imageSynchroDevices_.erase(std::remove_if(imageSynchroDevices_.begin(),
	imageSynchroDevices_.end(), DeviceWeakPtrInvalidOrMatches(device)),
	imageSynchroDevices_.end());
	LOG_DEBUG(coreLogger_) << "Removed " << label << " from image-synchro list";
	}
	/**
	* Clears the image synchro device list.
	*/
	void CMMCore::removeImageSynchroAll()
	{
	imageSynchroDevices_.clear();
	LOG_DEBUG(coreLogger_) << "Cleared image-synchro list";

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.

mmCoreAndDevices/MMCore/MMCore.cpp

Lines 2764 to 2767 in ab2fc60

	LOG_DEBUG(coreLogger_) << "Will start sequence acquisition from default camera";
	int nRet = camera->StartSequenceAcquisition(numImages, intervalMs, stopOnOverflow);
	if (nRet != DEVICE_OK)
	throw CMMError(getDeviceErrorText(nRet, camera).c_str(), MMERR_DEVICE_GENERIC);

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]

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]

@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]

@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.