mirror of
https://github.com/MarekZegare4/MeshCore-Solo.git
synced 2026-09-14 23:26:38 +00:00
refactor: split MeshTables::hasSeen into pure query + markSeen
hasSeen() was simultaneously a predicate and a mutator — it inserted the packet hash on every miss, making five call sites that only wanted to mark a packet as sent call it with the return value discarded. Split into: - wasSeen() — pure predicate, no side effects - markSeen() — explicit insert All query sites now call markSeen() immediately after wasSeen() returns false, preserving identical runtime behaviour. The five mark-only send sites (sendFlood, sendDirect, sendZeroHop x2) now call markSeen directly. Also fixes three bridge sites (BridgeBase, ESPNowBridge, RS232Bridge) that had the same query+implicit-insert pattern. Tests: add test/test_mesh_tables/ covering wasSeen purity, markSeen, dup stats, and clear. Update SHA256 mock to produce deterministic output (previously finalize() was a no-op). Add Packet.cpp to native build filter.
This commit is contained in:
+27
-16
@@ -55,7 +55,8 @@ DispatcherAction Mesh::onRecvPacket(Packet* pkt) {
|
||||
uint16_t offset = (uint16_t)pkt->path_len << path_sz;
|
||||
if (offset >= len) { // TRACE has reached end of given path
|
||||
onTraceRecv(pkt, trace_tag, auth_code, flags, pkt->path, &pkt->payload[i], len);
|
||||
} else if (self_id.isHashMatch(&pkt->payload[i + offset], 1 << path_sz) && allowPacketForward(pkt) && !_tables->hasSeen(pkt)) {
|
||||
} else if (self_id.isHashMatch(&pkt->payload[i + offset], 1 << path_sz) && allowPacketForward(pkt) && !_tables->wasSeen(pkt)) {
|
||||
_tables->markSeen(pkt);
|
||||
// append SNR (Not hash!)
|
||||
pkt->path[pkt->path_len++] = (int8_t) (pkt->getSNR()*4);
|
||||
|
||||
@@ -89,14 +90,16 @@ DispatcherAction Mesh::onRecvPacket(Packet* pkt) {
|
||||
if (pkt->getPayloadType() == PAYLOAD_TYPE_MULTIPART) {
|
||||
return forwardMultipartDirect(pkt);
|
||||
} else if (pkt->getPayloadType() == PAYLOAD_TYPE_ACK) {
|
||||
if (!_tables->hasSeen(pkt)) { // don't retransmit!
|
||||
if (!_tables->wasSeen(pkt)) { // don't retransmit!
|
||||
_tables->markSeen(pkt);
|
||||
removeSelfFromPath(pkt);
|
||||
routeDirectRecvAcks(pkt, 0);
|
||||
}
|
||||
return ACTION_RELEASE;
|
||||
}
|
||||
|
||||
if (!_tables->hasSeen(pkt)) {
|
||||
if (!_tables->wasSeen(pkt)) {
|
||||
_tables->markSeen(pkt);
|
||||
removeSelfFromPath(pkt);
|
||||
|
||||
uint32_t d = getDirectRetransmitDelay(pkt);
|
||||
@@ -117,7 +120,8 @@ DispatcherAction Mesh::onRecvPacket(Packet* pkt) {
|
||||
memcpy(&ack_crc, &pkt->payload[i], 4); i += 4;
|
||||
if (i > pkt->payload_len) {
|
||||
MESH_DEBUG_PRINTLN("%s Mesh::onRecvPacket(): incomplete ACK packet", getLogDateTime());
|
||||
} else if (!_tables->hasSeen(pkt)) {
|
||||
} else if (!_tables->wasSeen(pkt)) {
|
||||
_tables->markSeen(pkt);
|
||||
onAckRecv(pkt, ack_crc);
|
||||
action = routeRecvPacket(pkt);
|
||||
}
|
||||
@@ -134,7 +138,8 @@ DispatcherAction Mesh::onRecvPacket(Packet* pkt) {
|
||||
uint8_t* macAndData = &pkt->payload[i]; // MAC + encrypted data
|
||||
if (i + CIPHER_MAC_SIZE >= pkt->payload_len) {
|
||||
MESH_DEBUG_PRINTLN("%s Mesh::onRecvPacket(): incomplete data packet", getLogDateTime());
|
||||
} else if (!_tables->hasSeen(pkt)) {
|
||||
} else if (!_tables->wasSeen(pkt)) {
|
||||
_tables->markSeen(pkt);
|
||||
// NOTE: this is a 'first packet wins' impl. When receiving from multiple paths, the first to arrive wins.
|
||||
// For flood mode, the path may not be the 'best' in terms of hops.
|
||||
// FUTURE: could send back multiple paths, using createPathReturn(), and let sender choose which to use(?)
|
||||
@@ -197,7 +202,8 @@ DispatcherAction Mesh::onRecvPacket(Packet* pkt) {
|
||||
uint8_t* macAndData = &pkt->payload[i]; // MAC + encrypted data
|
||||
if (i + 2 >= pkt->payload_len) {
|
||||
MESH_DEBUG_PRINTLN("%s Mesh::onRecvPacket(): incomplete data packet", getLogDateTime());
|
||||
} else if (!_tables->hasSeen(pkt)) {
|
||||
} else if (!_tables->wasSeen(pkt)) {
|
||||
_tables->markSeen(pkt);
|
||||
if (self_id.isHashMatch(&dest_hash)) {
|
||||
Identity sender(sender_pub_key);
|
||||
|
||||
@@ -224,7 +230,8 @@ DispatcherAction Mesh::onRecvPacket(Packet* pkt) {
|
||||
uint8_t* macAndData = &pkt->payload[i]; // MAC + encrypted data
|
||||
if (i + 2 >= pkt->payload_len) {
|
||||
MESH_DEBUG_PRINTLN("%s Mesh::onRecvPacket(): incomplete data packet", getLogDateTime());
|
||||
} else if (!_tables->hasSeen(pkt)) {
|
||||
} else if (!_tables->wasSeen(pkt)) {
|
||||
_tables->markSeen(pkt);
|
||||
// scan channels DB, for all matching hashes of 'channel_hash' (max 4 matches supported ATM)
|
||||
GroupChannel channels[4];
|
||||
int num = searchChannelsByHash(&channel_hash, channels, 4);
|
||||
@@ -255,7 +262,8 @@ DispatcherAction Mesh::onRecvPacket(Packet* pkt) {
|
||||
MESH_DEBUG_PRINTLN("%s Mesh::onRecvPacket(): incomplete advertisement packet", getLogDateTime());
|
||||
} else if (self_id.matches(id.pub_key)) {
|
||||
MESH_DEBUG_PRINTLN("%s Mesh::onRecvPacket(): receiving SELF advert packet", getLogDateTime());
|
||||
} else if (!_tables->hasSeen(pkt)) {
|
||||
} else if (!_tables->wasSeen(pkt)) {
|
||||
_tables->markSeen(pkt);
|
||||
uint8_t* app_data = &pkt->payload[i];
|
||||
int app_data_len = pkt->payload_len - i;
|
||||
if (app_data_len > MAX_ADVERT_DATA_SIZE) { app_data_len = MAX_ADVERT_DATA_SIZE; }
|
||||
@@ -282,7 +290,8 @@ DispatcherAction Mesh::onRecvPacket(Packet* pkt) {
|
||||
break;
|
||||
}
|
||||
case PAYLOAD_TYPE_RAW_CUSTOM: {
|
||||
if (pkt->isRouteDirect() && !_tables->hasSeen(pkt)) {
|
||||
if (pkt->isRouteDirect() && !_tables->wasSeen(pkt)) {
|
||||
_tables->markSeen(pkt);
|
||||
onRawDataRecv(pkt);
|
||||
//action = routeRecvPacket(pkt); don't flood route these (yet)
|
||||
}
|
||||
@@ -300,7 +309,8 @@ DispatcherAction Mesh::onRecvPacket(Packet* pkt) {
|
||||
tmp.payload_len = pkt->payload_len - 1;
|
||||
memcpy(tmp.payload, &pkt->payload[1], tmp.payload_len);
|
||||
|
||||
if (!_tables->hasSeen(&tmp)) {
|
||||
if (!_tables->wasSeen(&tmp)) {
|
||||
_tables->markSeen(&tmp);
|
||||
uint32_t ack_crc;
|
||||
memcpy(&ack_crc, tmp.payload, 4);
|
||||
|
||||
@@ -357,7 +367,8 @@ DispatcherAction Mesh::forwardMultipartDirect(Packet* pkt) {
|
||||
tmp.payload_len = pkt->payload_len - 1;
|
||||
memcpy(tmp.payload, &pkt->payload[1], tmp.payload_len);
|
||||
|
||||
if (!_tables->hasSeen(&tmp)) { // don't retransmit!
|
||||
if (!_tables->wasSeen(&tmp)) { // don't retransmit!
|
||||
_tables->markSeen(&tmp);
|
||||
removeSelfFromPath(&tmp);
|
||||
routeDirectRecvAcks(&tmp, ((uint32_t)remaining + 1) * 300); // expect multipart ACKs 300ms apart (x2)
|
||||
}
|
||||
@@ -637,7 +648,7 @@ void Mesh::sendFlood(Packet* packet, uint32_t delay_millis, uint8_t path_hash_si
|
||||
packet->header |= ROUTE_TYPE_FLOOD;
|
||||
packet->setPathHashSizeAndCount(path_hash_size, 0);
|
||||
|
||||
_tables->hasSeen(packet); // mark this packet as already sent in case it is rebroadcast back to us
|
||||
_tables->markSeen(packet); // mark this packet as already sent in case it is rebroadcast back to us
|
||||
|
||||
uint8_t pri;
|
||||
if (packet->getPayloadType() == PAYLOAD_TYPE_PATH) {
|
||||
@@ -666,7 +677,7 @@ void Mesh::sendFlood(Packet* packet, uint16_t* transport_codes, uint32_t delay_m
|
||||
packet->transport_codes[1] = transport_codes[1];
|
||||
packet->setPathHashSizeAndCount(path_hash_size, 0);
|
||||
|
||||
_tables->hasSeen(packet); // mark this packet as already sent in case it is rebroadcast back to us
|
||||
_tables->markSeen(packet); // mark this packet as already sent in case it is rebroadcast back to us
|
||||
|
||||
uint8_t pri;
|
||||
if (packet->getPayloadType() == PAYLOAD_TYPE_PATH) {
|
||||
@@ -699,7 +710,7 @@ void Mesh::sendDirect(Packet* packet, const uint8_t* path, uint8_t path_len, uin
|
||||
pri = 0;
|
||||
}
|
||||
}
|
||||
_tables->hasSeen(packet); // mark this packet as already sent in case it is rebroadcast back to us
|
||||
_tables->markSeen(packet); // mark this packet as already sent in case it is rebroadcast back to us
|
||||
sendPacket(packet, pri, delay_millis);
|
||||
}
|
||||
|
||||
@@ -709,7 +720,7 @@ void Mesh::sendZeroHop(Packet* packet, uint32_t delay_millis) {
|
||||
|
||||
packet->path_len = 0; // path_len of zero means Zero Hop
|
||||
|
||||
_tables->hasSeen(packet); // mark this packet as already sent in case it is rebroadcast back to us
|
||||
_tables->markSeen(packet); // mark this packet as already sent in case it is rebroadcast back to us
|
||||
|
||||
sendPacket(packet, 0, delay_millis);
|
||||
}
|
||||
@@ -722,7 +733,7 @@ void Mesh::sendZeroHop(Packet* packet, uint16_t* transport_codes, uint32_t delay
|
||||
|
||||
packet->path_len = 0; // path_len of zero means Zero Hop
|
||||
|
||||
_tables->hasSeen(packet); // mark this packet as already sent in case it is rebroadcast back to us
|
||||
_tables->markSeen(packet); // mark this packet as already sent in case it is rebroadcast back to us
|
||||
|
||||
sendPacket(packet, 0, delay_millis);
|
||||
}
|
||||
|
||||
+3
-2
@@ -15,8 +15,9 @@ public:
|
||||
*/
|
||||
class MeshTables {
|
||||
public:
|
||||
virtual bool hasSeen(const Packet* packet) = 0;
|
||||
virtual void clear(const Packet* packet) = 0; // remove this packet hash from table
|
||||
virtual bool wasSeen(const Packet* packet) = 0;
|
||||
virtual void markSeen(const Packet* packet) = 0;
|
||||
virtual void clear(const Packet* packet) = 0; // remove this packet hash from table
|
||||
};
|
||||
|
||||
/**
|
||||
|
||||
@@ -31,27 +31,31 @@ public:
|
||||
}
|
||||
#endif
|
||||
|
||||
bool hasSeen(const mesh::Packet* packet) override {
|
||||
bool wasSeen(const mesh::Packet* packet) override {
|
||||
uint8_t hash[MAX_HASH_SIZE];
|
||||
packet->calculatePacketHash(hash);
|
||||
|
||||
const uint8_t* sp = _hashes;
|
||||
for (int i = 0; i < MAX_PACKET_HASHES; i++, sp += MAX_HASH_SIZE) {
|
||||
if (memcmp(hash, sp, MAX_HASH_SIZE) == 0) {
|
||||
if (memcmp(hash, sp, MAX_HASH_SIZE) == 0) {
|
||||
if (packet->isRouteDirect()) {
|
||||
_direct_dups++; // keep some stats
|
||||
_direct_dups++;
|
||||
} else {
|
||||
_flood_dups++;
|
||||
}
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
memcpy(&_hashes[_next_idx*MAX_HASH_SIZE], hash, MAX_HASH_SIZE);
|
||||
_next_idx = (_next_idx + 1) % MAX_PACKET_HASHES; // cyclic table
|
||||
return false;
|
||||
}
|
||||
|
||||
void markSeen(const mesh::Packet* packet) override {
|
||||
uint8_t hash[MAX_HASH_SIZE];
|
||||
packet->calculatePacketHash(hash);
|
||||
memcpy(&_hashes[_next_idx * MAX_HASH_SIZE], hash, MAX_HASH_SIZE);
|
||||
_next_idx = (_next_idx + 1) % MAX_PACKET_HASHES;
|
||||
}
|
||||
|
||||
void clear(const mesh::Packet* packet) override {
|
||||
uint8_t hash[MAX_HASH_SIZE];
|
||||
packet->calculatePacketHash(hash);
|
||||
|
||||
@@ -39,7 +39,8 @@ void BridgeBase::handleReceivedPacket(mesh::Packet *packet) {
|
||||
return;
|
||||
}
|
||||
|
||||
if (!_seen_packets.hasSeen(packet)) {
|
||||
if (!_seen_packets.wasSeen(packet)) {
|
||||
_seen_packets.markSeen(packet);
|
||||
// bridge_delay provides a buffer to prevent immediate processing conflicts in the mesh network.
|
||||
_mgr->queueInbound(packet, millis() + _prefs->bridge_delay);
|
||||
} else {
|
||||
|
||||
@@ -110,7 +110,7 @@ protected:
|
||||
* @brief Common packet handling for received packets
|
||||
*
|
||||
* Implements the standard pattern used by all bridges:
|
||||
* - Check if packet was seen before using _seen_packets.hasSeen()
|
||||
* - Check if packet was seen before using _seen_packets.wasSeen()
|
||||
* - Queue packet for mesh processing if not seen before
|
||||
* - Free packet if already seen to prevent duplicates
|
||||
*
|
||||
|
||||
@@ -167,7 +167,8 @@ void ESPNowBridge::sendPacket(mesh::Packet *packet) {
|
||||
return;
|
||||
}
|
||||
|
||||
if (!_seen_packets.hasSeen(packet)) {
|
||||
if (!_seen_packets.wasSeen(packet)) {
|
||||
_seen_packets.markSeen(packet);
|
||||
// Create a temporary buffer just for size calculation and reuse for actual writing
|
||||
uint8_t sizingBuffer[MAX_PAYLOAD_SIZE];
|
||||
uint16_t meshPacketLen = packet->writeTo(sizingBuffer);
|
||||
|
||||
@@ -115,7 +115,8 @@ void RS232Bridge::sendPacket(mesh::Packet *packet) {
|
||||
return;
|
||||
}
|
||||
|
||||
if (!_seen_packets.hasSeen(packet)) {
|
||||
if (!_seen_packets.wasSeen(packet)) {
|
||||
_seen_packets.markSeen(packet);
|
||||
|
||||
uint8_t buffer[MAX_SERIAL_PACKET_SIZE];
|
||||
uint16_t len = packet->writeTo(buffer + 4);
|
||||
|
||||
Reference in New Issue
Block a user