Addition of generic pump device

[Lars-Kool]

Hi Mark/Nico,

At our institute, I'm working on a project creating integrated microfluidic systems. One of the first extensions I wanted to create was the ability to control fluid flow through microfluidic channels (e.g. automatically control the size of droplets generated by a droplet generator using a simple PID controller). Since there are many different fluid controllers, with many different capabilities, I have developed a generic pump device (this pull request), which should cover all desires I (and the thread below) could come up with. The code is an extension of MM (no existing code is to be modified). Locally, I have successfully compiled it (it also passed the meson build test) and it is able to control a pressure controller and a demo syringe pump (device adapters coming in separate pull request).

Let me know what would be the next steps to get people to control fluids with MM.

All the best,
Lars

https://forum.image.sc/t/a-generic-pump-device-for-micro-manager-work-in-progress/44257/16

[Lars-Kool]

Hi @marktsuchida and @nicost,
Did anyone have the chance to have a look at the proposed additions?
Best,
Lars

[marktsuchida]

@Lars-Kool Sorry for not replying to this -- I need to dig myself out of a bit of a backlog. And thank you for the PR!

The code looks nice, so I think the main things to discuss are about API design. Please bear with me because my knowledge of the capabilities of these devices are somewhat limited. Here are my comments so far:

Would I be right that there are devices (syringe pumps?) that support both pressure control and volumetric dispensing? If so, it does make sense for a single device type to have functions for both, but it would probably be hard to write application code unless there is a way to check the device capabilities (e.g., does this channel support pressure control?). I would suggest adding functions for this, as well as for the other optional capabilities. (Invoking an actual action and getting back DEVICE_UNSUPPORTED_COMMAND is not ideal, because usually you want to be able to check without potential side effects.)

Along the same lines, I would add a bool RequiresHoming() that returns true if the pump is essentially unusable until homed (lesson learned from the stage interfaces).

Presumably (correct me if I'm wrong) the device operates either in constant pressure or constant flow rate, but not both at the same time. There seems to be missing a way to tell the device which to use.

It looks like all of the member functions are optional, with a default implementation in DeviceBase (would be nice if they appeared in the same order so that it's easy to check the correspondence). The default implementation should not be present for any functions that are mandatory. In particular, I think it would be best for Stop to be mandatory, if nothing else. (And the capability-querying functions mentioned above should of course be mandatory.)

This is a bit of bikeshedding, but I think "dispense" suggests pumping a prescribed amount (volume or duration). If Dispense() simply runs the pump till it hits the end of the syringe (or forever for peristaltic pumps), perhaps something like Start() or Pump() would be better.

---

• Device interface version will need to be bumped when merging (we may coordinate this with other changes)

[Lars-Kool]

@marktsuchida Thanks for taking the time to go through the code!

There are no devices that support both volumetric and pressure control directly, but it is possible indirectly. Some manufacturers (e.g. Fluigent) allow you to couple a flow-sensor to a pressure controller and have an internal PID-controller figure out the pressure for a provided flow rate. Hence, I thought it would be beneficial to have both in one device.

I see what you are saying that returning DEVICE_UNSUPPORTED_COMMAND is undesirable. I'll add functions to check for device capabilities, and a flag to indicate the mode of operation. Same with bool RequiresHoming(), it completely slipped my mind that some pumps might be inoperable before homing (the ones I tested all didn't need homing). I will also take a look at which functions to make mandatory (Stop(), and the functionality checks are definitely on the list). I'm also in favor of renaming Dispense() to Start().

[Lars-Kool]

Hi @marktsuchida,

After some more reflection on your comments, I agree that I is better to split pressure pumps and volumetric pumps. Although there are some pumps that are able to work in either mode (but not both at the same time), I cannot find a use case to be able to quickly change between the two modes (i.e. without having to reload the MM configuration). Hence, it makes more sense to split the two devices to simplify the interface, and require less probing for functionality.

Best,
Lars

[Lars-Kool]

Hi @marktsuchida, I hope you had a great summer. Did you have a chance to have a look at my pull request?
Best,
Lars

[marktsuchida]

@Lars-Kool Sorry, not yet (wasn't sure if you were still working on it). Happy to take a look, but hope you can give me a week or two (a lot of things happening at the moment including travel next week). Keeping this in my inbox.

[Lars-Kool]

@marktsuchida, of course, no problem. I made some changes a couple of weeks ago and undid them (I noticed I was working on the wrong branch). It runs fine locally, I already wrote some device adapters and a simple GUI, but I will PR them once this is resolved, to prevent any inter-PR dependence

[marktsuchida]

Apologies for the very long delay! This is looking very good, I just have a few comments:

• I don't think we need the concept of the "current" pump devices in MMCore ({get|set}{Volumetric|Pressure}PumpDevice()). All of the CMMCore functions you added take the pump device label as the first argument, so there is no scenario in which the "current" pump is used. And presumably it is quite possible to have multiple pumps in use, with no particular one being the primary one in any sense (correct me if I'm wrong).

• Can function names follow the existing convention for capitalization? CMMCore methods should start with lower case (for example, pressurePumpStop(), not PressurePumpStop()).

• I like how you added the unit suffixes (Ul, UlPerSecond) to the method names. Would you consider also adding them to the remaining methods? (setPressureKPa getPressureKPa dispenseDurationSeconds dispenseVolumeUl) I know the parameters are named with the units, but that would not be visible in code that calls these functions. Perhaps especially important for dispenseDuration because all other time parameters elsewhere in MMCore are in milliseconds.

• Could you remove the (presumably spurious) changes to micromanager.sln?

Other than that, this is looking very nice and clean! Appreciate your patience; I'll try to get back to you sooner next time.

[Lars-Kool]

Thanks for the comments! They all make perfect sense. I should be able to implement them either today or tomorrow.

[Lars-Kool]

@marktsuchida, I managed to sync the fork, and implement the changes.

[marktsuchida]

@Lars-Kool Thanks!

It looks like something went wrong with the branch history and your 3 actual commits appear on two branches that got merged. Do you think you can clean this up? I'm also happy to attempt a cleanup and force-push to your branch.

Here is the gitk view:

It looks like you rebased your 3 commits onto the current (official) main, but then that got merged with the corresponding 3 commits that were on your older branch -- though I'm not 100% sure what exactly happened. Right now the end result contains unrelated commits from other PRs, as you can see under Commits of this PR.

[Lars-Kool]

@marktsuchida I managed to squash the changes. I have no idea why these commits are visible here. If you could have a look, that would be great.

[marktsuchida]

@Lars-Kool I force-pushed your 3 commits from before the last time you force-pushed (I had kept these locally from yesterday). Hopefully this is exactly the changes you intended (I did check that the code matches what you last pushed, leaving out changes to unrelated files); would be great if you could confirm. There is no need to squash or merge main into your branch.

If it looks good, I will be pushing another commit to bump the version numbers before finally merging.

[Lars-Kool]

@marktsuchida I recompiled everything and tested a device adapter I had written, and all seems to be working as intended!

[Lars-Kool]

Hi @marktsuchida, any idea when you will be merging this pull request?