Ixxat CAN FD support

[fjburgos]

Add experimental support for CAN FD in IXXAT device(s).

Tryed and worked with "Ixxat USB-to-CAN FD".

[codecov]

Codecov Report

Merging #1119 (f9161cb) into develop (8ab55e8) will decrease coverage by 1.90%.
The diff coverage is 25.58%.

@@ Coverage Diff @@## develop #1119 +/- ## =========================================== - Coverage 70.83% 68.92% -1.91%  =========================================== Files 79 82 +3 Lines 7758 8266 +508 =========================================== + Hits 5495 5697 +202 - Misses 2263 2569 +306 

[fjburgos]

Please, tell me if any action is required from my side.

[zariiii9003]

Hello @fjburgos, thank you for your contribution.
Could you use the fd parameter of the normal ixxat interface to keep the API consistent? Defining a new separate inteface seems unnecessary.

[fjburgos]

Hello @fjburgos, thank you for your contribution.
Could you use the fd parameter of the normal ixxat interface to keep the API consistent? Defining a new separate inteface seems unnecessary.

I agree, the problem was I have no way to check the previous interface still works, but I think I can try to do it with minimal changes to the original file (can/interfaces/ixxat/canlib.py).

[fjburgos]

I was surprised to see that similar pull request was done 3 days ago (#1126).
I think that the main difference at this point is that in this pull request, the original implementation (based on vcinpl.dll) is being used when fd option is False, which has advantages and disadvantages.
One advantage is that backward-compatibility could be better.
One disadvantage is that some duplicated code exists (due to my approach). A refactor could be possible, but I would not do it without being able to test with a proper device that nothing gets broken.
One thing I don't know is if vcinpl2.dll can handle all IXXAT devices or only those that support CAN-FD. That's why I wanted to leave the original implementation available.
Feel free to choose what you think is better for the project.
@ausserlesh, you are welcome to share you opinion, I think it will be valuable.

[semcodech]

Thanks for your thoughts, @fjburgos, I actually implemented this quite a while ago, and only saw your PR after creating mine. Yes, fully agree with the pros and cons you mentioned. Regarding vcinpl2 it indeed seems that we might consider loading vcinpl instead of vcinpl2 if fd is False (see https://opensource.lely.com/canopen/docs/cross-compilation/), which then leads again to a similar solution like the one in your PR, maybe with a different level of abstraction (hide implementation behind fd flag rather than exposing it as a separate device). Unfortunately I only have a CAN-FD device available for testing.

To conclude, I think your approach is fine, if possible, maybe with hiding behind the fd option, for the sake of consistency with other devices (e.g. Socket-CAN)

[fjburgos]

I did the change in the last commit to use the fd option from interface (single ixxat interface, fd parameter decides which dll is loaded).
Thanks for your comments, this is my first conttribution to an open source project and one of the objectives was to share points of view.

[semcodech]

Just checked your implementation on my setup, can confirm that the communication works as expected. Thank you very much.

[zariiii9003]

@felixdivo can you do the second review?

[marcel-kanter]

@fjburgos Nice work. But the extended argument is not used in the init of vcimpl.py
Please set the default to True, because the old implementation opens the interface with

constants.CAN_OPMODE_EXTENDED

so extended ids are allowed by default.

[felixdivo]

A great set of changes!

[hardbyte]

@fjburgos many thanks for the contribution and for patience with the review process.

Once the changes requested by @felixdivo are addressed this is good to merge.

[fjburgos]

@fjburgos Nice work. But the extended argument is not used in the init of vcimpl.py Please set the default to True, because the old implementation opens the interface with

constants.CAN_OPMODE_EXTENDED

so extended ids are allowed by default.

I changed the default value to True, and also changed in vcimpl to use the parameter.

[fjburgos]

@fjburgos many thanks for the contribution and for patience with the review process.

Once the changes requested by @felixdivo are addressed this is good to merge.

No problem, I'm just worried about conflicts in case other pull requests are merged.

I didn't expect this process to be so long, but as I said, this is my first contribution to an open source project, so, I assume this is normal. In any case, code was improved in each review.

[felixdivo]

I didn't expect this process to be so long, but as I said, this is my first contribution to an open source project, so, I assume this is normal. In any case, code was improved in each review.

Well, this is also to a large part caused by us only responding after a few weeks right in the beginning. Thanks for the effort and patience, all!

And in general: Most PRs are indeed a bit quicker, but mostly because they are smaller and more narrow in their scope (my subjective impression). You, however, tackled a rather large change as your first project. Well done :)

[cowo78]

I've been working on #1141 that shares the same scope (IXXAT support) and found that canlib_vcinpl2.py duplicates some code from canlib_vcinpl.py. I would suggest some refactoring to create a cleaner codebase.

[fjburgos]

I would suggest first to improve testing. I did not refactor to avoid risk of introduce bugs on vcinpl side, I only have an fd device, so, I could not check that the previous functionalitites are still working after refactoring.

[cowo78]

@felixdivo if #1141 looks fine I propose to create one more PR to remove duplicate code and improve the overall architecture. I can work on such PR and improve testing. @fjburgos would you be available to perform some tests with connected hardware?

[felixdivo]

Yes, that sounds reasonable. I'll review #1141 soon 👍

[fjburgos]

I'm available, just say when code is ready and what concrete tests you want me to do.