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

digital: fix yaml and bindings for constellation #4018

Merged
merged 1 commit into from
Jan 8, 2021

Conversation

mormj
Copy link
Contributor

@mormj mormj commented Dec 21, 2020

Adds implicit conversion to the enum type
Cleans up the binding code a bit (remove some dead code)
Change the dropdown from int to enum (as in other places)

Fixes #4007

@drmpeg
Copy link
Member

drmpeg commented Dec 22, 2020

Still getting an error here.

Value "digital.constellation_calcdist(const_points, sym_map, rot_sym, dims, normalization) if (str(type) == "calcdist") else getattr(digital,'constellation_'+str(type))()" cannot be evaluated:
__init__(): incompatible constructor arguments. The following argument types are supported:
    1. gnuradio.digital.digital_python.constellation_calcdist(constell: List[complex], pre_diff_code: List[int], rotational_symmetry: int, dimensionality: int, normalization: gnuradio.digital.digital_python.constellation.normalization = normalization.AMPLITUDE_NORMALIZATION)

Invoked with: [(-1-1j), (-1+1j), (1+1j), (1-1j)], [0, 1, 3, 2], 4, 1, 'digital.constellation.AMPLITUDE_NORMALIZATION'

@mbr0wn
Copy link
Member

mbr0wn commented Jan 4, 2021

@mormj Also, you forgot to sign-off your commit.

@mormj
Copy link
Contributor Author

mormj commented Jan 4, 2021

Still getting an error here.

Value "digital.constellation_calcdist(const_points, sym_map, rot_sym, dims, normalization) if (str(type) == "calcdist") else getattr(digital,'constellation_'+str(type))()" cannot be evaluated:
__init__(): incompatible constructor arguments. The following argument types are supported:
    1. gnuradio.digital.digital_python.constellation_calcdist(constell: List[complex], pre_diff_code: List[int], rotational_symmetry: int, dimensionality: int, normalization: gnuradio.digital.digital_python.constellation.normalization = normalization.AMPLITUDE_NORMALIZATION)

Invoked with: [(-1-1j), (-1+1j), (1+1j), (1-1j)], [0, 1, 3, 2], 4, 1, 'digital.constellation.AMPLITUDE_NORMALIZATION'

@drmpeg - the value template in the yaml was taking in the normalization parameter as a string. It doesn't seem that value is used entirely consistently and just used to give grc a temporary object to work with and check parameters against? Removing the parameter from the value part of the yaml (still used in the make template) and relying on the default normalization there makes the issue go away.

@drmpeg
Copy link
Member

drmpeg commented Jan 4, 2021

Still getting an error here.

Value "digital.constellation_calcdist(const_points, sym_map, rot_sym, dims, normalization) if (str(type) == "calcdist") else getattr(digital,'constellation_'+str(type))()" cannot be evaluated:
__init__(): incompatible constructor arguments. The following argument types are supported:
    1. gnuradio.digital.digital_python.constellation_calcdist(constell: List[complex], pre_diff_code: List[int], rotational_symmetry: int, dimensionality: int, normalization: gnuradio.digital.digital_python.constellation.normalization = normalization.AMPLITUDE_NORMALIZATION)

Invoked with: [(-1-1j), (-1+1j), (1+1j), (1-1j)], [0, 1, 3, 2], 4, 1, 'digital.constellation.AMPLITUDE_NORMALIZATION'

@drmpeg - the value template in the yaml was taking in the normalization parameter as a string. It doesn't seem that value is used entirely consistently and just used to give grc a temporary object to work with and check parameters against? Removing the parameter from the value part of the yaml (still used in the make template) and relying on the default normalization there makes the issue go away.

That fixes the error, but makes the Normalization Type selection do nothing. You can test it by bypassing the rrc filter in generic_mod.py. Change this line:

https://github.com/gnuradio/gnuradio/blob/master/gr-digital/python/digital/generic_mod_demod.py#L147

to:

self._blocks += [self.chunks2symbols]

And then look at the output of the constellation modulator block with a QT GUI Constellation Sink or use a File Sink and look at the file with a hex editor (ghex shows floating point values).

Signed-off-by: Josh Morman <jmorman@perspectalabs.com>
@mormj
Copy link
Contributor Author

mormj commented Jan 6, 2021

@drmpeg - this should be fixed now - and changed the type to raw so that the stringifying is not done in the value field (and put normalization back on there).

Question though that may warrant a separate PR - the standard constellation types - QPSK, 16-QAM, etc. don't expose the normalization options - should they pass through the normalization as well?

Copy link
Member

@drmpeg drmpeg left a comment

Choose a reason for hiding this comment

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

Looks good now.

@drmpeg
Copy link
Member

drmpeg commented Jan 7, 2021

Question though that may warrant a separate PR - the standard constellation types - QPSK, 16-QAM, etc. don't expose the normalization options - should they pass through the normalization as well?

From reading the original PR #2771, I don't think so.

@mormj mormj merged commit a4f3160 into gnuradio:master Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

broken .yml for digital_constellation_block.yml
3 participants