-
Notifications
You must be signed in to change notification settings - Fork 604
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
Implement _detect_available_configs for the Ixxat bus. #1607
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
3dfa261
Add _detect_available_configs to ixxat bus
MattWoodhead 2c60427
Add typing and cover CI test failure
MattWoodhead 5e1aac3
Format code with black
MattWoodhead 01ca3f4
Format code with black
MattWoodhead 0af7543
re-order imports for ruff
MattWoodhead c96a8c8
Update ixxat docs
MattWoodhead 0fbd867
fix doctest
MattWoodhead 65bde81
Update test_interface_ixxat.py
MattWoodhead 65d9fe0
make ruff happy
zariiii9003 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Without any IXXAT-knowledge: does this detect FD-devices, too? What about
vcinpl2
?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.
That is a good question. I believe that
vcinpl.dll
andvcinpl2.dll
are just the CAN LS/HS driver and CAN FD driver respectively, and that these are both components of the VCI4 package (so I think that both APIs should work for the CAN FD capable devices, but I am not 100% certain).Based on the implementation of
get_ixxat_hwids()
in the__init__.py
, I would expect thatvcinpl.dll
does work for the FD devices, and also has the advantage that it is compatible with the older VCI3 drivers (pre-FD) - I think this is why #1119 was chosen in favour of #1126. Unfortunately I dont have the FD capable hardware to test the theory.If you would prefer, I can implement the function in the vcinpl2 file too and default to that implementation, falling back to to the vcinpl definition if
vcinpl2.dll
is not available?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.
@fjburgos can you take a look?
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.
@MattWoodhead Regarding #1119 and #1126, with hindsight i think that was the wrong choice. The current solution with two IXXATBus implementations and that proxy class has lead to many bug reports.
Isn't
vcinpl2.dll
backwards compatible to older devices and classical CAN 2.0? The docs sayThere 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.
The bug canopen #296 in the canopen library apears to be related, and is one that is currently causing me a headache :)
Yes, I am 90% sure that the
vcinpl2.dll
driver is backwards compatible with all of the older ixxat devices, it just wont be present on any systems that have the VCI3 or earlier drivers installed. I can modify my local branch and test with a non-FD USB-to-CAN Compact V2 to confirm tomorrow.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.
Good morning @zariiii9003.
I have tested this modified branch with the non-FD ixxat interface onto a bus with another non-ixxat interface and it all worked as expected.
Tests:
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.
That sounds promising. Would you like to reimplement the IXXATBus based on vcinpl2 without a proxy class? Maybe we could build upon #1126
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.
Hi @zariiii9003. Yes I am happy to give that a go. Do you want me to do it in this PR?
I have compared
canlib.py
from #1119 andcanlib_vcinpl2.py
#1126 from the time when 1119 was merged, and the only significant differences between them are:What are your thoughts?
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.
I'd suggest to create a new PR. If doesn't go anywhere, we can still merge this.
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.
Just for information Ixxat PCI_CAN interface is not compatible with VCI4. I know this is an old devicebut please dont forget user that use legacy board :)