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

Trc file support #1217

Merged
merged 139 commits into from Nov 15, 2022
Merged

Trc file support #1217

merged 139 commits into from Nov 15, 2022

Conversation

pkess
Copy link
Contributor

@pkess pkess commented Jan 15, 2022

With this i added basic support for the TRC file format as logging for can messages. There is only a very basic support at the moment but comments are welcome on what file versions might be relevant and if the basic code looks good

hardbyte and others added 30 commits February 26, 2019 18:31
Update version to 3.1.1 and record in changelog
Add notes to changelog.
In `PcanBus.send()`, we initially set `msgType` based on all the flags of `msg`, including RTR. In the if/else for `self.fd`, we are incorrectly resetting it if rtr. We should not, and so we are no longer doing it.
In `PcanBus.send()`, we initially set `msgType` based on all the flags of `msg`, including RTR. In the if/else for `self.fd`, we are incorrectly resetting it if rtr. We should not, and so we are no longer doing it.
Expose options to send or skip error frame messages
Currently bus.send_periodic blindly wraps the BCM socket API, and sends
a BCM operation with the TX_SETUP opcode. However, the semantics that
the kernel driver provides are an "upsert". As such, when one attempts
to create two Tasks with the same CAN Arbitration ID, it ends up
silently modifying the periodic messages, which is contrary to what our
API implies.

This fixes the problem by first sending a TX_READ opcode with the CAN
Arbitration ID that we wish to create. The kernel driver will return
-EINVAL if a periodic transmission with the given Arbitration ID does
not exist. If this is the case, then we can continue creating the Task,
otherwise we error and notify the user.

hardbyte#605
Pass kwargs through to BusABC's initializer.

Doesn't backport changing baud to bitrate to avoid changing the API
* pin versions of testing dependencies including coverage
In Python 2, the str type was used for text and bytes, whereas in Python
3, these are separate and incompatible types. This broke instantiation
of a VectorBus when the app_name parameter in __init__ was set to None.

This correctly sets it to a bytes object.

Fixes hardbyte#796
@pkess
Copy link
Contributor Author

pkess commented Jan 31, 2022

Hello, i updated the writer now to support text io and enabled the unittests for this again. Now i would really like to implement some class and subclass procedure for the support of multiple versions, but i am not really sure how to do this. The class type has to be defined before opening the file. The actual file version can only be determined by the content of the file. So maybe i have to add an extra "worker" class where the exact subclass will be used. Or do you have any other idea on this?

@felixdivo felixdivo added this to the 4.1.0 milestone Feb 9, 2022
@felixdivo
Copy link
Collaborator

Version 4.0.0 of python-can is out and since no major problems were brought up, I think new features like this can be merged.

@hardbyte
Copy link
Owner

@zariiii9003 up to you if you'd like to merge this now to include in 4.1 release.

@felixdivo
Copy link
Collaborator

I think we are a bit slow with merging new large features. There are many open PRs for new interfaces that we did not react to. Maybe we should actively find someone who is willing to review these larger PRs in particular.

@hardbyte
Copy link
Owner

Oh totally agree that the volunteers including myself are not keeping up. Any ideas how to recruit people to carry out code reviews though? I have thought about approaching some of the very large companies using python-can to contribute time or money. I wouldn't feel right taking donated money myself for this project (I'm barely active enough to call myself a maintainer!), but I also don't have the time to interview (and likely supervise) someone.

@felixdivo
Copy link
Collaborator

felixdivo commented Nov 14, 2022

Yeah, It's the same with me. Some occasional comments here and there are currently fine, but not proper maintenance.

Maybe we can do the low-effort think and pin an issue stating that we are looking for people to help? Maybe ask for people to do something like peer-review?

Orthogonal to that, it might be a good idea to reconsider which interfaces (+ file types) we want to support? Do we want to add any interface even if only 5 people will ever use it? Or do we want some relevance of the interface? While it is appealing to "catch them all 🎶", it does add a lot of

  1. review and development burden on all, and
  2. some significant maintenance effort, especially when changing base implementations, helper classes, adding typing, changes to the documentation, ... and makes the code base more heterogeneous each time.

@felixdivo
Copy link
Collaborator

@pkess This is not to say that your file type is irrelevant! This discussion just happens to take place here. ;)

@zariiii9003
Copy link
Collaborator

I'm not opposed to merging this if @pkess is available to fix bug reports and complaints. However, this should be mentioned in the docs.

@zariiii9003
Copy link
Collaborator

Orthogonal to that, it might be a good idea to reconsider which interfaces (+ file types) we want to support? Do we want to add any interface even if only 5 people will ever use it? Or do we want some relevance of the interface?

We should start telling people to use the plugin interface/entry point. If we merge all the interfaces, then this lib will become an unmaintainable mess. We'll never be able to make API changes if we have to update 30 interfaces without access to the hardware.

@felixdivo
Copy link
Collaborator

Let's continue the discussion in #1431

@pkess
Copy link
Contributor Author

pkess commented Nov 14, 2022

Hello all,
Thank you for this reminder. I was not able to work on this for a longer time now.
From my personal point of view the trc file format is the most relevant in connection with agricultural machines (this is my normal job). So I would really like to get this merged. Otherwise it might also be good to add this as plugin. But until now I did not know that there is some plug in architecture with this package. Can you point some information for this?

@felixdivo
Copy link
Collaborator

felixdivo commented Nov 14, 2022

Well, the discussion above (about plugins) is generally less about the file I/O like this one but more about what we call "interfaces" (connections to actual buses like socketcan, vector, pcan, ...). This certainly was the wrong place to kick it off. 😁 It is still good to motivate this PR tough!

We could also extend the discussion to file I/O, but there I see less of a problem. These code parts are most often simpler (less concurrency, no linking to binary code) and most of the time easy to test. It is therefore easier to maintain.

I'll comment on that in the linked issue.

@zariiii9003
Copy link
Collaborator

Just for completeness, we do have entry points for io, but the documentation is lacking there: #783

@pkess
Copy link
Contributor Author

pkess commented Nov 14, 2022

So if you aggree i will continue to work on this. Maybe for the very first support of trc files you can leave comment to this code so i can fix them to all relevant requirements?

@j-c-cook
Copy link
Contributor

From my personal point of view the trc file format is the most relevant in connection with agricultural machines (this is my normal job).

Interesting. Vector is the dominant application used for opening CAN data logs by my colleagues (agriculture as well). Therefore, I typically utilize the blf format. This is not to discount your comment, I just think your perspective is interesting.

@pkess
Copy link
Contributor Author

pkess commented Nov 15, 2022

From my personal point of view the trc file format is the most relevant in connection with agricultural machines (this is my normal job).

Interesting. Vector is the dominant application used for opening CAN data logs by my colleagues (agriculture as well). Therefore, I typically utilize the blf format. This is not to discount your comment, I just think your perspective is interesting.

Yes, vector is the other big player in this environment. Peak with trc files is some kind of low cost solution and vector is the premium solution. I don't know exactly all companies but I know companies using only peak I know some only vector and I know some using both

@hardbyte
Copy link
Owner

I'm going to merge this and cut a pre-release of 4.1.0

@hardbyte
Copy link
Owner

Eek actually I might rebase and check the tests pass first.

@hardbyte hardbyte merged commit 2d6e996 into hardbyte:develop Nov 15, 2022
@felixdivo
Copy link
Collaborator

@pkess Maybe to get things clear: The discussion happened on this PR rather randomly, don't take it personal. Thank you for this contribution! 🙂

@j-c-cook
Copy link
Contributor

Yes, vector is the other big player in this environment. Peak with trc files is some kind of low cost solution and vector is the premium solution. I don't know exactly all companies but I know companies using only peak I know some only vector and I know some using both

Okay, I did not know that Peak had a CAN data log format. I use a PCAN-USB device with socketcan as the interface as I spend most of my time on GNU/Linux. I agree that Peak is low cost when compared to Vector. Thank you for your contribution.

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.

None yet