-
Notifications
You must be signed in to change notification settings - Fork 149
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
JT4 enablement #374
base: develop
Are you sure you want to change the base?
JT4 enablement #374
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the pull request. The change looks good overall, just needs a final polishing pass, please see inline comments. Please also check the question about a potential impact on CPU usage by the increase in frequency tolerance. Cheers!
owrx/wsjt.py
Outdated
def getInterval(self): | ||
return 60 | ||
|
||
def getSubmode(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this method being used anywhere. Any plans for this? If not, I'd recommend removing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is needed by audio/chopper.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't. Please remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right... so... I saw the other comment, I believe it went a bit astray. I'm gonna stay on this thread in an attempt to keep things separate.
I also think I see what the issue is here, it's a simple misunderstanding. In pull requests, you usually put comments on the offending line, or the line that best matches the topic to be discussed. In this case, I put the initial comment on the line that has the getSubMode()
method signature, trying to tell you that it isn't used.
getInterval()
is indeed used, it is part of the AudioChopperProfile
API, but I wasn't referring to that one 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected;)
@jketterl how do you think, is it now old enough to be merged?:) |
Has there been any updates? |
Well I believe for basic functionality nothing more needed? Decoding of JT4 is currently based on the default bandwidth. I also checked as you requested the CPU Increase and described them so you can decide if to implement a possibility to modify the decode bandwidth (that I consider as a feature update)... So well is there anything missing? |
Your initial response to the discussion about the
This gave me the impression that being able to control the frequency tolerance was essential to this feature. As such, I did not consider merging this for as long as this setting is not available. |
It's true, but with agile approach it's better to have a MVP;) |
@jketterl any chance to merge it? |
Do you see any change? Did you do any changes yourself? |
Hey @jketterl I have added a generic possilbilty for wsjt modes to set frequency tolerance - only interpreted by JT4 now. |
Looks alright on first glance, but I'd prefer it to be labeled "JT4" both visible and internally. It only applies to JT4 modes (at least that's what I understand, and that's also what I'd expect). |
No, the feature of frequency tolerance is generic to all jt9 related modes. |
Ah, now I see, you wanted to extend this to all WSJT-X modes. Any reason to do so? I don't see any problems with the current default, at least not for the existing modes. I feel like this will only cause additional CPU overhead when used, and you'll probably need a separate setting for each mode. Why not limit this to JT4 (where it's needed)? |
Okey, will limit that to JT4 and Q65 if its okey for you? |
once again, why Q65? |
both Q65 and JT4 are often used by microwave beacons... where freq stability is not so good. |
Let me be more precise WHY, common setup to receive 10GHZ with openwebsdr is rtldongle + LNB from SAT TV. LNBs usually dont have stable frequency for SSB and for narrow modes its even more an issue. Plus on microwaves with digi often beacons are received through rain scatter or air plane scatter - both impacting the receiving frequency. So that frequency tolerance is important on microwaves to compensate those situations. True, I do not see any usage today beyond JT4 (g) or Q65 (c,d,e) so I would propose I will change this setting to JT4 and Q65 frequency tolerance. And apply that only to those if it will be ok for you. |
@jketterl any comment, suggestion so I can adjust it according to your comfort? |
Apologies, I thought I had sent a reply already. Probably typed something and lost it in some buried browser tab. For general practice, I'd say it would be better to have a separate pull request for Q65, mostly because there should be no depencencies here, I'd say both modes should have an independent setting. Other than that this looks alright to me. |
Signed-off-by: Dawid SQ6EMM <[email protected]>
Just a kindly reminder;) |
owrx/wsjt.py
Outdated
def getInterval(self): | ||
return 60 | ||
|
||
def getSubmode(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't. Please remove.
owrx/wsjt.py
Outdated
@@ -29,6 +29,13 @@ def decoding_depth(self): | |||
# default when no setting is provided | |||
return 3 | |||
|
|||
def jt4_frequency_tolerance(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has no business being on WsjtProfile
if it's only used in Jt4Profile
. Please move.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getInterval is used - as this mode is decoded every 60 seconds, if removed:
Traceback (most recent call last):
File "/usr/lib/python3.11/threading.py", line 1038, in _bootstrap_inner
self.run()
File "/opt/openwebrx/owrx/audio/chopper.py", line 59, in run
self.setup_writers()
File "/opt/openwebrx/owrx/audio/chopper.py", line 48, in setup_writers
sorted_profiles = sorted(self.profile_source.getProfiles(), key=lambda p: p.getInterval())
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/openwebrx/owrx/wsjt.py", line 79, in getProfiles
return [JT4Profile(i) for i in profiles if i in JT4Profile.availableSubmodes]
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/openwebrx/owrx/wsjt.py", line 79, in <listcomp>
return [JT4Profile(i) for i in profiles if i in JT4Profile.availableSubmodes]
^^^^^^^^^^^^^
TypeError: Can't instantiate abstract class JT4Profile with abstract method getInterval
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has no business being on
WsjtProfile
if it's only used inJt4Profile
. Please move.
Corrected.
This is happening due to strange jt9 behaviour. I confirmed it also using recordings from owrx outside of containers on my local computer:
this is with just background noise I have asked for it on wsjt group. (https://wsjtx.groups.io/g/main/message/55445) |
Signed-off-by: Dawid SQ6EMM <[email protected]>
Thanks for inquiring about the empty messages. Let's see what comes out of it. I'm not subscribed to that mailing list, so feel free to ping here if there's any updates. |
Signed-off-by: Dawid SQ6EMM <[email protected]>
I suggest we leave it as it is - native behaviour. |
This is the proposed script that will replace jt9 command for jt4 (if you think it make sense) Characters that need to be taken into account as end of message are: f, fN, dCN - not only "f" like in an example. Following wsjt-x Use Guide - 13.2 Decoded Lines Table 5 (Notations used on decoded text lines).
and output of execuction:
unless you prefer to leave it untouched. I have an idea how to do the same inside of WsjtParser() - but first decision needed how it should behave;) |
Well, I'm afraid I don't think that adding a script for this purpose makes sense. Not only does this add a new dependency ( If you want to filter those messages I recommend having a look at the existing parser code inside OpenWebRX. You have a new reply by Joe Taylor now to explain what those messages are. |
Signed-off-by: Dawid SQ6EMM <[email protected]>
Do we miss anything yet? |
This is fixing #373