p25: fix extended-address PDU CRC-32 scope and outbound status bits (#125)

src/common/p25/data/Assembler.cpp

@@ -278,26 +278,34 @@ bool Assembler::disassemble(const uint8_t* pduBlock, uint32_t blockLength, bool
             LogDebugEx(LOG_P25, "Assembler::disassemble()", "packetLength = %u, secondHeaderOffset = %u, padLength = %u, pduLength = %u", packetLength, secondHeaderOffset, padLength, dataHeader.getPDULength());
 #endif
             if (dataHeader.getBlocksToFollow() > 0U) {
-                if (padLength > 0U) {
+                // BUGFIX (verified against a real captured RF frame via brute-force range
-                    // move CRC-32 properly before padding to check CRC of user data
+                // search against the actual received CRC bytes -- see tools/verify_crc/):
-                    uint8_t crcBytes[P25_MAX_PDU_BLOCKS * P25_PDU_CONFIRMED_LENGTH_BYTES + 2U];
+                // CRC-32 covers the ENTIRE PDU content stream -- second header (if any) +
-                    ::memset(crcBytes, 0x00U, packetLength);
+                // payload + padding -- everything except its own trailing 4 bytes. It is
-                    ::memcpy(crcBytes, m_pduUserData, packetLength);
+                // NOT scoped to payload-only (my first attempted fix was wrong), and padding
-                    ::memcpy(crcBytes + packetLength, m_pduUserData + packetLength + padLength, 4U);
+                // is NOT excluded from it either (the original stock code was also wrong, in
-
+                // the other direction). Because the CRC bytes already sit immediately after
-                    bool crcRet = edac::CRC::checkCRC32(crcBytes, packetLength + 4U);
+                // the padding in m_pduUserData, no buffer rearrangement is needed at all --
+                // the old padded/non-padded branching collapses into one direct check.
+                uint32_t crcCoveredLength = packetLength + padLength;
+
+                bool crcRet = edac::CRC::checkCRC32(m_pduUserData, crcCoveredLength + 4U);
                 if (!crcRet) {
-                        LogWarning(LOG_P25, P25_PDU_STR ", failed CRC-32 check, blocks %u, len %u", dataHeader.getBlocksToFollow(), m_pduUserDataLength);
+                    // DIAGNOSTIC: log the received CRC bytes next to what we'd compute over
-                        m_packetCRCFailed = true;
+                    // this range, to distinguish a real algorithm/scope mismatch from RF
-                    }
+                    // corruption on any future failure.
-                } else {
+                    uint8_t recompute[P25_MAX_PDU_BLOCKS * P25_PDU_CONFIRMED_LENGTH_BYTES + 2U];
-                    bool crcRet = edac::CRC::checkCRC32(m_pduUserData, packetLength + 4U);
+                    ::memcpy(recompute, m_pduUserData, crcCoveredLength + 4U);
-                    if (!crcRet) {
+                    edac::CRC::addCRC32(recompute, crcCoveredLength + 4U);
-                        LogWarning(LOG_P25, P25_PDU_STR ", failed CRC-32 check, blocks %u, len %u", dataHeader.getBlocksToFollow(), m_pduUserDataLength);
+
+                    LogWarning(LOG_P25, P25_PDU_STR ", failed CRC-32 check, blocks %u, len %u, "
+                        "received = $%02X%02X%02X%02X, computed = $%02X%02X%02X%02X",
+                        dataHeader.getBlocksToFollow(), crcCoveredLength,
+                        m_pduUserData[crcCoveredLength], m_pduUserData[crcCoveredLength + 1U], m_pduUserData[crcCoveredLength + 2U], m_pduUserData[crcCoveredLength + 3U],
+                        recompute[crcCoveredLength], recompute[crcCoveredLength + 1U], recompute[crcCoveredLength + 2U], recompute[crcCoveredLength + 3U]);
                     m_packetCRCFailed = true;
                 }
             }
-            }
 
             // reorganize PDU buffer for second header offsetting
             if (secondHeaderOffset > 0U) {
@@ -426,16 +434,17 @@ UInt8Array Assembler::assemble(data::DataHeader& dataHeader, bool extendedAddres
         LogDebugEx(LOG_P25, "Assembler::assemble()", "packetLength = %u, secondHeaderOffset = %u, padLength = %u, pduLength = %u", packetLength, secondHeaderOffset, padLength, pduLength);
 #endif
         if (dataHeader.getFormat() != PDUFormatType::AMBT) {
-            ::memcpy(packetData + secondHeaderOffset, pduUserData, packetLength);
+            // BUGFIX (verified against a real captured RF frame -- see disassemble() above
-            edac::CRC::addCRC32(packetData, packetLength + 4U);
+            // for the brute-force verification detail): pduUserData is netPayloadLength bytes
-
+            // (packetLength - secondHeaderOffset), not the gross packetLength, so copy only
-            if (padLength > 0U) {
+            // that much into its correct position after the second header. CRC-32 then covers
-                // move the CRC-32 to the end of the packet data after the padding
+            // the ENTIRE stream -- second header + payload + padding (already zero from
-                uint8_t crcBytes[4U];
+            // DECLARE_UINT8_ARRAY's zero-init) -- everything except its own 4 bytes.
-                ::memcpy(crcBytes, packetData + packetLength, 4U);
+            // addCRC32 writes those bytes directly to their final position, so the old
-                ::memset(packetData + packetLength, 0x00U, 4U);
+            // separate "move the CRC after padding" step is no longer needed at all.
-                ::memcpy(packetData + (packetLength + padLength), crcBytes, 4U);
+            uint32_t netPayloadLength = packetLength - secondHeaderOffset;
-            }
+            ::memcpy(packetData + secondHeaderOffset, pduUserData, netPayloadLength);
+            edac::CRC::addCRC32(packetData, packetLength + padLength + 4U);
         } else {
             // our AMBTs have a pre-calculated CRC-32 -- we don't need to do it ourselves
             ::memcpy(packetData + secondHeaderOffset, pduUserData, pduLength);

src/host/p25/packet/Data.cpp

@@ -128,6 +128,8 @@ bool Data::process(uint8_t* data, uint32_t len)
 
             // did we receive a response header?
             if (m_rfAssembler->dataHeader.getFormat() == PDUFormatType::RSP) {
+                // clear inbound before emitting outbound OSP bursts (ACK/NACK, retries) below
+                m_inbound = false;
                 LogInfoEx(LOG_RF, P25_PDU_STR ", ISP, response, fmt = $%02X, rspClass = $%02X, rspType = $%02X, rspStatus = $%02X, llId = %u, srcLlId = %u",
                         m_rfAssembler->dataHeader.getFormat(), m_rfAssembler->dataHeader.getResponseClass(), m_rfAssembler->dataHeader.getResponseType(), m_rfAssembler->dataHeader.getResponseStatus(),
                         m_rfAssembler->dataHeader.getLLId(), m_rfAssembler->dataHeader.getSrcLLId());
@@ -188,7 +190,7 @@ bool Data::process(uint8_t* data, uint32_t len)
                                 LogInfoEx(LOG_RF, P25_PDU_STR ", ISP, response, OSP ACK RETRY, llId = %u, exceeded retries, undeliverable",
                                     m_rfAssembler->dataHeader.getLLId());
 
-                                writeRF_PDU_Ack_Response(PDUAckClass::NACK, PDUAckType::NACK_UNDELIVERABLE, m_rfAssembler->dataHeader.getNs(), m_rfAssembler->dataHeader.getLLId(), m_rfAssembler->dataHeader.getSrcLLId());
+                                writeRF_PDU_Ack_Response(PDUAckClass::NACK, PDUAckType::NACK_UNDELIVERABLE, m_rfAssembler->dataHeader.getNs(), m_rfAssembler->dataHeader.getLLId(), (m_rfAssembler->dataHeader.getSrcLLId() > 0U), m_rfAssembler->dataHeader.getSrcLLId());
                             }
                         }
                     }
@@ -200,7 +202,7 @@ bool Data::process(uint8_t* data, uint32_t len)
                 // only repeat the PDU locally if the packet isn't for the FNE
                 if (m_repeatPDU && m_rfAssembler->dataHeader.getLLId() != WUID_FNE) {
                     writeRF_PDU_Ack_Response(m_rfAssembler->dataHeader.getResponseClass(), m_rfAssembler->dataHeader.getResponseType(), m_rfAssembler->dataHeader.getResponseStatus(),
-                        m_rfAssembler->dataHeader.getLLId(), m_rfAssembler->dataHeader.getSrcLLId());
+                        m_rfAssembler->dataHeader.getLLId(), (m_rfAssembler->dataHeader.getSrcLLId() > 0U), m_rfAssembler->dataHeader.getSrcLLId());
                 }
 
                 m_rfPDUCount = 0U;
@@ -253,6 +255,8 @@ bool Data::process(uint8_t* data, uint32_t len)
             }
 
             if (m_rfAssembler->getComplete()) {
+                // clear inbound before the SAP switch emits outbound OSP bursts (local repeat, reg)
+                m_inbound = false;
                 m_rfPduUserDataLength = m_rfAssembler->getUserDataLength();
                 m_rfAssembler->getUserData(m_rfPduUserData);