Disable nifgen get_fir_filter_coefficients() #891

texasaggie97-zz · 2018-06-05T22:19:43Z

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?

Remove get_fir_filter_coefficients() because it only applies for OSP and OSP is not included in the Python API yet in general
Keep all created files and metadata in case we need to enable again in the future
- Don't want to have to figure this out again.
~~Fixed nifgen get_fir_filter_coefficients()~~
- ~~Create specific implementation instead of relying on code generator~~
  - ~~Not really ivi-dance because the size is returned in an out parameter instead of the return value (error)~~
  - ~~Not worth trying to make the code generator support this use case for this one function~~
- ~~Don't use code generator for documentation~~

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

Fix Generated documentation for return parameters is incomplete #535
Fix Return values of nifgen_GetFIRFilterCoefficients seem to be incorrect #596
We no longer care about Bad NI-FGEN metadata for GetFIRFilterCoefficients #534

What testing has been done?

Unit
System
Visual inspection of generated files

Is not like the rest of the ivi-dance functions Not worth torqueing the code generator around to support this one use case

marcoskirsch · 2018-06-07T16:29:58Z

docs/nifgen/class.rst

-
-.. py:class:: Session(self, resource_name, channel_name=None, reset_device=False, options={})
-
+==============


Why is all this noise in the diff? Something about line endings got messed up?

I am not sure. Looking into it but I don't see anything obvious

marcoskirsch · 2018-06-07T18:13:25Z

docs/nifgen/class.rst

+
+            | Returns the FIR filter coefficients used by the onboard signal
+              processing block. These coefficients are determined by NI-FGEN and
+              based on the FIR filter type and corresponding property (Alpha,


Since we have control over this text now, we could type the actual property name so they get linked).

marcoskirsch · 2018-06-07T18:20:48Z

generated/nifgen/unit_tests/_mock_helper.py

@@ -105,8 +105,6 @@ def __init__(self):
        self._defaults['GetExtCalRecommendedInterval']['Months'] = None
        self._defaults['GetFIRFilterCoefficients'] = {}
        self._defaults['GetFIRFilterCoefficients']['return'] = 0
-        self._defaults['GetFIRFilterCoefficients']['numberOfCoefficientsRead'] = None


If this mocks the C function, why did it change at all?

Because I cleaned up (I.e. removed) the overrides we had in functions_addon. So directions changed.

marcoskirsch · 2018-06-07T18:22:48Z

src/nifgen/system_tests/test_system_nifgen.py

-        coeff_array = [1, 0, -1]
+        coeff_array = [0 for i in range(95)]
+        coeff_array[0] = -1.0
+        coeff_array[2] = 1.0


This should error because the device expected symmetrical coefficients.
I think you need to also enable OSP so that the OSP configuration gets validated.

Test disabled since OSP disabled, can't check this.

Disabled function since OSP

marcoskirsch · 2018-06-07T22:44:50Z

Do we have an issue about adding OSP to NI-FGEN?
If we don't we should open it and reference all those Issues that are getting closed now but would become a problem again when we start tackling OSP.

marcoskirsch · 2018-06-07T22:46:11Z

src/nifgen/templates/functions.rst/get_fir_filter_coefficients.rst.mako

@@ -0,0 +1,42 @@
+<%page args="function, config, method_template, indent"/>\


Are you keeping these on purpose because they may be useful in the future?
I'm not sure what the proper way of handling this sort of thing is.

Yes, I am keeping this on purpose. I mentioned that in the PR description. I don't want to lose this knowledge since it isn't obvious how this needs to work.

It just seems like dead code so there's a risk of it being removed in the future too.

How about removing it but in the issue recording the last commit that contained it to show a solution?

texasaggie97-zz · 2018-06-07T23:00:07Z

We have an issue about complex datatypes (#189 and #514) and OSP only applies to complex data.

Comments addressed?

marcoskirsch · 2018-06-08T01:47:09Z

Comments addressed?

I think we should leverage the Git repo, rather than have dead code in master.
This can be done by having a branch with the files (I don't like this idea) or simply documenting in the related issues, even in the test comment, the Git ID where the fix existed.

texasaggie97-zz · 2018-06-08T01:52:49Z

We have to keep the branch around if we want to be able to reference the commit hash. Once we delete the branch, that deleted all the commits as well. Get Outlook for Android<https://aka.ms/ghei36>

…

________________________________ From: Marcos <notifications@github.com> Sent: Thursday, June 7, 2018 8:47:09 PM To: ni/nimi-python Cc: Mark Silva; Author Subject: Re: [ni/nimi-python] Disable nifgen get_fir_filter_coefficients() (#891) Comments addressed? I think we should leverage the Git repo, rather than have dead code in master. This can be done by having a branch with the files (I don't like this idea) or simply documenting in the related issues, even in the test comment, the Git ID where the fix existed. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_ni_nimi-2Dpython_pull_891-23issuecomment-2D395619678&d=DwMFaQ&c=I_0YwoKy7z5LMTVdyO6YCiE2uzI1jjZZuIPelcSjixA&r=oQTngAhgD6lgydjZGyMY1uYA3Zxdr2P5nC7-XoWqcUc&m=IPlQcJHNqMWc5pZNL5F_K6aRAz9BOj2K5HvGLyJxt3A&s=dx3zS0umz2tHSdgwv0CHYPdJjiHjmGev2mrkOxiQAYg&e=>, or mute the thread<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AK86-5F-2Dc0vbvP-5FKpxH7vh1qHoml6EdrqRks5t6dedgaJpZM4UbptI&d=DwMFaQ&c=I_0YwoKy7z5LMTVdyO6YCiE2uzI1jjZZuIPelcSjixA&r=oQTngAhgD6lgydjZGyMY1uYA3Zxdr2P5nC7-XoWqcUc&m=IPlQcJHNqMWc5pZNL5F_K6aRAz9BOj2K5HvGLyJxt3A&s=q2HwFifoZraChX6XE47Ul2AelgQochgMDC8DHW5Eeug&e=>.

marcoskirsch · 2018-06-08T14:35:06Z

What if we commit to master, then remove?
How is this typically done?

texasaggie97-zz · 2018-06-08T16:14:56Z

Add more comments about how these are not used until OSP is supported

* Remove templates that are no longer required * Per #891 OSP functions should not be public * Use ivi-dance-with-a-twist in case ever made public * Update generated files * Update CHANGELOG.md

texasaggie97-zz added 6 commits June 5, 2018 15:39

Add specific implementation for get_fir_filter_coefficients()

79a5975

Is not like the rest of the ivi-dance functions Not worth torqueing the code generator around to support this one use case

Enable and fix get_fir_filter_coefficients test

7c8bb79

Don't try to use generated documentation

04d11ab

Don't ise generated documentation

42faab7

Update generated files

1f437de

Update changelog

3aa70ea

texasaggie97-zz changed the title ~~Bug596/get fir filter coefficients~~ Fix nifgen get_fir_filter_coefficients() Jun 5, 2018

texasaggie97-zz added bug API nifgen source-breaker labels Jun 5, 2018

texasaggie97-zz added this to the nimi-python 1.0 release milestone Jun 5, 2018

Merge branch 'master' into bug596/get_fir_filter_coefficients

e5737a1

Fladolcetta mentioned this pull request Jun 7, 2018

Bad NI-FGEN metadata for GetFIRFilterCoefficients #534

Closed

marcoskirsch previously requested changes Jun 7, 2018

View reviewed changes

texasaggie97-zz added 4 commits June 7, 2018 16:08

Disable get_fir_filter_coefficients()

0572306

Update changelog

41f8476

Update generated files

ecd703d

Disable get_fir_filter_coefficients() test

de8438f

texasaggie97-zz changed the title ~~Fix nifgen get_fir_filter_coefficients()~~ Disable nifgen get_fir_filter_coefficients() Jun 7, 2018

marcoskirsch previously requested changes Jun 7, 2018

View reviewed changes

Merge branch 'master' into bug596/get_fir_filter_coefficients

5b32900

Update comments to be explicit about applying only once OSP supported

acc1752

marcoskirsch approved these changes Jun 8, 2018

View reviewed changes

marcoskirsch merged commit 92734b9 into master Jun 8, 2018

marcoskirsch deleted the bug596/get_fir_filter_coefficients branch June 8, 2018 16:46

This was referenced Jun 4, 2019

nifgen.Session.get_fir_filter_coefficients can be code-generated #995

Closed

nifgen.Session.configure_custom_fir_filter_coefficients(coefficients_array) should not be exposed #996

Closed

texasaggie97-zz added a commit that referenced this pull request Jun 18, 2019

Per #891 OSP functions should not be public

21ed639

texasaggie97-zz mentioned this pull request Jun 18, 2019

Minor fixes in nifgen metadata #1006

Merged

3 tasks

ni-jfitzger mentioned this pull request Nov 7, 2022

Enable nifgen tests for gRPC. #1848

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable nifgen get_fir_filter_coefficients() #891

Disable nifgen get_fir_filter_coefficients() #891

texasaggie97-zz commented Jun 5, 2018 •

edited by marcoskirsch

Loading

marcoskirsch Jun 7, 2018

texasaggie97-zz Jun 7, 2018

marcoskirsch Jun 7, 2018

marcoskirsch Jun 7, 2018

texasaggie97-zz Jun 7, 2018

marcoskirsch Jun 7, 2018

texasaggie97-zz Jun 7, 2018

marcoskirsch commented Jun 7, 2018

marcoskirsch Jun 7, 2018

texasaggie97-zz Jun 7, 2018

marcoskirsch Jun 7, 2018

texasaggie97-zz commented Jun 7, 2018

marcoskirsch commented Jun 8, 2018

texasaggie97-zz commented Jun 8, 2018 via email

marcoskirsch commented Jun 8, 2018

texasaggie97-zz commented Jun 8, 2018


		.. py:class:: Session(self, resource_name, channel_name=None, reset_device=False, options={})

		==============

		@@ -0,0 +1,42 @@
		<%page args="function, config, method_template, indent"/>\

Disable nifgen get_fir_filter_coefficients() #891

Disable nifgen get_fir_filter_coefficients() #891

Conversation

texasaggie97-zz commented Jun 5, 2018 • edited by marcoskirsch Loading

What does this Pull Request accomplish?

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

What testing has been done?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoskirsch commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

texasaggie97-zz commented Jun 7, 2018

marcoskirsch commented Jun 8, 2018

texasaggie97-zz commented Jun 8, 2018 via email

marcoskirsch commented Jun 8, 2018

texasaggie97-zz commented Jun 8, 2018

texasaggie97-zz commented Jun 5, 2018 •

edited by marcoskirsch

Loading