Skip to content
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

Add Acquisition or Configuration category to public properties #226

Merged
merged 4 commits into from
Aug 22, 2024

Conversation

bparks13
Copy link
Member

@bparks13 bparks13 commented Aug 20, 2024

To ensure a consistent user experience across all operators, this PR adds a string to the Category attribute for all public properties that defines the property as Acquisition, Configuration, or Devices.

Configuration properties are set prior to the start of the workflow. These are not updated in real-time, and are configured once when the workflow starts.

Acquisition properties can have an initial state defined before the workflow begins, but they can be updated in real-time with new values sent to hardware while the workflow is running.

Devices properties are only applicable for aggregates, where this category is applied to each device in an aggregate.

@bparks13 bparks13 added this to the 0.2.0 milestone Aug 20, 2024
@bparks13 bparks13 linked an issue Aug 20, 2024 that may be closed by this pull request
Copy link
Member

@jonnew jonnew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you do a quick review of all these changes and make sure that they comport with my commands on MutliDeviceFactory.Name? Basically, a property should be marked if its something exposed to the user that changes the behavior of a device at start or start/runtime. Anything outside of that scope should not be marked.

Aside from this, in ConfigureBreakoutAnalogIO the direction properties are all marked as Acquisition, but should be configuration. Thats left over from when they were settable at runtime and I forgot to change.

OpenEphys.Onix1/MultiDeviceFactory.cs Show resolved Hide resolved
OpenEphys.Onix1/ConfigureNeuropixelsV2eLinkController.cs Outdated Show resolved Hide resolved
- Incorrectly assigned BreakoutAnalogIO properties to Acquisition instead of Configuration category
@bparks13 bparks13 requested a review from jonnew August 21, 2024 17:28
@jonnew
Copy link
Member

jonnew commented Aug 21, 2024

The remaining thought I have before merging is the members of a MultiDeviceFactory being marked as Configuration

image

I understand the reasoning here, as the properties referred to by the attribute are indeed Configuration level. But really the user is going to be manipulating the inner properties of the devices, which can be both Acquisition or Configuration. These are not shown in the current property pane, but the point kinda still stands.

I think a way around this to define another category defintion

public abstract class DeviceFactory : Sink<ContextTask>
{
    internal const string ConfigurationCategory = "Configuration";
    internal const string AcquisitionCategory = "Acquisition";
    internal const string DevicesCategory = "Devices";

    internal abstract IEnumerable<IDeviceConfiguration> GetDevices();
}

which can be used for member devices of MultiDeviceFactorys

image

The Devices name is up for suggestions.

@bparks13
Copy link
Member Author

I am in favor of adding Devices as a category, I was just looking at the docs to see how we refer to these; they are classified as Device Configuration Operators, and we refer to them as devices from the aggregate level.

My only caveat with adding this category is that @cjsha and I were discussing what to name the headings in the aggregate page (for instance, ConfigureBreakoutBoard). Currently they are listed as Properties, but if we add this new category I would argue that we change the heading to match it, since the subheadings are the devices.

@jonnew
Copy link
Member

jonnew commented Aug 22, 2024

@bparks13 I agree with all. I feel a small force against this because in the ONI standard, device has very particular meaning. However, the ONI standard does not get to uniformly define what a device is for all its possible uses. In the context of this library, its very useful to refer to these leaf objects as devices, so let's do that (here and in the docs).

- For aggregates, use `Devices` category for encapsulated devices
@bparks13 bparks13 merged commit 6e47c63 into main Aug 22, 2024
6 checks passed
@bparks13 bparks13 deleted the issue-206 branch August 22, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure that all node Properties are categorized
2 participants