Code re-use in blocks / C++ inheritance with gr::sync_block

GNU Radio
1

Hi,

So what I’m trying to achieve is to share a lot of code (including the
gr::sync_block implementation for work/start/stop) and a common
interface between several gnuradio blocks.

What I have currently come up with is (short version, see bottom for
more complete version) :

class API base_sink_c : virtual public gr::sync_block {};
class API glfw_sink_c : virtual public base_sink_c {};

class base_sink_c_impl : virtual public base_sink_c {};
class glfw_sink_c_impl : public glfw_sink_c, public base_sink_c_impl {};

  • base_sink_c is a collection of virtual methods that define the
    common functionality between subblocks.
  • glfw_sink_c is the interface of the ‘glfw’ variant (only one here
    atm) and is a super-set of the one in base_sink_c.
  • base_sink_c_impl is the implementation of all the common stuff,
    including the start/stop/work of gr::sync_block
  • glfw_sink_c_impl is the ‘glfw’ variant and is the class to be
    instanciated, it contains all the glfw specific code.

But this doesn’t seem to work … it crashes when GR tries to attach
the block in the block graph ( backtrace http://pastebin.com/JeBUEgtK
).

This is the resulting C++ layout (dumped with -fdump-class-hierarchy )
http://pastebin.com/raw.php?i=cDWQQwHn

Anyone has a clue what I’m doing wrong ?

Cheers,

Sylvain M.

More complete version:

class API base_sink_c : virtual public gr::sync_block
{
/* Common exposed interface stuff (all virtual) */
}

class API glfw_sink_c : virtual public base_sink_c
{
public:
typedef boost::shared_ptr<glfw_sink_c> sptr;
static sptr make();

/* + Interface stuff specific to this variant (all virtual) */
}

class base_sink_c_impl : virtual public base_sink_c
{
protected:
base_sink_c_impl(const char *block_name);

public:
int work (int noutput_items,
gr_vector_const_void_star &input_items,
gr_vector_void_star &output_items);
bool start();
bool stop();

private:
/* all private stuff to the base implementation */
}

class glfw_sink_c_impl : public glfw_sink_c, public base_sink_c_impl
{
public:
glfw_sink_c_impl();
virtual ~glfw_sink_c_impl();
}

Then on the implementation side I have :

base_sink_c_impl::base_sink_c_impl(const char block_name)
: gr::sync_block(block_name,
gr::io_signature::make(1, 1, sizeof(gr_complex)),
gr::io_signature::make(0, 0, 0))
{
/ block specific stuff */
}

glfw_sink_c::sptr
glfw_sink_c::make()
{
return gnuradio::get_initial_sptr(new glfw_sink_c_impl());
}

glfw_sink_c_impl::glfw_sink_c_impl()
: base_sink_c_impl(“glfw_sink_c”)
{
/* … */
}

2

Hi Sylvain,

this is yet another incarnation of the diamond problem I discussed with
Nemanja:
http://lists.gnu.org/archive/html/discuss-gnuradio/2013-09/msg00150.html

Your glfw_…_impl is a double indirect subclass to base_sink_c:
glfw_sink_c_impl->glfw_sink_c->base_sink_c
glfw_sink_c_impl->base_sink_c_impl->base_sink_c
and thus, your glfw_sink_c_impl instance holds two separate instances of
base_sink_c, therefore two instances of gr::basic_block and you won’t
necessarily connect the correct one (whichever that may be), and even if
you did, the other one will be left unconnected, which also will lead to
an error.
So: one of glfw_sink_c or base_sink_c must not inherit from
gr::basic_block.
personally, I’d just not let glfw_sink_c_impl inherit from
base_sink_c_impl, and move the common stuff directly to base_sink_c.

Greetings,
Marcus

3

Hi,

this is yet another incarnation of the diamond problem I discussed with
Nemanja:
Re: [Discuss-gnuradio] Proper block inheritance

Your glfw_…_impl is a double indirect subclass to base_sink_c:
glfw_sink_c_impl->glfw_sink_c->base_sink_c
glfw_sink_c_impl->base_sink_c_impl->base_sink_c

Yeah and that’s what the ‘virtual’ are for in the inheritance tree.

And if you look at the resulting tree according to C++,
http://pastebin.com/raw.php?i=cDWQQwHn

you have in the first path :

gr::fosphor::base_sink_c (0x7f7a522eebc8) 0 nearly-empty virtual

and in the second path, it ends with :

gr::fosphor::base_sink_c (0x7f7a522eebc8) alternative-path

with the same address and “alternative-path” leading me to believe it
did the right thing and pointed to the first instance of it rather
than create a second one.

So: one of glfw_sink_c or base_sink_c must not inherit from gr::basic_block.

The issue there is that the exposed interface glfw_sink_c must have a
path to gr::sync_block visible to the user.
And I think that base_sink_c_impl also need a path to gr::sync_block
in it’s hierarchy because it’s the one actually implementing its
virtual methods (start/stop/work/…) and also using some of
gr::sync_block’s methods.

personally, I’d just not let glfw_sink_c_impl inherit from base_sink_c_impl,
and move the common stuff directly to base_sink_c.

The problem I have with this is that I must expose a bunch of stuff in
the public header that I don’t want to.

base_sink_c_impl has plenty of private method and members, none of
which the “user” should see (ie. not be installed in the public
headers)

Cheers,

Sylvain
4

Hi Sylvain,
this is getting really interesting :slight_smile:
On 10/22/2013 11:55 AM, Sylvain M. wrote:

Yeah and that’s what the ‘virtual’ are for in the inheritance tree.

And if you look at the resulting tree according to C++,
http://pastebin.com/raw.php?i=cDWQQwHn
I totally agree! So, only one basic_block instance, most probably.
So I dived into gnuradio-runtime/lib/flowgraph.cc, specifically looked
at check_valid_port (lines 137ff);
could you verify that your program does not output something like “port
number X exceeds…” or the like upon termination?

If you correctly called the chain of superconstructors, that shouldn’t
happen if you correctly set your io_signatures.

So: one of glfw_sink_c or base_sink_c must not inherit from gr::basic_block.
The issue there is that the exposed interface glfw_sink_c must have a
path to gr::sync_block visible to the user.
It has, by either inheritance detour :slight_smile: all your inheritance is public.
And I think that base_sink_c_impl also need a path to gr::sync_block
in it’s hierarchy because it’s the one actually implementing its
virtual methods (start/stop/work/…) and also using some of
gr::sync_block’s methods.
That’s what I thought; therefore, I suggested moving the code shared
among the glfw_sink_c_impl and the base_sink_c_impl to the common super
class, base_sink_c.
If you do not want your flowgraphs to use base_sink_c, there is no sense
in having base_sink_c_impl as a proxy between base_sink_c and
glfw_sink_c_impl. Anyway, even if you use that intermediate class, you
inheritance will become a path without the dreaded inheritance diamond.
personally, I’d just not let glfw_sink_c_impl inherit from base_sink_c_impl,
and move the common stuff directly to base_sink_c.
The problem I have with this is that I must expose a bunch of stuff in
the public header that I don’t want to.
base_sink_c_impl has plenty of private method and members, none of
which the “user” should see (ie. not be installed in the public
headers)
I think that’s the point of private and protected members, you shouldn’t
have to go to the total extreme of hiding everything away in an impl, if
you plan to inherit from that impl later…

Cheers,

Marcus

5

Hi Marcus,

If you correctly called the chain of superconstructors, that shouldn’t
happen if you correctly set your io_signatures.

Yup, that was the issue.

I was defining the call to gr::sync_block constructor as part of the
base_sink_c_impl class constructor since it seemed the most logical
place for it to be.
But given the virtual inheritance , I can only define it as part of
glfw_sink_c_impl constructor and without this, the default constructor
was called (which sets neither the name nor the io signature).

Now, it works as intended :slight_smile:

The problem I have with this is that I must expose a bunch of stuff in
the public header that I don’t want to.
base_sink_c_impl has plenty of private method and members, none of
which the “user” should see (ie. not be installed in the public
headers)

I think that’s the point of private and protected members, you shouldn’t
have to go to the total extreme of hiding everything away in an impl, if you
plan to inherit from that impl later…

Well it’s mostly because the private part include plenty of other
headers to define structures and objects that shouldn’t be part of the
ABI.

Cheers and thanks for pointing me in the right direction :slight_smile:

Sylvain
6

On Tue, Oct 22, 2013 at 6:40 AM, Sylvain M. [email protected]
wrote:

I think that’s the point of private and protected members, you shouldn’t
have to go to the total extreme of hiding everything away in an impl, if
you
plan to inherit from that impl later…

Well it’s mostly because the private part include plenty of other
headers to define structures and objects that shouldn’t be part of the
ABI.

This. A major goal of the 3.7 refactoring was to expose as little as
possible of the internal needs of individual blocks, so that external
users
of block classes wouldn’t need to have headers installed, etc.

Glad you got it working.