Skip to content

Add Parameter for Enabling PCAN Auto Bus-Off Reset #1345

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

Merged
merged 3 commits into from
Aug 12, 2022

Conversation

1atabey1
Copy link
Contributor

@1atabey1 1atabey1 commented Jul 8, 2022

Allows enabling of automatic hardware reinitialisation in case of a bus off event by adding the parameter auto_reset (defaults to False in order to not change current default behaviour)

@codecov
Copy link

codecov bot commented Jul 10, 2022

Codecov Report

Merging #1345 (2ba1a10) into develop (c4f0789) will decrease coverage by 0.12%.
The diff coverage is 100.00%.

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

@@             Coverage Diff             @@
##           develop    #1345      +/-   ##
===========================================
- Coverage    66.18%   66.06%   -0.13%     
===========================================
  Files           86       86              
  Lines         9005     8964      -41     
===========================================
- Hits          5960     5922      -38     
+ Misses        3045     3042       -3     

@1atabey1
Copy link
Contributor Author

Hmm looks like some tests are failing for the UdpMulticastBus and to be very honest I have no clue how that bus works and how I could have broken anything there... Can someone with a bit more experience have a brief look and help a newbie out? :)

(I noticed that @felixdivo has done a lot of work on the virtual UDP bus, do you have any idea what's going wrong here?)

@felixdivo
Copy link
Collaborator

Hm. No, I don't really have a clue. It wasn't an issue back when I developed and merged it. Perhaps, the CI platform changed. This has happened before with regards to changing IP(v6) support.

I'm not available to investigate this any further, at the moment at least. Sorry.

@1atabey1
Copy link
Contributor Author

Okay no worries, I'll see if I can make some sense out of it!

@lumagi
Copy link
Collaborator

lumagi commented Jul 24, 2022

Hey @1atabey1, @felixdivo
I have the same failure reports on my pull request. I don't believe it has anything to do with the pull requests because for my second commit the test passed just fine. I rather think it is a race condition in the test runner that only fails badly part of the time.

I did some investigation, the failing tests all do the following:

  1. Send out CAN message on bus 1 with arbitration ID i
  2. Receive message on bus 2 and assert that it is equal to the original message
  3. Clear out the reception queue of bus 1 by receiving one message non-blocking. This operation is supposed to clear out the datagram that is looped back to the sending socket during step 1
  4. Increment arbitration ID of the original message to i + 1 and send it out on bus 2
  5. Receive message on bus 1 and assert that it is equal to the original message with incremented arbitration ID i + 1

All tests fail because the arbitration ID of the message received during step 5 is still equal to i and not i + 1. The only reason I can think of is that the looped back datagram is not yet available in the reception buffer of bus 1 in step 3 and hence not cleared out. Then, during step 5 of the test, the code reads the message with arbitration ID i instead of i + 1 and fails.

Unfortunately, I am unable to reproduce this bug locally. I will try to create a new pull request to debug this further.

@@ -259,6 +264,14 @@ def __init__(
"Ignoring error. PCAN_ALLOW_ERROR_FRAMES is still unsupported by OSX Library PCANUSB v0.10"
)

if kwargs.get("auto_reset", False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add the parameter to the __init__() method instead of using kwargs.get?
I do not know why some of the other parameters are not __init__-arguments but i thinks that's bad for readability.

@zariiii9003 zariiii9003 merged commit ed124c9 into hardbyte:develop Aug 12, 2022
@1atabey1
Copy link
Contributor Author

1atabey1 commented Sep 1, 2022

sorry, was too busy last month unfortunately.. Thank you two for finishing this up @zariiii9003 @lumagi !

@1atabey1 1atabey1 deleted the pcan-auto-reset branch September 1, 2022 11:29
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.

4 participants