-
Notifications
You must be signed in to change notification settings - Fork 111
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
Standard properties for device adapters #584
base: main
Are you sure you want to change the base?
Conversation
Those comments described the requirement prior to C++11, so yes, it's fine.
I think it is fine to do it in MMCore -- in fact it gives me more confidence because it can't be messed with by the device adapter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments.
I think the major issue that needs to be solved is how we support future migration upon adding new standard properties. If we have a YES/NO distinction between implementing or not implementing all required standard properties, it will be very difficult to ever add any new required ones; this then calls into question the point of having required standard properties in the first place.
There may certainly be some properties that require others to also exist, but that kind of complex check is probably best limited to runtime.
I view the requirement as useful for when making new device types/APIs. I think that using the No need to merge this now. I will use it when developing the camera API and see if I get further insights about how best to structure it |
…d add explicit skipping
In implementing the new camera API, I've gained more clarity on how to effectively implement standard properties: I've removed the required field entirely. All standard properties for a device type now default to being required, but this requirement can be bypassed by calling a The design allows declaring a set of standard properties for a device type (e.g.,
This approach accomplishes several goals:
For application layer code, checking for standard property support is straightforward: Finally, I added the ability to delete a standard property, because I noticed on the Basler camera that the existence of properties often depended on the values of others. Nothing else to do on this on my end. Ready to merge, or happy to make further refinements if needed. |
Standard Properties for Device Adapters
This PR introduces a standard properties system to Micro-Manager device adapters, addressing #258
Motivation
Currently, standardized functionality across devices is only expressed through methods in different device types. However, many features are more naturally expressed as properties (eg camera triggering in the new camera API #243). Furthermore, many device adapters have common functionality that is accessed through properties with identical or nearly identical names (e.g. Gain in a camera). This PR allows certain properties to be made part of the API.
To the higher-level application, standard properties function just like regular device properties, only with an
api//
prefix, signaling that they have consistent meaning across different device adapters. This is thus for now a fully backwards-compatible, opt-in mechanism. Regular properties without theapi//
prefix will continue to work indefinitelyFor device adapter developers, each standard property has a dedicated creation method (e.g.,
CreateTriggerTypeStandardProperty
) that can only be called in device types to which it is applicableTwo dummy test standard properties are implemented in this PR, added to the demo camera
How to Create a New Standard Property
LINK_STANDARD_PROP_TO_DEVICE_TYPE(CameraDevice, g_TriggerTypeStandardProperty)
Technical notes/questions
@marktsuchida
DeviceType
from the class definition to the declaration and make it aconstexpr
. These was a note about compiler warnings related to this inMMDevice.cpp
, but these didn't seems to happen. Perhaps because of the use ofconstexpr
? or perhaps because this comment is 11 years old and no longer applicable). @marktsuchida you think this is okay?TODO