gnuradio / gnuradio

GNU Radio – the Free and Open Software Radio Ecosystem
https://gnuradio.org
GNU General Public License v3.0
4.95k stars 1.88k forks source link

Invent new `stateless` block type #2666

Open marcusmueller opened 5 years ago

marcusmueller commented 5 years ago

After a discussion with @osh this sounds like a very good idea:

Blocks that don't actually need any block state shouldn't have a (general_)work method that's allowed to modify any local block state. Instead, a simple

unsigned int gr::stateless_block::general_work(items_out, const items_in, tags_out) const

would do.

Motivation:

michaelld commented 5 years ago

Love the idea. I wonder if *_work is const whether buffers get updated correctly, whether the current R/W pointers or tags. Especially tags ... can I add a tag to a stream from a const method? Thinking not, but I haven't looked at code in a long time ... so maybe so? Interesting idea!

marcusmueller commented 5 years ago

@michaelld a const member function can't modify any class instance data. Basically, the this pointer becomes const (and you'd have to imagine every single member access to be prefixed by this->).

So, what definitely works is instead of letting the function call add_item_tag is simply passing it an empty (or prefilled? Maybe that's more elegant when tags get propagated?) tag vector by reference. The function then just modifies that argument.

What doesn't work is using methods like produce or set_relative_rate, and that's the reason why I'd start doing this for the "simplest" type of blocks: sync blocks.

marcusmueller commented 5 years ago

In fact, at this point, even things like nitems_read would be non-guaranteed to be callable because they're not const, but #2665 strives to change that. (It's mostly an issue of declaring them as such.)

michaelld commented 5 years ago

Maybe we need a const version of the most critical methods, which then internally de-const-ifies & allows calling the non-const method? Hacky, but it might work.

I think it's worth exploring moving various methods to const & seeing what compiles successfully and what breaks. We'd learn about GR internals, and maybe come up with better ways to do some required tasks (such as setting actual topology).

marcusmueller commented 5 years ago

Well, the good thing is that most of these functions really don't change any state and could really well be made const without touching any code, as far as I can see.

michaelld commented 5 years ago

Well, the good thing is that most of these functions really don't change any state and could really well be made const without touching any code, as far as I can see.

So true! I'd stay if "we" are going to do this, start with the less-used methods & verify that GR builds correctly with each as const. Then iterate working toward more-used methods.

I think that we can divide the methods into 3 categories: (1) A reasonable number that can be made const without repercussions (at all); (2) some that can be with minimal repercussions; (3) some that will just critically break things either IT or OOT.

My advice on the path forward here is to stop initially somewhere toward the latter part of (2): let's try tweaking some that will likely cause breakage for a few OOT modules that shouldn't be doing what they do there anyway, and get those fixed. Try to get worked all the way through (2) and start on (3) in this manner. (3) will be more challenging to fix both IT and OOT, so maybe stop once the challenges get too much.

marcusmueller commented 5 years ago

Very wise! From the top of my head, the main suspect for (3) here would be check_topology, which is a commonly used way of figuring out how many in- and output ports are connected, assuming the last call to check_topology is parameterized with the final connectivity.

Another candidate would be forecast.

michaelld commented 5 years ago

Agreed! I can think of a few GR/IT blocks that use check_topology in non-const ways. I can't think of any for forecast, but I wouldn't doubt it if some exist either IT or OOT. I do agree, with respect to the former, that there needs to be a standard way to handle setting the topology ... not sure what the best solution is, which I think is why folks have used check_topology.