Integrate CANTools Log File Readers about canbus-analyzer HOT 8 OPEN

Why is this needed? Why not just load all at once and then iterate through the Data enumerable (see DataWithColumns) at whatever rate you want?

I'm not saying "no" to the per-line capability (callback or whatever) but it doesn't seem to be required, right?

from canbus-analyzer.

@Bokonon79
Let me know if this (DataColumns.Read) covers what you need:
https://brianman.visualstudio.com/_git/CANTools/commit/2a1bba820dbf207b5594ea92d3e712fe96c120e7?refName=refs%2Fheads%2Fu%2Fbrianman%2FLoadIncremental

I'll push to master but I figured it would be good to have you test it first.

from canbus-analyzer.

Why is this needed? Why not just load all at once and then iterate through the Data enumerable (see DataWithColumns) at whatever rate you want?

I could be prematurely optimizing, but my reasoning was this: longer captures are going to be several hundred MBs or a few GBs in size. If we were to read the entire file into memory all at once, only to pass it to the timerCallback() loop one line at a time, we'd end up consuming ~twice the memory by the end of the load, which could become a problem on a multi-GB file. Since both DataWithColumns and CANBUS-Analyzer's timerCallback() loop are already built around line-by-line iteration, it seemed like a natural solution to connect the two with minimal effort.

Let me know if this (DataColumns.Read) covers what you need:

I could be missing something, but just from looking at the commit, I didn't see where the callback was actually invoked.... Does something like this need to be added to Read() at line 199?

if (callback != null) 
{
    callback(row);
}

I'll push to master but I figured it would be good to have you test it first.

Sounds good. I've added CANTools/u/brianman/LoadIncremental as a submodule on my development branch and will try it out later today (adding the code above if needed).

from canbus-analyzer.

Good catch on the missing callback(row). That pointed out a bug in my unit test. The unit test bug is now fixed in master and u/brianman/LoadIncremental has been rebased onto the new master and had the fix applied. Sorry for the inconvenience.

from canbus-analyzer.

No apologies necessary, thanks for making those changes! I've updated the submodule in my main dev branch and will try wiring it up this weekend.

from canbus-analyzer.

Sorry for the absence... I've got the CANTools log readers integrated in my CANToolsReaders branch, along with a bug fix for #26.

To get the log readers to work, I ended up making three small changes to DataWithColumns in a cloned copy of CANTools/LoadIncremental

1. Extract ReadHeaders() and ReadNext() methods from Read():
   Bokonon79/CANTools@ae7a4fd

2. Add support for multi-space delimited files:
   Bokonon79/CANTools@f32163b

@brian-man, I didn't see a way to submit a pull request to your CANTools repo on Azure DevOps, but if I could, it would contain the above two commits. Could you please have a look at them when you get a chance, and consider merging them into the LoadIncremental branch? Thanks.

from canbus-analyzer.

Initial feedback:

• ReadHeaders doesn't need to be public.
• What's the benefit of adding ReadNext, and why is it public?

Perhaps I'm missing something. Can you point me to your branch's commit in CANBUS-Analyzer the uses CANLib?

from canbus-analyzer.

Also, I don't like multi-space because it's lossy -- which means it breaks, for example, round-trip serialization. What benefit is it giving you?

from canbus-analyzer.