Drop streaming flag after cancelling all transfers

[ggatis]

Otherwise transfer_tread_started is false, but streaming is still true. No big deal of course.

┆Issue is synchronized with this Basecamp todo by Unito

[martinling]

Hi @ggatis, I'm sorry this has PR sat around for a while without much response.

It's obviously a very simple change, but it's been looked at a couple of times without it being immediately obvious if it's correct, so it's sat in the list for a while.

Do you happen to remember why you made this change? Was there a problem you were seeing that it fixed?

We do see a scenario in which the streaming flag may not get cleared, if a program that starts streaming using hackrf_start_rx or hackrf_start_tx, goes straight to calling hackrf_close without first calling hackrf_stop_{rx|tx}. Was that part of the scenario that led you to make this change? It would be nice to have a test case so that we can reproduce the problem and understand the fix better.

We were thinking that it might make more sense to clear the streaming flag in hackrf_close, rather than in cancel_transfers, but I'm not sure if there's a particular reason you put it there.

[martinling]

I've now incorporated this change in #1069.

[ggatis]

Hi Martin, I am sorry - I did not answer your question immediately and forgot about it later (was working on an article and then got sick). I am more than a year (or two) away from that code - that's the main reason why I did not answer immediately since I just do not remember exactly. But the thing was similar to the one you described - since streaming was a flag the code (I do not remember it was original code or my) in some place analysed it and got an incorrect idea what actually is happening (cancelled streaming or no streaming actually). I remember I corrected it for me but was not sure if it was the right way so I did not push it but just wrote a PR. Kind regards ~ Gatis

…

On Tue, 7 Dec 2021 at 04:43, Martin Ling ***@***.***> wrote: Hi @ggatis <https://github.com/ggatis>, I'm sorry this has PR sat around for a while without much response. It's obviously a very simple change, but it's been looked at a couple of times without it being immediately obvious if it's correct, so it's sat in the list for a while. Do you happen to remember why you made this change? Was there a problem you were seeing that it fixed? We do see a scenario in which the streaming flag may not get cleared, if a program that starts streaming using hackrf_start_rx or hackrf_start_tx, goes straight to calling hackrf_close without first calling hackrf_stop_{rx|tx}. Was that part of the scenario that led you to make this change? It would be nice to have a test case so that we can reproduce the problem and understand the fix better. We were thinking that it might make more sense to clear the streaming flag in hackrf_close, rather than in cancel_transfers, but I'm not sure if there's a particular reason you put it there. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#773 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AM2RS3YJP5UATUGGGT2CLZDUPVYFPANCNFSM4PXQGHEA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[martinling]

Hi @ggatis, thanks for confirming.

I was pretty sure your fix was correct, but I'd been a bit unclear on exactly what that flag was supposed to mean, and when and where it should be cleared.

Eventually some other bugs showed up where streaming wasn't being cleared (#1056 and #1068), so I went through all the usage in detail in PR #1069.

Once I'd done that and simplified the result, it turns out the streaming flag needs to be cleared in only two places: one of them being cancel_transfers, just as you suggested. So this PR was very helpful in getting to that point, thank you.