From 2f6034b06772f3292a951d1c2b18ecd9593d8241 Mon Sep 17 00:00:00 2001 From: Kevin Hester Date: Thu, 4 Mar 2021 22:09:02 +0800 Subject: [PATCH 1/4] update todos --- docs/software/TODO.md | 31 +++++++++++++++++-------------- proto | 2 +- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/docs/software/TODO.md b/docs/software/TODO.md index 5547110d..b96deb3a 100644 --- a/docs/software/TODO.md +++ b/docs/software/TODO.md @@ -21,32 +21,35 @@ You probably don't care about this section - skip to the next one. * DONE implement 'get channels' Admin plugin operation * DONE use get-channels from python * DONE use get channels & get settings from android -* use set-channel from python +* DONE use set-channel from python * DONE make settings changes from python work * DONE pthon should stop fetching channels once we've reached our first empty channel definition (hasSettings == true) * DONE add check for old devices with new API library * DONE release python api * DONE release protobufs * DONE release to developers -* fix 1.1.50 android debug panel display -* warn in android app about unset regions -* use set-channel from android -* DONE add gui in android app for setting region -* stress test channel download from python, sometimes it seems like we don't get all replies -* investigate @mc-hamster report of heap corruption -* DONE use set-user from android +* DONE fix setch-fast in python tool * combine acks and responses in a single message if possible (do routing plugin LAST and drop ACK if someone else has already replied) * don't send packets we received from the phone BACK TOWARDS THE PHONE (possibly use fromnode 0 for packets the phone sends?) -* use portuino TCP connection to debug with python API +* fix 1.1.50 android debug panel display +* DONE warn in android app about unset regions +* DONE use set-channel from android +* DONE add gui in android app for setting region +* stress test channel download from python, sometimes it seems like we don't get all replies * make python tests more exhaustive -* document the relationship between want_response (indicating remote node received it) and want_ack (indicating that this message should be sent reliably - and also get acks from the first rx node and naks if it is never delivered) -* stress test multi channel +* pick default random admin key +* exclude admin channels from URL? +* make a way to share just secondary channels via URL * use single byte 'well known' channel names for the four default channel names (longslow etc), and for admin, gpio, etc... * use presence of gpio channel to enable gpio ops, same for serial etc... -* pick default random admin key -* DONE android should stop fetching channels once we've reached our first empty channel definition (hasSettings == true) -* add channel restrictions for plugins (and restrict routing plugin to the "control" channel) * restrict gpio & serial & settings operations to the admin channel (unless local to the current node) +* add channel restrictions for plugins (and restrict routing plugin to the "control" channel) +* stress test multi channel +* investigate @mc-hamster report of heap corruption +* DONE use set-user from android +* use portuino TCP connection to debug with python API +* document the relationship between want_response (indicating remote node received it) and want_ack (indicating that this message should be sent reliably - and also get acks from the first rx node and naks if it is never delivered) +* DONE android should stop fetching channels once we've reached our first empty channel definition (hasSettings == true) * DONE warn in python api if we are too new to talk to the device code * DONE make a post warning about 1.2, telling how to stay on old android & python clients. link to this from the android dialog message and python version warning. * DONE "FIXME - move the radioconfig/user/channel READ operations into SettingsMessage as well" diff --git a/proto b/proto index 270cbdb6..ac4f53ed 160000 --- a/proto +++ b/proto @@ -1 +1 @@ -Subproject commit 270cbdb6801761f054cb79f64b68b8a75cfb50f6 +Subproject commit ac4f53ed8c903a5bdf3d19727e96791d6be71022 From 950b32232f493d196b275fce40ea0a423d2d18b3 Mon Sep 17 00:00:00 2001 From: Kevin Hester Date: Fri, 5 Mar 2021 10:19:27 +0800 Subject: [PATCH 2/4] don't send messages the phone sent us back towards the phone --- docs/software/TODO.md | 6 +- src/graphics/Screen.cpp | 2 +- src/mesh/DSRRouter.cpp | 2 +- src/mesh/MeshPacketQueue.cpp | 2 +- src/mesh/MeshPlugin.cpp | 4 +- src/mesh/MeshService.cpp | 59 +++++++++++-------- src/mesh/MeshTypes.h | 8 ++- src/mesh/NodeDB.cpp | 10 +++- src/mesh/NodeDB.h | 1 + src/mesh/PacketHistory.cpp | 4 +- src/mesh/ReliableRouter.cpp | 10 ++-- src/mesh/ReliableRouter.h | 2 +- src/mesh/Router.cpp | 8 ++- src/plugins/ExternalNotificationPlugin.cpp | 2 +- src/plugins/NodeInfoPlugin.cpp | 2 +- src/plugins/PositionPlugin.cpp | 2 +- src/plugins/SerialPlugin.cpp | 2 +- .../esp32/EnvironmentalMeasurementPlugin.cpp | 5 +- src/plugins/esp32/RangeTestPlugin.cpp | 8 +-- src/plugins/esp32/StoreForwardPlugin.cpp | 6 +- 20 files changed, 87 insertions(+), 58 deletions(-) diff --git a/docs/software/TODO.md b/docs/software/TODO.md index b96deb3a..dfef0427 100644 --- a/docs/software/TODO.md +++ b/docs/software/TODO.md @@ -29,13 +29,14 @@ You probably don't care about this section - skip to the next one. * DONE release protobufs * DONE release to developers * DONE fix setch-fast in python tool +* age out pendingrequests in the python API +* DONE stress test channel download from python, sometimes it seems like we don't get all replies, bug was due to simultaneous android connection * combine acks and responses in a single message if possible (do routing plugin LAST and drop ACK if someone else has already replied) -* don't send packets we received from the phone BACK TOWARDS THE PHONE (possibly use fromnode 0 for packets the phone sends?) +* DONE don't send packets we received from the phone BACK TOWARDS THE PHONE (possibly use fromnode 0 for packets the phone sends?) * fix 1.1.50 android debug panel display * DONE warn in android app about unset regions * DONE use set-channel from android * DONE add gui in android app for setting region -* stress test channel download from python, sometimes it seems like we don't get all replies * make python tests more exhaustive * pick default random admin key * exclude admin channels from URL? @@ -61,6 +62,7 @@ You probably don't care about this section - skip to the next one. * confirm we are still calling the plugins for messages inbound from the phone (or generated locally) * confirm we are still multi hop routing flood broadcasts * confirm we are still doing resends on unicast reliable packets +* add history to routed packets: https://meshtastic.discourse.group/t/packet-source-tracking/2764/2 * add support for full DSR unicast delivery * DONE move acks into routing * DONE make all subpackets different versions of data diff --git a/src/graphics/Screen.cpp b/src/graphics/Screen.cpp index d04b9960..6e52abdd 100644 --- a/src/graphics/Screen.cpp +++ b/src/graphics/Screen.cpp @@ -229,7 +229,7 @@ static void drawTextMessageFrame(OLEDDisplay *display, OLEDDisplayUiState *state displayedNodeNum = 0; // Not currently showing a node pane MeshPacket &mp = devicestate.rx_text_message; - NodeInfo *node = nodeDB.getNode(mp.from); + NodeInfo *node = nodeDB.getNode(getFrom(&mp)); // DEBUG_MSG("drawing text message from 0x%x: %s\n", mp.from, // mp.decoded.variant.data.decoded.bytes); diff --git a/src/mesh/DSRRouter.cpp b/src/mesh/DSRRouter.cpp index 5fa6b160..f7d8588e 100644 --- a/src/mesh/DSRRouter.cpp +++ b/src/mesh/DSRRouter.cpp @@ -69,7 +69,7 @@ void DSRRouter::sniffReceived(const MeshPacket *p, const Routing *c) // ignore rebroadcasts. // this will also add records for any ACKs we receive for our messages if (p->to != NODENUM_BROADCAST || p->hop_limit != HOP_RELIABLE) { - addRoute(p->from, p->from, 0); // We are adjacent with zero hops + addRoute(getFrom(p), getFrom(p), 0); // We are adjacent with zero hops } if (c) diff --git a/src/mesh/MeshPacketQueue.cpp b/src/mesh/MeshPacketQueue.cpp index 56db3feb..6c964e50 100644 --- a/src/mesh/MeshPacketQueue.cpp +++ b/src/mesh/MeshPacketQueue.cpp @@ -71,7 +71,7 @@ static PacketId findId; static bool isMyPacket(MeshPacket *p) { - return p->id == findId && p->from == findFrom; + return p->id == findId && getFrom(p) == findFrom; } /** Attempt to find and remove a packet from this queue. Returns true the packet which was removed from the queue */ diff --git a/src/mesh/MeshPlugin.cpp b/src/mesh/MeshPlugin.cpp index 6fd3c72b..34aaa236 100644 --- a/src/mesh/MeshPlugin.cpp +++ b/src/mesh/MeshPlugin.cpp @@ -52,7 +52,7 @@ void MeshPlugin::callPlugins(const MeshPacket &mp) // NOTE: we send a reply *even if the (non broadcast) request was from us* which is unfortunate but necessary because currently when the phone // sends things, it sends things using the local node ID as the from address. A better solution (FIXME) would be to let phones // have their own distinct addresses and we 'route' to them like any other node. - if (mp.decoded.want_response && toUs && (mp.from != ourNodeNum || mp.to == ourNodeNum)) { + if (mp.decoded.want_response && toUs && (getFrom(&mp) != ourNodeNum || mp.to == ourNodeNum)) { pi.sendResponse(mp); DEBUG_MSG("Plugin %s sent a response\n", pi.name); } @@ -94,7 +94,7 @@ void MeshPlugin::sendResponse(const MeshPacket &req) { */ void setReplyTo(MeshPacket *p, const MeshPacket &to) { assert(p->which_payloadVariant == MeshPacket_decoded_tag); // Should already be set by now - p->to = to.from; + p->to = getFrom(&to); p->want_ack = to.want_ack; p->decoded.request_id = to.id; } diff --git a/src/mesh/MeshService.cpp b/src/mesh/MeshService.cpp index 753d942d..f21742ea 100644 --- a/src/mesh/MeshService.cpp +++ b/src/mesh/MeshService.cpp @@ -13,8 +13,8 @@ #include "RTC.h" #include "main.h" #include "mesh-pb-constants.h" -#include "plugins/PositionPlugin.h" #include "plugins/NodeInfoPlugin.h" +#include "plugins/PositionPlugin.h" #include "power.h" /* @@ -51,8 +51,6 @@ MeshService service; #include "Router.h" - - MeshService::MeshService() : toPhoneQueue(MAX_RX_TOPHONE) { // assert(MAX_RX_TOPHONE == 32); // FIXME, delete this, just checking my clever macro @@ -67,25 +65,29 @@ void MeshService::init() gpsObserver.observe(&gps->newStatus); } - int MeshService::handleFromRadio(const MeshPacket *mp) { powerFSM.trigger(EVENT_RECEIVED_PACKET); // Possibly keep the node from sleeping - printPacket("Forwarding to phone", mp); - nodeDB.updateFrom(*mp); // update our DB state based off sniffing every RX packet from the radio + if (mp->from != 0) { + printPacket("Forwarding to phone", mp); + nodeDB.updateFrom(*mp); // update our DB state based off sniffing every RX packet from the radio - fromNum++; + fromNum++; - if (toPhoneQueue.numFree() == 0) { - DEBUG_MSG("NOTE: tophone queue is full, discarding oldest\n"); - MeshPacket *d = toPhoneQueue.dequeuePtr(0); - if (d) - releaseToPool(d); + if (toPhoneQueue.numFree() == 0) { + DEBUG_MSG("NOTE: tophone queue is full, discarding oldest\n"); + MeshPacket *d = toPhoneQueue.dequeuePtr(0); + if (d) + releaseToPool(d); + } + + MeshPacket *copied = packetPool.allocCopy(*mp); + assert(toPhoneQueue.enqueue(copied, 0)); // FIXME, instead of failing for full queue, delete the oldest mssages + } + else { + DEBUG_MSG("Packet originally from phone, no need to send back that way...\n"); } - - MeshPacket *copied = packetPool.allocCopy(*mp); - assert(toPhoneQueue.enqueue(copied, 0)); // FIXME, instead of failing for full queue, delete the oldest mssages return 0; } @@ -128,8 +130,12 @@ void MeshService::reloadOwner() */ void MeshService::handleToRadio(MeshPacket &p) { - if (p.from == 0) // If the phone didn't set a sending node ID, use ours - p.from = nodeDB.getNodeNum(); + if (p.from != 0) { // We don't let phones assign nodenums to their sent messages + DEBUG_MSG("Warning: phone tried to pick a nodenum, we don't allow that.\n"); + p.from = 0; + } else { + // p.from = nodeDB.getNodeNum(); + } if (p.id == 0) p.id = generatePacketId(); // If the phone didn't supply one, then pick one @@ -151,7 +157,8 @@ void MeshService::handleToRadio(MeshPacket &p) } /** Attempt to cancel a previously sent packet from this _local_ node. Returns true if a packet was found we could cancel */ -bool MeshService::cancelSending(PacketId id) { +bool MeshService::cancelSending(PacketId id) +{ return router->cancelSending(nodeDB.getNodeNum(), id); } @@ -176,21 +183,22 @@ void MeshService::sendNetworkPing(NodeNum dest, bool wantReplies) nodeInfoPlugin->sendOurNodeInfo(dest, wantReplies); } - -NodeInfo *MeshService::refreshMyNodeInfo() { +NodeInfo *MeshService::refreshMyNodeInfo() +{ NodeInfo *node = nodeDB.getNode(nodeDB.getNodeNum()); assert(node); // We might not have a position yet for our local node, in that case, at least try to send the time - if(!node->has_position) { + if (!node->has_position) { memset(&node->position, 0, sizeof(node->position)); node->has_position = true; } - + Position &position = node->position; // Update our local node info with our position (even if we don't decide to update anyone else) - position.time = getValidTime(RTCQualityFromNet); // This nodedb timestamp might be stale, so update it if our clock is kinda valid + position.time = + getValidTime(RTCQualityFromNet); // This nodedb timestamp might be stale, so update it if our clock is kinda valid position.battery_level = powerStatus->getBatteryChargePercent(); updateBatteryLevel(position.battery_level); @@ -209,11 +217,10 @@ int MeshService::onGPSChanged(const meshtastic::GPSStatus *unused) pos.altitude = gps->altitude; pos.latitude_i = gps->latitude; pos.longitude_i = gps->longitude; - } - else { + } else { // The GPS has lost lock, if we are fixed position we should just keep using // the old position - if(radioConfig.preferences.fixed_position) { + if (radioConfig.preferences.fixed_position) { DEBUG_MSG("WARNING: Using fixed position\n"); } else { // throw away old position diff --git a/src/mesh/MeshTypes.h b/src/mesh/MeshTypes.h index 06f2bf48..e7e6265a 100644 --- a/src/mesh/MeshTypes.h +++ b/src/mesh/MeshTypes.h @@ -32,4 +32,10 @@ typedef uint32_t PacketId; // A packet sequence number typedef int ErrorCode; /// Alloc and free packets to our global, ISR safe pool -extern Allocator &packetPool; \ No newline at end of file +extern Allocator &packetPool; + +/** + * Most (but not always) of the time we want to treat packets 'from' the local phone (where from == 0), as if they originated on the local node. + * If from is zero this function returns our node number instead + */ +NodeNum getFrom(const MeshPacket *p); \ No newline at end of file diff --git a/src/mesh/NodeDB.cpp b/src/mesh/NodeDB.cpp index 22c1ccb6..166e159f 100644 --- a/src/mesh/NodeDB.cpp +++ b/src/mesh/NodeDB.cpp @@ -66,6 +66,14 @@ NodeNum displayedNodeNum; NodeDB::NodeDB() : nodes(devicestate.node_db), numNodes(&devicestate.node_db_count) {} +/** + * Most (but not always) of the time we want to treat packets 'from' the local phone (where from == 0), as if they originated on the local node. + * If from is zero this function returns our node number instead + */ +NodeNum getFrom(const MeshPacket *p) { + return (p->from == 0) ? nodeDB.getNodeNum() : p->from; +} + bool NodeDB::resetRadioConfig() { bool didFactoryReset = false; @@ -406,7 +414,7 @@ void NodeDB::updateFrom(const MeshPacket &mp) if (mp.which_payloadVariant == MeshPacket_decoded_tag) { DEBUG_MSG("Update DB node 0x%x, rx_time=%u\n", mp.from, mp.rx_time); - NodeInfo *info = getOrCreateNode(mp.from); + NodeInfo *info = getOrCreateNode(getFrom(&mp)); if (mp.rx_time) { // if the packet has a valid timestamp use it to update our last_seen info->has_position = true; // at least the time is valid diff --git a/src/mesh/NodeDB.h b/src/mesh/NodeDB.h index b424adcb..e5f1b656 100644 --- a/src/mesh/NodeDB.h +++ b/src/mesh/NodeDB.h @@ -183,3 +183,4 @@ PREF_GET(min_wake_secs, 10) * might have changed is incremented. Allows others to detect they might now be on a new channel. */ extern uint32_t radioGeneration; + diff --git a/src/mesh/PacketHistory.cpp b/src/mesh/PacketHistory.cpp index 3d1884ac..19ce41bc 100644 --- a/src/mesh/PacketHistory.cpp +++ b/src/mesh/PacketHistory.cpp @@ -26,7 +26,7 @@ bool PacketHistory::wasSeenRecently(const MeshPacket *p, bool withUpdate) // DEBUG_MSG("Deleting old broadcast record %d\n", i); recentPackets.erase(recentPackets.begin() + i); // delete old record } else { - if (r.id == p->id && r.sender == p->from) { + if (r.id == p->id && r.sender == getFrom(p)) { DEBUG_MSG("Found existing packet record for fr=0x%x,to=0x%x,id=%d\n", p->from, p->to, p->id); // Update the time on this record to now @@ -43,7 +43,7 @@ bool PacketHistory::wasSeenRecently(const MeshPacket *p, bool withUpdate) if (withUpdate) { PacketRecord r; r.id = p->id; - r.sender = p->from; + r.sender = getFrom(p); r.rxTimeMsec = now; recentPackets.push_back(r); printPacket("Adding packet record", p); diff --git a/src/mesh/ReliableRouter.cpp b/src/mesh/ReliableRouter.cpp index da743c38..80ee9770 100644 --- a/src/mesh/ReliableRouter.cpp +++ b/src/mesh/ReliableRouter.cpp @@ -26,15 +26,15 @@ ErrorCode ReliableRouter::send(MeshPacket *p) bool ReliableRouter::shouldFilterReceived(const MeshPacket *p) { - if (p->to == NODENUM_BROADCAST && p->from == getNodeNum()) { + if (p->to == NODENUM_BROADCAST && getFrom(p) == getNodeNum()) { printPacket("Rx someone rebroadcasting for us", p); // We are seeing someone rebroadcast one of our broadcast attempts. // If this is the first time we saw this, cancel any retransmissions we have queued up and generate an internal ack for // the original sending process. - if (stopRetransmission(p->from, p->id)) { + if (stopRetransmission(getFrom(p), p->id)) { DEBUG_MSG("Someone is retransmitting for us, generate implicit ack\n"); - sendAckNak(Routing_Error_NONE, p->from, p->id); + sendAckNak(Routing_Error_NONE, getFrom(p), p->id); } } @@ -60,7 +60,7 @@ void ReliableRouter::sniffReceived(const MeshPacket *p, const Routing *c) if (p->to == ourNode) { // ignore ack/nak/want_ack packets that are not address to us (we only handle 0 hop reliability // - not DSR routing) if (p->want_ack) { - sendAckNak(Routing_Error_NONE, p->from, p->id); + sendAckNak(Routing_Error_NONE, getFrom(p), p->id); } // If the payload is valid, look for ack/nak @@ -131,7 +131,7 @@ PendingPacket *ReliableRouter::startRetransmission(MeshPacket *p) auto rec = PendingPacket(p); setNextTx(&rec); - stopRetransmission(p->from, p->id); + stopRetransmission(getFrom(p), p->id); pending[id] = rec; return &pending[id]; diff --git a/src/mesh/ReliableRouter.h b/src/mesh/ReliableRouter.h index e6a71f42..fefc85cb 100644 --- a/src/mesh/ReliableRouter.h +++ b/src/mesh/ReliableRouter.h @@ -15,7 +15,7 @@ struct GlobalPacketId { GlobalPacketId(const MeshPacket *p) { - node = p->from; + node = getFrom(p); id = p->id; } diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index a6afe952..b4bb149d 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -111,7 +111,7 @@ void Router::sendAckNak(Routing_Error err, NodeNum to, PacketId idFrom) void Router::abortSendAndNak(Routing_Error err, MeshPacket *p) { DEBUG_MSG("Error=%d, returning NAK and dropping packet.\n", err); - sendAckNak(Routing_Error_NO_INTERFACE, p->from, p->id); + sendAckNak(Routing_Error_NO_INTERFACE, getFrom(p), p->id); packetPool.release(p); } @@ -155,6 +155,10 @@ ErrorCode Router::send(MeshPacket *p) if (p->to == NODENUM_BROADCAST) p->want_ack = false; + // Up until this point we might have been using 0 for the from address (if it started with the phone), but when we send over + // the lora we need to make sure we have replaced it with our local address + p->from = getFrom(p); + // If the packet hasn't yet been encrypted, do so now (it might already be encrypted if we are just forwarding it) assert(p->which_payloadVariant == MeshPacket_encrypted_tag || @@ -273,7 +277,7 @@ void Router::handleReceived(MeshPacket *p) void Router::perhapsHandleReceived(MeshPacket *p) { assert(radioConfig.has_preferences); - bool ignore = is_in_repeated(radioConfig.preferences.ignore_incoming, p->from); + bool ignore = is_in_repeated(radioConfig.preferences.ignore_incoming, getFrom(p)); if (ignore) DEBUG_MSG("Ignoring incoming message, 0x%x is in our ignore list\n", p->from); diff --git a/src/plugins/ExternalNotificationPlugin.cpp b/src/plugins/ExternalNotificationPlugin.cpp index 75425982..42865f3c 100644 --- a/src/plugins/ExternalNotificationPlugin.cpp +++ b/src/plugins/ExternalNotificationPlugin.cpp @@ -147,7 +147,7 @@ bool ExternalNotificationPluginRadio::handleReceived(const MeshPacket &mp) auto &p = mp.decoded; - if (mp.from != nodeDB.getNodeNum()) { + if (getFrom(&mp) != nodeDB.getNodeNum()) { // TODO: This may be a problem if messages are sent in unicide, but I'm not sure if it will. // Need to know if and how this could be a problem. diff --git a/src/plugins/NodeInfoPlugin.cpp b/src/plugins/NodeInfoPlugin.cpp index 2524ffa5..82c16cbd 100644 --- a/src/plugins/NodeInfoPlugin.cpp +++ b/src/plugins/NodeInfoPlugin.cpp @@ -12,7 +12,7 @@ bool NodeInfoPlugin::handleReceivedProtobuf(const MeshPacket &mp, const User *pp { auto p = *pptr; - nodeDB.updateUser(mp.from, p); + nodeDB.updateUser(getFrom(&mp), p); bool wasBroadcast = mp.to == NODENUM_BROADCAST; diff --git a/src/plugins/PositionPlugin.cpp b/src/plugins/PositionPlugin.cpp index 25160586..b6f76ad7 100644 --- a/src/plugins/PositionPlugin.cpp +++ b/src/plugins/PositionPlugin.cpp @@ -30,7 +30,7 @@ bool PositionPlugin::handleReceivedProtobuf(const MeshPacket &mp, const Position perhapsSetRTC(RTCQualityFromNet, &tv); } - nodeDB.updatePosition(mp.from, p); + nodeDB.updatePosition(getFrom(&mp), p); return false; // Let others look at this message also if they want } diff --git a/src/plugins/SerialPlugin.cpp b/src/plugins/SerialPlugin.cpp index 060f05ba..c93bf852 100644 --- a/src/plugins/SerialPlugin.cpp +++ b/src/plugins/SerialPlugin.cpp @@ -160,7 +160,7 @@ bool SerialPluginRadio::handleReceived(const MeshPacket &mp) // DEBUG_MSG("Received text msg self=0x%0x, from=0x%0x, to=0x%0x, id=%d, msg=%.*s\n", // nodeDB.getNodeNum(), mp.from, mp.to, mp.id, p.payload.size, p.payload.bytes); - if (mp.from == nodeDB.getNodeNum()) { + if (getFrom(&mp) == nodeDB.getNodeNum()) { /* * If radioConfig.preferences.serialplugin_echo is true, then echo the packets that are sent out back to the TX diff --git a/src/plugins/esp32/EnvironmentalMeasurementPlugin.cpp b/src/plugins/esp32/EnvironmentalMeasurementPlugin.cpp index 4ade032b..69482dbc 100644 --- a/src/plugins/esp32/EnvironmentalMeasurementPlugin.cpp +++ b/src/plugins/esp32/EnvironmentalMeasurementPlugin.cpp @@ -137,8 +137,9 @@ bool EnvironmentalMeasurementPlugin::wantUIFrame() { String GetSenderName(const MeshPacket &mp) { String sender; - if (nodeDB.getNode(mp.from)){ - sender = nodeDB.getNode(mp.from)->user.short_name; + auto node = nodeDB.getNode(getFrom(&mp)); + if (node){ + sender = node->user.short_name; } else { sender = "UNK"; diff --git a/src/plugins/esp32/RangeTestPlugin.cpp b/src/plugins/esp32/RangeTestPlugin.cpp index 227deba4..2332b687 100644 --- a/src/plugins/esp32/RangeTestPlugin.cpp +++ b/src/plugins/esp32/RangeTestPlugin.cpp @@ -133,7 +133,7 @@ bool RangeTestPluginRadio::handleReceived(const MeshPacket &mp) // DEBUG_MSG("Received text msg self=0x%0x, from=0x%0x, to=0x%0x, id=%d, msg=%.*s\n", // nodeDB.getNodeNum(), mp.from, mp.to, mp.id, p.payload.size, p.payload.bytes); - if (mp.from != nodeDB.getNodeNum()) { + if (getFrom(&mp) != nodeDB.getNodeNum()) { // DEBUG_MSG("* * Message came from the mesh\n"); // Serial2.println("* * Message came from the mesh"); @@ -144,7 +144,7 @@ bool RangeTestPluginRadio::handleReceived(const MeshPacket &mp) */ - NodeInfo *n = nodeDB.getNode(mp.from); + NodeInfo *n = nodeDB.getNode(getFrom(&mp)); if (radioConfig.preferences.range_test_plugin_save) { appendFile(mp); @@ -209,7 +209,7 @@ bool RangeTestPluginRadio::appendFile(const MeshPacket &mp) { auto &p = mp.decoded; - NodeInfo *n = nodeDB.getNode(mp.from); + NodeInfo *n = nodeDB.getNode(getFrom(&mp)); /* DEBUG_MSG("-----------------------------------------\n"); DEBUG_MSG("p.payload.bytes \"%s\"\n", p.payload.bytes); @@ -290,7 +290,7 @@ bool RangeTestPluginRadio::appendFile(const MeshPacket &mp) fileToAppend.printf("??:??:??,"); // Time } - fileToAppend.printf("%d,", mp.from); // From + fileToAppend.printf("%d,", getFrom(&mp)); // From fileToAppend.printf("%s,", n->user.long_name); // Long Name fileToAppend.printf("%f,", n->position.latitude_i * 1e-7); // Sender Lat fileToAppend.printf("%f,", n->position.longitude_i * 1e-7); // Sender Long diff --git a/src/plugins/esp32/StoreForwardPlugin.cpp b/src/plugins/esp32/StoreForwardPlugin.cpp index d55e076b..5340c48a 100644 --- a/src/plugins/esp32/StoreForwardPlugin.cpp +++ b/src/plugins/esp32/StoreForwardPlugin.cpp @@ -240,12 +240,12 @@ bool StoreForwardPluginRadio::handleReceived(const MeshPacket &mp) #ifndef NO_ESP32 if (radioConfig.preferences.store_forward_plugin_enabled) { - if (mp.from != nodeDB.getNodeNum()) { + if (getFrom(&mp) != nodeDB.getNodeNum()) { // DEBUG_MSG("Store & Forward Plugin -- Print Start ---------- ---------- ---------- ---------- ----------\n\n\n"); // DEBUG_MSG("%s (id=0x%08x Fr0x%02x To0x%02x, WantAck%d, HopLim%d", prefix, p->id, p->from & 0xff, p->to & 0xff, // p->want_ack, p->hop_limit); printPacket("----- PACKET FROM RADIO -----", &mp); - uint32_t sawTime = storeForwardPlugin->sawNode(mp.from & 0xffffffff); + uint32_t sawTime = storeForwardPlugin->sawNode(getFrom(&mp) & 0xffffffff); DEBUG_MSG("We last saw this node (%u), %u sec ago\n", mp.from & 0xffffffff, (millis() - sawTime) / 1000); if (mp.decoded.portnum == PortNum_UNKNOWN_APP) { @@ -283,7 +283,7 @@ bool StoreForwardPluginRadio::handleReceived(const MeshPacket &mp) if ((millis() - sawTime) > STOREFORWARD_SEND_HISTORY_SHORT) { // Node has been away for a while. - storeForwardPlugin->historySend(sawTime, mp.from); + storeForwardPlugin->historySend(sawTime, getFrom(&mp)); } } From 0c0c0babba9d415f481cb2b46a274158e9c447f0 Mon Sep 17 00:00:00 2001 From: Kevin Hester Date: Fri, 5 Mar 2021 11:44:45 +0800 Subject: [PATCH 3/4] combine acks works --- docs/software/TODO.md | 2 +- src/mesh/FloodingRouter.cpp | 2 +- src/mesh/MeshPlugin.cpp | 64 +++++++++++++++++++++-------------- src/mesh/MeshPlugin.h | 7 ++++ src/mesh/MeshService.cpp | 27 ++++++--------- src/mesh/PhoneAPI.cpp | 1 + src/mesh/ProtobufPlugin.h | 3 +- src/mesh/ReliableRouter.cpp | 38 ++++++++++++--------- src/mesh/Router.cpp | 1 + src/plugins/Plugins.cpp | 4 ++- src/plugins/RoutingPlugin.cpp | 5 +-- 11 files changed, 91 insertions(+), 63 deletions(-) diff --git a/docs/software/TODO.md b/docs/software/TODO.md index dfef0427..622a95af 100644 --- a/docs/software/TODO.md +++ b/docs/software/TODO.md @@ -31,7 +31,7 @@ You probably don't care about this section - skip to the next one. * DONE fix setch-fast in python tool * age out pendingrequests in the python API * DONE stress test channel download from python, sometimes it seems like we don't get all replies, bug was due to simultaneous android connection -* combine acks and responses in a single message if possible (do routing plugin LAST and drop ACK if someone else has already replied) +* DONE combine acks and responses in a single message if possible (do routing plugin LAST and drop ACK if someone else has already replied) * DONE don't send packets we received from the phone BACK TOWARDS THE PHONE (possibly use fromnode 0 for packets the phone sends?) * fix 1.1.50 android debug panel display * DONE warn in android app about unset regions diff --git a/src/mesh/FloodingRouter.cpp b/src/mesh/FloodingRouter.cpp index 049d038c..491501e9 100644 --- a/src/mesh/FloodingRouter.cpp +++ b/src/mesh/FloodingRouter.cpp @@ -31,7 +31,7 @@ void FloodingRouter::sniffReceived(const MeshPacket *p, const Routing *c) { // If a broadcast, possibly _also_ send copies out into the mesh. // (FIXME, do something smarter than naive flooding here) - if (p->to == NODENUM_BROADCAST && p->hop_limit > 0 && p->from != getNodeNum()) { + if (p->to == NODENUM_BROADCAST && p->hop_limit > 0 && getFrom(p) != getNodeNum()) { if (p->id != 0) { MeshPacket *tosend = packetPool.allocCopy(*p); // keep a copy because we will be sending it diff --git a/src/mesh/MeshPlugin.cpp b/src/mesh/MeshPlugin.cpp index 34aaa236..562f31c7 100644 --- a/src/mesh/MeshPlugin.cpp +++ b/src/mesh/MeshPlugin.cpp @@ -1,23 +1,28 @@ #include "MeshPlugin.h" -#include "NodeDB.h" #include "MeshService.h" +#include "NodeDB.h" #include std::vector *MeshPlugin::plugins; const MeshPacket *MeshPlugin::currentRequest; +/** + * If any of the current chain of plugins has already sent a reply, it will be here. This is useful to allow + * the RoutingPlugin to avoid sending redundant acks + */ + MeshPacket *MeshPlugin::currentReply; + MeshPlugin::MeshPlugin(const char *_name) : name(_name) { // Can't trust static initalizer order, so we check each time - if(!plugins) + if (!plugins) plugins = new std::vector(); plugins->push_back(this); } -void MeshPlugin::setup() { -} +void MeshPlugin::setup() {} MeshPlugin::~MeshPlugin() { @@ -31,6 +36,8 @@ void MeshPlugin::callPlugins(const MeshPacket &mp) assert(mp.which_payloadVariant == MeshPacket_decoded_tag); // I think we are guarnteed the packet is decoded by this point? + currentReply = NULL; // No reply yet + // Was this message directed to us specifically? Will be false if we are sniffing someone elses packets auto ourNodeNum = nodeDB.getNodeNum(); bool toUs = mp.to == NODENUM_BROADCAST || mp.to == ourNodeNum; @@ -48,15 +55,16 @@ void MeshPlugin::callPlugins(const MeshPacket &mp) bool handled = pi.handleReceived(mp); // Possibly send replies (but only if the message was directed to us specifically, i.e. not for promiscious sniffing) + // also: we only let the one plugin send a reply, once that happens, remaining plugins are not considered - // NOTE: we send a reply *even if the (non broadcast) request was from us* which is unfortunate but necessary because currently when the phone - // sends things, it sends things using the local node ID as the from address. A better solution (FIXME) would be to let phones - // have their own distinct addresses and we 'route' to them like any other node. - if (mp.decoded.want_response && toUs && (getFrom(&mp) != ourNodeNum || mp.to == ourNodeNum)) { + // NOTE: we send a reply *even if the (non broadcast) request was from us* which is unfortunate but necessary because + // currently when the phone sends things, it sends things using the local node ID as the from address. A better + // solution (FIXME) would be to let phones have their own distinct addresses and we 'route' to them like any other + // node. + if (mp.decoded.want_response && toUs && (getFrom(&mp) != ourNodeNum || mp.to == ourNodeNum) && !currentReply) { pi.sendResponse(mp); DEBUG_MSG("Plugin %s sent a response\n", pi.name); - } - else { + } else { DEBUG_MSG("Plugin %s considered\n", pi.name); } if (handled) { @@ -64,11 +72,17 @@ void MeshPlugin::callPlugins(const MeshPacket &mp) break; } } - + pi.currentRequest = NULL; } - if(!pluginFound) + if(currentReply) { + DEBUG_MSG("Sending response\n"); + service.sendToMesh(currentReply); + currentReply = NULL; + } + + if (!pluginFound) DEBUG_MSG("No plugins interested in portnum=%d\n", mp.decoded.portnum); } @@ -76,39 +90,39 @@ void MeshPlugin::callPlugins(const MeshPacket &mp) * so that subclasses can (optionally) send a response back to the original sender. Implementing this method * is optional */ -void MeshPlugin::sendResponse(const MeshPacket &req) { +void MeshPlugin::sendResponse(const MeshPacket &req) +{ auto r = allocReply(); - if(r) { - DEBUG_MSG("Sending response\n"); + if (r) { setReplyTo(r, req); - service.sendToMesh(r); - } - else { + currentReply = r; + } else { // Ignore - this is now expected behavior for routing plugin (because it ignores some replies) // DEBUG_MSG("WARNING: Client requested response but this plugin did not provide\n"); } } -/** set the destination and packet parameters of packet p intended as a reply to a particular "to" packet +/** set the destination and packet parameters of packet p intended as a reply to a particular "to" packet * This ensures that if the request packet was sent reliably, the reply is sent that way as well. -*/ -void setReplyTo(MeshPacket *p, const MeshPacket &to) { + */ +void setReplyTo(MeshPacket *p, const MeshPacket &to) +{ assert(p->which_payloadVariant == MeshPacket_decoded_tag); // Should already be set by now p->to = getFrom(&to); p->want_ack = to.want_ack; p->decoded.request_id = to.id; } -std::vector MeshPlugin::GetMeshPluginsWithUIFrames() { +std::vector MeshPlugin::GetMeshPluginsWithUIFrames() +{ - std::vector pluginsWithUIFrames; + std::vector pluginsWithUIFrames; for (auto i = plugins->begin(); i != plugins->end(); ++i) { auto &pi = **i; - if ( pi.wantUIFrame()) { + if (pi.wantUIFrame()) { DEBUG_MSG("Plugin wants a UI Frame\n"); pluginsWithUIFrames.push_back(&pi); } } return pluginsWithUIFrames; - } diff --git a/src/mesh/MeshPlugin.h b/src/mesh/MeshPlugin.h index 7220a198..7234b6b0 100644 --- a/src/mesh/MeshPlugin.h +++ b/src/mesh/MeshPlugin.h @@ -82,6 +82,13 @@ class MeshPlugin private: + /** + * If any of the current chain of plugins has already sent a reply, it will be here. This is useful to allow + * the RoutingPlugin to avoid sending redundant acks + */ + static MeshPacket *currentReply; + friend class ReliableRouter; + /** Messages can be received that have the want_response bit set. If set, this callback will be invoked * so that subclasses can (optionally) send a response back to the original sender. This method calls allocReply() * to generate the reply message, and if !NULL that message will be delivered to whoever sent req diff --git a/src/mesh/MeshService.cpp b/src/mesh/MeshService.cpp index f21742ea..4dcc1785 100644 --- a/src/mesh/MeshService.cpp +++ b/src/mesh/MeshService.cpp @@ -69,26 +69,21 @@ int MeshService::handleFromRadio(const MeshPacket *mp) { powerFSM.trigger(EVENT_RECEIVED_PACKET); // Possibly keep the node from sleeping - if (mp->from != 0) { - printPacket("Forwarding to phone", mp); - nodeDB.updateFrom(*mp); // update our DB state based off sniffing every RX packet from the radio + printPacket("Forwarding to phone", mp); + nodeDB.updateFrom(*mp); // update our DB state based off sniffing every RX packet from the radio - fromNum++; + fromNum++; - if (toPhoneQueue.numFree() == 0) { - DEBUG_MSG("NOTE: tophone queue is full, discarding oldest\n"); - MeshPacket *d = toPhoneQueue.dequeuePtr(0); - if (d) - releaseToPool(d); - } - - MeshPacket *copied = packetPool.allocCopy(*mp); - assert(toPhoneQueue.enqueue(copied, 0)); // FIXME, instead of failing for full queue, delete the oldest mssages - } - else { - DEBUG_MSG("Packet originally from phone, no need to send back that way...\n"); + if (toPhoneQueue.numFree() == 0) { + DEBUG_MSG("NOTE: tophone queue is full, discarding oldest\n"); + MeshPacket *d = toPhoneQueue.dequeuePtr(0); + if (d) + releaseToPool(d); } + MeshPacket *copied = packetPool.allocCopy(*mp); + assert(toPhoneQueue.enqueue(copied, 0)); // FIXME, instead of failing for full queue, delete the oldest mssages + return 0; } diff --git a/src/mesh/PhoneAPI.cpp b/src/mesh/PhoneAPI.cpp index a7797c25..5a7666b2 100644 --- a/src/mesh/PhoneAPI.cpp +++ b/src/mesh/PhoneAPI.cpp @@ -59,6 +59,7 @@ void PhoneAPI::handleToRadio(const uint8_t *buf, size_t bufLength) } // return (lastContactMsec != 0) && + memset(&toRadioScratch, 0, sizeof(toRadioScratch)); if (pb_decode_from_bytes(buf, bufLength, ToRadio_fields, &toRadioScratch)) { switch (toRadioScratch.which_payloadVariant) { case ToRadio_packet_tag: { diff --git a/src/mesh/ProtobufPlugin.h b/src/mesh/ProtobufPlugin.h index c68885df..30874b24 100644 --- a/src/mesh/ProtobufPlugin.h +++ b/src/mesh/ProtobufPlugin.h @@ -58,11 +58,12 @@ template class ProtobufPlugin : protected SinglePortPlugin // it would be better to update even if the message was destined to others. auto &p = mp.decoded; - DEBUG_MSG("Received %s from=0x%0x, id=0x%x, payloadlen=%d\n", name, mp.from, mp.id, p.payload.size); + DEBUG_MSG("Received %s from=0x%0x, id=0x%x, portnum=%d, payloadlen=%d\n", name, mp.from, mp.id, p.portnum, p.payload.size); T scratch; T *decoded = NULL; if(mp.decoded.portnum == ourPortNum) { + memset(&scratch, 0, sizeof(scratch)); if (pb_decode_from_bytes(p.payload.bytes, p.payload.size, fields, &scratch)) decoded = &scratch; else diff --git a/src/mesh/ReliableRouter.cpp b/src/mesh/ReliableRouter.cpp index 80ee9770..eb22117f 100644 --- a/src/mesh/ReliableRouter.cpp +++ b/src/mesh/ReliableRouter.cpp @@ -2,6 +2,7 @@ #include "MeshTypes.h" #include "configuration.h" #include "mesh-pb-constants.h" +#include "MeshPlugin.h" // ReliableRouter::ReliableRouter() {} @@ -26,7 +27,8 @@ ErrorCode ReliableRouter::send(MeshPacket *p) bool ReliableRouter::shouldFilterReceived(const MeshPacket *p) { - if (p->to == NODENUM_BROADCAST && getFrom(p) == getNodeNum()) { + // Note: do not use getFrom() here, because we want to ignore messages sent from phone + if (p->to == NODENUM_BROADCAST && p->from == getNodeNum()) { printPacket("Rx someone rebroadcasting for us", p); // We are seeing someone rebroadcast one of our broadcast attempts. @@ -34,7 +36,8 @@ bool ReliableRouter::shouldFilterReceived(const MeshPacket *p) // the original sending process. if (stopRetransmission(getFrom(p), p->id)) { DEBUG_MSG("Someone is retransmitting for us, generate implicit ack\n"); - sendAckNak(Routing_Error_NONE, getFrom(p), p->id); + if(p->want_ack) + sendAckNak(Routing_Error_NONE, getFrom(p), p->id); } } @@ -60,23 +63,26 @@ void ReliableRouter::sniffReceived(const MeshPacket *p, const Routing *c) if (p->to == ourNode) { // ignore ack/nak/want_ack packets that are not address to us (we only handle 0 hop reliability // - not DSR routing) if (p->want_ack) { - sendAckNak(Routing_Error_NONE, getFrom(p), p->id); + if(MeshPlugin::currentReply) + DEBUG_MSG("Someone else has replied to this message, no need for a 2nd ack"); + else + sendAckNak(Routing_Error_NONE, getFrom(p), p->id); } - // If the payload is valid, look for ack/nak - if (c) { - PacketId ackId = c->error_reason == Routing_Error_NONE ? p->decoded.request_id : 0; - PacketId nakId = c->error_reason != Routing_Error_NONE ? p->decoded.request_id : 0; + // We consider an ack to be either a !routing packet with a request ID or a routing packet with !error + PacketId ackId = ((c && c->error_reason == Routing_Error_NONE) || !c) ? p->decoded.request_id : 0; - // We intentionally don't check wasSeenRecently, because it is harmless to delete non existent retransmission records - if (ackId || nakId) { - if (ackId) { - DEBUG_MSG("Received a ack=%d, stopping retransmissions\n", ackId); - stopRetransmission(p->to, ackId); - } else { - DEBUG_MSG("Received a nak=%d, stopping retransmissions\n", nakId); - stopRetransmission(p->to, nakId); - } + // A nak is a routing packt that has an error code + PacketId nakId = (c && c->error_reason != Routing_Error_NONE) ? p->decoded.request_id : 0; + + // We intentionally don't check wasSeenRecently, because it is harmless to delete non existent retransmission records + if (ackId || nakId) { + if (ackId) { + DEBUG_MSG("Received a ack for 0x%x, stopping retransmissions\n", ackId); + stopRetransmission(p->to, ackId); + } else { + DEBUG_MSG("Received a nak for 0x%x, stopping retransmissions\n", nakId); + stopRetransmission(p->to, nakId); } } } diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index b4bb149d..17f6c50f 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -231,6 +231,7 @@ bool Router::perhapsDecode(MeshPacket *p) crypto->decrypt(p->from, p->id, p->encrypted.size, bytes); // Take those raw bytes and convert them back into a well structured protobuf we can understand + memset(p->decoded, 0, sizeof(p->decoded)); if (!pb_decode_from_bytes(bytes, p->encrypted.size, Data_fields, &p->decoded)) { DEBUG_MSG("Invalid protobufs in received mesh packet (bad psk?!\n"); } else { diff --git a/src/plugins/Plugins.cpp b/src/plugins/Plugins.cpp index 65f1f5f0..ca4cb52f 100644 --- a/src/plugins/Plugins.cpp +++ b/src/plugins/Plugins.cpp @@ -20,7 +20,6 @@ */ void setupPlugins() { - routingPlugin = new RoutingPlugin(); adminPlugin = new AdminPlugin(); nodeInfoPlugin = new NodeInfoPlugin(); positionPlugin = new PositionPlugin(); @@ -48,4 +47,7 @@ void setupPlugins() // new StoreForwardPlugin(); new EnvironmentalMeasurementPlugin(); #endif + + // NOTE! This plugin must be added LAST because it likes to check for replies from other plugins and avoid sending extra acks + routingPlugin = new RoutingPlugin(); } \ No newline at end of file diff --git a/src/plugins/RoutingPlugin.cpp b/src/plugins/RoutingPlugin.cpp index 18427bdc..f6c2ea03 100644 --- a/src/plugins/RoutingPlugin.cpp +++ b/src/plugins/RoutingPlugin.cpp @@ -9,11 +9,12 @@ RoutingPlugin *routingPlugin; bool RoutingPlugin::handleReceivedProtobuf(const MeshPacket &mp, const Routing *r) { - DEBUG_MSG("Routing sniffing", &mp); + printPacket("Routing sniffing", &mp); router->sniffReceived(&mp, r); // FIXME - move this to a non promsicious PhoneAPI plugin? - if (mp.to == NODENUM_BROADCAST || mp.to == nodeDB.getNodeNum()) { + // Note: we are careful not to send back packets that started with the phone back to the phone + if ((mp.to == NODENUM_BROADCAST || mp.to == nodeDB.getNodeNum()) && (mp.from != 0)) { printPacket("Delivering rx packet", &mp); service.handleFromRadio(&mp); } From 8739469db30139cbfffcebf5de6502ea337579e6 Mon Sep 17 00:00:00 2001 From: Kevin Hester Date: Fri, 5 Mar 2021 11:49:37 +0800 Subject: [PATCH 4/4] oops typo --- src/mesh/Router.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index 17f6c50f..87e8e429 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -231,7 +231,7 @@ bool Router::perhapsDecode(MeshPacket *p) crypto->decrypt(p->from, p->id, p->encrypted.size, bytes); // Take those raw bytes and convert them back into a well structured protobuf we can understand - memset(p->decoded, 0, sizeof(p->decoded)); + memset(&p->decoded, 0, sizeof(p->decoded)); if (!pb_decode_from_bytes(bytes, p->encrypted.size, Data_fields, &p->decoded)) { DEBUG_MSG("Invalid protobufs in received mesh packet (bad psk?!\n"); } else {