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

SD channel indexing improvements #92

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

maij
Copy link
Collaborator

@maij maij commented Jul 28, 2020

Adds channel indexing option to digitizer __init__.
Also adds a distinct set of default indices for legacy instruments c.f. modern instruments.

Incorporates the changes from this PR and thus supercedes.

@nulinspiratie
Copy link
Owner

nulinspiratie commented Aug 5, 2020

I believe there were some issues with the digitizer instrument and/or interface regarding one-based indexing that I fixed in Venus (PR #36 and perhaps local changes on Venus). We should add those changes into this PR.

@nulinspiratie
Copy link
Owner

I pushed all the changes in Venus to this branch. One remaining issue is that PR #36 seems very similar, so we should probably absorb that PR into this one.
Once that's done, we should probably beta test this PR in Queenie to verify that it doesn't break zero-based indexing.

@nulinspiratie
Copy link
Owner

Ah didn't realize this PR includes all changes from #36. I'll do a review soon

Copy link
Collaborator

@RostyslavSavytskyy RostyslavSavytskyy left a comment

Choose a reason for hiding this comment

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

Found minor thing, but otherwise looks good to me. Has anyone tested this already? If yes, can approve

@maij maij dismissed RostyslavSavytskyy’s stale review June 6, 2022 01:30

Addressed issues.

@maij maij requested a review from Ifefu June 6, 2022 01:31
@maij
Copy link
Collaborator Author

maij commented Jun 6, 2022

@Ifefu Do you have any further changes for the M3202A? If not, then it would be good to merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants