Not sure how you'd want me to review this. Do you want to simply improve on what we have and leave some problems to be dealt with later, or do you want to fix all known problems?

If the later, there is more to be done.

Check packet full

To quote https://pskreporter.info/pskdev.html :

The datagrams should be sent at a rate of no more than one every five minutes (unless the packet becomes full).

I have not looked into this in detail, but this "unless the packet becomes full" promises to become tricky: We need to test whether we have enough data to fill the packet to the brim and send it. This test is most conveniently done the moment the new information comes in from the decoder. Yet, we would do a bad job if we sent right then and there. That would make sending more probable at the times when signals complete in JS8. Once we have a more sizable user crowd than we do now, this would result in load spikes at PSKReporter.

I suggest to use a random wait time that, in its maximum, is the same than the current mode's period, once enough data to fill a packet has become available.

Precision timer

I have found in the Qt documentation: When you do not use precision timers, Qt tries to run several timers' triggers at the same time. This could potentially concentrate network sending triggers to such times as when the protocol has activities. So, even though this may look like overkill (and probably is, though I'm not sure), I would feel better if we used a Qt precision timer here.

Put network stuff out of the main thread

I have not looked into this in detail, but I find it quite suspicious that addRemoteStation doesn't take part in Qt's signal+slot mechanism. I think it should.

And I think network stuff belongs into a thread of its own. If a user switches TCP/IP on and connection attempt fails with a timeout, there is danger that this will freeze the UI.

It is "all five minutes"

... so the min_send_interval should be increased from 120 to 600.

Send last available info, not first

If several transmissions come in for the same station on the same band while we're waiting for our 5 minutes to elapse before sending again, I would argue the sender wants to know about the last transmission, not the first; but our present code sends the first, not the last.

Avoid lifecycle issues - Laster edit: This is ok. The queue and cache copy on insert.

I'm a bit concerned about the line

      m_->spots_.enqueue({call, grid, snr, freq, mode, DriftingDateTime::currentDateTimeUtc()});
      m_->calls_.insert(cache_key, std::time(nullptr));

The objects behind call and cache_key will go out of scope and disappear. If I have the rules correctly, cache_key disappears when the function is complete, while call is an object controlled by the caller that may disappear whenever the caller decides it no longer needs it. So I think we need to copy before pushing stuff into our long-term data structures, and we need to delete those copied objects when we remove them from those data structures. I do not see any of that here. But maybe my C++ is rusty and I'm all wrong?

Finally, we are in a Qt project here, so I would slightly prefer to we standardized on QDateTime instead of using std::time. But that may be more a matter of taste and doesn't really matter much.

As far as I'm concerned, you can rebase to the master's HEAD and also squash future commits together with this into one before my next round of reviews.

A small cosmetic problem with

m_->spots_.enqueue({call, grid, snr, freq, mode, DriftingDateTime::currentDateTimeUtc()});

In my ALL.TXT, I find a line

2025-07-30 16:00:45-13  0.3 2090 A  OWIi-BRpAZy0         1    2E0EEY: DJ3EI  

That line tells me that my friend Chris 2E0EEY sent something to me with the transmission starting 16:00:45. But to PSK reporter, we will report that same transmission with a timestamp of 16:00:58 or thereabouts. Why this inconsistency?

For ALL.TXT writing and other purposes, we already have code that calculates the start of a transmission. That timestamp should be handed to this functionality here and used when sending data to PSK reporter.

Maybe we should start a new issue around this, my changes were minimally invasive, these questions are really revisiting basic assumptions about how the whole class works.

I think your only question about my changes has to do with lifecycle issues, but when cache_key is inserted into the QHash, it's copied by value, so no problems there. Looks like the enque into spots has worked the same way since Allan worked on it last year.

Regarding the difference in timestamps, I can't determine why the delta is so wide in your example - were you perhaps running in a debugger when you saw this? My read on the situation is that different places in the code make calls to DriftingDateTime::currentDateTimeUtc() to generate timestamps to use for the same event, which is sub-optimal, especially since the data structures ActivityDetail and CallDetail contain utcTimestamp members that are already set using that method. I can see that potentially being off by a second every now and then, but the only explanation I can come up with for being that far off is a pause in execution.

At minimum we should be passing that same timestamp into things like PSKReporter::addRemoteStation() rather than generating a new timestamp inside of the method. Alternatively, we could construct a Spot instance from the CallDetail and pass that around instead of the raw values. My inner OO astronaut (I'm a JAVA guy in real life) wants to change all of these obviously related and overlapping structs to classes and build out an object model that allows us to pass polymorphic objects around instead of dealing with decomposing them into long argument lists. But if we start going down that road, we're in for some major refactoring because that 11K line MainWindow class is more of an issue than this little annoyance.

struct CallDetail
  {
    QString call;
    QString through;
    QString grid;
    int dial;
    int offset;
    QDateTime cqTimestamp;
    QDateTime ackTimestamp;
    QDateTime utcTimestamp; // <-- We should always be using this
    int snr;
    int bits;
    float tdrift;
    int submode;
  };

void MainWindow::processSpots() {
      [...]
      CallDetail d = m_rxCallQueue.dequeue();
      [...]
      // Should be sending d.utcTimestamp here...
      pskLogReport("JS8", d.dial, d.offset, d.snr, d.call, d.grid);
      [...]
}

// Receiving it here...
MainWindow::pskLogReport(QString const & mode,
                         int     const   dial,
                         int     const   offset,
                         int     const   snr,
                         QString const & callsign,
                         QString const & grid)
{
[...]
  // Emitting it here...
  Q_EMIT pskReporterAddRemoteStation (callsign,
                                      grid,
                                      dial + offset,
                                      mode,
                                      snr);
}

// Which gets passed though here...
connect (this, &MainWindow::pskReporterAddRemoteStation, m_pskReporter, &PSKReporter::addRemoteStation);

// And ends up in a new argument here
PSKReporter::addRemoteStation(QString           const & call,
                               QString          const & grid,
                               Radio::Frequency const   freq,
                               QString          const & mode,
                               int              const   snr)
{
      [...]
      // And finally set here
      // Why generate a new timestamp? It's in the call detail struct that our args came from
      m_->spots_.enqueue({call, grid, snr, freq, mode, DriftingDateTime::currentDateTimeUtc()});
      [...]
}

@rruchte @aknrdureegaesr I think here that Rob proposed a minimally invasive change, as he called it. I see nothing wrong with the PR. If we are going to do a complete overhaul of the class, that should be discussed via the issues list to arrive at a consensus, and submitted as a separate PR. I would suggest just rebasing this one so it will merge. Merge it. Then move to the issues list for the rest.

Not sure how you'd want me to review this. Do you want to simply improve on what we have and leave some problems to be dealt with later, or do you want to fix all known problems?

i'll make a comment here. This is a valid PR and it should be merged. It is unreasonable through code review to require the author of a PR to attempt an overhaul of an entire class. I just got done fixing an issue that came from js8call that used a deprecated Qt function to make logging timestamps UTC instead of local time. I would've never approved such a merge because there was an actual problem with the code. But at js8call it got merged anyway. That is the purpose of code review. In this case, I see nothing wrong with the code.

This PR is totally legit and an improvement over what we had previously, so if @rruchte wants to merge it, it looks good to me.

I was asking whether we want to improve on all known problems in this area. I had no intention towards demanding it, in this PR.

Regarding the difference in timestamps, I can't determine why the delta is so wide in your example

It was speculation, guided by some knowledge of the code, not observation.

I have seen (and partly repaired) the code that does the ALL.TXT writing. It writes the timestamp that catches the beginning of the reception of the received data.

I somehow presume that the "log to PSK-Reporter" functionality is only called after that data is actually received. I assume that happens, e.g., some 13 or 14 seconds later (my example was normal mode), as it takes a while to push the data through the ether.

@rruchte Rob, after rebasing so this merges, please merge it. Thanks!

Put network stuff out of the main thread

I have not looked into this in detail, but I find it quite suspicious that addRemoteStation doesn't take part in Qt's signal+slot mechanism. I think it should.

And I think network stuff belongs into a thread of its own. If a user switches TCP/IP on and connection attempt fails with a timeout, there is danger that this will freeze the UI.

There should be, to my knowledge, nothing network related left on the main thread post 2.3.0; this was indeed a problem in 2.2.0, but should have all been addressed in the 2.3.0 changes.

Finally, we are in a Qt project here, so I would slightly prefer to we standardized on QDateTime instead of using std::time. But that may be more a matter of taste and doesn't really matter much.

I'll register an opposing view here; the lower-level the code, the more we should see std:: in play. So here's the thing, Andreas. The C++ standard is formed by a committee of people for whom I am mostly unqualified to fetch coffee. It is painstakingly peer reviewed and, in that it's conforming with an ISO process, is extremely well-defined and trustworthy. Further, on the committee, there are many....Germans, and the degree to which they enforce order is as you would expect.

In this particular instance, I think either is fine, but as a general rule, if we're out of the UI, i'll tend to reach for the standard library first.

I have seen (and partly repaired) the code that does the ALL.TXT writing. It writes the timestamp that catches the beginning of the reception of the received data.

I saw that after some debugging and a close examination of ALL.TXT output.

Before we just close this out with the simple comment change, do we want to try to roll a handful of improvements in in the interest of keeping the changes with this discussion for historical purposes?

For instance:

If several transmissions come in for the same station on the same band while we're waiting for our 5 minutes to elapse before sending again, I would argue the sender wants to know about the last transmission, not the first; but our present code sends the first, not the last.

This would simply be a matter of updating the frequency and timestamp in the cached spot if one is found, instead of a noop. Since some of these issue #9 changes are related to this cache, it makes sense to go ahead and do that in this context.

For ALL.TXT writing and other purposes, we already have code that calculates the start of a transmission. That timestamp should be handed to this functionality here and used when sending data to PSK reporter.

This is an issue that goes all the way up to MainWindow::processDecodeEvent, where all of the timestamp values in the detail structs are set to the current time with DriftingDateTime::currentDateTimeUtc(), not using the start of receipt time.

Example

          //ActivityDetail d = {};
          d.isLowConfidence = decodedtext.isLowConfidence();
          d.isCompound = decodedtext.isCompound();
          d.isDirected = decodedtext.isDirectedMessage();
          d.bits = decodedtext.bits();
          d.dial = freq;
          d.offset = offset;
          d.text = decodedtext.message();
          d.utcTimestamp = DriftingDateTime::currentDateTimeUtc();
          d.snr = decodedtext.snr();
          d.isBuffered = false;
          d.submode = decodedtext.submode();
          d.tdrift = m_wideGraph->shouldAutoSyncSubmode(d.submode) ? DriftingDateTime::drift()/1000.0 : decodedtext.dt();

I've made changes locally to pass the timestamp from the CallDetail through to PSKReporter::addRemoteStation so we're not generating new timestamps there, but if we want to change to use the start of receipt time as the canonical time for all events instead of setting the current time when we process the event, that's a bigger task that I think deserves it's own issue. From my first glance though it looks like we can just use decodedtext.time() for that. People with more time spent in that area of the app can fact check me on that.

After my change to pass the CallDetail timestamp, that change would flow through to PSKReporter::addRemoteStation. There may be (probably are) similar cases elsewhere.

Rob, what you want to put in it is totally up to you. This is your PR.

Thanks, this was only supposed to be a comment change, but I'll add the handful of new things that are "in the spirit of" the PR that this is a sequel to, then we can address the other items under discussion separately.

One request on PRs is that we try to stick to one topic at a time, ie do a separate PR for network stuffs or for mainwindow refactor, etc. and that it points back to an issue. Makes it easier for discussions, review, and changelog/documentation.

I have pushed a set of changes that address the requests that I believe are in scope of the original issue, new issues have been created that can serve as context for broader changes.

-Increases MIN_SEND_INTERVAL to 5 minutes
-Changes PSKReporter::addRemoteStation to use timestamp passed as an argument instead of generating a new timestamp
-Changes spot handling in PSKReporter::addRemoteStation to keep queued spots updated with the newest info, rather keeping the oldest
-PSKReporter::addRemoteStation refactored to remove multiple calls to std::time(nullptr), increase clarity, and enhance Fahrvergnügen

I ran a build with these changes with enhanced logging during a fairly active net tonight, it behaved the way it is supposed to.

Merged. Thanks!

Oh, I like that Rob, thanks for your work!

Finally, we are in a Qt project here, so I would slightly prefer to we standardized on QDateTime instead of using std::time. But that may be more a matter of taste and doesn't really matter much.

I'll register an opposing view here; the lower-level the code, the more we should see std:: in play. So here's the thing, Andreas. The C++ standard is formed by a committee of people for whom I am mostly unqualified to fetch coffee. It is painstakingly peer reviewed and, in that it's conforming with an ISO process, is extremely well-defined and trustworthy. Further, on the committee, there are many....Germans, and the degree to which they enforce order is as you would expect.

In this particular instance, I think either is fine, but as a general rule, if we're out of the UI, i'll tend to reach for the standard library first.

Thinking about it some more, I'll agree to that, @bazineta . The Qt stuff does not have an impeccable track record of being bug-free, so, if both could do the trick, it is probably better to use the std:: stuff that I, together with you, assume to be as mature as code gets in the C++ universe.