Skip to content

Conversation

@vnktshr21
Copy link
Contributor

@vnktshr21 vnktshr21 commented Jul 15, 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?

  • Enables intflag parameter for self_calibrate_range and adds reset_with_options method
    • Overriding doc_tip for stepsToOmit parameter to call out Bitwise input (this should be done in codegen itself in the future)
  • Expands shortened terms in enum names to full terms
  • Adds lo_channels repeated capability and changed deembedding_port into ports repcap since ports is used by API other then deembedding as well
    • Marks relevant attributes with the same
  • Makes waveform repcap name plural to align with repcap naming policy
  • Fixes few enum values and documentation snippets
  • Removes few enums that are not used by any attribute or function
  • Ensures all method description start with \n consistently
  • Removes p2p related methods, enums, enum values and attributes
  • Removes leftover configuration list related enum values
  • Removes few missed configuration list related enums (list mode API will not be supported for 1st release)
  • Makes script trigger configuration methods to have triggerID as repeated capability
  • Removes methods which are configuring just one attribute
    • Removes references to these in documentation

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

None

What testing has been done?

System tests for newly added API and updated enum names all pass on simulated and real hardware.

@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.21%. Comparing base (37ea6ef) to head (f09b69d).
Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2121      +/-   ##
==========================================
+ Coverage   88.99%   89.21%   +0.22%     
==========================================
  Files          71       71              
  Lines       18950    18820     -130     
==========================================
- Hits        16864    16790      -74     
+ Misses       2086     2030      -56     
Flag Coverage Δ
codegenunittests 84.44% <ø> (ø)
nidcpowersystemtests 94.65% <ø> (+0.04%) ⬆️
nidcpowerunittests 89.53% <ø> (ø)
nidigitalsystemtests 92.26% <ø> (ø)
nidigitalunittests 68.44% <ø> (ø)
nidmmsystemtests 92.72% <ø> (ø)
nifakeunittests 86.81% <ø> (ø)
nifgensystemtests 94.86% <ø> (ø)
nimodinstsystemtests 73.85% <ø> (ø)
nimodinstunittests 94.20% <ø> (ø)
nirfsgsystemtests 75.35% <100.00%> (+0.90%) ⬆️
niscopesystemtests 92.94% <ø> (ø)
niscopeunittests 43.20% <ø> (ø)
nisesystemtests 91.50% <ø> (ø)
niswitchsystemtests 82.03% <ø> (ø)
nitclksystemtests 94.87% <ø> (ø)
nitclkunittests 98.26% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
generated/nirfsg/nirfsg/_library.py 68.90% <100.00%> (+1.12%) ⬆️
generated/nirfsg/nirfsg/_library_interpreter.py 64.06% <100.00%> (+1.33%) ⬆️
generated/nirfsg/nirfsg/session.py 90.15% <ø> (+0.19%) ⬆️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37ea6ef...f09b69d. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ni-jfitzger
Copy link
Collaborator

I won't hold up this PR for it, but Lo1OutputFilter enum values are missing descriptions.

@ni-jfitzger
Copy link
Collaborator

test_waveform_rep_cap --> test_waveforms_rep_cap
test_port_rep_cap --> test_ports_rep_cap
test_lo_rep_cap --> test_lo_channels_rep_cap

@ni-jfitzger
Copy link
Collaborator

ni-jfitzger commented Jul 16, 2025

Looking at

    'Lo1OutputFilter': {
        'codegen_method': 'public',
        'values': [
            {
                'documentation': {
                    'description': 'yet to be defined'
                },
                'name': 'NIRFSG_VAL_5.5GHz_HIGH_PASS',
                'value': 0
            },
            {
                'documentation': {
                    'description': 'yet to be defined'
                },
                'name': 'NIRFSG_VAL_5.5GHz_LOW_PASS',
                'value': 1
            }
        ]
    },

Something about our codegen that you should be aware of: by default we trim common prefixes in the enum value names for a given enum. This is how we get rid of NIRFSG_VAL in the above enum values. But this also means that "5.5GHz_" is not part of the enum value name for either. This may or may not be what's desired.

You can override this behavior by specifying the python_name for an enum value. Or at least, that would be true, if #2073 were in.

You may want to go through your generated enums.py to look for any enum values that aren't what you want.

@vnktshr21
Copy link
Contributor Author

Fixed the repcap test names. For enums, it turns out the enums which had numbers in names are not used by any attribute or function. I've gone ahead and turned off generation for them. No 'yet to be defined' descriptions anymore in enums. Also, caught a wrong unit name case while looking into this, so fixed that.

@ni-jfitzger
Copy link
Collaborator

For enums, it turns out the enums which had numbers in names are not used by any attribute or function.

_add_enum_codegen_method in the build helpers will normally automatically exclude any unused enums from the generated code. This didn't happen because you set codgen_method for those enums, yourself.

I'd advise against setting codegen_method: "public" for enums, as you may be generating something that's unused.
I'm sure you review these things, but it's really better to lean on codegen than assume that your review was perfect, especially with the API being so fluid, right now.

@ni-jfitzger ni-jfitzger merged commit 456b9b7 into ni:master Jul 17, 2025
39 checks passed
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.

3 participants