ASCII Reader/Writer enhancements #820

sou1hacker · 2020-04-27T16:46:29Z

Changes

Added support for new formats for CAN remote frames
Reworked and cleaned existing implementation
Some minor fixes (e.g. dlc != len(data) for FD frames)
Added parsing ASCII header parsing

Time analysis for both new & old implementations

We tested with an ASCII logs file with ~ 7 million messages (both classic CAN & FD) across 8 channels.
Old implementation: Took an average of 128 seconds across 3 runs
New implementation: Took an average of 117 seconds across 3 runs

…implementation

can/io/asc.py

Co-Authored-By: pierreluctg <pierreluctg@gmail.com>

codecov · 2020-04-27T17:09:10Z

Codecov Report

Merging #820 into develop will increase coverage by 0.13%.
The diff coverage is 96.80%.

@@             Coverage Diff             @@
##           develop     #820      +/-   ##
===========================================
+ Coverage    69.81%   69.95%   +0.13%     
===========================================
  Files           70       70              
  Lines         6603     6640      +37     
===========================================
+ Hits          4610     4645      +35     
- Misses        1993     1995       +2

can/io/asc.py

Co-Authored-By: pierreluctg <pierreluctg@gmail.com>

…into asc_remote

karlding · 2020-04-29T04:31:04Z

can/io/asc.py

+        self._converted_base = self._check_base(base)
+        self.date = None
+        self.timestamps_format = None
+        self.internal_events_logged = None


This doesn't seem to be used anywhere. It seems to just indicate the presence of the following line:

internal events logged

Can you help me understand why this needs to be exposed?

@karlding The line can be either "internal events logged" or "no internal events logged". It may be useful in the future as we add support for more kinds of frames/events in the AscReader.

karlding · 2020-04-29T04:31:13Z

can/io/asc.py

+                    _, base, _, timestamp_format = line.split()
+                except ValueError:
+                    raise Exception("Unsupported header string format: {}".format(line))
+                self.base = base


The __init__ takes a given base, whereas here right after that we're potentially overwriting it. What happens if the constructor specified hex, but the file is dec?

If you were intending to use this value from the .asc to validate, I don't think you should've overwritten self.base on this line.

@karlding In both the current and previous implementations, we were ignoring the base passed on to the constructor and using the base defined in the ASCII logs file to parse all the messages. It seems accurate to set base to the value actually used during parsing so I think overwriting it is fine.

Hmm, if the idea is for the ASCReader to parse the value from the file, then we should probably just deprecate those parameters in __init__ and raise a DeprecationWarning or something.

I don't have access to Vector's tools so I can't check, but are there cases where an ASC file is considered valid but it doesn't specify a base?

Perhaps the only use case I can think of is when we have a logs file split across two ASCII files. Not completely sure if the header is present in files other than the first file. If the intention is perhaps to read only the second file, then having a manually specified base can be useful. Your call.

Let's just leave this for now then. If someone has a use case for this, we can revisit it at that point.

karlding · 2020-04-29T04:39:00Z

Out of curiosity, do you have a sample file with some of the missing functionality that the current samples don't have? For example, I don't think the current ASC has any examples of error frames (and I think RTR frames? not sure if there's anything else). If so, could you include them?

It would be nice to have specific tests for each type of frame using "golden files" to test parsing 🙂

felixdivo

Looks good to me. I didn't test against an actual file though.

sou1hacker · 2020-04-29T12:37:09Z

@karlding Added some examples for CAN / CANFD error, remote frames, etc to the current ASC logs file.

karlding · 2020-05-02T19:56:18Z

Thanks for adding the additional frames!

I think it might make test-writing easier if we split out the different frames into their own separate files, similar to what exists for the BLF format? That way we can also write tests that check that the parser is working by asserting on the type of frame that is converted to a Message, instead of one giant file (ie. is_remote_frame, is_fd). What do you think?

…tions

sou1hacker · 2020-05-04T19:59:54Z

Thanks for adding the additional frames!

I think it might make test-writing easier if we split out the different frames into their own separate files, similar to what exists for the BLF format? That way we can also write tests that check that the parser is working by asserting on the type of frame that is converted to a Message, instead of one giant file (ie. is_remote_frame, is_fd). What do you think?

Will look at BLF and replicate here.

… asc_remote

karlding

Thanks for adding the extra tests!

Syed Raza added 5 commits April 23, 2020 10:05

Adding header attributes parsing to AscReader

3337cd5

Minor enhancement

4729594

Removing accidentally added import

96617df

Adding remote frame support to ASC and a little clean-up of existing …

11ba10c

…implementation

Merging ASCI header changes into this PR

be95fd3

sou1hacker mentioned this pull request Apr 27, 2020

ASCII header #819

Closed

Black changes

7e01c3d

pierreluctg reviewed Apr 27, 2020

View reviewed changes

can/io/asc.py Outdated Show resolved Hide resolved

Update can/io/asc.py

b89aece

Co-Authored-By: pierreluctg <pierreluctg@gmail.com>

sou1hacker marked this pull request as ready for review April 27, 2020 17:12

Syed Raza added 2 commits April 27, 2020 14:07

Review based changes

713f7de

black changes

f7899fb

sou1hacker requested a review from pierreluctg April 27, 2020 18:21

pierreluctg reviewed Apr 27, 2020

View reviewed changes

sou1hacker and others added 7 commits April 27, 2020 14:44

Update can/io/asc.py

e17cdd4

Co-Authored-By: pierreluctg <pierreluctg@gmail.com>

Update can/io/asc.py

dd7bd34

Co-Authored-By: pierreluctg <pierreluctg@gmail.com>

Update can/io/asc.py

1536e42

Co-Authored-By: pierreluctg <pierreluctg@gmail.com>

Review based changes

cbaedee

Merge branch 'asc_remote' of https://github.com/sou1hacker/python-can …

4bc9b02

…into asc_remote

Removing msg_found bool

4b1dd3e

Adding some comments back

822770d

pierreluctg added enhancement file-io about reading & writing to files labels Apr 28, 2020

pierreluctg requested review from christiansandberg, felixdivo and karlding April 28, 2020 13:53

pierreluctg approved these changes Apr 28, 2020

View reviewed changes

karlding reviewed Apr 29, 2020

View reviewed changes

felixdivo approved these changes Apr 29, 2020

View reviewed changes

Adding some CAN / CANFD remote frames and error frames examples

19c9aa0

felixdivo mentioned this pull request Apr 29, 2020

Add support for base 10 in CANReader #685

Closed

Syed Raza added 4 commits May 4, 2020 08:17

Resolving conflicts with master + adding type annotations to new func…

1193679

…tions

Black changes

ab35948

Pylink typing annotation changes

d57c780

Black changes ...

954f1bc

Syed Raza added 3 commits May 5, 2020 08:49

Merge branch 'develop' of https://github.com/hardbyte/python-can into…

88c4d71

… asc_remote

Adding frame specific tests for AscReader

393b57f

Black changes

0ddba46

karlding approved these changes May 6, 2020

View reviewed changes

Merge branch 'develop' into asc_remote

25fffda

hardbyte merged commit 30ef0c3 into hardbyte:develop May 12, 2020

hardbyte added this to the 4.0 Release milestone May 12, 2020

ASCII Reader/Writer enhancements #820

ASCII Reader/Writer enhancements #820

Uh oh!

Conversation

sou1hacker commented Apr 27, 2020 • edited by pierreluctg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Time analysis for both new & old implementations

Uh oh!

Uh oh!

codecov bot commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

karlding Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

sou1hacker Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

karlding Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

sou1hacker Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

karlding May 2, 2020

Choose a reason for hiding this comment

Uh oh!

sou1hacker May 4, 2020

Choose a reason for hiding this comment

Uh oh!

karlding May 6, 2020

Choose a reason for hiding this comment

Uh oh!

karlding commented Apr 29, 2020

Uh oh!

felixdivo left a comment

Choose a reason for hiding this comment

Uh oh!

sou1hacker commented Apr 29, 2020

Uh oh!

karlding commented May 2, 2020

Uh oh!

sou1hacker commented May 4, 2020

Uh oh!

karlding left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sou1hacker commented Apr 27, 2020 •

edited by pierreluctg

Loading

codecov bot commented Apr 27, 2020 •

edited

Loading