Implement _detect_available_configs for the Ixxat bus. #1607
Conversation
|
|
||
| @staticmethod | ||
| def _detect_available_configs() -> List[AutoDetectedConfig]: | ||
| return vcinpl._detect_available_configs() |
There was a problem hiding this comment.
Without any IXXAT-knowledge: does this detect FD-devices, too? What about vcinpl2?
There was a problem hiding this comment.
That is a good question. I believe that vcinpl.dll and vcinpl2.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 that vcinpl.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.
@fjburgos can you take a look?
There was a problem hiding this comment.
@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 say
There was a problem hiding this comment.
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.
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:
- receive
- can.viewer
- send
- send periodic (using bus.bus.send_periodic workaround for proxy class)
There was a problem hiding this comment.
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.
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 and canlib_vcinpl2.py #1126 from the time when 1119 was merged, and the only significant differences between them are:
- The way in which the predefined bitrates are implemented (1126 stores them as class variables, 1119 stores them in the constants.py file.
- 1119 implements a check of the hardware interfaces capabilities before adding features such as CAN FD to the open method of the VCI driver, whereas 1126 just adds them if the parameter provided to the class demands it. In my opinion the check of the interfaces available functionality is valuable if we are converging on a single implementation.
What are your thoughts?
There was a problem hiding this comment.
I'd suggest to create a new PR. If doesn't go anywhere, we can still merge this.
- I'd prefer the constants file. Regarding the bitrates, it would be nice if support for the BitTiming classes was added, too.
- I agree. Checking the capabilities doesn't hurt.
There was a problem hiding this comment.
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 :)
zariiii9003
force-pushed
the
ixxat_dev
branch
from
00d3904 to
65d9fe0
Compare
* Upate Changelog for Release v.4.3.0 * Update CHANGELOG.md Co-authored-by: zariiii9003 <[email protected]> * Update CHANGELOG.md Co-authored-by: zariiii9003 <[email protected]> * Update CHANGELOG.md Co-authored-by: zariiii9003 <[email protected]> * Update CHANGELOG.md Co-authored-by: zariiii9003 <[email protected]> * Update CHANGELOG.md Co-authored-by: zariiii9003 <[email protected]> * Update CHANGELOG.md Co-authored-by: zariiii9003 <[email protected]> * Make requested changes to CHANGELOG, sort by PR ID * Fix remaining comments * Add PR for Kvaser BitTiming support * add 1679 (bugfix for 1666) * Add PR #1607, change changelog version to 4.3.0rc * Bump project version to 4.3.0rc0 --------- Co-authored-by: zariiii9003 <[email protected]>
Tested on local hardware, although I only have single channel devices so have not been able to validate that the multichannel detection works correctly.
I only realised once writing the pull request that there is some overlap with #1180 in the changes I have made. I have submitted a seperate PR anyway as I believe that the solution in these commits is much more in line with the behaviour of the rest of the library, but please feel free to close if you wish to progress the other PR.