-
-
Notifications
You must be signed in to change notification settings - Fork 511
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
plugins: add log streaming #2280
Conversation
d262023
to
0d969b4
Compare
@bkueng it would be awesome if you could give this a quick check/review. I'm basically just saving the data stream but not doing any reordering or retrying. |
0d969b4
to
ecd3360
Compare
Signed-off-by: Julian Oes <[email protected]>
ecd3360
to
6cb93a7
Compare
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.
Looks good so far. I would also have a look at the mavlink-router implementation, as it has additional timeout handling.
You might already have thought about it, but also make sure to clear the buffer and _maybe_current_sequence
after stopping (or when starting).
Instead of the magic wait time, what about just waiting 3s. This fixes the problem after we send a LOGGING_STOP to PX4 which takes a bit of time to wrap up. Signed-off-by: Julian Oes <[email protected]>
Signed-off-by: Julian Oes <[email protected]>
This means we can ignore incoming messages if we haven't actually started logging which makes stopping less confusing. Signed-off-by: Julian Oes <[email protected]>
Signed-off-by: Julian Oes <[email protected]>
Signed-off-by: Julian Oes <[email protected]>
And don't bother warning about something being too short, just log it. Signed-off-by: Julian Oes <[email protected]>
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 @bkueng. I think I have addressed it all, minus the timeout that you suggested.
Is the timeout to basically restart logging if we haven't received data in a bit?
Quality Gate failedFailed conditions |
Yes exactly. It's not strictly required. |
We'll see if I have to add it once I'm testing it. |
Forgot to update proto submodule, duh. |
Just keep in mind that without considering the sequence number, a message drop will likely lead to a corrupt ulog stream. Which is very likely going to happen if going through a serial link. |
Aha, so what do you do on drop? Remove what's currently in the buffer to be written and start fresh? |
Yes, that's what |
Ok thanks. I'll try to add that. |
Coming with #2290. |
* plugins: add log streaming * core: wait long enough after IN_PROGRESS * log_streaming: additional length checks * log_streaming: use active flag to track state * log_streaming: add missing include * log_streaming: prevent double stopping * log_streaming: write what we have anyway
This adds log streaming. The stream is transmitted via a base64 string because
bytes
didn't compile using our gRPC infrastructure.This PR also exposes base64 encoding and decoding functions for C++ users of the API. For Python, this comes with the batteries included.