Refactor/#50 optimizations from pr 33 and others#81
Merged
GUVWAF merged 21 commits intoMay 15, 2026
Merged
Conversation
We just copy.deepcopy the config object so we operate on our own config. setup_asymmetric_links sets up asymmetric links by adding a LINK_OFFSET member to the config. Add a docstring for the function mentioning this but otherwise leave it as-is. Also add a test to make sure that running a DiscreteEventSim doesn't change the config object we provide to create the sim.
The timing can vary quite a bit between runs depending on what else is happening on the system, but this at least helps get some numbers when working on optimizations. Hopefully we can make improvements on the big-O runtime that will be easy to see even in rough numbers.
This uses a connectivity map to track what nodes could feasibly 'hear' each other. It is symmetric and with ample margin to accomodate asymmetric link behavior. A margin of 8 is chosen based on the 10-node 'gold standard' test run, where one link has an offset of +7.5. Later reimplementation of asymmetric link simulation can remove this link offset data structure and narrow this margin further, which should improve performance slightly, but will necessarily change our 'gold standard' reference test run. The connectivity map is initialized before simulation start by a O(n^2) loop that computes the set of reachable nodes from any arbitrary transmitting node. This runtime could probably be reduced by 1/2 since links are symmetric, but worry about that later (and it would still be O(n^2)). This connectivity map must be updated every time a node moves, but this is computed only from the perspective of the moving node, with some set operations used to update the connectivity maps of other nodes as necessary. So this is O(n), multiplied by every time a mobile node moves. This connectivity map is used in MeshPacket to skip path loss/rssi/sensed computations for nodes that aren't in the transmitting node's connectivity map. This should skip computations for receiving nodes which can't even sense the packet. This step is still O(n), but can skip potentially many unnecessary computations. It all depends on node configuration. Before this optimization, for path loss computations the O(n^2) precomputation obviously doesn't happen, and neither would the O(n) computation for each node move. However each packet creation has a O(n) computation of path loss that can include potentially many unnecessary calculations, and there can be many more packets in a simulation than nodes. Consider this all back-of-the-envelope calculations rather than a rigorous analysis. This gets the most benefit in situations with: - dispersed nodes that can't reach many other nodes - none or fewer moving nodes - very many packets (long-running sim/more active network) - and perhaps many nodes? But may be worse for situations with: - many moving nodes - a densely connected network - perhaps very many nodes (precomputation gets bad) It is probably best to gate this behind an option w/ a config setting, so that it can be enabled/disabled as appropriate for the scenario being simulated. This can be further improved (perhaps greatly?) by caching path loss calculations in the connectivity map, since we've already computed those and can just re-use that value. Do this once asymmetric link simulation is moved into the packet, rather than baked into the config using link offsets.
Since we're already computing the baseline path loss between nodes for our connectivity matrix, just cache those values and reuse them later when constructing packets. This at least saves us distance and path loss calculations for each packet that gets created.
…ity map optimization. Default True. This way we can easily enable/disable this optimization, which may be beneficial or detrimental in different situations. This will make it easy to add CLI arguments later to toggle this, and make it easy to evaluate the optimization across a variety of scenarios. This could potentially let us encode some heuristics for when to enable or disable the optimization based on the scenario given.
… of test runs We (hardcoded) throw away the 5 slowest test runs, so must have 6 or more to not crash. Just gate this argument with a manual check/exception.
Just make sure our optimization isn't unintentionally creating different simulations/results. Previous in-dev testing made sure of this, but now it's encoded in a test case. Currently only checking second-order results since comparing first-order results like the list of packets & nodes requires adding more comparison functions which would be handy, but is more work and can be its own thing.
…dle defaults for txZ, rxZ accepting the model as a parameter gives us some nice flexibility, and the new default handling makes it clearer we are getting defaults from the config passed as a parameter and not the module-level config.
…n MeshNode Just more data we track for later analysis. Not actually used yet, just updated. Mostly because it's in abandoned PR meshtastic#33
Additionally add some explanatory comments and some TODOs. This all comes from debugging the double-count issue fixed in meshtastic#65, and I felt it was helpful to keep around for detailed debugging of packet flows
Ideally the unique packet counter would be part of a sim's state. For now, add a static method so we can reset the counter, which should let us have consistent start-from-0 unique packet sequence numbers between simulation runs. Perhaps later refactor this counter to be part of sim state so we don't have to manually remember to reset it.
Previously asymmetric links were simulated with static, pre-computed offsets to each potential link. Instead, add a random offset to the path loss at each potential receiver on MeshPacket initialization. This makes the asymmetric link simulation dynamic to better simulate transient conditions like moving clutter or other uncontrollable factors. To keep the simulation consistent between the connectivity map optimization being enabled/disabled, do 'useless' asym_rng calls so runs can be deterministic and consistent across that optimization being enabled/disabled. We tune the asymmetric standard deviation to seemingly be quite low so that all RSSIs are still within the connectivity_map margin. Exceeding that margin can lead to inconsistent simulations depending on the connectivity map optimization being enabled/disabled. Log if this happens. Always compute the connectivity map on simulation init and remove the call to setup_asymmetric_links. The asymmetric link generation was already doing an O(n^2) iteration of nodes so we don't save on complexity, and the functionality we still want from setup_asymmetric_links (totalPairs/noLink) can be rolled into initialize_connectivity_map. Rip out everywhere we can find the now-obsolete symmetricLinks/asymmetricLinks variables and whatever depends on them. Finally, update our 'gold standard' test, since this necessarily changes the results.
…iables to NodeConfig This deduplicates crucial code for computing rssi & pathloss between nodes. We put this in NodeConfig so we can use it in discrete sim setup before nodes are created. This requires moving some extra variables into NodeConfig. Also add the NodeConfig provided to create a MeshNode as a member variable so we can access the computation method, and supply the rx node's config to it. Many other member variables are already convenience bindings into this object, so may as well bind to the object itself. Add some docstrings along the way.
These convenience bindings are useful for not having `self.node_conf` repeated all over, but more intentionally show we consider the node_conf a mutable member variable by referencing `self.node_conf` rather than the `nodeConfig` parameter. Later we may add a copy() so we can give the MeshNode its own node config and not have it risk mutating some external one provided to the constructor.
The way CW windows and delays are calculated must have changed, the simulation doesn't seem to match current stable firmware. Update the constants and computations necessary so we match current stable firmware. At time of writing, tag v2.7.15.567b8ea is the most recent 'stable' release on the meshtastic website. This necessarily changes the simulation and thus the results, so update the 'gold standard' 10-node test to match. Additionally, add and improve logging around CW selection.
Contributor
Author
|
I just updated the various CW-related computation functions to match firmware v2.7.15.567b8ea, the current stable release. For reference, here's how the basic results of the 'gold standard' 10-node sim have changed. before: After: The changes seem small to me, except the average delay, which I bet comes from removing a |
Member
The |
GUVWAF
reviewed
May 14, 2026
Member
GUVWAF
left a comment
GUVWAF
left a comment
There was a problem hiding this comment.
Very nice! Left a few comments still.
Contributor
Author
|
Thanks for catching that! I'll double check the firmware and fix it.
…On Thu, May 14, 2026, 6:25 AM GUVWAF ***@***.***> wrote:
*GUVWAF* left a comment (meshtastic/Meshtasticator#81)
<#81 (comment)>
The changes seem small to me, except the average delay, which I bet comes
from removing a * 1000 in get_current_slot_time, which didn't match
firmware.
The * 1000 should be there, because in the firmware the bandwidth is
stored in kHz, whereas in conf.py, MODEM_PRESETS stores the bandwidth in
Hz.
—
Reply to this email directly, view it on GitHub
<#81 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG3ASTCWYAQG7OQSFPOM2342WNJPAVCNFSM6AAAAACYVBKVYSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DINBZHAZDCNJUGE>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Contributor
Author
|
I thought about making the timing script a test, but I don't like a test
that will take a very long time, or pass/fail depending on the hardware (my
slow laptop vs my fast desktop). I could add it as a test that's skipped,
or maybe make a utilities folder? It's the script I've been using to
measure differences with the optimizations so I think it's worth keeping.
Also yes, I can gate the asymmetric simulation behind that config knob.
…On Thu, May 14, 2026, 6:43 AM GUVWAF ***@***.***> wrote:
***@***.**** commented on this pull request.
Very nice! Left a few comments still.
------------------------------
In time-discrete-sim.py
<#81 (comment)>
:
> @@ -0,0 +1,68 @@
+#!/usr/bin/env python3
Maybe we can turn this into a test? Not sure I like it as a separate
script in the root of the repository.
------------------------------
In lib/packet.py
<#81 (comment)>
:
> +
+ # reduce calculations just to plausibly reachable nodes. This is initialized
+ # before sim start, and updated whenever a moving node's position is updated,
+ # so is always an accurate map of what nodes could (with extra margin) even
+ # sense each other.
+ if self.conf.ENABLE_CONNECTIVITY_MAP and not connectivity_map[self.txNodeId].__contains__(rx_node.nodeid):
+ logger.debug(f"skipping {self.txNodeId} -> {rx_node.nodeid} computation. connectivity map: {connectivity_map[self.txNodeId]}")
+ # Each tx -> rx computation we skip gets the asrm_rng one call out
+ # of sync between simulations with and without the connectivity
+ # map optimization. Thus particular tx -> rx calculations
+ # change between the optimization, which can lead to changes
+ # in sim behavior between the optimization being on/off, leading
+ # to inconsistencies beteen the optimization being on/off.
+ #
+ # Keep things balanced by 'unnecessarily' calling the rng here.
+ MeshPacket.asym_rng.gauss(0, conf.MODEL_ASYMMETRIC_LINKS_STDDEV)
Probably better to only call this when conf.MODEL_ASSYMMETRIC_LINKS is
set.
—
Reply to this email directly, view it on GitHub
<#81 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAG3ASXACIGTPOTG5TRPKSD42WPNJAVCNFSM6AAAAACYVBKVYSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DEOBZGI2TSOBSGE>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Member
Yes, either of them would work for me! |
This directory can be for miscellaneous utilities that are worth keeping around, but are meant to be used by developers or testers rather than meshtasticator proper, or general users.
… knob. Just cleaner. This call is to keep simulations using asymmetric links consistent between having the connectivity map optimization on/off. It's unnecessary to do this when not simulating asymmetric links, although doing so doesn't change simulation behavior.
Add `now` to log lines, and change a debug log to a warning log, because it's warning about a condition which will lead to inconsistent or unexpected simulation results.
The firwmare stores bandwidth in KHz, but our config stores bandwidth in Hz. So, scale it to convert from Hz to KHz so our calculation can match the firmware. This slightly changes the 'gold standard' 10-node unit test, so also update that.
Contributor
Author
|
Let me know how that all looks. While correcting the bandwidth unit conversion in get_current_slot_time, I wanted to just change the config to be in KHz to match the firmware, and update everywhere that bandwidth is used. But I decided to not go down that rabbithole now, maybe that would make a good separate issue/beginner issue? |
Member
|
This looks good to me, thanks for the adjustments. Yeah, maybe storing the bandwidth in kHz would be better to be similar to the firmware, but I'm fine with like it is now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Big-ish change incorporating some optimizations from the stale PR #33. At a high level this includes:
As far as the optimizations go, they do seem worth it, but may have drawbacks for certain simulations. For example because the connectivity map must be updated on every node movement, we get an O(n) computation (n: # of nodes) for every node move. If there are lots of moving nodes this could get bad, but I haven't tested it rigorously.
I made sure the connectivity map optimization has no impact on simulation results, which is a bit tricky. Likewise I make sure the refactored asymmetric link simulation in combination with the connectivity map doesn't outrageously change simulation results, but this might give us a more limited range of signal strengths that are simulated so we don't generate a packet that would give inconsistent results with the connectivity map enabled.
Also add a lot of debug lines when I was working on an optimization to let us skip iterating over every node for every packet when collecting simulation results, but this was getting tricky and I wasn't sure the time savings would be worth it. Maybe pick back up on this later.
Here's some abbreviated test results from default config simulations of 30, 40, 50 and 60 nodes, on my laptop. I can post the full files if you want:
Just counting the fastest times, the optimizations reduce runtime for a default configuration sim by about 1.6% - 9.5%. More testing would be interesting to see if there's any sizes of a default config that wind up with this being slower.
I skipped the more drastic implementation from #33 that limited pathloss calculations to only 100 nodes, assuming that would certainly change simulation results. But I haven't tried a sim that large, and maybe there's an acceptable way to do that and get sim results that approximate a full sim without that shortcut.