core: add mavlink statustexts as debug printfs #119
Conversation
| mavlink_statustext_t statustext; | ||
| mavlink_msg_statustext_decode(&message, &statustext); | ||
|
|
||
| std::string debug_str = "mavlink "; |
There was a problem hiding this comment.
Is this enough to differentiate device as opposed to DroneCore originated warnings? Maybe text:
MAVLink: emergency or Device: emergency.
Might even be worth having a special class of debug for these. Just a thought.
There was a problem hiding this comment.
Agreed @hamishwillee, it would be good to understand when are these called and if we should be raising exceptions or alerting the users rather than just using them for debugging.
That said this PR is to have more debugging data, so we should merge and get someone to research onto what the messages mean, I want to know if we should be taking more critical actions
There was a problem hiding this comment.
@hamishwillee I can change it to MAVLink: emergency.
@mrpollo to catch errors, it might make sense that the plugins check for errors and inform the API user if it is relevant.
There was a problem hiding this comment.
@mrpollo That said this PR is to have more debugging data, so we should merge and get someone to research onto what the messages mean, I want to know if we should be taking more critical actions
@mrpollo to catch errors, it might make sense that the plugins check for errors and inform the API user if it is relevant.
@hamishwillee I can change it to MAVLink: emergency.
Agree with all above - yes, ideally plugins should check for errors and work out what is relevant but we need to do more research on this. IMO we should do change suggested (MAVLink: xxxx) for now, accept PR, and raise issue to look at errors as a separate task.
There was a problem hiding this comment.
Yeah, merge whenever. I have created #127 to add eventual task to look at these messages.
| mavlink_statustext_t statustext; | ||
| mavlink_msg_statustext_decode(&message, &statustext); | ||
|
|
||
| std::string debug_str = "mavlink "; |
There was a problem hiding this comment.
Agreed @hamishwillee, it would be good to understand when are these called and if we should be raising exceptions or alerting the users rather than just using them for debugging.
That said this PR is to have more debugging data, so we should merge and get someone to research onto what the messages mean, I want to know if we should be taking more critical actions
| // statustext.text is not null terminated, therefore we copy it first to | ||
| // an array big enough that is zeroed. | ||
| char text_with_null[sizeof(statustext.text) + 1] {}; | ||
| memcpy(text_with_null, statustext.text, sizeof(statustext.text)); |
There was a problem hiding this comment.
dirty indeed, but I don't see any other way around this if we can't control the input, and now that I think about it, can this lead to memory leaks? can someone use this to DOS an app using DroneCore by crafting big malicious messages?
There was a problem hiding this comment.
I think all that a malicious drone can do here is to print a bunch of memory that follows after statustext.text until 0 appears.
There was a problem hiding this comment.
In conclusion, yes we make ourselves open to a crafty mavlink attack, but I'm sure this isn't the only place where this occurs
|
So, does anyone feel like this can be approved and be merged now? 😄 |
julianoes
force-pushed
the
add-mavlink-statustexts
branch
from
f9f23e1
to
dcf20b6
Compare
|
@julianoes ready be to merged as soon as you rebase |
By printing all mavlink statustexts we should get a bit more context when something goes wrong on the firmware side.
julianoes
force-pushed
the
add-mavlink-statustexts
branch
from
dcf20b6
to
09fc6f2
Compare
|
When do we plan to merge all this stuff into master. I don't want to document stuff that isn't merged, since currently people build off master. |
I think the docs need to reference to a version. I thought you could do that? |
|
We can do versions using this plugin. I have not tried it because none of the other projects wanted real versioning. I guess I could try creating a develop branch and merge in docs that way. I'll look into that today. |
By printing all mavlink statustexts we should get a bit more context
when something goes wrong on the firmware side.
@bkueng do you have a better suggestion how to make sure the
statustext.textstring is null terminated