Guard against too low TX delays. by aknrdureegaesr · Pull Request #63 · JS8Call-improved/JS8Call-improved

aknrdureegaesr · 2025-11-23T03:40:14Z

To continue the title: They can cause initial symbols to be dropped in turbo mode.

We had a premature push to our main branch. I object to the pull request solution that was merged, on the following grounds:

It essentially undoes work that I had previously done. We need to keep the knowledge in the code that symbol time gets eaten if we use a delay below 0.1 s. It took me a while to find that out, and I suggest we should strive to keep that knowledge in the code.
The first step is to give those 0.1 s a name. In general, it is considered bad style to have unexplained magic constants somewhere hidden in the code. Just pulling that constant into the header file, giving it a comment explaining it, and giving it a proper name, is already an improvement I would not want undone.
Unfortunately, the UI file does not seem to allow comments. So, in the UI file, we cannot help using magic constants.
But putting some code in the configuration source that checks against the named constant makes it clearer why the UI file has the magic constant of 0.1 s instead of 0.
Finally, that check code also guards against the case that some user edits the ini-file directly and uses tx delay values that are out of bounds. The checks in the configuration source explain that the user cannot do that. This is a better user experience, as compared to running into problems and never knowing why.

They can cause initial symbols to be dropped in turbo mode.

Chris-AC9KH · 2025-11-23T05:19:14Z

              do_post_ptt (false);
-              QThread::msleep (100); // some rigs cannot process CAT
+              QThread::msleep (PTT_DELAY_MS); // some rigs cannot process CAT
                                     // commands while switching from


Actually, this already explained here in the code with a comment. Why are we adding code bloat for this? I specifically object to that. Code bloat is a common problem with open source projects "just because you can". But that doesn't make it right. As an example, on another project I work on where code runs on STM32 microcontroller chips, enough developers kept doing this type of stuff, and then it ends up having to be trimmed out because the binary wouldn't fit on the STM32 memory anymore.

Chris-AC9KH · 2025-11-23T05:55:25Z

Unfortunately, the UI file does not seem to allow comments. So, in the UI file, we cannot help using magic constants.

Bingo! It's just a minimum allowable setting in the UI code. It fixes the original issue without any fanfare, without any bloat, the issue created by a misconfigured setting in the first place. The fix I applied prevents you from setting that Tx delay wrongly, which was the real issue.

Now, if you want to edit the js8call.ini file and push the limits, good luck with all that. You are now in unsupported territory, attempting to create your own issues. You can break more than the Tx delay by editing that file.

That PR was actually merged here 02781de, and then I reversed it to remove the bloat for a simple fix when you finally admitted you set the Tx delay to zero. Which anybody who understands how transmitter relays work would never do in the first place.

So this just blows my mind that you insist on adding in code bloat again for something that is not needed. The tooltip popup would be the obvious place to explain the min setting and no users are going to question that.

Chris-AC9KH · 2025-11-23T06:05:21Z

  return true;
 }

+void Configuration::impl::check_tx_delay() {


This function is totally unnecessary. The UI will not allow a setting below 100ms or above 500ms. Why is it here?

"Why is it here?" is the fair part of the question.

This "totally unnecessary" function serves three different audiences, and I hope it serves them well:

People who used prior versions of JS8Call or JS8Call-improved that did allow smaller values.

People who dare to use an editor to configure stuff in the init file.

Whatever developers come after us and may not know about the connection of a number in the UI file and a number somewhere in the depths of TransceiverBase.cpp. This code ties those two together.

the issue created by a misconfigured setting in the first place.

I happen to know that there is a silence at the beginning of each JS8-transmission of at least 100 ms anyway, depending on mode. I have reason to suspect my TX switches within a couple of millisecond or so. The UI offers me to tell it "no, I don't need any additional tx delay". How should I know that it is a misconfiguration to take that offer?

So then this appears. What we're doing here is calling a function to check the setting range of the Tx delay on the outside chance that somebody might edit js8call.ini.

On the not-so-outside chance that someone may have been using previous versions of JS8Call or JS8Call-improved and followed its invitation to configure values that lead to this undesired behavior.

I want to make sure this is corrected on first run, and I want to make sure the user is informed about the correction.

It is always a touchy thing for a software to disobey something a user has demanded. If that need arises, as is the case here, the software should at least be transparent about its disobedience.

On the not-so-outside chance that someone may have been using previous versions of JS8Call or JS8Call-improved and followed its invitation to configure values that lead to this undesired behavior.

The counterpoint to this is that if somebody is moving from an older version to the newer version they expect to not have to re-configure their settings. If what they had was working, you are now forcing a change that they didn't ask for. People that have spent time setting up their system, and it works, do not like that. As a programmer you have no right to force this new setting on them. As it is now, if their js8call.ini has 0ms set for Tx delay, the UI limit change won't change that unless they try to change it again. At which point the tool tip should pop up and explain the new changes.

Microsoft is famous for overriding people's settings, why are we doing it here?

Joe-K0OG · 2025-11-23T22:38:35Z

I hope this doesn't open up an ugly can of worms... I wonder if some pre-tone might be helpful for those who use VOX? This could be a function that is enabled/disabled completely in settings, and the length of pre-tone can be set (within reasonable limits, like limiting the delay to between 0.1 and 0.5 seconds). I personally have no use for this, and am not specifically aware of a problem related to this, but find that Fldigi has such a function, so it must be helpful to some operators.

The pre-tone could be simply a tone within the bandwidth of the transmitted mode, perhaps on the selected waterfall audio frequency (applying the "Fake it" split to the pre-tone the same as it is to the message tones).
The pre-tone could be user-settable to be sent on every frame, or only on the first frame.
The pre-tone could begin at at the end of the previous frame-time-slot (before the beginning of the upcoming time slot) so that the pre-tone won't impinge on the upcoming time slot data flow, giving some little delay between the pre-tone and the beginning of data flow.
If we find that the pre-tone causes confusion in the receiving station decoder, maybe the pre-tone could be sent just outside the mode bandwidth (say 10Hz below the lowest mode frequency) so that it can't be confused as part of the data being sent when the frame begins.

If this is an unnecessary or bad idea, just ignore my comments and let's move on. Just thinking "out loud" here... Maybe there is not a real problem that requires a pre-tone solution for VOX users?

73,
-Joe-
K0OG

Chris-AC9KH · 2025-11-24T00:23:20Z

This PR issue actually has nothing to do with VOX. It has to do with the fact that the Tx delay in the UI was either deliberately or inadvertently misconfigured to have 0ms Tx delay. And then an issue with clipping leading symbols was created from this.

The fix is simple - I just changed the UI so Tx delay can't be set below 100ms. The original "fix" coded in a built-in delay that was not presented to the UI end user.

So then this appears. What we're doing here is calling a function to check the setting range of the Tx delay on the outside chance that somebody might edit js8call.ini. More code bloat for something I have never heard of anybody doing, especially since the UI limits the min to 0.1s and max to 0.5s after I fixed it.

So where does it end? I can't approve that. If somebody else wants to, then fine. But I don't think it's good coding practice. What about my setup where I got a Beverage for Rx on 630m and I use the 127 for Tx? The antennas are switched with a downstream relay. If I set my Tx delay to 0.1s it won't work. Requires minimum 0.3s for relays to settle.

The thing is, ham radio operators are supposed to know this. And if they don't they learn it.

The VOX is a separate issue. It doesn't work with my FlexRadio because of too much latency. I know it does work for some, because I have a friend that uses it with a Xiegu G90. A keying tone that obeys the same UI Tx delay setting as CAT PTT would be ideal. But I'm not going get a chance to work on that any time soon as I got a bunch of administrative stuff going right now to get JS8Call-improved ready for the next release.

Joe-K0OG · 2025-11-24T16:32:50Z

The VOX is a separate issue. It doesn't work with my FlexRadio because of too much latency. I know it does work for some, because I have a friend that uses it with a Xiegu G90. A keying tone that obeys the same UI Tx delay setting as CAT PTT would be ideal. But I'm not going get a chance to work on that any time soon as I got a bunch of administrative stuff going right now to get JS8Call-improved ready for the next release.

Do you think that this is something which would be good to put on the Issues list of things to look at later (obviously, low priority)? If not, then just let it drop.

73,
-Joe-
K0OG

Chris-AC9KH · 2025-11-24T16:39:34Z

I actually posted it on the issues list here: #56

I believe there is a very small subset of people using VOX, mostly with small QRP radios. Just during my testing with this PR issue I found it doesn't work with my FlexRadio - couldn't get a single decodable frame out of it. It does work with my IC-7300, but there is considerable clipping of leading symbols that the FEC mostly takes care of. So there is three choices

code in a keying tone (and we must evaluate if the added code bloat is actually worth it)
leave it as-is
remove VOX altogether (which I have a feeling would make some users unhappy).

Chris-AC9KH · 2025-11-24T17:22:46Z

@aknrdureegaesr I really don't want to leave this PR laying here. I would say either merge it if you're comfortable with what it does. Or ask for further review.

In the case of further review here is my objections:

The first "fix" on the original PR coded in what is essentially a bug. Why is it a bug? The expectation from users is that when a UI setting is made the backend subsystem obeys that setting. Your PR over-rode a setting. From what I've seen of Microsoft Windows this might be a common practice there, but I don't think it's a good coding practice here. As a programmer you are essentially taking over control of the users' software because "you think "
The fix I applied to that PR was to simply make the Tx delay setting in the UI so it can't go below 0.1s. Which addressed your UI misconfiguration issue. But I was not totally comfortable with that as some users might want to set it to 0 if they are experimenting with something. Since experimentation is in the spirit of Amateur Radio, and if the user is savvy enough with JS8Call-improved to realize they can modify that setting by editing js8call.ini, you have now once again over-ridden what the user wants to do because "you think ". Plus it adds in what I view as an unnecessary function to check this setting "just because", when the UI already limits that setting to parameters in the function.
Now, I did somewhat the same thing with the HB ACK modifications, based on complaints going way back to the 2.x days. And modified it further in the 2.3 versions based on more complaints from nets and @CALLGROUP users that it wasn't restrictive enough because it was still breaking messaging. I wasn't totally happy about doing the further modifications but I did it anyway based on user feedback, not because "I think ". At the time I did the original modification, I decided it was not a good idea to do this without providing a UI feedback mechanism to show the user when it is being activated. This is the part you broke with the HB/CQ timer update that required a subsequent bug fix. At the time, I figured if some people don't like this automated supression of HB ACK's, and complain about it, I can always add in a user setting in the UI to turn it on or off. But nobody who has used it in 2.3 and 2.4 has ever complained about it. So I'm not adding code for a user setting just because "I think ". I'm leaving as-is until somebody points out that it is an actual issue, while the only feedback on it I've gotten is that people love the way it works now.
Again my suggestion for changes here, if you want document why you did what you did here, is to put that documentation in the Help menu (which would be a first step in actually making the Help menu useful in providing help in using the program), or put it in the tooltip.

So that is my review of this. Either merge if you're comfortable with what you're doing, or ask for further review if you're not comfortable with over-riding what users want to do with their software. Either way, please don't let it just sit here. I have to admit I was not happy when this one popped up. I looked at it and I go "what the hell is this???" There's no way I can approve this. But I am not the ultimate authority on what gets merged into the code.

Chris-AC9KH · 2025-11-24T17:29:32Z

Further suggestion; on the outside chance you might decide it could be a good idea to start a working "Help" menu to document some of these things, for pete's sake DON'T put it in mainwindow. mainwindow already has issues the way it is, we don't need more code in it.

Joe-K0OG · 2025-11-24T17:35:06Z

I agree with @Chris-AC9KH Chris' comments, and think the simple fix is sufficient, and recommend that this be closed.

73,
-Joe-
K0OG

aknrdureegaesr · 2025-11-25T16:25:28Z

I have another concern that I should try to explain to you people.

From decades of coding as a professional, I have learned that good software isn't just software that works as intended. That, too. But it is also important that it is maintainable.

Helpful for maintainability:

For something you want to do, you find the pieces of code that pertain to that "something".
All of them. There is no dark corner where pertinent code also lurks that's difficult to find.

To get that, this is frequently helpful:

Split the code up into pieces which each have one responsibility each.
The pieces are loosely copied.
Any coupling you have should be explicit. No code hiding from you in that a dark corner ready to produce a bug if you don't find it.

Our code as it stands now, it is not very high on the maintainability scale. I think everybody complains about the moloch mainwindow.cpp, and rightly so. This PR does nothing to reduce that problem.

But that line in TransceiverBase.cpp I found the hard way: By seeing that something is amiss (symbol time does not start in time) and debugging the situation. That line, once found, raises a red flag. People can change the tx delay in the UI file, and nothing will point them to this line, even though it is pertinent. People might do stuff in Configuration.cpp and again, nothing will point them to TransceiverBase.cpp.

I feel an urge to change that. I feel an urge to leave a trail leading from the obvious places that concern themselves with tx delay to TransceiverBase.cpp. I think the situation would be (somewhat) improved if some light shone into that dark corner.

aknrdureegaesr · 2025-11-25T16:25:51Z

I have another idea with less "code bloat". Please stand by.

wmiler · 2026-01-02T16:14:54Z

Any update @aknrdureegaesr ?

aknrdureegaesr · 2026-01-15T13:23:33Z

I want to see at least the source code formatting stuff proposed in #49 finished first.

Chris-AC9KH · 2026-01-15T13:26:48Z

Already been done when I moved the loose files out of the root of the source tree. I did it manually with Xcode.

aknrdureegaesr · 2026-01-15T13:31:13Z

Let's discuss that over at #141 please.

wmiler · 2026-02-01T19:21:35Z

I've marked this as stale for now, as the code here will need a rewrite after the big refactor. I would suggest that @aknrdureegaesr close this and submit a new PR.

Chris-AC9KH · 2026-02-01T19:26:47Z

Have to first decide if it's even valid. Analysis with an oscilloscope revealed that even VOX works without clipping any leading symbols.

Chris-AC9KH · 2026-03-29T14:02:20Z

I'm going to close this PR. It's not only seriously out of date, we have new encoder/decoder fixes since the JS8 40 and JS8 60 modes were added. With JS8 60 it would be desirable to remove the 0.1s minimum Tx delay and let the software "figure it out". So this PR no longer applies.

JS8Call-improved#3 waterfall right-panel Control/Display/Timing tabs CN visible (User verified). - 69 EN sources → CN: 控制/显示/时序三 tab + 子控件 (label/tooltip/spinbox 后缀) - QSY 保英文 (§4 缩写锁); NORMAL/SLOW 保英文 (tooltip 内 JS8 mode names) - TX cycle → 发射 (JS8Call-improved#63-66); Timing → 时序 (JS8Call-improved#52) - Offset → 频偏 (JS8Call-improved#3, 与 JS8Call-improved#5 Offset:→频偏: 对偶, 覆盖 lupdate same-text body-fill stub 偏移) - lupdate fresh: Found 851 source text(s) (0 new), FALSIFIER① 排除 Known limitations: - EN switch 需 restart (PARK-092 sub-window changeEvent override 共缺, 接受) - Directed-to 切换"闪现-回 EN" → PARK-095 (动态 menu lifecycle, 另开 sub-phase) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Guard against too low TX delays.

63042a9

They can cause initial symbols to be dropped in turbo mode.

Chris-AC9KH reviewed Nov 23, 2025

View reviewed changes

aknrdureegaesr mentioned this pull request Jan 21, 2026

Enable hidden ULTRA mode. #155

Closed

wmiler added the stale label Feb 1, 2026

Chris-AC9KH closed this Mar 29, 2026

Uh oh!

Conversation

aknrdureegaesr commented Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Chris-AC9KH Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Chris-AC9KH commented Nov 23, 2025

Uh oh!

Chris-AC9KH Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

aknrdureegaesr Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

aknrdureegaesr Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chris-AC9KH Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Joe-K0OG commented Nov 23, 2025

Uh oh!

Chris-AC9KH commented Nov 24, 2025

Uh oh!

Joe-K0OG commented Nov 24, 2025

Uh oh!

Chris-AC9KH commented Nov 24, 2025

Uh oh!

Chris-AC9KH commented Nov 24, 2025

Uh oh!

Chris-AC9KH commented Nov 24, 2025

Uh oh!

Joe-K0OG commented Nov 24, 2025

Uh oh!

aknrdureegaesr commented Nov 25, 2025

Uh oh!

aknrdureegaesr commented Nov 25, 2025

Uh oh!

wmiler commented Jan 2, 2026

Uh oh!

aknrdureegaesr commented Jan 15, 2026

Uh oh!

Chris-AC9KH commented Jan 15, 2026

Uh oh!

aknrdureegaesr commented Jan 15, 2026

Uh oh!

wmiler commented Feb 1, 2026

Uh oh!

Chris-AC9KH commented Feb 1, 2026

Uh oh!

Chris-AC9KH commented Mar 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aknrdureegaesr commented Nov 23, 2025 •

edited

Loading

aknrdureegaesr Nov 25, 2025 •

edited

Loading