Temporary prop as dynamicprop

[hunt0r]

[NOTE]
This PR is dependent on #33 . If we make changes to that, we can rebase this one as you specified in an email on March 2nd.

[Description]
Some properties of the Ardupilog class were temporary. We used them as "global within the class", but didn't intend to store their info. I've made them each into a DynamicProperty, so that we can delete them at the end.

In the future, if we re-organize the methods, they might become local variables to a single method.

I also deleted a few properties that don't seem to be used anymore.

[Georacer]

Any reason this went before the filter?
If it goes before, then it will be run regardless of whether this message should be filtered out or not.

[Georacer]

Ah, I see that you need to fill the valid_msgheader_cell. Okay.
Would it be possible to avoid that, to avoid unnecessary cycles?

[Georacer]

Okay, I think you do that for all the messages, so as to have all the locations of the valid message headers. Understandable, but it will cause a major hit in efficiency. Effectively, it will be the same as parsing ALL the headers.

We should evaluate if this delay is unacceptable or not.

[Georacer]

So what you do here is: (correct me if I'm wrong)

1. You sort the locations of all messages
2. For each filtered message you search the line numbers of the corresponding messages and store them in the LineNo property

[Georacer]

I'm a bit concerned with the fact that this change makes the program search for all the messages, regardless of filter. This removes any time savings filtering brought.

Granted, I don't see any other way to do it, if we need the line numbers. Are we ok with doing away with those time savings? Indicatively, the 150MB file would parse in my PC in 20 secs and proportionally less for any kind of filtering.

[Georacer]

Is this done in favour of simplicity or size reduction?

[hunt0r]

I agree there's a design-choice to be made here, with pros-and-cons for any choice.

With the log parsing speed so fast, I propose the following general design:

1. Read the entire file into memory (a single call to fread).
2. Find all FMT messages.
3. Process them (at least) to extract the lengths and ID's of other message types which may exist.
   3.5) At this point, there's probably no significant increase in time to also get also their names, fields, format strings, etc... but if we know the message will be filtered, we could theoretically skip this.
   3.75) I think at this point, it also makes sense to search for the MSG type, and extract the firmware version, etc. (I think this is the findInfo() method, presently)
4. For FMT message defining an id/length, locate all valid messages of this type.

At this intermediate point, we have everything we need: the position of all valid messages in the log. We could also check for any regions of the log which are NOT part of a valid message, as well as any overlap. (if that's even possible) We also have a count of the total number of log lines, the number of lines of each type, etc.

5. For each message that was not filtered out, create a LogMsgGroup (if we didn't already do it in step3) and store the data. (Adding LineNo info wherever appropriate) Note that "storing the data" may get more complex in the near future, as we determine units, interpolability, etc. If we did create the LogMsgGroup back in line 3, I think at this point we agree that we should delete it for messages which are filtered out.

A con of this design is that we spend time finding (and counting) messages that we intend to filter, and could have skipped. Note that we still don't spend time processing-and-storing those messages, so the filter is still valuable for increasing speed.

I hope the pros of this design outweigh the cons. Here are a few pros I see: having true LineNo info, having a count of the total log messages, having the ability to see if the log has lots of undefined/invalid messages (not presently implemented)

What do you think?

[Georacer]

I run a profiler on the 150MB log on your latest version.
The delays are primarily split around line 170
(msg_LineNo = LineNo_vec(ismember(LineNo_ndx_vec, obj.valid_msgheader_cell{i,2}));)
and 197-198.

In both cases we might be able to pull one or two tricks to make it faster.
But for the time being I agree in biting the bullet and going for it. It's orders of magnitude better than what's out there anyway.

[Georacer]

Can you also address my question regarding the need for creating and deleting dynamic props?

[Georacer]

Rebased and force-pushed the last 2 PRs, after merging LineNos

[hunt0r]

With respect to changing temporary properties to become dynamic properties:
My 1st intent was cleanliness, lowering the number of properties in the class. I think this contributes to being easier to understand by others, too.
My 2nd intent was a (probably trivial) reduction in the filesize, if we ever save an Ardupilog to a .mat file.

[hunt0r]

With regards to my proposed design above:
Are you in agreement? If so, I will draft a version of the code which re-organizes around this process in the near future, and pull-request it for review.

[Georacer]

I think this adds a lot of code complexity which isn't good when we'll be going through the code a month later, looking for a property which isn't at the start of the file. In other words, it hurts readability.
This is what the private properties are for: Stuff them and the end user will never see them.
If the code eventually gets so bloated from the amount of properties, it's probably time for refacturing anyway. Plus, we will definitely get a hit in speed, each time we allocate and free a property in real time.

Besides, as you said the reduction is space is probably not an issue.

I actually would prefer that this wouldn't go in. Do you have a serious problem with that?

[hunt0r]

Okay, no problem, that's fine with me. We will keep them as private properties.

Do we still want to overwrite them with trivial values at the end of log-loading?

And this is a new Github experience for me. Do we close the pull request without merging, and also delete the branch? If so, it's probably easiest to delete the dependent PR #42 and re-code that one as a new pull-request according to the discussion over there.

[Georacer]

(Let's keep the overwrites, I think it's a free space bonus)
This PR will be closed, first of all. The branch itself will still be around.
You have 2 choices:

1. You can rebase Complete num msgs #42 onto your master, so as to make it independent from Temporary prop as dynamicprop #41. Its PR will still be active. Then delete the dynamic_prop branch, OR
2. Delete everything and re-commit and re-PR.
   Whatever you like.

[hunt0r]

Excellent. In this case I'm going to opt for the "manual" option over the rebase. I'll close the PR and delete the branch.