-
Notifications
You must be signed in to change notification settings - Fork 586
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check for messages[mtype]._instances is None #822
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this change and rebased. Tests pass locally. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, I added the if test before the docstring. Fixed.
a11ee02
to
d21387c
Compare
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