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

Combine all polled Bno055 classes into common type #303

Merged
merged 3 commits into from
Sep 19, 2024
Merged

Combine all polled Bno055 classes into common type #303

merged 3 commits into from
Sep 19, 2024

Conversation

jonnew
Copy link
Member

@jonnew jonnew commented Sep 17, 2024

@jonnew jonnew added this to the 0.3.0 milestone Sep 17, 2024
commit 7616b9013b43d2cedaf01f2f30a740ad2ec907e8
Author: jonnew <jpn@open-ephys.org>
Date:   Tue Sep 17 17:11:02 2024 -0400

    Add missing Design files

    - I didn't add these in my last commit

commit 760215b
Author: jonnew <jpn@open-ephys.org>
Date:   Tue Sep 17 16:49:30 2024 -0400

    Combine all polled Bno055 classes into common type

    - ConfigurePolledBno055 and PolledBno055Data classes replace headstage
      specific versions of the same thing. Axis map is defined via a couple
      properties that are not browsable in editor
    - Old configuraiton and data operators are marked obsolete
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, I pulled down the changes and tested on my machine. During testing, I noticed that there were some GUI changes that needed to be made to the GenericDeviceDialog so the buttons did not get clipped. I have added those changes in a new commit, and I also fixed the small nit-picks from my review. Those conversations are closed, there is one outstanding question we should consider, but other than that this PR looks good.

I will say, if someone has an existing workflow containing any of the Neuropixels*Headstage's, they will not be able to save the workflow until they replace the headstage operator with the new one. The reason is because the old headstage operator did not instantiate the AxisMap or the AxisSign variables for the BNO055, therefore the XML serializer will throw an error. I am not sure if this is worth putting any effort into.

OpenEphys.Onix1/ConfigurePolledBno055.cs Outdated Show resolved Hide resolved
OpenEphys.Onix1/ConfigurePolledBno055.cs Outdated Show resolved Hide resolved
OpenEphys.Onix1/ConfigureNeuropixelsV1eHeadstage.cs Outdated Show resolved Hide resolved
OpenEphys.Onix1/ConfigurePolledBno055.cs Outdated Show resolved Hide resolved
OpenEphys.Onix1/ConfigurePolledBno055.cs Outdated Show resolved Hide resolved
bparks13 and others added 2 commits September 18, 2024 09:56
- GenericDeviceDialog was not updated to use `TableLayoutPanel` and `FlowLayoutPanel`, so the buttons were getting clipped at different scales. This has been corrected
- Typos and whitespace fixes
- This way they can be set if this is used as a standalone device
- Add PolledBno055SingleDeviceFactoryConverter to remove them from the
  properties pane when used in the context of MultiDeviceFactory with a
  specified orientation
- Add correct AxisMap and AxisSign values to obsolete overrides of
  ConfigurePolledBno055
@jonnew jonnew merged commit 9bfdc9a into main Sep 19, 2024
7 checks passed
@jonnew jonnew deleted the issue-281 branch September 19, 2024 16:48
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.

Consolidate passthrough BNO055 configure and data operators
2 participants