Skip to content
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

Mf4 io #1289

Merged
merged 53 commits into from Apr 3, 2023
Merged

Mf4 io #1289

merged 53 commits into from Apr 3, 2023

Conversation

driftregion
Copy link
Contributor

@driftregion driftregion commented Apr 7, 2022

I rebased #554

Closes #506

@driftregion driftregion marked this pull request as ready for review April 14, 2022 08:57
@felixdivo
Copy link
Collaborator

mf4-io can probably be deleted, should this be merged.

@codecov
Copy link

codecov bot commented May 26, 2022

Codecov Report

Merging #1289 (e048df9) into develop (4b1acde) will increase coverage by 0.44%.
The diff coverage is 83.55%.

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

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1289      +/-   ##
===========================================
+ Coverage    65.16%   65.60%   +0.44%     
===========================================
  Files           81       82       +1     
  Lines         8853     9074     +221     
===========================================
+ Hits          5769     5953     +184     
- Misses        3084     3121      +37     

@zariiii9003
Copy link
Collaborator

@driftregion can i force push to this PR?
I fixed a few things here. I can open a new PR, too.

@driftregion
Copy link
Contributor Author

@zariiii9003 sure, go ahead.

@zariiii9003
Copy link
Collaborator

@danielhrisca would it be possible to stream to a file directly?
Also: i accessed a private attribute to get the current file size, is there a better way to do that?

@danielhrisca
Copy link
Contributor

@danielhrisca would it be possible to stream to a file directly?

The only way to stream to the file is to use the extend method, but this is performance bottleneck for high data rates.

Also: i accessed a private attribute to get the current file size, is there a better way to do that?
I don't think so. Why do you need that information?

@zariiii9003
Copy link
Collaborator

@danielhrisca
The SizedRotatingLogger starts a new file when a give file size is reached. That's why i added the file_size function to the logger classes.

The BlfWriter compresses and writes data in chunks so maybe that could be possible here too?

setup.py Outdated
@@ -36,6 +36,10 @@
"viewer": [
'windows-curses;platform_system=="Windows" and platform_python_implementation=="CPython"'
],
"mf4": [
'asammdf>=6.0.0;platform_python_implementation=="CPython" and python_version<"3.11"',
'numpy;platform_python_implementation=="CPython" and python_version<"3.11"',
Copy link
Owner

@hardbyte hardbyte Nov 14, 2022

Choose a reason for hiding this comment

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

I'm going to remove the platform and python version requirements unless there is a good reason not to?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think i added this so CI does not fail on PyPy and CPython 3.11. But there's probably a better solution for that. I admit this is ugly.

doc/listeners.rst Outdated Show resolved Hide resolved
"""
Iterator of CAN messages from a MF4 logging file.

The MF4Reader only supports MF4 files that were recorded with python-can.
Copy link
Owner

Choose a reason for hiding this comment

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

Why is that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a limitation of asammdf. See here

@zariiii9003 zariiii9003 merged commit d7d617b into hardbyte:develop Apr 3, 2023
31 checks passed
@zariiii9003
Copy link
Collaborator

@danielhrisca merged after only 3.5 years 💪

@zariiii9003 zariiii9003 mentioned this pull request Apr 3, 2023
@danielhrisca
Copy link
Contributor

It's like a good wine

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.

add MF4 io writer
5 participants