-
Notifications
You must be signed in to change notification settings - Fork 128
WPI syringe pump adapter #616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The WPI device adapter is conform CVolumetricPump
… discharged constant.
- Avoid C-style casts - Remove redundant initialization to "" - Use const reference when passing as read-only parameter - etc.
|
Thanks, @Lars-Kool! I think it will be good to use a more specific name than I pushed some commits to:
Other than the naming issue, I think this is good to merge. I'll resolve the |
|
Hi @marktsuchida, Renaming the device adapter to There are arguments to be made to not crowd the list of available device adapters, as it can be hard to find the thing you want. But, there are also arguments to not hide functionality behind a generic name. Personally, I would create one hub per manufacturer, and then have the hub initialize all devices of that manufacturer. But, I leave that decision completely up to you. |
|
@Lars-Kool I agree that the list of adapters is in need of better organization, but I think we should try to achieve that in a way that doesn't dictate how we organize our code (for example by tagging device adapters by manufacturer, which is something I'm planning). For now, at least all WPI devices will be sorted under I think a single device adapter that controls a range of devices makes total sense if they use the same vendor SDK or serial command set. Otherwise it is usually not worth the added complexity and it's best to keep the code better separated. For example, it would be difficult to reorganize code in the device adapter if we needed to test several different devices (to which we probably don't have access) to exercise the different protocols each time. In the SDK case, requiring mutiple SDKs to be installed for development (or use!) would be problematic. The hub device was originally intended to handle the case where the hardware (or, in some cases, driver) requires making a single connection to control multiple devices -- the prototypical case being motorized microscope stands. It sometimes makes sense in other cases where enumerating available devices works best through a hub device (this often depends on the vendor API). But the DemoCamera adapter is not exactly exemplary, not being real hardware. I don't know WPI's product range very well, so just went by your source comment saying that this is curretnly for AL-series (=Aladdin) pumps. If there are other WPI pumps that use a similar command set, we should probably go with WPIPumps or WPISyringePumps, but I wasn't able to immediately find any non-Aladdin pumps with serial control -- please let me know if you're aware of any! If WPI adds non-"Aladdin" pumps in the future that use the same command set -- well in that case we'll just have to document that the new pumps use the "Aladdin command set" so it is supported by the same device adapter. (I do have some ideas around allowing device adapters to be renamed without breaking existing config files.) And, by the way, I don't have any preference between |
Makes sense! I found that WPI also have an SP series of syringe pumps (WPI SP-110) but they have different serial commands (and there is no documentation for some models...). So lets keep it to |
It's me again. I can finally start sharing the device adapter I've had running locally. This one is for WPI (World Precision Instruments) syringe pumps.