Check for messages[mtype]._instances is None #822
Conversation
mavutil.py
Outdated
| @@ -97,7 +97,7 @@ def add_message(messages, mtype, msg): | |||
| messages[mtype] = msg | |||
| return | |||
| instance_value = getattr(msg, msg._instance_field) | |||
| if not mtype in messages: | |||
| if mtype not in messages or messages[mtype]._instances is None: | |||
There was a problem hiding this comment.
Why not check for msg.get_type() == 'BAD_DATA' at the top of this method, and not do silly things with invalid data?
There was a problem hiding this comment.
Thanks for the review! @peterbarker
That fixes the bug, and pytest succeeds. But has the side effect of never adding BAD_DATA messages to mavfile sysid_state.messages, which is a slight change to behavior. I looked for all usages in this repo, and I don't see any problems, and in fact I imagine it might fix an obscure class of bugs. If you are OK w/ this I'll make the change.
There was a problem hiding this comment.
I made this change and rebased. Tests pass locally. Thanks.
There was a problem hiding this comment.
Whoops, I added the if test before the docstring. Fixed.
clydemcqueen
force-pushed
the
clyde_fix_807
branch
3 times, most recently
from
a11ee02
to
d21387c
Compare
clydemcqueen
force-pushed
the
clyde_fix_807
branch
from
54a19ce
to
2e91367
Compare
|
Whoops, I lost track of this. Thanks @peterbarker and @tridge for the review and fix. I rebased. |
This is a small change to fix #807. I've tested this against quite a few tlog files over the past 2 weeks and it works great. Running pytest comes up clean.
See the longer discussion in https://discuss.bluerobotics.com/t/mavlogdump-py-crashes-while-reading-distance-sensor-msgs-in-some-tlog-files/14228
Thanks,
/Clyde