Add Headstage64 GUIs

[bparks13]

This PR adds GUIs for the electrical and optical stimulators on the Headstage64.

• Fixes HS64 Electrical Stimulator GUI #98
• Fixes HS64 Optical Stimulator GUI #99

[jonnew]

This code review was performed during a live demonstration via verbal notes.

[bparks13]

@aacuevas @jonnew

Before finalizing the edits to this PR, I have an outstanding question at the bottom that I need some help answering before moving forward. This comment also serves as a general update on the status of the PR so that when the review comes it is easier to see the status of the GUI elements, as well as to provide context for the question.

Current Status

The current status of this PR is the following; all configuration properties have been removed from the Trigger nodes (optical and electrical) and have been moved to the Configure node (ConfigureHeadstage64). This is consistent with the pattern we are adopting where all configuration, and all GUIs, are in the configuration chain, and any relevant settings are passed to their respective Trigger nodes through the use of a DeviceInfo object. This way, we only have one place where the state is modifiable (the Configure node).

Side note @jonnew: after our discussion this morning, I actually think that we no longer need the DeviceInfo object, as all of the register writes are occurring in the Configure node, since we decided that is the only place where the user would be modifying properties.

GUIs are functionally complete, I will be stress testing them today to make sure I haven't missed any obvious issues, but here is how they look:

Headstage64 GUI

Electrical Stimulator

Optical Stimulator

Rhd2164

Bno055

TS4231

I have most of these changes as local commits, as there is some back and forth I want to clean up before requesting a review. Additionally, I will need to bring this branch up to date with main before we could merge it.

Outstanding Question

The question that we need to answer now, before moving forward, is whether or not the stimulus sequence parameters should be packaged into a single StimulusSequence object or kept as individual parameters.

Whichever method is used, they will both be written in real-time during acquisition so that the stimulus sequence can be updated on the fly.

Stimulus Sequence object

Pros:

• Easier to package into the DeviceInfo object
• Easier to pass back and forth to the GUIs
• With the usage of the GenericPropertyConverter, users are able to expand the object in the Bonsai editor and access individual elements for changing settings during acquisition

Cons:

• This is not currently externalizable from the ConfigureHeadstage64 node, as the stimulus sequence would be nested under the Electrical/OpticalStimulator device
• To minimize register writes, we would need to track which parameter is updated in real-time and only write registers for the values that have been changed, increasing complexity and code needed

Individual parameters

Pros:

• This is the current implementation in the library, so not much behavior needs to change other than moving it from the Trigger to the Configure node
• Externalizable as a lot of individual parameters

Cons:

• Increases the number of parameters that must be passed to DeviceInfo / GUIs

Final Thoughts

Given that we do not know when bonsai-rx/bonsai#2370 will be approved and merged into a release version of Bonsai, it is probably prudent to leave the parameters as individual parameters and not worry about packing them into a StimulusSequence object for now. Let me know your thoughts, and if I might have missed another solution.

[aacuevas]

I need to think about this, but one thing: Once you move this to the configure node, those will be not externalizable anyways, whether or not they are packed into a StimulusSequence, since now they will be nested inside the specific device inside of the configure node.

[bparks13]

Ah, that's right. I was thinking about it from the context of the device node, but anything in the headstage node will not be externalizable until the property mapper is added

[bparks13]

@aacuevas @jonnew Update based on discussion during our meeting today: what does it look like if we were to implement this PR using individual parameters (i.e., copying the behavior from the original Trigger nodes)

Exploring Individual Parameters

Starting from the current main branch, and only implementing this for the PhaseOneCurrent property of the Headstage64ElectricalStimulator, how does this pattern work? What are the pros and cons?

View from the Bonsai editor for the ConfigureHeadstage64 node

• This mirrors the Headstage64ElectricalStimulatorTrigger node, and does not require the user to explore nested properties like the StimulusSequence above
• Maintains the real-time acquisition behavior of this property by copying the BehaviorSubject code from the Trigger node

Passing Parameters to/from the GUI

Here there are two options:

1. Pass all properties one-by-one to the constructors for each step (Configure node --> Editor --> Dialog and back)
2. Pass the ConfigureHeadstage64ElectricalStimulator object itself and extract the individual properties

Option 2 is the option I went with, as it keeps the construction argument list short once all 12 parameters are implemented.

Following the pattern of option 2, it is fairly easy to pass the Configure object down the chain and back up so that the stimulus sequence properties can be easily modified by the GUI and subsequently saved.

In the Headstage64Dialog, to create a new stimulus sequence dialog:

ElectricalStimulatorSequenceDialog = new(configureNode.ElectricalStimulator);

In the Headstage64Editor, to save any changes made to the stimulus sequence:

configureNode.ElectricalStimulator = editorDialog.ElectricalStimulatorSequenceDialog.ElectricalStimulator;

Serializing / Deserializing the Stimulus Sequence

This is the only point where I think that keeping the individual properties runs into a hard issue to solve. Previously, when I was using StimulusSequence, I could serialize this object to/from a JSON file so that the stimulus sequence parameters could be easily saved/loaded in the GUI.

The main work for serializing/deserializing is done by the Newtonsoft.Json library (JsonConvert.SerializeObject), which takes a single object and serializes it to a string. While we could serialize the ConfigureHeadstage64ElectricalStimulator, this would also include the Enable / DeviceName / DeviceAddress properties in the stimulus sequence file, which does not seem correct to me.

There is support for serializing collections, but this would require manually pulling out the individual properties and adding them to a List which is not good for maintainability.

Externalizing Parameters

Similar to the issue of serializing above, once we have the ability to externalize nested parameters there is a question of usage for externalizing the stimulus parameters. If the user only needs to externalize and manipulate a single property then the individual parameters make this very easy, but if they wanted to cycle between two totally different sequences they would have to externalize and update all twelve parameters individually.

The StimulusSequence class could potentially be easier to handle this, as it only requires externalizing a single property to update the whole sequence, but then it becomes a question of how to handle the new incoming sequence. Previously we have talked about creating a static class that compares two sequences and only writes registers for the differences to minimize register access.

Maintaining real-time updates

Individual Properties
Individual properties at the level of the device can be easily subscribed to using the original pattern in the Trigger node, where the BehaviorSubject is updated whenever the parameter changes, and this is used to write to the register during acquisition. Tested this pattern in the ConfigureHeadstage64 node and it correctly hits the setter when a parameter is changed.

readonly BehaviorSubject<double> phaseOneCurrent = new(0);

public double PhaseOneCurrent
{
    get => phaseOneCurrent.Value;
    set => phaseOneCurrent.OnNext(value);
}

// [...]

phaseOneCurrent.SubscribeSafe(observer, value => device.WriteRegister(Headstage64ElectricalStimulator.CURRENT1, uAToCode(value)))

Stimulus Sequence
To compare, I also tested the behavior if the StimulusSequence was wrapped in a BehaviorSubject, and found that this presents an issue; the StimulusSequence setter is not called when an inner property is updated, which makes sense but I hadn't considered until now. The consequence of this is that we cannot use the same pattern for writing to registers in real-time with the stimulus class.

If we want to use the StimulusSequence class we would need to come up with an alternative pattern for subscribing to changes in the class. During testing, I noted that it is possible for the properties of StimulusSequence to follow the same pattern as the individual properties section above (i.e., each property has a private BehaviorSubject), and I can catch when the individual property setters are called even inside of the nested class; however, the difficult part is then taking that information and subscribing to it, since the StimulusSequence class does not contain any notion of the Device or a Process() method to handle the subscriptions and register writes appropriately.

Final Thoughts

We can port over the individual properties to the Configure node while maintaining real-time capabilities. The only caveat would be how to handle saving/loading the stimulus sequence, if we want to have that functionality.

If we wanted to use the StimulusSequence class we could load/save sequences easily, but would need to think carefully about how to handle the real-time register writes.

[bparks13]

Based on our meeting this morning, we will probably implement the stimulator sequence as individual parameters in the Configure* node. This enables us to transition from Trigger node to Configure node with the lowest friction, since the functionality will be identical in terms of how we hook into the Subject changes during acquisition.

If we do go with individual parameters, this also means that I will need to remove the ability to save the stimulus sequence as a file. Originally, I was following the pattern we started with Rhs2116 to save stimulus sequence, but since these two devices here only have one or two channels, it is not needed to save as a stimulus sequence file like it was for the Rhs2116.

If this is the route we want to go, I can finalize this PR and rebase so that the updates can be reviewed.

[bparks13]

@jonnew

This PR is ready for review again.

Editor

All properties have been moved from the Trigger node to the Configure node, while the real-time functionality has been maintained.

The GUIs are the same as previously shown, with some minor visualization tweaks to align elements, and ensuring that the units are correct.

This PR touches a lot of files, specifically a lot of Design library files were created or modified during some refactoring. Additionally, to align with the convention in the rest of the project, the private modifier is removed from the Dialog files, since that is the default if nothing else is defined.

Requested changes were implemented.

[jonnew]

One thing I'm noticing right off the bat is that the "blank" Bno055 dialog (and similar) are OK in the context of the headstage, they don't make sense in the context of the ConfigureBno055 (and similar). Whats weirder still is that dialogs that do make sense in this context are not available:

[jonnew]

I think we have to think about another way of dealing with configuration operators that don't need a GUI and what to do with them in the context of the GUI for the aggregate (headstage) configuration operator. There is a lot of lines of code and files added to this comment that, at the end of the day, actually are there to precisely nothing. I think we need to rethink about this and what were are trying to achieve with the GUI.

[jonnew]

This attribute, and the creation of 3 design files (TS4231V1Dialog.cs, TS4231V1Dialog.Designer.cs, and TS4231V1Editor.cs) were added to do... nothing. This seems very wrong to me. Same story for Bno055. I realize that in the Bno055 case, we've been using this pattern for a while and I'm just noticing it now, but something seems wrong here. This is a lot of boilerplate to generate a blank canvas and redundant properties pane. I think that these design files should be removed and the solution will need to come from another direction. Perhaps a generic DefaultDialog.cs et al.

[bparks13]

The original intent was to build the framework for all of the dialogs, using a common GUI (GenericDeviceDialog) at its base. Then, when we wanted to create a custom UI for a specific device, we would stop inheriting from GenericDeviceDialog and already have everything in place to load and save the GUI data.

I definitely understand what you mean about there being a lot of files added that don't seem to do a lot, so I tested an implementation where I removed all of the boilerplate files, and instead created a GenericDeviceEditor that all of them can point to. This keeps the functionality of adding a tab for each device in the headstage GUI, but it still shows the same redundant editor so we can discuss if that is useful or not.

I also wanted to say that there is not a GUI for ConfigureHeadstage64ElectricalStimulator because at one point we discussed whether or not there should be GUIs attached to the device nodes or only the headstage nodes, to prompt users to use the headstage nodes in all cases. I am not sure why I added the dialog to the Bno055 node, so I will need to go through and make sure we are consistent with whichever direction we want to go; adding GUIs to all device nodes, or removing them.

[jonnew]

This is a completely backwards incompatible change, correct?

• If a saved workflow has values here, they will be ignored
• If a saved workflow exposed these as externalized properties, it will have a compiler error.

[bparks13]

That is correct, I just tested it using v0.6.7.

Saved parameters are ignored, and if there is an externalized property it throws an error in the editor as shown below:

[jonnew]

See comment on line 71.

[jonnew]

See comment on line 71.

[jonnew]

This addition seems like a reversal of needs to me. You have created a copy constructor so you can use a GUI that appears to add no functionality over the default property pane for the sake of having a tab in the headstage. This is backwards I think.

[bparks13]

This PR is ready for review again, but it is not ready to merge.

To be specific, we are still deciding whether or not keep the tabs for devices that do not have a custom UI yet. In the current iteration (as of ac8e445) the boilerplate files have been removed and only a single generic dialog remains, which all devices can then utilize to be rendered as a separate tab in a headstage GUI.

Another topic to discuss is where GUIs should exist; should they only apply to the headstage configure nodes? Or should they be attached to the device configure nodes as well? To retain the full functionality, we might need to split the editor into two different files, since the same editor cannot be attached to a class and a property. For more details, see the implementation in ee3607d, where the HS64 electrical/optical stimulators are given two editors.

Once these topics (and any others that come up) are finalized, I can finish the implementation and then clean up the history prior to merging.

[bparks13]

@jonnew This PR is ready for a final review, the history has been cleaned so that files are not added and then removed

[jonnew]

Almost there. I rebased all the recent headstage-64 functionality into this branch. There are some issues still:

1. Corner cases in the GUI. The one that I found is when single pulse is defined, the GUI complains that the period is too short to support it. But this does not make sense because stim isnt period. Correct solution would be to grey out pulse period when num pulses is one and IBI when num bursts is 1.

2. Because stimulators are now data sources, the have a standard Enable property. This will need to be added to the GUIs. Also the stimulator enable property should be added to each as well.

[jonnew]

See my comments.