fix(loader): reset per-load state so msg_ids don't leak between logs#20
Merged
Merged
Conversation
All parser bookkeeping lives in DataLoadAPBIN member variables (has_fmt / has_fmtu / has_instance / instance_idx / instance_offset / formats / format_units / msg_id2name / msg_name2id / field_name2idx / messages_map / multipliers / units), but the plugin instance is reused for every file load in a session and readDataFromFile never reset any of them. ArduPilot recycles non-static msg_ids per boot, so loading two logs in sequence can put a different message at the same id. Because the FMTU handler guards the '#' check with `if (!has_instance[msg_id])`, a stale `true` from the previous file's message is never cleared. The next log's message at that id gets treated as instanced and a byte from its payload (at the previous file's instance_offset) is read as the instance number, producing dozens of bogus "#N" children — e.g. HEAT exploded into 300+ spurious instances after a log where msg_id 251 was SURF. The accumulated messages_map also caused already-multiplied timestamps from earlier loads to be re-multiplied and re-time-synced, leaving a stray data point on the far right of plots. Clearing the state at the top of readDataFromFile fixes both.
Contributor
|
Thank you for that! It has been a problem for so long! Perhaps related to this, I have noticed that if you try to open a .bin after already loading one and a layout, it would take a VERY long time to load a hoard a bunch of RAM. Did you also notice by any chance if this got resolved as well? |
Contributor
|
It does seem to load the 2nd file quickly and correctly now. Very nice! |
Contributor
Author
I did not notice that! |
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
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.
Summary
All parser bookkeeping (
has_fmt/has_fmtu/has_instance/instance_idx/instance_offset/formats/format_units/msg_id2name/msg_name2id/field_name2idx/messages_map/multipliers/units) lives inDataLoadAPBINmember variables, but the plugin instance is reused for every file load in a session andreadDataFromFilenever reset any of them. This patch clears them at the top ofreadDataFromFile.Problem
ArduPilot assigns msg_ids dynamically per boot, so loading two logs in sequence can put a different message at the same id. The FMTU handler guards the
#check withif (!has_instance[msg_id]), so a staletruefrom a previous file's message is never re-evaluated. When the next log puts a non-instanced message at that id, it gets treated as instanced and a byte from its payload (at the previous file'sinstance_offset) is read as the instance number.Reproducer using two real ArduPilot logs from the same vehicle (one had msg_id 251 = SURF, the next had msg_id 251 = HEAT):
The accumulated
messages_mapalso causes timestamps that already went throughapply_multipliers+apply_timesyncto be re-processed on the next load, producing far-future stray data points that compress the plot's useful range against one edge.Solution
std::fillthe bool/int arrays, value-initialize thelog_Format/log_Format_Unitsstructs, clear the strings inmsg_id2name, and call.clear()on the maps. Verified the fix by loading the same two-log sequence viaQPluginLoaderand dumping the resultingPlotDataMapRef.