Georacer / ardupilog

A ardupilot log to MATLAB converter
GNU General Public License v3.0
49 stars 36 forks source link

UNSURE: Changes which support (incorrect) 3DR Solo logs? #62

Closed hunt0r closed 6 years ago

hunt0r commented 6 years ago

I modified to use Ardupilog on some 3DR Solo logs, which are similar to the Ardupilot format, but have some slight differences. It would be easier to NOT support these, but I thought I'd let you take a look and have them saved in a Pull Request, even if we don't accept/merge them.

(Line 290ish) I think wrapping the FMT message processing in a "try-catch" might be useful in case any old/new versions of Ardupilot have formatting errors like the Solo did?

(Line 350ish) I think the GPS hack is less useful, probably not worth keeping, or bothering to code well.

(Line 47) I've forgotten why I changed fileparts(which(X)) to just fileparts(X).

Georacer commented 6 years ago

Can you share a Solo log, so I can take a look?

hunt0r commented 6 years ago

Here's a link: https://drive.google.com/file/d/1gRNR3dgeWxtYWE_JptI7ryVaYP0Zyrkv/view?usp=sharing

Running it under "master" is a strange error. But it runs with just warning messages under the solo modifications.

Georacer commented 6 years ago

Okay, after 6 months, I started parsing your PRs... not ideal response time, sorry about that.

Anyway, on master the two offending format lines from the Solo log are: 2017-09-11 18:14:27.34: FMT {Type : 186, Length : 27, Name : R10C, Format : IffII, Columns : TimMS,pitch_ref,roll_out,pitch_out,roll_pwm,pitch_pwm} 2017-09-11 18:14:27.34: FMT {Type : 10, Length : 3, Name : STRT, Format : , Columns : }

For the first one, the problem is that the Format field has 5 chars, while Columns has length of 6. For the second, both fields are empty, which is obviously a problem, but not necessarily an error.

I suggest treating these two cases separately from the rest of this PR. (continued...)

Georacer commented 6 years ago

Okay, I added a commit which targets specifically those two issues. The first one now raises a warning.

I removed the try/catch block, I think it is too generic. This is open for debate: On one hand, this specific try/catch gives a leeway to the program to build only those formats which can be covered. On the other hand, since it may fail at any point, the log might contain an unpredictably corrupted message group. Removing the try/catch forces us to be more careful with corrupt logs.

A middle ground may be possible, but I won't pursue it right now.

The solo log parses correctly right now. Test and accept the PR if you are content with it.

hunt0r commented 6 years ago

Looks great, thanks!