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

Fix double call to receiver::set_rf_freq, may fix #1129 #1309

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

0penBrain
Copy link

@0penBrain 0penBrain commented Oct 8, 2023

Also commented at conf loading because same thing arises there.

@argilo
Copy link
Member

argilo commented Oct 8, 2023

may fix #1129

Before merging a change intended to fix a bug, I'd want to understand the root cause of the bug in question, and have some confirmation that the bug is actually fixed.

I have my doubts that the duplicate setFrequency calls are actually causing #1129, because it wouldn't explain why it only happens when tuned to a bookmarked frequency. And even if duplicate setFrequency calls were triggering the issue, removing duplicate calls here wouldn't fix the root cause (which presumably would be in in rtl-sdr or gr-osmosdr). There are other ways of triggering rapid setFrequency calls, like dragging the mouse across the plotter.

@0penBrain
Copy link
Author

Before merging a change intended to fix a bug, I'd want to understand the root cause of the bug in question, and have some confirmation that the bug is actually fixed.

I'm sorry I can't help because I'm not reproducing here. But at least guys reproducing can get this branch and test. 😉

Also notice that even if it doesn't fix the bug, it still is more proper implementation. 😄

I have my doubts that the duplicate setFrequency calls are actually causing #1129, because it wouldn't explain why it only happens when tuned to a bookmarked frequency.

I share this but at some point you have to try something. 😄 Also notice in the bug discussion, no one formally said "it happens only when I'm on a bookmark and activate another bookmark, but doesn't happen if I'm on an arbitrary frequency and activate a bookmark". 😉

And even if duplicate setFrequency calls were triggering the issue, removing duplicate calls here wouldn't fix the root cause (which presumably would be in in rtl-sdr or gr-osmosdr). There are other ways of triggering rapid setFrequency calls, like dragging the mouse across the plotter.

The time frame of the dup call wouldn't be exactly the same, as when dragging the mouse it would happen on (at least) 2 different runs of Qt event loop. Here it happen in the same run (guess so, a bit unsure about Qt signal/slot).

@argilo
Copy link
Member

argilo commented Oct 8, 2023

it still is more proper implementation

Sure, if there are reasons other than #1129 for making this change, we can make it. I'll need to review to make sure there aren't any unintended side effects.

you have to try something

Yeah, experiments to get to the bottom of #1129 are a good idea. Another thing to try might be intentionally stressing the RTL-SDR with very frequent setFrequency calls.

@willcode
Copy link
Contributor

willcode commented Oct 8, 2023

Blocking the signal in freqctrl::setFrequency() to avoid the loop would be better, architecturally. There are probably multiple places where we depend on a signal being routed through a GUI element, but it's not terribly modular that way.

The problem here is that setFrequency() is also used internally in the widget. That would have to be changed, so it was obvious that the change was due to a signal.

@0penBrain
Copy link
Author

Blocking the signal in freqctrl::setFrequency() to avoid the loop would be better, architecturally.

Yes. Thought to that but this is far more changing and testing, because the signal is actually connected to 3 or 4 slots in MainApplication. Would deserve an architectural decision first.

The main problem IMHO is that 2 functions are calling for a (hardware) frequency change in the code (1 in main application and 1 in the frequency controller). There should be only one (I have no clue which one is better to keep). 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants