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

gr-filter: Polyphase Arbitrary Resampler Block filter taps #6894

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

basti-schr
Copy link

@basti-schr basti-schr commented Oct 7, 2023

Description

The Polyphase Arbitrary Resampler Block trimmed of the first and last character in taps. Therfore the Example gr-analog/fm_tx.grc failed cpp code generation as described in #6103.

Related Issue

Fixes #6103
Fixes #3638

Which blocks/areas does this affect?

Testing Done

In the gr-analog fm_tx Example example i run cpp code generation.

Checklist

@willcode willcode added filter Bug Fix Backport-3.10 Should be backported to 3.10 labels Oct 7, 2023
@willcode
Copy link
Member

willcode commented Oct 7, 2023

As a guess, this was a way of stripping off [ ] but not sure. Does anyone know of a reason?

@willcode
Copy link
Member

willcode commented Oct 7, 2023

@basti-schr We can't change the example to C++, though. Could you remove the changes to the grc file?

@basti-schr
Copy link
Author

I noticed this still not works in all cases!
There are two scenarios i tested:

  1. As in the example an main: define the Taps in one expression in the Text field.
  2. Define a new Low-pass Filter Taps Block and paste the variable name in the Polyphase Arbitrary Resampler Block.

in this version only the second scenario is working.

As i am new to the yaml code i will try to understand this better and try to make it work in both scenarios.

@willcode willcode marked this pull request as draft October 8, 2023 19:58
@basti-schr
Copy link
Author

Now the block will work for the two scenarios described. I had to manually import the firdes file when defining the filter taps as strings in the resampler block. I would appreciate some feedback as I am new to contributing to gnuradio.

@basti-schr basti-schr marked this pull request as ready for review October 15, 2023 16:31
@willcode
Copy link
Member

@basti-schr For muliple commits, unless they are meant to be separable, squash to one commit and force push.

@willcode
Copy link
Member

willcode commented Oct 15, 2023

Tried this out. I think the [1:-1] was intentional. If the taps are specified for Python, they would be of the form [x0, x1, ...] and the slicing strips the brackets for C++. However, if something like lpf_taps (referencing a separate block) is specified, then it is transformed to pf_tap for C++. We need a solution that handles both of these cases.

I tried Decimating FIR Filter and it does not strip, so variable names work, brackets or raw lists of numbers do not. FFT Filter failed for another reason. Interpolating FIR Filter does strip, so a bracketed list works but a variable name does not. Let's look for some opinions on this and try to make everything work at once.

I would suggest that allowing the user to specify taps the same way for Python and C++ would be good, and Python has always used list format with brackets. Obviously, we shouldn't strip off the beginning and end of variable names.

 - removes the trimming of the first and last character in taps
 - fixes issue gnuradio#6103
 - changed gr-analog fm_tx example filter taps
 - and enabled cpp code generation
 - include filter/firdes header file
 - adapt vaiables for cpp
 - add replace rules for [] -> {}

Signed-off-by: Sebastian Schröder <github@basti.rocks>
@basti-schr
Copy link
Author

Thank you for taking a look. I tweaked it a bit more so that it now works with a list. But it looks a bit messy and I think there might be a nicer solution.

If you use other filter types, you need to import them as well. However, in my opinion, it would be a bit more of a mess to import all kinds of filters in this template.
But at this point I have much less insight into gnuradio. I thought this might be an easy fix.

@basti-schr basti-schr marked this pull request as draft October 15, 2023 21:51
@willcode
Copy link
Member

That works. Maybe there's a cleaner way, but it now works better than the other blocks.

Do you have any inclination to look through all the other filter blocks that support C++?

 - Interpolating and Decimating FIR Filter
 - filter taps adaption for cpp generation

Signed-off-by: Sebastian Schröder <github@basti.rocks>
@basti-schr
Copy link
Author

basti-schr commented Oct 16, 2023

Yes, I started with the FIR filters as they were quite simple. The other filters have some problems that I need to look at more closely.

I have identified ten more blocks to work on and added them to the comment above as checklist. Do you see another one?
I use this flowchart to check the results: test_filter.grc.txt

I have a question about testing: when installing gnuradio the classic way (cmake .. / make / sudo make install) the modified yaml files are not copied to the /usr/local/share/gnuradio/grc/blocks/ directory. So my solution is to manually copy the *.block.yml files to that location. Is this intended?

@willcode
Copy link
Member

The yml blocks should be installed. Make sure you hit the refresh blocks button on GRC (or restart it) after installing so it reads the new yml files.

Have you tested set_taps() in C++? Elsewhere, it always seems to be set_x(${x}), rather than set_x(x). See Add Const for example. I haven't looked at the generated code both ways to see what the difference is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport-3.10 Should be backported to 3.10 Bug Fix filter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gr-analog: fm_tx example generates invalid cpp code Wrong C++ code generated for low and high pass filters
2 participants