Restore demodulator settings after recreating it

[vladisslav2011]

Fix regression, introduced in e5e16cc.
Store filter limits and shape to private class members.
Restore filter bandwidth/offset, squelch level, cw offset after the
demodulator is recreated in connect_all to match actual settings.
Track state of RDS decoder and restore it too to prevent a freeze.

[argilo]

Destroying the whole receiver in #1036 seems like a heavy-handed approach that's now resulting in the need to add more complexity to the receivers here. Do you think it would be simpler to just add a reset method to the receivers that would re-initialize things like IIR blocks?

[vladisslav2011]

Do you think it would be simpler to just add a reset method to the receivers that would re-initialize things like IIR blocks?

I don't think so. Resetting IIR blocks by calling set_taps may be enough to fix the white waterfall and no sound issue, but this would leave the second bug (triggering gnuradio/gnuradio#5436) open. Recreating the whole receiver is quick and efficient fix. If the receiver will be changed: more modulations added, digital modes and more, then we'll have to track IIR block usage and not forget to add them to reset method. More work, more code, more possible bugs.
I can implement a different solution to #1018 (IIR filters resetting with set_taps) in separate PR just to check, how would it work and to see, how much changes would it contain.

I have updated the linked issue: #1039. It looks, like this is a separate bug, not a regression after e5e16cc...

And it looks, like #1030 should be reopened with slightly different description: "Random GUI freeze after stopping WAV recorder and changing receiver settings".
I did some tests and was able to reproduce it:

1. Select AM demodulator
2. Start WAV recorder
3. Stop WAV recorder
4. Change AM demodulator DCR setting

[vladisslav2011]

I can implement a different solution to #1018

Done. See my branch https://github.com/vladisslav2011/gqrx/tree/white_waterfall_reset_iir.
Long story short: it does not work at all...

[argilo]

What I had in mind was to destroy & recreate the IIR blocks, not just set the taps.

[vladisslav2011]

What I had in mind was to destroy & recreate the IIR blocks

Hmmm... set_taps is expected to do exactly the thing, we need: https://github.com/gnuradio/gnuradio/blob/v3.7.11/gr-filter/include/gnuradio/filter/iir_filter.h#L154-L181
But it does not work.
I have tried destroying and recreating IIR blocks too. The waterfall/spectrum is correct, but the audio is absent.
It looks, like simple_squelch is blocking the signal. I'll try to recreate it too.

[argilo]

set_taps is expected to do exactly the thing, we need

Oh, nice!

But it does not work. [...] It looks, like simple_squelch is blocking the signal. I'll try to recreate it too.

Sounds good. Whether or not we go with this approach, it will be good to understand which blocks have an infinite impulse response and therefore cannot tolerate invalid input.

[argilo]

The Simple Squelch block does use an IIR filter under the covers, so that would explain it:

https://github.com/gnuradio/gnuradio/blob/b8d853a1a04b16cf9261e216b5330a4e7523b6ad/gr-analog/lib/simple_squelch_cc_impl.h#L25

[argilo]

Also, its set_alpha method resets the IIR filter's taps, so I expect that would get it back into a good state.

[vladisslav2011]

The second attempt: https://github.com/vladisslav2011/gqrx/tree/white_waterfall_reset_iir2
Setting taps does not work, recreating the block works.

[argilo]

Setting taps does not work

Why not? I'd want to understand why that's the case before rejecting the approach.

[vladisslav2011]

It does not work, because there are still invalid samples in the buffer...
This may be solved by emitting a tag from src, marking the first valid buffer start and setting taps from the tag handler at exact point of device switch...
Recreating the block works, because the unlock method calls tb->stop(), tb->wait() to flush all buffers and then tb->start(), that recreates the flowgraph with clean buffers.
...That's not correct. Setting taps multiple times after having the buffer fully overwritten and clean from invalid samples does not solve the problem. There should be another reason.

[vladisslav2011]

There are 2 reasons, why resetting iir filters does not work:

1. single_pole_iir_filter_cc block, used by dc_corr block does not have a reset method. The underlying single_pole_iir template class does have a reset method, but it is not exposed by the single_pole_iir_filter_cc class. So, there is now way to reset a single_pole_iir_filter_cc block without recreating it.
2. simple_squelch block have exactly the same issue: It uses the single_pole_iir template class, but does not use it's reset method.

[vladisslav2011]

The https://github.com/vladisslav2011/gqrx/tree/white_waterfall_reset_iir2 branch does not work well on my 'weak' test system - Intel Atom N270 (single core, 32-bit only) laptop. The waterfall gets restored, but the sound does not.
The previous solution, merged into master, works almost well. The spectrogram gets overflowed and needs to be reset by switching to different fft width - that's different issue, related to GNU Radio 3.7.9, or Ubuntu 16.04 or 32-bit CPU. My 'test' branch https://github.com/vladisslav2011/gqrx/tree/combined works very well on such an ancient hardware: WBFM mono @2.88Msps. Better, than ever. WBFM stereo is still limited to 300ksps max.

[argilo]

I'm starting to think that it might not be worth the effort to fix #1018 (and potentially introduce new bugs along the way), and that a better course of action might be just to revert #1036. If a user plays an invalid I/Q file and messes up the IIR filters, they can always restart Gqrx.

[argilo]

In that case, an alternate solution for #1030 would be needed, but a temporary workaround might be more appropriate there since gnuradio/gnuradio#5436 should eventually be fixed.

[vladisslav2011]

If a user plays an invalid I/Q file and messes up the IIR filters, they can always restart Gqrx.

The restart is not required. Disabling/enabling "DC Cancel" is enough to fix the 'White waterfall' for one time. Switching from narrow to WFM and back or vice versa is enough to restore the sound for one time. The #1018 is not so critical, as some crashes are. It's more a usability issue. I've faced it multiple times while trying to play different IQ files, downloaded from internet to test AFSK1200 tool for regressions (have not found correct AX.25 recording yet). #1036 is a quick attempt to fix it in easy way. It exposed the #1039 and introduced a small regression (triggering gnuradio/gnuradio#5436 after switching input devices with RDS decoder enabled). Both issues are fixed by this PR. From my point of view, always resetting the receiver after demodulator/input device change is good thing as it leaves much less points, where more regressions, related to triggering gnuradio/gnuradio#5436, may be introduced.

In that case, an alternate solution for #1030 would be needed

There is an alternate fix, included here: https://github.com/vladisslav2011/gqrx/commits/white_waterfall_reset_iir2
Should I split it into separate commit and open a PR?

[argilo]

Closing this, as it's not needed now that #1036 is reverted.

[argilo]

always resetting the receiver after demodulator/input device change is good thing as it leaves much less points, where more regressions, related to triggering gnuradio/gnuradio#5436, may be introduced.

There were still plenty of places that gnuradio/gnuradio#5436 could be triggered even after #1036. I'd rather make a targeted workaround (as in #1049) instead of redesigning Gqrx to work around an upstream bug that will eventually be gone.

[argilo]

This may be solved by emitting a tag from src, marking the first valid buffer start and setting taps from the tag handler at exact point of device switch...

Agreed, stream tags might be a good solution.