add callbacks for device (un)loading

[ianhi]

Closes: #149

Couldn't test locally due to my struggles with pymmcore described here: micro-manager/pymmcore#62 tested locally, seems to be work as expected.

I have two main worries:

1. What happens if we send a deviceWillBeUnloaded callback and then device unload fails
2. Can devices ever be unloaded other than through core? LIke can they trigger their own unloading?

[ianhi]

Ideally I think this would be made irrelevant by the single device unloading signal. But that doesn't look like it's possible without pushing the external callback into the deviceManager which currently doesn't happen. Happy to do that, but figured Id start with the less invasive option.

[marktsuchida]

One problem is that devices are not available for interaction until they are initialized. Would it make more sense to have onDeviceDidBecomeAvailable and onDeviceWillBecomeUnavailable, and issue the former when initialization succeeds (and, technically, the latter before shutdown)? Otherwise, I'm not sure how an application could make use of these events.

If you want to react to actual (uninitialized) loading of devices (useful iff you want to modify pre-init properties), there could be a third event. The only use case I can think of is an implementation of a hardware configuration GUI, but even then, the GUI (being the one calling loadDevice()) probably doesn't need such an event from the Core.

Also, the events need to be generated on code paths that are guaranteed to be taken when devices are initialized/shut down. This probably means in DeviceManager.

I agree that there should only be events issued for each device individually. There is no need for an "unload all devices" event, and clients listening to these new events should ignore the (now redundant) config loaded event.

[ianhi]

Thinking about this more (xref to #164) is it even ok to just add callbacks like this? I think with how the callback system is set up adding any new callback is an immediate backwards compatibility break. If you subclass MMEventCallback then downstream users are safe, but if not and you register a completely custom callback then when these get called they will result in errors.

Would it make more sense to have onDeviceDidBecomeAvailable and onDeviceWillBecomeUnavailable, and issue the former when initialization succeeds

That sounds reasonable. What's the difference between loaded and initialized? I naively assumed loaded meant ready for use.

Also, the events need to be generated on code paths that are guaranteed to be taken when devices are initialized/shut down. This probably means in DeviceManager.

Sounds good. Is the way to do that to pass a pointer to the externalCallback into the manager constructor here?

mmCoreAndDevices/MMCore/MMCore.cpp

Line 143 in ab2fc60

deviceManager_(new mm::DeviceManager()),

[marktsuchida]

Devices cannot trigger their own unloading currently, other than by failing to initialize. If we add such a feature in the future (e.g. to react to somebody unplugging the USB cable -- some scopes can notify us of this), we can deal with it then. But it is unlikely, because unexpected unloading would break all existing code, such as MMStudio.

Although unloading itself (the device object's destructor) never "fails", it is possible for the device's Shutdown() to fail. However, there is nothing that can be done in such a case other than log and continue. The device may have shut down, or may be in an inconsistent state from which recovery is not possible. So if the event is named onDeviceWillBecomeUnavailable, it will be correct to have generated it.

[marktsuchida]

I think with how the callback system is set up adding any new callback is an immediate backwards compatibility break.

I don't think so -- MMEventCallback is not abstract; it provides default implementations of each virtual member function. So one can derive from it but omit overrides of some of its members, only requiring recompilation. Hopefully with MMCoreJ we won't even require recompiling MMStudio, although we should test this before merging -- it's been a while since I thought through this.

For pymmcore there is no worry, because the mmCoreAndDevices submodule in that repo is not automatically updated. We'll just release a new version of pymmcore when the time comes.

We don't really guarantee anything about backward binary compatibility of MMCore as a C++ library (which we don't officially support building). Please see the comment in the code regarding versioning.

mmCoreAndDevices/MMCore/MMCore.cpp

Line 82 in ab2fc60

* Important! Read this before changing this file:

The MMCore minor version should be bumped in this PR assuming I'm right.

(Some day we'll need separate version numbers for MMCore vs MMCoreJ vs pymmcore.)

EDIT: To clarify, I don't think we need to support backward compatibility for the unusual use case of directly instantiating the base MMEventCallback class and registering it. But even that would only be a problem if using directly from C++ code, and would only require recompiling.

[marktsuchida]

What's the difference between loaded and initialized?

The device objects use a two-step initialization process. The first step is the call to the global function CreateDevice() of the device adapter module. This is almost always implemented by constructing the device object. Although this can fail (e.g. if an invalid device name was given), it cannot return error information, so should avoid actually talking to the device if possible.

The second step is to call the device object's Initialize(), which usually does the actual work of trying to establish connection to the device (though there are exceptions). Initialize() can return an error code.

Between the two steps, the app can modify the values of the device's "pre-initialization properties". Regular properties can only (correctly) be accessed after Initialize() has succeeded.

When tearing down, first Shutdown() is called to finish talking to the device and disconnect. Then the module's global DeleteDevice() is called, which almost always calls the device object's destructor, but usually by this time all that is left to do is deallocate memory (which is good, because DeleteDevice() cannot report an error). But unlike when loading, these two steps are not exposed by the MMCore API.

[marktsuchida]

Is the way to do that to pass a pointer to the externalCallback into the manager constructor here?

Yes, but it would be a violation of separation of concerns for the DeviceManager to know about MMEventCallback. Also, the externalCallback_ of CMMCore is set by the app and can change over time.

The easiest thing to do would be to commit the even greater architectural crime of passing the pointer to CMMCore to the DeviceManager constructor. A better way might be to create a small interface containing the functions that DeviceManager needs to call on CMMCore -- i.e., a sort of DeviceManagerEventCallback -- that CMMCore can implement.

[ianhi]

I think with how the callback system is set up adding any new callback is an immediate backwards compatibility break.

I don't think so -- MMEventCallback is not abstract; it provides default implementations of each virtual member function. So one can derive from it but omit overrides of some of its members, only requiring recompilation. Hopefully with MMCoreJ we won't even require recompiling MMStudio, although we should test this before merging -- it's been a while since I thought through this.

Ah I missed the type requirements of register callback. I mistakenly thought you could avoid subclassing.

Please see the comment in the code regarding versioning.

ah missed that! I'll bump the minor version.

Thanks for the description of initialized vs loaded! At some point we should gather up all your comments on these PRs and put them into the docs. They have been extremely helpful and it would great to centralize them.

[ianhi]

A better way might be to create a small interface containing the functions that DeviceManager needs to call on CMMCore -- i.e., a sort of DeviceManagerEventCallback -- that CMMCore can implement.

So MMCore should be a callback object that implements something like:

private void onDeviceDidBecomeAvailable(...){
   if (externalCallback_ != 0{
      ....
   }
}

and declare the DeviceManager as a friend class?

also thanks for all your c++ help. I'm definitely learning as I go here and I really appreciate the guidance. That said I know that teaching c++ is probably not what you signed up for, so I'll try to spend some time reading up on c++ (any recommendations?)

[marktsuchida]

At some point we should gather up all your comments on these PRs and put them into the docs. They have been extremely helpful and it would great to centralize them.

Good to hear. I'm definitely writing partially with that intent. Updating (or replacing) the how-to-write-device-adapters docs is something I've never gotten around to since starting working on Micro-Manager. It's daunting to me due to all the corner cases, but easier to write single-topic paragraphs like this.

[ianhi]

It's daunting to me due to all the corner cases, but easier to write single-topic paragraphs like this.

Don't let the perfect be the enemy of the good!

Maybe we should create an issue on the docs repo and use it as a place to link to all these comments so that we don't lose track of them?

[marktsuchida]

So MMCore should be a callback object that implements something like:

private void onDeviceDidBecomeAvailable(...){
   if (externalCallback_ != 0{
      ....
   }
}

and declare the DeviceManager as a friend class?

The private method of MMCore can look like that, but you can avoid using friend now that we have modern C++. Something like:

// In DeviceManager.h
struct DeviceManagerCallback
{
   boost::function<std::string> deviceDidBecomeAvailableFunc;
   boost::function<std::string> deviceWillBecomeUnavailableFunc;
};

// Called from CMMCore constructor and return value passed to DeviceManager constructor
private DeviceManagerCallback CMMCore::makeDeviceManagerCallback()
{
   DeviceManagerCallback callbacks;

   // Use a lambda expression to set the functions:
   callbacks.deviceDidBecomeAvailableFunc = [this](const std::string& deviceName) {
      onDeviceDidBecomeAvailable(deviceName);  // Or write the code here to call externalCallback_
   };

   // Likewise for deviceWillBecomeUnavailableFunc

   return callbacks;
}

(boost::function works like std::function; at some point we'll switch to std but best to mimic surrounding conventions for now.)

I would also be okay with a friend-based implementation: just don't friend the base class (DeviceManagerCallback) but friend a derived class of it that is internal to CMMCore.

also thanks for all your c++ help. I'm definitely learning as I go here and I really appreciate the guidance. That said I know that teaching c++ is probably not what you signed up for, so I'll try to spend some time reading up on c++ (any recommendations?)

It's been a pleasure, since you ask specific questions about concrete code!

It's been a long time since I first learned C++ (mid-to-late '90s), so I don't know what is a good book these days. There is the added challenge that you should be learning modern C++ (C++11 and later), but much of the existing MM code is written in '80s or '90s style C++.

I always look things up on cppreference.com. The "Back to the Basics" videos on the CppCon Youtube channel are excellent, though some of them might be more in-depth than you need.

[marktsuchida]

See also #164 (comment) regarding how these callbacks may require different concurrency considerations from existing ones.