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

Minor updates to documentation #189

Merged
merged 2 commits into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 21 additions & 13 deletions OpenEphys.Onix1/MultiDeviceFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,26 @@ namespace OpenEphys.Onix1
{
/// <summary>
/// Provides an abstract base class for configuration operators responsible for
/// registering all devices in an ONI device aggregate in the context device table.
/// registering logical groups of <see cref="oni.Device"/>s.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This somehow seems now less clear, since it makes it seem the operator is registering the group rather than each individual device. Maybe we shouldn't talk about "device table" since that is a hardware specific concept, but mentioning that there is a context "device manager" might be important so people understand why sometimes the drop down shows devices and sometimes not.

Copy link
Member Author

@jonnew jonnew Aug 4, 2024

Choose a reason for hiding this comment

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

Really, as far as I see, we are both wrong.

  • Device table is hardware-agnostic, its required by the ONI API regardless of hardware
  • This class does not configure aggregates of oni.Devices (nullifying impact of last bullet), but OpenEphys.Onix1's concept of device, which is much looser (e.g. any dev using passthrough), is really what this class is used to configure. We don't really make it clear that this library kinda cares about devices, but will happily abstract a device if it makes things work better.

I m going to remove references to oni.Devices because its just not correct. I agree with Device manager comment ill try to add some text.

/// </summary>
/// <remarks>
/// <para>
/// ONI devices are often grouped into multi-device aggregates connected to hubs or
/// headstages. These aggregates provide access to multiple devices through hub-specific
/// addresses and usually require a specific sequence of configuration steps to determine
/// operational port voltages and other link-specific settings.
/// The ONI standard states that devices are grouped into aggregates called hubs, each of which is
/// governed by a single, potentially asynchronous clock and share a common base address. The devices on
/// a headstage are an example of a hub. Devices within a hub are accessed through hub-specific addresses
/// and usually require a specific sequence of configuration steps prior to acquisition.
/// </para>
/// <para>
/// These multi-device aggregates are the most common starting point for configuration
/// of an ONI system, and the <see cref="MultiDeviceFactory"/> provides a modular abstraction
/// for flexible assembly and sequencing of multiple such aggregates.
/// This class allows configuration of logical device groups of <see cref="oni.Device"/>s across ONI-defined
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// This class allows configuration of logical device groups of <see cref="oni.Device"/>s across ONI-defined
/// This class allows configuration of logical groups of <see cref="oni.Device"/>s across ONI-defined

/// hubs. For instance, the group of devices within a headstage (a single hub) can be combined with a device
/// from another hub that is used to control its port voltage and communication status
/// (e.g. <see cref="ConfigureHeadstage64"/>). Alternatively, diagnostic devices that are present within
/// an ONI hub can be omitted from a device group to aid it useability (e.g. <see cref="ConfigureBreakoutBoard"/>).
bparks13 marked this conversation as resolved.
Show resolved Hide resolved
/// </para>
/// <para>
/// These device groups are the most common starting point for configuration
/// of an ONI system, and the <see cref="MultiDeviceFactory"/> provides a modular abstraction for flexible
/// assembly and sequencing of device groups.
/// </para>
/// </remarks>
public abstract class MultiDeviceFactory : DeviceFactory, INamedElement
Expand All @@ -34,10 +41,10 @@ internal MultiDeviceFactory()
}

/// <summary>
/// Gets or sets a unique hub device name.
/// Gets or sets a unique device group name.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be useful to clarify here that this name is used as a prefix for individual device names in the group.

/// </summary>
/// <inheritdoc cref = "SingleDeviceFactory.DeviceName"/>
[Description("The unique hub device name.")]
[Description("The unique device group name.")]
public string Name
{
get { return _name; }
Expand All @@ -62,7 +69,7 @@ internal virtual void UpdateDeviceNames()
}

/// <summary>
/// Configure all the ONI devices in the multi-device aggregate.
/// Configure all devices in the device group.
/// </summary>
/// <remarks>
/// This will schedule configuration actions to be applied by a <see cref="StartAcquisition"/> instance
Expand All @@ -71,13 +78,13 @@ internal virtual void UpdateDeviceNames()
/// <param name="source">A sequence of <see cref="ContextTask"/> instances that hold configuration actions.</param>
/// <returns>
/// The original sequence modified by adding additional configuration actions required to configure
/// all the ONI devices in the multi-device aggregate.
/// all the devices in the device group.
/// </returns>
public override IObservable<ContextTask> Process(IObservable<ContextTask> source)
{
if (string.IsNullOrEmpty(_name))
{
throw new InvalidOperationException("A valid hub device name must be specified.");
throw new InvalidOperationException("A valid device group name must be specified.");
}

var output = source;
Expand All @@ -90,3 +97,4 @@ public override IObservable<ContextTask> Process(IObservable<ContextTask> source
}
}
}

4 changes: 2 additions & 2 deletions OpenEphys.Onix1/StartAcquisition.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ namespace OpenEphys.Onix1
public class StartAcquisition : Combinator<ContextTask, IGroupedObservable<uint, oni.Frame>>
{
/// <summary>
/// Gets or sets the number of bytes read by the device driver access to the read channel.
/// Gets or sets the number of bytes read per cycle of the <see cref="ContextTask"/>'s acquisition thread.
/// </summary>
/// <remarks>
/// This option allows control over a fundamental trade-off between closed-loop response time and overall bandwidth.
Expand All @@ -23,7 +23,7 @@ public class StartAcquisition : Combinator<ContextTask, IGroupedObservable<uint,
/// The optimal value depends on the host computer and hardware configuration and must be determined via testing (e.g.
/// using <see cref="MemoryMonitorData"/>).
/// </remarks>
[Description("The number of bytes read by the device driver access to the read channel.")]
[Description("Number of bytes read per cycle of the acquisition thread.")]
public int ReadSize { get; set; } = 2048;

/// <summary>
Expand Down