Skip to content

Fix the compressed BLFWriter #1379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 9 commits into from
Closed

Conversation

j-c-cook
Copy link
Contributor

closes #1378

@codecov
Copy link

codecov bot commented Aug 22, 2022

Codecov Report

Merging #1379 (876dacb) into develop (2da28c1) will increase coverage by 0.01%.
The diff coverage is 90.90%.

❗ Current head 876dacb differs from pull request most recent head f64b6e7. Consider uploading reports for the commit f64b6e7 to get more accurate results

@@             Coverage Diff             @@
##           develop    #1379      +/-   ##
===========================================
+ Coverage    66.21%   66.22%   +0.01%     
===========================================
  Files           86       86              
  Lines         9021     9027       +6     
===========================================
+ Hits          5973     5978       +5     
- Misses        3048     3049       +1     

can/io/blf.py Outdated
Comment on lines 587 to 590
filesize = self.file.tell()
# Write header in the beginning of the file
self.file.seek(0)
self._write_header(filesize)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zariiii9003 The call self.file.seek(0) results in an OSError. It appears that the code block is not required if write mode wb is used (rather than ab). However, if the append option is requested, then some of this idea may be required.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The header contains things that are only known after the log has been written, so that's why we need to go back and write it later. Previously we closed and reopened the file to update it, but it seemed unnecessary if it was possible to seek instead.

@j-c-cook
Copy link
Contributor Author

j-c-cook commented Aug 22, 2022

Remaining tasks:

  • If the append option is requested, then open the compressed Writer with append mode
  • Get the compressed BLFWriter functional with the append option

can/io/blf.py Outdated
Comment on lines 587 to 592
filesize = (
self.file.tell()
) # == self.file.seek(0, 1) if type == gzip.GzipFile
if type(self.file) != gzip.GzipFile:
# Write header in the beginning of the file
self.file.seek(0)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the type of self.file is a gzip.GzipFile, then the self.file.tell() call appears to enter gzip.seek(0, 1). Does it need to be called twice?

@j-c-cook
Copy link
Contributor Author

The current changes make it so that blf.gz files are written with a complete header. The solution is not long term, because it simply ensures that nothing is written until the rollover occurs.

import can
from datetime import datetime


def u_to_dt(ts):
    return datetime.utcfromtimestamp(ts).strftime("%Y-%m-%d %H:%M:%S")


def main():
    file_name = "private_2022-08-24T084948_#000.blf.gz"

    with can.LogReader(file_name) as reader:
        for m in reader:
            print(m.timestamp)

    reader = can.LogReader(file_name)
    print(
        f"start: {u_to_dt(reader.start_timestamp)} "
        f"stop: {u_to_dt(reader.stop_timestamp)}"
    )


if __name__ == "__main__":
    main()

@j-c-cook
Copy link
Contributor Author

I have modified the description of issue #1378 and this PR now contains changes that are out of scope. A combination of #1383, #1384 and #1385 will cover the material previously intended to be addressed in this PR.

@j-c-cook j-c-cook closed this Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GzipFile BLFWriter negative seek error
2 participants