Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ixxat CAN FD support #1119

Merged
merged 24 commits into from
Dec 7, 2021
Merged

Ixxat CAN FD support #1119

merged 24 commits into from
Dec 7, 2021

Conversation

fjburgos
Copy link
Contributor

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

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

@mergify mergify bot requested a review from hardbyte August 18, 2021 17:49
@fjburgos fjburgos changed the title Feature/ixxat fd Ixxat CAN FD support Aug 18, 2021
@codecov
Copy link

codecov bot commented Aug 20, 2021 •
edited
Loading

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
Related to ixxat_fd:
2dfd3cb 
Added: Format with black.
Added: test to improve coverage.
Changed: Default baudrate for data segment is now 2 Mbps.
@fjburgos
Merge branch 'develop' into feature/ixxat_fd
f341058
@fjburgos
Copy link
Contributor Author

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

@zariiii9003
Copy link
Collaborator

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.

@semcodech semcodech mentioned this pull request Sep 10, 2021
Closed
@fjburgos
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

semcodech commented Sep 13, 2021 •
edited
Loading

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@zariiii9003
Copy link
Collaborator

@felixdivo can you do the second review?

@marcel-kanter
Copy link
Contributor

@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
felixdivo requested changes Dec 4, 2021
View reviewed changes
Copy link
Collaborator

@felixdivo felixdivo left a comment

Choose a reason for hiding this comment

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

A great set of changes!

can/interfaces/ixxat/canlib.py Outdated Show resolved Hide resolved
test/test_interface_ixxat_fd.py Outdated Show resolved Hide resolved
test/test_interface_ixxat_fd.py Outdated Show resolved Hide resolved
test/test_interface_ixxat_fd.py Show resolved Hide resolved
@felixdivo felixdivo mentioned this pull request Dec 4, 2021
Open
@hardbyte
Copy link
Owner

hardbyte commented Dec 5, 2021

@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.

@hardbyte hardbyte removed their request for review December 5, 2021 19:28
@mergify mergify bot requested a review from hardbyte December 5, 2021 19:28
@fjburgos
Copy link
Contributor Author

fjburgos commented Dec 6, 2021

@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
Copy link
Contributor Author

fjburgos commented Dec 6, 2021 •
edited
Loading

@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 felixdivo added this to the 4.0.0 Release milestone Dec 7, 2021
@felixdivo
Copy link
Collaborator

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 :)
image

@felixdivo felixdivo merged commit b886203 into hardbyte:develop Dec 7, 2021
@felixdivo felixdivo mentioned this pull request Dec 7, 2021
Merged
cowo78 pushed a commit to cowo78/python-can that referenced this pull request Dec 9, 2021
@fjburgos @zariiii9003
Ixxat CAN FD support (hardbyte#1119)
3933573 
* Added: Ixxat interface copy before start changing for canfd.

* Changed: ixxat_fd adapting for usage with vcinpl2.dll instead of vcinpl.dll.

* Fixed: ixxat fd receive function with timeout = 0 was causing to receive always last frame. Now, if function does not return VCI_OK, no new message is returned.

* Added: ixxat_fd baudrate from user options instead of hardcoded.

* Added: ixxat_fd channel capabilities check.

* Added: Myself to contributors list.
Added: Documentation for ixxat_fd usage.

* Related to ixxat_fd:
Added: Format with black.
Added: test to improve coverage.
Changed: Default baudrate for data segment is now 2 Mbps.

* Changed: renamed files before "merging" ixxat and ixxat_fd interfaces.

* Changed: Merged structures, exceptions and constants from ixxat-fd to ixxat.

* Added: Ixxat interface proxy class delegating to related implementation where required.
Changed: Documentation updated accordingly.

* Changed: Tests for CAN-FD in ixxat updated according to last changes.

* Changed: Suggested changes from pull request review.

* Changed: Suggested changes from pull request review.

* Changed: Type hint, typing.Tuple instead of tuple.

* Changed: TSEG1, TSEG2 and SJW parameters available as optional advanced settings.
Changed: Deprecated parameters UniqueHardwareId, rxFifoSize, txFifoSize (use unique_hardware_id, rx_fifo_size, tx_fifo_size instead).
Changed: Replaced ctypes.c_long return value for ctypes functions returning HRESULT with an alias "hresult_type" that is a ctypes.c_ulong.

* Update doc/interfaces/ixxat.rst

Co-authored-by: zariiii9003 <52598363+zariiii9003@users.noreply.github.com>

* Changed: Documentation.

* Changed: Ixxat extended parameter default from False to True. Now it is also considered in "no fd" implementation.

* Changed: Ixxat parameter unique_hardware_id type hint is now optional.

* Changed: Ixxat doc comment types removed (let Sphinx derive them from the method signature).

* Changed: Updated Ixxat tests to use new argument names.

* Fixed: Missing **kwargs parameter in vcinpl and vcinpl2.

Co-authored-by: zariiii9003 <52598363+zariiii9003@users.noreply.github.com>
@cowo78
Copy link
Collaborator

cowo78 commented Dec 10, 2021

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
Copy link
Contributor Author

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
Copy link
Collaborator

cowo78 commented Dec 16, 2021

@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
Copy link
Collaborator

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

@fjburgos
Copy link
Contributor Author

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

@felixdivo felixdivo mentioned this pull request Feb 4, 2022
Closed
@MattWoodhead MattWoodhead mentioned this pull request May 30, 2023
Merged
@MattWoodhead MattWoodhead mentioned this pull request Jun 24, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcel-kanter marcel-kanter marcel-kanter left review comments

@semcodech semcodech semcodech approved these changes

@felixdivo felixdivo felixdivo approved these changes

@zariiii9003 zariiii9003 zariiii9003 approved these changes

@hardbyte hardbyte Awaiting requested review from hardbyte

Assignees

@fjburgos fjburgos

Labels
backend:ixxat enhancement
Projects
None yet
Milestone
4.0.0 Release
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@fjburgos @zariiii9003 @semcodech @felixdivo @marcel-kanter @hardbyte @cowo78