Feature/logger no eof

[m3d]

This PR opens possibility to read log from incomplete file. Note, that there was a bug, that cut data were distributed into system (asserts now).

[zbynekwinkler]

How do you envision using this code? The only way I can see right now is on shared filesystem, right? So either the LogReader runs on the same computer as LogWriter or they both share the file using something like NFS. Have I missed something?

[zbynekwinkler]

I see that you feel the need to have a log.read method. Why not add it? Then log.read_gen can be implemented using this method. This stuff with next seems like a hack.

[m3d]

There used to be one and it is needed for tests only.

Edit:
There is actually internal read() function, but maybe I should rename it to _read(self, size).

[zbynekwinkler]

Well if you want to iterate over the file in different places, just create the iterator and repeatedly call next on the same iterator. Don't create new iterator relying on the strange fact that new iterator does not start at the beginning of the file.

i = log.read_gen(only_stream_id=1)
dt, channel, data = next(i)
dt, channel, data = next(i)

[zbynekwinkler]

Ok, I see now that is what you did. Thanks. I would also consider changing read_gen to first seek to the start of the file before returning anything so that this does not ever return.

[zbynekwinkler]

Just a side note only partially related to this PR: I think the tests should be using a temporary directory.

[m3d]

OK - I used to analyze failing tests from available intermediate files, but it is no longer needed. Ack.

[zbynekwinkler]

Autocreating a temporary directory for the test will be left to a different time, right?

[m3d]

Yes please

[m3d]

How do you envision using this code? The only way I can see right now is on shared filesystem, right? So either the LogReader runs on the same computer as LogWriter or they both share the file using something like NFS. Have I missed something?

Well, yes, this is the first step. You read some local file. I was thinking also about URL specifiers like
http://osgar.robotika.cz/subt-go1m-181127_221851.log but at the end you want to have a local copy (optional), so that multiple sources and process the log file (SubT scenario you may run some viewers and map processing in parallel.

[m3d]

Well, I renamed read() to _read() so it is clear that nobody from outside should use it.

I was going to change the read_gen behavior, but ... it is not ready for it:

• if you create two generators the second would reset also the first one
• LogRead reads the header already in constructor and details like time overflow should be reflected (I was going to add self.f.seek(0), but I would not be happy with self.f.seek(4+12)

I would leave it for now (I added test for this strange current behavior), merge it, integrate it with lidarview and then maybe return to it in some later pull request, OK?

[zbynekwinkler]

I would leave it for now (I added test for this strange current behavior), merge it, integrate it with lidarview and then maybe return to it in some later pull request, OK?

👎 Not a good idea IMHO. Adding the test codifying the existence of the behavior violating the principle of least surprise makes it even worse.

If you feel the pressing need to support multiple concurrent iterators over the same log file, the better way to do it is to open the file inside the method read_gen and make it a local variable there.

[m3d]

Well, OK. Maybe we should simply have an iterator ready when LogReader is created and provide __next__ that the log itself could iterate?? If you need two generators then just open the filename with two separate LogReaders ...?

[m3d]

p.s. I realized why I do not want to use seek - if you have read only then it should be simpler to use external source like URL external file

[m3d]

OK, now LogReader is directly iterator - is it good idea or not? If yes then I will fix all tests and usages ...
(now tests are expected to fail)

[m3d]

I fixed unittests, but also all usages have to be replaced ...

[zbynekwinkler]

👍

[zbynekwinkler]

Is there a reason for not using record in launch? It look asymmetric when replay is used but record is not.

[m3d]

Is there a reason for not using record in launch? It look asymmetric when replay is used but record is not.

Well, good point, but I would not handle it is this PR (you maybe wanted to comment #128 ?
Now, when we broke all usages of LogReader I would just sqhash commits, fix unittests and merge.

[zbynekwinkler]

👍

[m3d]

thanks m.