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-qtgui: refactoring old sinks #6790

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

R-ohit-B-isht
Copy link
Contributor

Description

Refactoring the float and complex implementations of qtgui sink block into a single block using generics and templates.

Related Issue

Which blocks/areas does this affect?

Frequency Sink
Waterfall Sink
Sink
Time Raster Sink
Time Sink
Eye Sink

Testing Done

yes

Checklist

@willcode
Copy link
Member

Just took a quick look at the includes. It looks like the API should be backward compatible, but removing some of the include files will break build backward compatibility. What would people think about putting back these includes and making them wrappers around the common includes? The goal is to make it so anyone compiling against the current code can compile without changes against the new code.

@willcode
Copy link
Member

@R-ohit-B-isht this PR picked up a bunch of unrelated commits.

@willcode willcode added the No Backport Do not backport to previous versions label Aug 21, 2023
@R-ohit-B-isht R-ohit-B-isht force-pushed the qt-gui-refactoring branch 6 times, most recently from ac257c5 to 45b8328 Compare August 25, 2023 12:35
@noc0lour noc0lour self-assigned this Aug 25, 2023
@willcode willcode added Backport-3.10 Should be backported to 3.10 and removed No Backport Do not backport to previous versions labels Aug 26, 2023
Signed-off-by: R-ohit-B-isht <rbtunes0@gmail.com>
Signed-off-by: Jeff Long <willcode4@gmail.com>
@willcode
Copy link
Member

willcode commented Sep 28, 2023

@R-ohit-B-isht I removed the include cmake changes and pushed to your branch.

I'll work on removing the old code from the specialized include files.

Signed-off-by: Jeff Long <willcode4@gmail.com>
@willcode
Copy link
Member

@marcusmueller could you take a quick look. The only goal here is to reduce duplicate code, no improvements. The old include files are left as wrappers for the new ones, for API backward compatibility. We don't have any test code that explicitly tests the old includes, as far as I know.

@@ -466,7 +466,7 @@ templates:
${ gui_hint() % win}

cpp_templates:
includes: ['#include <gnuradio/qtgui/${type.fcn}.h>']
includes: ['#include <gnuradio/qtgui/freq_sink.h>','#include <gnuradio/qtgui/${type.fcn}.h>']
Copy link
Member

Choose a reason for hiding this comment

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

The second #include isn't needed here. Same in the other yml files.

Comment on lines +96 to +99
// void set_time_domain_axis(double min, double max);
// void set_constellation_axis(double xmin, double xmax,
// double ymin, double ymax);
// void set_constellation_pen_size(int size);
Copy link
Member

Choose a reason for hiding this comment

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

good time to also get rid of these

Comment on lines 38 to 40
# spectrumUpdateEvents_python.cc
# spectrumdisplayform_python.cc
# timeRasterGlobalData_python.cc
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we'll ever expose these to python. Might be a good time to remove.

Comment on lines 34 to 35
# plot_raster_python.cc
# plot_waterfall_python.cc
Copy link
Member

Choose a reason for hiding this comment

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

./.

Comment on lines +218 to +237
/*
void
sink_impl<T>::set_time_domain_axis(double min, double max)
{
d_main_gui.setTimeDomainAxis(min, max);
}

void
sink_impl<T>::set_constellation_axis(double xmin, double xmax,
double ymin, double ymax)
{
d_main_gui.setConstellationAxis(xmin, xmax, ymin, ymax);
}

void
sink_impl<T>::set_constellation_pen_size(int size)
{
d_main_gui.setConstellationPenSize(size);
}
*/
Copy link
Member

Choose a reason for hiding this comment

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

clean out

@willcode
Copy link
Member

@marcusmueller I'll come back and clean up the things you mentioned another time ... not hurting anything for the moment and I want to get the RC out.

@willcode willcode assigned willcode and unassigned noc0lour Sep 30, 2023
Signed-off-by: Jeff Long <willcode4@gmail.com>
Signed-off-by: Jeff Long <willcode4@gmail.com>
@willcode willcode added Needs Action Hold Backport Hold off on backport labels Sep 30, 2023
@willcode
Copy link
Member

Tested cpp gen. Time Sink fails with a topology error and GUI sink seems to lock up. We'll have to iron this out later - not going in to 3.10.8.0.

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 Cleanup Hold Backport Hold off on backport Needs Action QTGUI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants