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

niscope: expand channels repeated_capability before populating channel and record info in fetched data #1958

Merged
merged 11 commits into from
May 9, 2023

Conversation

ni-jfitzger
Copy link
Collaborator

@ni-jfitzger ni-jfitzger commented Apr 29, 2023

  • This contribution adheres to CONTRIBUTING.md.
  • I've updated CHANGELOG.md if applicable.
  • I've added tests applicable for this pull request

What does this Pull Request accomplish?

Based on how nidcpower does things for fetch_multiple, measure_multiple, etc., expand the _repeated_capability (channels) before trying to do anything with it, other than pass it to a driver function.

The driver knows how to handle unexpanded channel strings (or it will error if it can't), but the simple code in
_populate_channel_and_record_info assumes that all channels in the channels iterable are individual channels.

List issues fixed by this Pull Request below, if any.

What testing has been done?

  • Locally, I used the tests changes to catch the bug, then ran codegen with the changes in this PR (prior to the latest merge) and Add get_channel_names to niscope API #1957 to confirm that the issue would be fixed.
  • PR Checks
  • I played with the tests to make sure there wasn't some other bad data case to worry about for single-instrument sessions. Passing a qualified name to fetch() or get_channel_names will error, so no bad data. Passing test_channels: '0:1', test_channels_expanded: 0,1 to test_fetch, passed.

@ni-jfitzger ni-jfitzger marked this pull request as ready for review May 4, 2023 13:21
@ni-jfitzger ni-jfitzger requested a review from marcoskirsch as a code owner May 4, 2023 13:21
@ni-jfitzger ni-jfitzger changed the title DRAFT: niscope: expand channels repeated_capability before populating channel and record info in fetched data niscope: expand channels repeated_capability before populating channel and record info in fetched data May 4, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
src/niscope/templates/session.py/fancy_fetch.py.mako Outdated Show resolved Hide resolved
@ni-jfitzger ni-jfitzger requested a review from marcoskirsch May 9, 2023 16:38
@marcoskirsch
Copy link
Member

If this is fixing a bug, how come it does not add a test that would fail without the bugfix?

@ni-jfitzger
Copy link
Collaborator Author

ni-jfitzger commented May 9, 2023

If this is fixing a bug, how come it does not add a test that would fail without the bugfix?

@marcoskirsch That's what the change to src/niscope/system_tests/test_system_niscope.py does.
See the first bullet under "What testing has been done" in the PR description.

The test changes to catch the bug were implemented in #1956 with failing parameter values commented out, so that we wouldn't submit failing tests. Since we are fixing the bug here, I restored the failing parameter values.

@marcoskirsch marcoskirsch merged commit 6aa529b into ni:master May 9, 2023
@ni-jfitzger ni-jfitzger deleted the niscope_fetch_fixes branch May 9, 2023 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants