Skip to content

Conversation

@mikeprosserni
Copy link
Contributor

@mikeprosserni mikeprosserni commented Nov 24, 2025

  • 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?

Investigating AB#3178052 revealed that the signal indices are supposed to be reversed compared to the data array indices. There were several # TODO: [AB#3178052](https://dev.azure.com/ni/94b22d7b-ad7b-4f5e-88f0-867910f91c94/_workitems/edit/3178052) comments in the nidaqmx-python digital waveform tests that were expecting this signal ordering behavior to be changed. But it turned out that the existing behavior was actually correct.

This PR overhauls the digital waveform tests to use and expect the correct signal ordering (MSB first, with line numbers counting down instead of up) throughout.

  • In conftest.py, all the channel strings used with CHAN_FOR_ALL_LINES in add_di_chan() have been reversed (using [::-1]), so they count down instead of up. (Note that this matches the behavior of port expansions)
  • In _digital_utils.py, I updated _get_expected_data_for_lines() and _get_waveform_data() to interpret data as MSB (big-endian), to match the standard line/signal ordering. Also, I added _get_waveform_bitstrings() for tests where this is the best way to see the the expected values.
  • Renamed _get_waveform_data_msb to _get_waveform_port_data, since MSB is standard and correct for ports in waveforms.
    • Addressed/removed all TODO: AB# 3178052 comments
  • Added _validate_waveform_signals() to all the tests that assert waveform data. This method verifies the signals' names and data.

Why should this Pull Request be merged?

Our tests should use industry standard line/signal ordering, for clarity.

We need to test the waveform signals.

What testing has been done?

All the tests pass.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 25, 2025

Test Results

    48 files  ±0      48 suites  ±0   1h 18m 44s ⏱️ +59s
 3 207 tests ±0   2 556 ✅ ±0     651 💤 ±0  0 ❌ ±0 
62 172 runs  ±0  49 398 ✅ ±0  12 774 💤 ±0  0 ❌ ±0 

Results for commit e6b8fd6. ± Comparison against base commit f0444fb.

♻️ This comment has been updated with latest results.

@mikeprosserni mikeprosserni marked this pull request as ready for review November 26, 2025 21:55
@mikeprosserni mikeprosserni requested a review from bkeryan December 8, 2025 23:42
Copy link
Collaborator

@bkeryan bkeryan left a comment

Choose a reason for hiding this comment

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

I think this looks correct. I would appreciate if @zhindes would take another look at the latest version before we merge this.

@mikeprosserni mikeprosserni changed the title Configure all CHAN_FOR_ALL_LINES tasks to use MSB instead of LSB Validate Waveform Signals in Tests Dec 11, 2025
@mikeprosserni mikeprosserni merged commit 065ca88 into master Dec 12, 2025
34 checks passed
@mikeprosserni mikeprosserni deleted the users/mprosser/bug-3178052-todos branch December 15, 2025 15:39
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.

4 participants