AP_Scripting: add support for dataflash logging #13315
Conversation
|
F405-Wing is over flowing flash, apart from that seems to have passed everything |
IamPete1
force-pushed
the
scripting-logging3
branch
from
c0ed381
to
9bddcc5
Compare
|
rebased to get F405-wing passing thanks to #13314 |
| } | ||
|
|
||
| // ask for a mesage type | ||
| struct AP_Logger::log_write_fmt *f = AP_logger->msg_fmt_for_name(name, labels, nullptr, nullptr, fmt); |
There was a problem hiding this comment.
is name as a pointer stable?
There was a problem hiding this comment.
The name is stable during this call, but as soon as it's not on the stack it's not anymore, which can lead to future issues. If msg_fmt_for_name actually copied aside the name we'd be okay, but it doesn't, it just copies the pointer. We'd probably need to allocate our own store space/track it in the C++ code for duplicates before allocating a new one, and pass that down to the logger.
|
should be tested in autotest, checking for good results |
|
need to only send FMT msg once |
| } | ||
|
|
||
| // ask for a mesage type | ||
| struct AP_Logger::log_write_fmt *f = AP_logger->msg_fmt_for_name(name, labels, nullptr, nullptr, fmt); |
There was a problem hiding this comment.
The name is stable during this call, but as soon as it's not on the stack it's not anymore, which can lead to future issues. If msg_fmt_for_name actually copied aside the name we'd be okay, but it doesn't, it just copies the pointer. We'd probably need to allocate our own store space/track it in the C++ code for duplicates before allocating a new one, and pass that down to the logger.
IamPete1
force-pushed
the
scripting-logging3
branch
from
9bddcc5
to
927852f
Compare
|
fixed a few things and rebased |
| -- format characters specify the type of variable to be logged, see AP_Logger/README.md | ||
| -- not all format types are supported by scripting only: i, L, e, f, n, M, B, I, E, and N | ||
| -- lua automatically adds a timestamp in micro seconds | ||
| logger.write('SCR','roll(deg),pitch(deg),yaw(deg)','fff',table.unpack(interesting_data)) |
There was a problem hiding this comment.
The units comment was more I think about exposing the units argument to scripting? (I think?) I don't see how we can do that without always requiring it though.
There was a problem hiding this comment.
we could do logger write_units or something, imho without units is more useful for most than with, could probably do both with some changes tho, cant use the same function name i don't think tho.
|
fixed a few more low hanging review issues |
IamPete1
force-pushed
the
scripting-logging3
branch
2 times, most recently
from
57e8c9a
to
d16dd6a
Compare
|
@peterbarker if you could have a look over this that would be great. (I still have to fix the buffer heap stuff, but apart from that i think its ready) |
IamPete1
force-pushed
the
scripting-logging3
branch
from
d16dd6a
to
b6bb3d4
Compare
IamPete1
force-pushed
the
scripting-logging3
branch
from
b6bb3d4
to
f950b34
Compare
|
I think it should be possible to include units and multipliers by checking the length of the format string against the number of arguments, might be better to get this basic functionally in first. |
| @@ -738,6 +738,18 @@ void AP_Logger::Write_Rally() | |||
| FOR_EACH_BACKEND(Write_Rally()); | |||
| } | |||
|
|
|||
| void AP_Logger::Safe_Write_Emit_FMT(log_write_fmt *f) | |||
There was a problem hiding this comment.
needs a comment!
need to reset sent_mask on new log?
|
check that we only get one format message, and that it resets if disarmed and re-armed |
IamPete1
force-pushed
the
scripting-logging3
branch
3 times, most recently
from
43f0443
to
ba8c895
Compare
|
I tested this today and it worked well. |
|
to be rebasesd and improve example readability |
IamPete1
force-pushed
the
scripting-logging3
branch
from
58f1285
to
6d44b3d
Compare
|
Rebased and Re-tested under Valgrind |
IamPete1
force-pushed
the
scripting-logging3
branch
from
6d44b3d
to
409a69a
Compare
|
merged after dev call discussion |
This add support for writing to dataflash logs from scripting. This also adds a example writing to both dataflash logs and a file on the SD card. There is also a second example that reads in data over serial and writes to dataflash closing #12801. The examples use find_baudrate() and tabel.concat from #13268. Tested both in SITL and on real hardware.
This generally works OK, not all the the formats are supported for lua so I added a helper to auto fill the timestamp for a 'Q' character in the format string.
Is there a nicer way to give the binding access to AP_logger? I had to make a couple of functions public to get this working. Possibly we may have also bypassed some checking that gets done by logger before logging.
Many thanks to @WickedShell for helping me out when I got stuck (more than once)