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

enabled support for DAQmxSelfCal #460

Merged
merged 13 commits into from
Dec 19, 2023
Merged

Conversation

Sov-trotter
Copy link
Contributor

@Sov-trotter Sov-trotter commented Dec 11, 2023

What does this Pull Request accomplish?

This PR enables the support for DAQmxSelfCal hence enabling self calibration of the supported DAQ devices.

Why should this Pull Request be merged?

Self Calibration is critical functionality for many devices that is encouraged to be run regularly. This also a simple and straight-forward addition that doesn't require tackling the complexity of #22 and #23

What testing has been done?

I have manually tested this on Ni-6133 and Ni-4309 DAQ devices.
cc: @neutronstriker

This is my first time contributing to this project. Please let me know if I have missed something. :)

@bkeryan
Copy link
Collaborator

bkeryan commented Dec 11, 2023

Hi @Sov-trotter, thank you for contributing.

source/codegen/metadata/functions.py is auto-generated by a build process located in an NI internal repo. Someone at NI will need to update that build process to produce the updated SelfCal function definition, or else this change will get reverted the next time we update the API metadata.

@bkeryan
Copy link
Collaborator

bkeryan commented Dec 11, 2023

Will close #22 and #23

This is a step in the right direction but we shouldn't close either issue yet.

#22 includes the external calibration session class and external calibration adjustment functions, which is a sizeable effort.

#23 includes the calibration info properties, which is a smaller effort.

@zhindes
Copy link
Collaborator

zhindes commented Dec 11, 2023

Hi @Sov-trotter, thank you for contributing.

source/codegen/metadata/functions.py is auto-generated by a build process located in an NI internal repo. Someone at NI will need to update that build process to produce the updated SelfCal function definition, or else this change will get reverted the next time we update the API metadata.

I am willing to take on that effort for this contribution!

@zhindes
Copy link
Collaborator

zhindes commented Dec 11, 2023

Will close #22 and #23

This is a step in the right direction but we shouldn't close either issue yet.

#22 includes the external calibration session class and external calibration adjustment functions, which is a sizeable effort.

#23 includes the calibration info properties, which is a smaller effort.

I agree. I'll clean up this PR description a bit.

@bkeryan
Copy link
Collaborator

bkeryan commented Dec 11, 2023

Hi @Sov-trotter, thank you for contributing.
source/codegen/metadata/functions.py is auto-generated by a build process located in an NI internal repo. Someone at NI will need to update that build process to produce the updated SelfCal function definition, or else this change will get reverted the next time we update the API metadata.

I am willing to take on that effort for this contribution!

I forgot: we can put customizations in https://github.com/ni/nidaqmx-python/blob/master/src/codegen/metadata/functions_addon.py so that they are not overwritten.

However, updating the build process that generates functions.py is better in the long term.

@Sov-trotter
Copy link
Contributor Author

Thanks for the feedback @bkeryan. I moved the SelfCal customization to functions_addon.py . This doesn't quite seem to work though.

@flef
Copy link

flef commented Dec 11, 2023

Can you please add support for LibraryInterpreter ? selfCal has the same C prototype than Selftest.

The body of the Selftest function can be almost copy paste.

Regards

@bkeryan
Copy link
Collaborator

bkeryan commented Dec 11, 2023

Can you please add support for LibraryInterpreter ? selfCal has the same C prototype than Selftest.

The body of the Selftest function can be almost copy paste.

The interpreter classes are auto-generated and LibraryInterpreter was supported in the first commit, before @Sov-trotter tried my suggestion of using functions_addon.py (which didn't work and I need to look into).

@Sov-trotter
Copy link
Contributor Author

Sov-trotter commented Dec 12, 2023

Hi @Sov-trotter, thank you for contributing.
source/codegen/metadata/functions.py is auto-generated by a build process located in an NI internal repo. Someone at NI will need to update that build process to produce the updated SelfCal function definition, or else this change will get reverted the next time we update the API metadata.

I am willing to take on that effort for this contribution!

Hey @zhindes @bkeryan let me know if guys are able to update the said build process.

Also it would be really great if you can tag a release as soon as possible. :)

@zhindes
Copy link
Collaborator

zhindes commented Dec 14, 2023

Hi @Sov-trotter, thank you for contributing.
source/codegen/metadata/functions.py is auto-generated by a build process located in an NI internal repo. Someone at NI will need to update that build process to produce the updated SelfCal function definition, or else this change will get reverted the next time we update the API metadata.

I am willing to take on that effort for this contribution!

Hey @zhindes @bkeryan let me know if guys are able to update the said build process.

Also it would be really great if you can tag a release as soon as possible. :)

This week has been hell, but next week will be much more calm. I'll dig in then, and I also want to make a release soon as well.

@bkeryan
Copy link
Collaborator

bkeryan commented Dec 14, 2023

Thanks for the feedback @bkeryan. I moved the SelfCal customization to functions_addon.py . This doesn't quite seem to work though.

I figured out what's going on with functions_addon.py. In your first commit, you removed 'python_codegen_method': 'no',. However, src/codegen/metadata/__init__.py merges the original function metadata with the addon metadata, so the override in your second commit leaves 'python_codegen_method': 'no', in place.

Adding 'python_codegen_method': 'yes', or 'python_codegen_method': 'None', to the override doesn't entirely work, because some of the codegen assumes that the presence of python_codegen_method means that the function is either hand-written or unsupported.

It's possible to work around this by adding del all_functions["SelfCal"]["python_codegen_method"] to src/codegen/metadata/__init__.py, but it would be better to change src/codegen/utilities/function_helpers.py to do if function_data.get("python_codegen_method") or function_name in EXCLUDED_FUNCTIONS: instead of if "python_codegen_method" in function_data or function_name in EXCLUDED_FUNCTIONS: and set 'python_codegen_method': 'None', in the override.

Since @zhindes is planning to update the metadata next week, this is kind of a moot point, but I wanted to understand why the override didn't work.

@bkeryan
Copy link
Collaborator

bkeryan commented Dec 14, 2023

it would be better to change src/codegen/utilities/function_helpers.py to do if function_data.get("python_codegen_method") or function_name in EXCLUDED_FUNCTIONS: instead of if "python_codegen_method" in function_data or function_name in EXCLUDED_FUNCTIONS:

BTW, I made this change in #462

@zhindes
Copy link
Collaborator

zhindes commented Dec 18, 2023

Hi @Sov-trotter, thank you for contributing.
source/codegen/metadata/functions.py is auto-generated by a build process located in an NI internal repo. Someone at NI will need to update that build process to produce the updated SelfCal function definition, or else this change will get reverted the next time we update the API metadata.

I am willing to take on that effort for this contribution!

@Sov-trotter - You can update your metadata from users/zhindes/self_cal_metadata. The SelfCal changes look identical to me, but some other metadata was updated in the process that has no functional impact on the generated Python API (it is used by other tooling throughout our stack).

Copy link
Collaborator

@zhindes zhindes left a comment

Choose a reason for hiding this comment

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

As mentioned in other places: add a test and update to the latest official metadata

Copy link
Collaborator

@zhindes zhindes left a comment

Choose a reason for hiding this comment

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

One more issue (I think), but then we'll be good. Since we're here, can you also add this to the changelog under Merged Pull Requests? A link like this: 460: enabled support for DAQmxSelfCal.

I can make a release ASAP once this is in

src/codegen/metadata/functions_addon.py Outdated Show resolved Hide resolved
generated/nidaqmx/_library_interpreter.py Show resolved Hide resolved
@zhindes
Copy link
Collaborator

zhindes commented Dec 19, 2023

@Sov-trotter - looks great. I'll get a release out ASAP.

@zhindes zhindes enabled auto-merge (squash) December 19, 2023 15:42
@zhindes
Copy link
Collaborator

zhindes commented Dec 19, 2023

@Sov-trotter - bah, one last issue, I think.

./tests/component/system/test_device.py:33:1: BLK100 Black would make changes.

You can run poetry run black tests/component/system/test_device.py, and it should fix it for you.

auto-merge was automatically disabled December 19, 2023 15:45

Head branch was pushed to by a user without write access

@zhindes
Copy link
Collaborator

zhindes commented Dec 19, 2023

@Sov-trotter - bah, one last issue, I think.

./tests/component/system/test_device.py:33:1: BLK100 Black would make changes.

You can run poetry run black tests/component/system/test_device.py, and it should fix it for you.

We're good now. I'll babysit it from here.

@zhindes zhindes merged commit 48e4034 into ni:master Dec 19, 2023
13 of 15 checks passed
@zhindes
Copy link
Collaborator

zhindes commented Dec 19, 2023

@Sov-trotter its in! I should be able to make a release this afternoon, 🤞

@zhindes
Copy link
Collaborator

zhindes commented Dec 19, 2023

@Sov-trotter its in! I should be able to make a release this afternoon, 🤞

0.9.0 is out

@Sov-trotter
Copy link
Contributor Author

Thanks for this @zhindes @bkeryan

@ClaesFredo
Copy link

@zhindes @bkeryan

Just upgraded to 0.9.0 - thx. :)

I find the ai_dev_scaling_coeff but not much else.

Dumb questions?
a) Has the device calibration date been added? (I assume no & yes, self calibration is more important)
b) will the calibration info of #22 be added? It is great info for QA purposes.

@zhindes
Copy link
Collaborator

zhindes commented Jan 4, 2024

@zhindes @bkeryan

Just upgraded to 0.9.0 - thx. :)

I find the ai_dev_scaling_coeff but not much else.

Dumb questions? a) Has the device calibration date been added? (I assume no & yes, self calibration is more important) b) will the calibration info of #22 be added? It is great info for QA purposes.

a) No, we did not add that. However, nisyscfg (pypi, github) is high quality and should have support for that.

b) We do not have any current plans to add broader calibration support. Reflection of calibration metadata should already be covered by nisyscfg, and I don't think calibration adjustment is a feature we will prioritize highly, likely ever. Self Calibration is a small-impact API that is expected to be run regularly for the health of a DAQ board, so that was something that I prioritized adding based on the high level of interest shown in this PR.

@ClaesFredo
Copy link

Hi

Thx for clarifying matters and for the nisycfg info. :)

For what it is worth and, perhaps, as food 4 thought - the device calibration_date and its calibration_due_date is useful as Quality information data, ie to assess the device status.

@alra0351
Copy link

alra0351 commented Jan 8, 2024

Hi,
thank you for the new release: I am trying to use the self_cal() function on the NI 9205, but i am getting an error... whtas could be wrong here?

for device in nidaqmx.system.System.local().devices: if device.product_type == 'NI 9205': device.self_cal()

Output:

raise DaqError(extended_error_info, error_code)
nidaqmx.errors.DaqError: Some or all of the samples requested have not yet been acquired.
To wait for the samples to become available use a longer read timeout or read later in your program. To make the samples available sooner, increase the sample rate. If your task uses a start trigger,  make sure that your start trigger is configured correctly. It is also possible that you configured the task for external timing, and no clock was supplied. If this is the case, supply an external clock.
Property: DAQmx_Read_RelativeTo
Corresponding Value: DAQmx_Val_CurrReadPos
Property: DAQmx_Read_Offset
Corresponding Value: 0

Status Code: -200284

@zhindes
Copy link
Collaborator

zhindes commented Jan 8, 2024

@alra0351 That error being returned from self_cal is definitely unexpected, and I doubt it is related to the Python API. Can you try hitting the "Self Calibrate" button in MAX? I expect it will return the same error.

@alra0351
Copy link

alra0351 commented Jan 9, 2024

@zhindes thank you for your response. "Self Calibration" from MAX works without any problems. Any further ideas what I can try?
The Python API runs under Linux. MAX runs under Windows. That is the only difference.

@zhindes
Copy link
Collaborator

zhindes commented Jan 9, 2024

@alra0351 Can you create a new discussion and share the full code?

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.

None yet

6 participants