-
Notifications
You must be signed in to change notification settings - Fork 617
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
canalystii: Replace binary library with python driver #1127
canalystii: Replace binary library with python driver #1127
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1127 +/- ##
===========================================
+ Coverage 70.48% 70.93% +0.44%
===========================================
Files 79 79
Lines 7684 7675 -9
===========================================
+ Hits 5416 5444 +28
+ Misses 2268 2231 -37 |
82843ea
to
a3a5c43
Compare
@hardbyte I added some tests for the canalystii interface (mocking at the driver interface layer), when you have a minute could you please trigger another workflow run? (Alternatively, if you're amenable to it, you could merge my approved smaller PR #1114 and I'll rebase this branch onto it - then I won't be a first-time contributor and workflows should trigger automatically. 🙂.) |
Have squashed your other PR. Appreciated it if if you wouldn't mind rebasing this one |
3587e34
to
9d5a0f0
Compare
@hardbyte Thanks! Have rebased and squashed the commits in this branch, too. |
@hardbyte @felixdivo I know you're both probably busy and that's perfectly understandable, but is there anything else I can do from my side to assist with this PR? |
Hey @projectgus, thanks for bringing this up so politely, really! I kinda slipped away from this project over the last months but I will definitely do a review of this PR within a week, maybe this weekend. Maybe we could also finally manage a 4.0.0 release soon, so you could install your changes via the stable channel of the library too. Basically only #1046 is missing but it was much more work than anticipated. |
@felixdivo Hey, no worries, I know how this can happen. I'm not in a particular rush to get this landed, but I know it's also possible to get in the situation where everyone is waiting on something else to happen and then it slips off the radar. :)
I've pushed a commit here to change the one instance of CanError in canalystii.py to CanTimeoutError, in line with that issue. 👍 |
Hmm, I should have this PR alone. 😆 https://github.com/hardbyte/python-can/runs/3986308723?check_suite_focus=true
This looks like a change in the macos runner image...? EDIT: Fixed here #1143 |
21e2f33
to
e62acdd
Compare
I suppose this is absolutely fine. |
Can't we add #726 to the list of issues fixed by this PR? Overall, a nice piece of code! |
e62acdd
to
2bdfed4
Compare
@felixdivo Thanks for the review! Have made all the suggested changes apart from not raising |
We should be good after this. You also tested the interface locally? |
We don't strictly require typing annotations, but if your want, you are welcome to add them. If not, we can also just merge this. |
Ah, and thank you for completeing #1088 for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me also add my thanks for your patience and work preparing this.
The changes look good to me and unless we hear from anyone who would be put out by the performance hit I agree its okay. If you can take a look at using the BitTiming
class that would help to bring the Bus.__init__
method more inline with other interfaces.
- Project 'canalystii' hosted at https://github.com/projectgus/python-canalystii Compared to previous implementation: - No more binary library, can support MacOS as well as Windows and Linux - Can receive on all enabled channels (previous implementation received on one channel only) - Performance is slightly slower but still easily possible to receive at max message rate on both channels - send timeout is now based on CAN layer send not USB layer send - User configurable software rx message queue size - Adds tests (mocking at the layer of the underlying driver 'canalystii') - Adds type annotations - Replaces Timing0 & Timing1 parameters (for BTR0 & BTR1) with the BitTiming class Closes hardbyte#726
c7bd196
to
69b70db
Compare
@hardbyte @felixdivo Thanks for the thoughtful review comments. I think this is up to date now!
Yes, I've been using my Canalyst-II to do some RE work on a 2019 model car (so lots of CAN buses, lots of CAN traffic). Mostly logging but also some interactions using the isotp package. Also, in the lower level driver layer I have some tests that use a real interface (loopback tests, and also one that captures specific messages sent from an Arduino Due). I did an on-car smoke test just now with the updated patch and everything seems to work as expected. Feel free to tag me in if anyone raises an issue related to this interface in the future, will try to help. |
Replaces the binary ControlCAN.dll/controlcan.so library with a Python implementation of the driver.
Python driver is created by me, found at https://pypi.org/project/canalystii/
Improvements
recv()
would always receive from the first-configured channel).send()
on a busy bus that was also noted in canalystii: Fix transmitting onto a busy bus #1114Performance
Replacing native code with Python code creates some performance impact. On my Linux machine (i7-6500U, approx 5 years old) current 'develop' branch uses 27% CPU to run can.logger at maximum message rate (~7800 messages/sec at 1Mbps bitrate). On this branch, it increases to 38% CPU. Logging two channels simultaneously at maximum rate is not possible on 'develop', but on this branch CPU usage is 44%.
My method to measure this is pretty unscientific:
time timeout 20 python -m can.logger -i canalystii -c 0 -b 1000000 -f spammer.txt
and then use this script to check no messages were dropped in the log file. Sender is this Arduino Sketch.