- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5
Add Headstage64GpoTrigger #529
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
base: main
Are you sure you want to change the base?
Conversation
- Remove explcity ostim and estim trigger delay registers in favor of dynamic delay via trigger register - Add the ability to turn off indication LED - Fix register values and frame defintions - Add persistant heartbeat device to Headstage64 device aggregate
- This is a sink operator that takes booleans and can issue stimulus triggers to the headstage-64 optical and electrical stimulators using the port controller's GPO - To implement this functionality, derivatives of ConfigurePortController now must explicitly specify their DeviceType. This allows the creation of specialized versions of the PortController for particular implementations of MultiDeviceFactory. In most cases, PortController will be used. - In the case of ConfigureHeadstage64, a type called Headstate64PortController was created and is used by ConfigureHeadstage64's specialization of ConfigurePortController. This is also the device filter used by Headstage64GpoTrigger. - This allows headstage-64 stimulus triggers using GPO, but will only work if a ConfigureHeadstage64 operator is in the configuration chain and does not permit general access to GPO since the meaning of each line depends on the headstage that is attached to the port.
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.
Looks good, and is definitely good to restrict access to the GPOs unless the Headstage64 is present. I have one comment that needs to be addressed, and some small nitpicks, plus after this review I will go in and add the persistent heartbeat tab to the HS64 GUI for completeness.
- Readd inter phase current property to ConfigureHeadstage64ElectricalStimulator - Improve code comments
- The range is a readonly register value that is device specific and not know at compile time
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.
Looks good to me
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.
I locally merged this branch into main so I can test and document the stimulus reports that are already merged into main and the features of this branch with the hs-64 0.4 that we have. I couldn't get any Headstage64OpticalStimulatorDataFrames despite sending values to the corresponding trigger node. Also, I don't get a frame for the first estim trigger, but the rest report back just fine. I'm not sure where these behaviors are coming from (my workflow, software, firmware, etc.), but I don't want to let this slip through the cracks so I'm going to request changes here.
example workflow:
hs64-example.zip
 
      
    | @cjsha This is expected behavior. If estim is armed, then optical stimulation is disarmed by the headstage firmware. This is because they share an SPI bus and only one stimulator can be used at a time. If the user wants to interleave, they need to coordinate the stimulators and dynamically arm and disarm them. | 
DeviceNamedropdown #537