Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add fetch_capture_waveform() fancy wrapper #1096
Add fetch_capture_waveform() fancy wrapper #1096
Changes from all commits
2709431
77db9ae
f65cd59
0b5473e
c4edad6
0ea1cb6
2f95ea5
d19768b
286b698
bd308c3
5c39cc2
3b4315a
878cf89
2029662
0f55cbe
151b667
ebb4474
36443c8
9e29d92
ea2a318
e275ea7
9ca0d06
c841828
d342658
a42fdfd
2250729
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 wonder if named tuple of int/array of int is the best way to return this.
What are all the places in the API in which this type could be used? Can it ever be an input?
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.
@sbethur?
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.
It's used in two places:
niDigital_SortSiteResultsViUInt32Waveform
- This will NOT be added in Python, since its use is superseded by multi-instrument sessions.niDigital_WriteSourceWaveformSiteUniqueU32
- This should use the same type as what we use here forfetch_capture_waveform
. Tracked here.I looked at one of the use-cases for
fetch_capture_waveform
- streaming capture waveform data from h/w test program. It would be ideal forfetch_capture_waveform
to accept a buffer and populate that with the waveform data (like fetch_into in niscope).But the streaming use-case is not common and even then the size is about tens of MBs. If we learn about a use-case where performance is not sufficient, we can add
fetch_into
later. Usability of streaming withfetch_capture_waveform
that returns a dict is not bad:So, now the question is whether to use dict or namedtuple..
One drawback for dict is iteration order is not deterministic (at least in Python 3.5). It looks like in 3.6 and later, iteration is ordered. Either way, I think it's better to not guarantee any order, because trying to do that is little tricky:
If method is called with
site=''
then the order will 0,1,2... with disabled sites skipped.If method is called with
site='3,0,1'
(crazy but allowed), then the order will be 3,0,1 with disabled sites skipped.Given that order depends on input and also can contain gaps, users should not really rely on order. Even if they want to iterate in a particular order, it's not difficult to do that in Python (Everything's easy in Python )
In summary, the recommendation is dict(int, array of unsigned int)
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.
Is the order important?
Maybe it would be nicer for clients to return a dictionary of site -> data?
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.
@sbethur?
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.
See answer above.
TL;DR - dict is better.
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.
This does not appear to be related to this PR.
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.
This does not appear to be related to this PR.
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.
These does not appear to be related to this PR.
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.
This does not appear to be related to this PR.