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

Functional onix-headstage-neuropix1 support #315

Merged
merged 8 commits into from
Oct 14, 2024
Merged

Functional onix-headstage-neuropix1 support #315

merged 8 commits into from
Oct 14, 2024

Conversation

jonnew
Copy link
Member

@jonnew jonnew commented Sep 29, 2024

  • Needs XML docs
  • Needs GUI integration
  • Needs testing with more than one probe

- Needs XML docs
- Needs GUI integration
- Needs testing with more than one probe
@jonnew
Copy link
Member Author

jonnew commented Sep 29, 2024

@bparks13

  • I removed the "e" suffix in a bunch of NeuropixelsV1e classes that are actually generic across headstages.
  • This headstage is needs GUI integration
  • It also needs xml docs, but I can do that once you've done the GUI work.

jonnew and others added 2 commits October 2, 2024 13:18
- Fix error in applicaiton of gain correction values
- Common NeuropixelsV1Frame for e and f variants
- Common IConfigureNeuropixelsV1 interface for eventual unified GUI
  support for both e and f headstage variants
- Add XML commonets to f variants
- Made NeuropixelsV1 dialogs more generalized, so they can be shared between V1e and V1f
- Added V1 Probe Configuration to match V2 to aid in modularity of dialogs
@jonnew
Copy link
Member Author

jonnew commented Oct 3, 2024

It seems like there is a little left over icon at the bottom of the GUI still:

image

@jonnew
Copy link
Member Author

jonnew commented Oct 3, 2024

When i run the GUI on a ConfigureNeuropixelsV1f (no headstage), there is a "Probe A" indicator in the window title:

image

I dont think that should be there because the A vs. B is given by the device address in this case. Basically, nothing needs to change compared to the ConfigureNeuropixelsV1e in this case.

@bparks13
Copy link
Member

bparks13 commented Oct 3, 2024

@jonnew The icon is supposed to be there, it indicates if the calibration files are valid, and if the serial numbers of the files match. This does not touch hardware, but is meant as an easy way to see which files are currently loaded and if they match.

However, the text is being cut off, which is incorrect, so that needs to be fixed.

@jonnew
Copy link
Member Author

jonnew commented Oct 3, 2024

When I see the circle arrow icon, I expect to be able to click it to refresh, but that does not seem to be the case here. I would be a yellow warning triagle or something to indicate that cal files are needed

- Removed "Probe A/B" from dialog when opening 1.0f probes
- Changed the Refresh symbol to a Warning symbol
- Allow the status strip labels to auto size, so the text doesn't get cut off
@jonnew
Copy link
Member Author

jonnew commented Oct 9, 2024

Last critique. I think changing this:

image

to this:

image

would be more clear. I deleted the colons, but those should stay.

Copy link
Member

@bparks13 bparks13 left a comment

Choose a reason for hiding this comment

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

Looks good, ready for a final review with hardware and testing the GUI.

@jonnew
Copy link
Member Author

jonnew commented Oct 10, 2024

Looks good now, however, the NP2.0 GUIs show this:

image

it would be nice if it had the same look as NP1.0:

image

@jonnew jonnew closed this Oct 11, 2024
@jonnew jonnew reopened this Oct 11, 2024
- This is to match the look and feel of the 1.0 dialog status strip
@bparks13 bparks13 linked an issue Oct 11, 2024 that may be closed by this pull request
@bparks13
Copy link
Member

@jonnew the 2.0 dialogs have been updated to match these changes to 1.0

@bparks13 bparks13 added this to the 0.4.0 milestone Oct 11, 2024
@jonnew jonnew merged commit 450f829 into main Oct 14, 2024
7 checks passed
@jonnew jonnew deleted the issue-100-2 branch October 14, 2024 14:05
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.

headstage-neuropix1 support
2 participants