V.24 Corrects for Call Stability (#117)

gatekeep · web-flow · commit d40b83328659 · 2026-03-06T12:24:40.000-05:00
* cherrypick changes for V.24 fixes from tsawyer:dvmhost:codex/fix-v24-r05a05-prready;

* finish review and cherrypick changes for FIFO configuration from tsawyer:dvmhost:codex/fix-v24-r05a05-prready;

* based on Tims changes for writeP25Frame() instead of checking if the frame being written is greater than the available space then decrementing the m_p25Space, lets properly decrement the space and do an underflow check, because I am pretty confident that is the real problem here;

* report underflows in debug mode only;

* whoops should be LogDebugEx not LogError;

* fix signedness;
diff --git a/src/host/Host.Config.cpp b/src/host/Host.Config.cpp
@@ -5,6 +5,7 @@
 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 *
 *  Copyright (C) 2017-2026 Bryan Biedenkapp, N2PLL
+*  Copyright (C) 2026 Timothy Sawyer, WD6AWP
 *
 */
 #include "Defines.h"
@@ -511,6 +512,7 @@ bool Host::createModem()
     uint16_t dmrFifoLength = (uint16_t)modemConf["dmrFifoLength"].as<uint32_t>(DMR_TX_BUFFER_LEN);
     uint16_t p25FifoLength = (uint16_t)modemConf["p25FifoLength"].as<uint32_t>(P25_TX_BUFFER_LEN);
     uint16_t nxdnFifoLength = (uint16_t)modemConf["nxdnFifoLength"].as<uint32_t>(NXDN_TX_BUFFER_LEN);
+    uint32_t v24P25TxQueueSize = p25FifoLength;
 
     yaml::Node dfsiParams = modemConf["dfsi"];
 
@@ -645,6 +647,14 @@ bool Host::createModem()
             LogInfo("    DFSI FSC Initiator: %s", fscInitiator ? "yes" : "no");
             LogInfo("    DFSI TIA-102 Frames: %s", dfsiTIAMode ? "yes" : "no");
         }
+
+        // DFSI startup can enqueue a burst of timed frames before the modem
+        // thread starts draining; keep the TX scheduler queue larger than the
+        // raw modem FIFO to avoid clipping first-call onset.
+        uint32_t minV24TxQueueSize = m_p25QueueSizeBytes + p25FifoLength;
+        if (v24P25TxQueueSize < minV24TxQueueSize) {
+            v24P25TxQueueSize = minV24TxQueueSize;
+        }
     }
 
     if (g_remoteModemMode) {
@@ -708,6 +718,8 @@ bool Host::createModem()
         LogInfo("    NXDN Queue Size: %u (%u bytes)", nxdnQueueSize, m_nxdnQueueSizeBytes);
         LogInfo("    DMR FIFO Size: %u bytes", dmrFifoLength);
         LogInfo("    P25 FIFO Size: %u bytes", p25FifoLength);
+        if (m_isModemDFSI)
+            LogInfo("    P25 DFSI TX Queue Size: %u bytes", v24P25TxQueueSize);
         LogInfo("    NXDN FIFO Size: %u bytes", nxdnFifoLength);
 
         if (ignoreModemConfigArea) {
@@ -728,7 +740,7 @@ bool Host::createModem()
     }
 
     if (m_isModemDFSI) {
-        m_modem = new ModemV24(modemPort, m_duplex, m_p25QueueSizeBytes, p25FifoLength, rtrt, jitter,
+        m_modem = new ModemV24(modemPort, m_duplex, m_p25QueueSizeBytes, v24P25TxQueueSize, rtrt, jitter,
             dumpModemStatus, displayModemDebugMessages, trace, debug);
         ((ModemV24*)m_modem)->setCallTimeout(dfsiCallTimeout);
         ((ModemV24*)m_modem)->setTIAFormat(dfsiTIAMode);
diff --git a/src/host/Host.P25.cpp b/src/host/Host.P25.cpp
@@ -4,7 +4,8 @@
 * GPLv2 Open Source. Use is subject to license terms.
 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 *
-*  Copyright (C) 2017-2024 Bryan Biedenkapp, N2PLL
+*  Copyright (C) 2017-2026 Bryan Biedenkapp, N2PLL
+*  Copyright (C) 2026 Timothy Sawyer, WD6AWP
 *
 */
 #include "Defines.h"
@@ -255,6 +256,14 @@ void* Host::threadP25Writer(void* arg)
 
                         if (nextLen > 0U) {
                             bool ret = host->m_modem->hasP25Space(nextLen);
+
+                            // are we connected to a V.24 device? if so, always force
+                            // allow the write, V.24 modem status space can lag at call start; do not block
+                            // initial Net->RF voice frames on stale p25Space accounting
+                            if (host->m_modem->isV24Connected()) {
+                                ret = true;
+                            }
+
                             if (ret) {
                                 bool imm = false;
                                 uint32_t len = host->m_p25->getFrame(data, &imm);
@@ -267,7 +276,7 @@ void* Host::threadP25Writer(void* arg)
 
                                     // if the state is P25; write P25 frame data
                                     if (host->m_state == STATE_P25) {
-                                        host->m_modem->writeP25Frame(data, len);
+                                        host->m_modem->writeP25Frame(data, len, imm);
 
                                         afterWriteCallback();
 
diff --git a/src/host/modem/Modem.cpp b/src/host/modem/Modem.cpp
@@ -5,8 +5,9 @@
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  *  Copyright (C) 2011-2021 Jonathan Naylor, G4KLX
- *  Copyright (C) 2017-2024 Bryan Biedenkapp, N2PLL
+ *  Copyright (C) 2017-2026 Bryan Biedenkapp, N2PLL
  *  Copyright (C) 2021 Nat Moore
+ *  Copyright (C) 2026 Timothy Sawyer, WD6AWP
  *
  */
 #include "Defines.h"
@@ -125,7 +126,7 @@ Modem::Modem(port::IModemPort* port, bool duplex, bool rxInvert, bool txInvert,
     m_nxdnFifoLength(NXDN_TX_BUFFER_LEN),
     m_adcOverFlowCount(0U),
     m_dacOverFlowCount(0U),
-    m_v24Connected(true),
+    m_v24Connected(false),
     m_modemState(STATE_IDLE),
     m_buffer(nullptr),
     m_length(0U),
@@ -754,7 +755,7 @@ void Modem::clock(uint32_t ms)
             // flag indicating if free space is being reported in 16-byte blocks instead of LDUs
             bool spaceInBlocks = (m_buffer[3U] & 0x80U) == 0x80U;
 
-            m_v24Connected = true;
+            m_v24Connected = false;
             m_modemState = (DVM_STATE)m_buffer[4U];
 
             m_tx = (m_buffer[5U] & 0x01U) == 0x01U;
@@ -1411,6 +1412,11 @@ bool Modem::writeDMRFrame1(const uint8_t* data, uint32_t length, bool imm)
             }
 
             m_dmrSpace1 -= length;
+            if ((int32_t)m_dmrSpace1 < 0) {
+                if (m_debug)
+                    LogDebugEx(LOG_MODEM, "Modem::writeDMRFrame1()", "dmrSpace1 underflow, space = %u, length = %u", m_dmrSpace1, length);
+                m_dmrSpace1 = 0U;
+            }
         }
         else {
             return false;
@@ -1465,6 +1471,11 @@ bool Modem::writeDMRFrame2(const uint8_t* data, uint32_t length, bool imm)
             }
 
             m_dmrSpace2 -= length;
+            if ((int32_t)m_dmrSpace2 < 0) {
+                if (m_debug)
+                    LogDebugEx(LOG_MODEM, "Modem::writeDMRFrame2()", "dmrSpace2 underflow, space = %u, length = %u", m_dmrSpace2, length);
+                m_dmrSpace2 = 0U;
+            }
         }
         else {
             return false;
@@ -1517,7 +1528,9 @@ bool Modem::writeP25Frame(const uint8_t* data, uint32_t length, bool imm)
         uint32_t len = length + 2U;
 
         // write or buffer P25 data to air interface
-        if (m_p25Space >= length) {
+        // for V.24/DFSI, the modem status space value may lag call startup; do
+        // not block initial Net->RF frames solely on stale p25Space
+        if (m_p25Space >= length || isV24Connected()) {
             if (m_debug)
                 LogDebugEx(LOG_MODEM, "Modem::writeP25Frame()", "immediate write (len %u)", length);
             if (m_trace)
@@ -1530,6 +1543,11 @@ bool Modem::writeP25Frame(const uint8_t* data, uint32_t length, bool imm)
             }
 
             m_p25Space -= length;
+            if ((int32_t)m_p25Space < 0) {
+                if (m_debug)
+                    LogDebugEx(LOG_MODEM, "Modem::writeP25Frame()", "p25Space underflow, space = %u, length = %u", m_p25Space, length);
+                m_p25Space = 0U;
+            }
         }
         else {
             return false;
@@ -1584,6 +1602,11 @@ bool Modem::writeNXDNFrame(const uint8_t* data, uint32_t length, bool imm)
             }
 
             m_nxdnSpace -= length;
+            if ((int32_t)m_nxdnSpace < 0) {
+                if (m_debug)
+                    LogDebugEx(LOG_MODEM, "Modem::writeNXDNFrame()", "nxdnSpace underflow, space = %u, length = %u", m_nxdnSpace, length);
+                m_nxdnSpace = 0U;
+            }
         }
         else {
             return false;
diff --git a/src/host/modem/ModemV24.cpp b/src/host/modem/ModemV24.cpp
@@ -4,8 +4,9 @@
  * GPLv2 Open Source. Use is subject to license terms.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
- *  Copyright (C) 2024-2025 Bryan Biedenkapp, N2PLL
+ *  Copyright (C) 2024-2026 Bryan Biedenkapp, N2PLL
  *  Copyright (C) 2024 Patrick McDonnell, W3AXL
+ *  Copyright (C) 2026 Timothy Sawyer, WD6AWP
  *
  */
 #include "Defines.h"
@@ -394,6 +395,14 @@ void ModemV24::clock(uint32_t ms)
     int len = 0;
 
     // write anything waiting to the serial port
+
+    /*
+    ** bryanb: Tim's fork changes the order of this, however, this is a terrible idea becuase it will ultimately break
+    **  the intended purpose of this code which is to allow prioritized frames, reasonably speaking this code should
+    **  not cause problems for timing -- but we should keep an eye on this (if its problematic for some reason,
+    **  we can consider perhaps adding a flag that disables the immediate queue, forcing all packets in to the normal
+    **  queue)
+    */
     if (!m_txImmP25Queue.isEmpty())
         len = writeSerial(&m_txImmP25Queue);
     else
@@ -477,6 +486,8 @@ int ModemV24::writeSerial(RingBuffer<uint8_t>* queue)
      *  |   Length    | Tag  |               int64_t timestamp in ms                 |   data     |
      */
 
+    std::lock_guard<std::mutex> lock(m_txP25QueueLock);
+
     // check empty
     if (queue->isEmpty())
         return 0U;
@@ -486,24 +497,19 @@ int ModemV24::writeSerial(RingBuffer<uint8_t>* queue)
     ::memset(length, 0x00U, 2U);
     queue->peek(length, 2U);
 
-    // convert length byets to int
+    // convert length bytes to int
     uint16_t len = 0U;
     len = (length[0U] << 8) + length[1U];
 
-    // this ensures we never get in a situation where we have length & type bytes stuck in the queue by themselves
+    // this ensures we never get in a situation where we have length bytes stuck in the queue by themselves
     if (queue->dataSize() == 2U && len > queue->dataSize()) {
         queue->get(length, 2U); // ensure we pop bytes off
         return 0U;
     }
 
-    // check available modem space
-    if (m_p25Space < len)
-        return 0U;
-
-    std::lock_guard<std::mutex> lock(m_txP25QueueLock);
-
     // get current timestamp
     int64_t now = std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::system_clock::now().time_since_epoch()).count();
+    int64_t ts = 0L;
 
     // peek the timestamp to see if we should wait
     if (queue->dataSize() >= 11U) {
@@ -512,7 +518,6 @@ int ModemV24::writeSerial(RingBuffer<uint8_t>* queue)
         queue->peek(lengthTagTs, 11U);
 
         // get the timestamp
-        int64_t ts;
         assert(sizeof ts == 8);
         ::memcpy(&ts, lengthTagTs + 3U, 8U);
 
@@ -524,23 +529,34 @@ int ModemV24::writeSerial(RingBuffer<uint8_t>* queue)
 
     // check if we have enough data to get everything - len + 2U (length bytes) + 1U (tag) + 8U (timestamp)
     if (queue->dataSize() >= len + 11U) {
-        // Get the length, tag and timestamp
-        uint8_t lengthTagTs[11U];
-        queue->get(lengthTagTs, 11U);
-        
-        // Get the actual data
-        DECLARE_UINT8_ARRAY(buffer, len);
-        queue->get(buffer, len);
-        
-        // Sanity check on data tag
+        // peek the full entry so we can keep it queued if a write fails
+        DECLARE_UINT8_ARRAY(entry, len + 11U);
+        queue->peek(entry, len + 11U);
+
+        // get the length, tag, timestamp and payload pointers from peeked data
+        uint8_t* lengthTagTs = entry;
+        uint8_t* buffer = entry + 11U;
+
+        // sanity check on data tag
         uint8_t tag = lengthTagTs[2U];
         if (tag != TAG_DATA) {
             LogError(LOG_MODEM, "Got unexpected data tag from TX P25 ringbuffer! %02X", tag);
+
+            // drop malformed entry so we can recover queue alignment
+            DECLARE_UINT8_ARRAY(discard, len + 11U);
+            queue->get(discard, len + 11U);
             return 0U;
         }
 
-        // we already checked the timestamp above, so we just get the data and write it
-        return m_port->write(buffer, len);
+        // we already checked the timestamp above, so we just write it
+        int ret = m_port->write(buffer, len);
+        if (ret > 0) {
+            // only remove an entry once it was written successfully
+            DECLARE_UINT8_ARRAY(discard, len + 11U);
+            queue->get(discard, len + 11U);
+        }
+
+        return ret;
     }
 
     return 0U;
@@ -2363,12 +2379,19 @@ bool ModemV24::queueP25Frame(uint8_t* data, uint16_t len, SERIAL_TX_TYPE msgType
     std::lock_guard<std::mutex> lock(m_txP25QueueLock);
 
     // check available ringbuffer space
+    // keep one byte of headroom to avoid the ring buffer "full == empty"
+    // ambiguity (iPtr == oPtr) when a write would consume exactly all free space
+    uint32_t needed = len + 11U;
     if (imm) {
-        if (m_txImmP25Queue.freeSpace() < (len + 11U))
+        uint32_t free = m_txImmP25Queue.freeSpace();
+        if (free <= needed) {
             return false;
+        }
     } else {
-        if (m_txP25Queue.freeSpace() < (len + 11U))
+        uint32_t free = m_txP25Queue.freeSpace();
+        if (free <= needed) {
             return false;
+        }
     }
 
     // convert 16-bit length to 2 bytes
@@ -2429,6 +2452,10 @@ void ModemV24::startOfStreamV24(const p25::lc::LC& control)
 {
     m_txCallInProgress = true;
 
+    // start each Net->RF stream with a fresh scheduler epoch so a new call
+    // doesn't inherit a future-biased timestamp from the previous call
+    m_lastP25Tx = 0U;
+
     MotStartOfStream start = MotStartOfStream();
     start.setOpcode(m_rtrt ? MotStartStreamOpcode::TRANSMIT : MotStartStreamOpcode::RECEIVE);
     start.setParam1(DSFI_MOT_ICW_PARM_PAYLOAD);
@@ -2546,6 +2573,10 @@ void ModemV24::startOfStreamTIA(const p25::lc::LC& control)
     m_txCallInProgress = true;
     m_superFrameCnt = 1U;
 
+    // start each Net->RF stream with a fresh scheduler epoch so a new call
+    // doesn't inherit a future-biased timestamp from the previous call
+    m_lastP25Tx = 0U;
+
     p25::lc::LC lc = p25::lc::LC(control);
     
     uint16_t length = 0U;
@@ -2806,7 +2837,8 @@ void ModemV24::convertFromAirV24(uint8_t* data, uint32_t length, bool imm)
         break;
 
         case DUID::TDU:
-            endOfStreamV24(); // this may incorrectly sent STOP ICW's with the VOICE payload, but it's better than nothing for now
+            if (m_txCallInProgress)
+                endOfStreamV24();
             break;
 
         case DUID::TDULC:
