Add `protocol` property to BusABC to determine active CAN Protocol #1532

lumagi · 2023-03-05T09:30:48Z

Something that I have been missing in the past about the generic BusABC is to introspect CAN-FD support independent of the bus implementation. The way I would imagine this is to simply have an attribute called is_fd or something similar that will return True if the bus supports the CAN-FD protocol.

As an example, I have implemented this for the PCAN bus to gather some feedback. If this gets support, I will implement it for all interfaces.

zariiii9003 · 2023-03-05T22:37:16Z

Just to clarify: you say

return True if the bus supports the CAN-FD protocol

but you actually just want to know whether CAN FD is currently active, right?

Generally i don't mind standardizing this for all interfaces, but with CAN XL coming soon it might make sense to use an enum instead of a boolean flag. Otherwise we would need to add is_xl soon.

lumagi · 2023-03-06T13:12:46Z

Sure, I can implement this as an enum. Let me know what you want it to look like, something along the lines:

CAN_ISO_11898
CAN_FD
CAN_XL

zariiii9003 · 2023-03-06T21:27:41Z

CAN FD is already part of ISO 11898 and CAN XL will also be part of 11898. So instead of CAN_ISO_11898 i'd suggest CAN_20 or something like that. Do you have any ideas for the class name? CanSpec? Protocol? PhysicalLayer?

@hardbyte not sure how to contact you but i recommend @lumagi as maintainer ;)

hardbyte · 2023-03-07T19:09:58Z

@lumagi - Lukas thanks so much for your contributions so far. As suggested I've invited you to the repository as a collaborator. Use your new powers for good! In essence, please be sensible and request reviews for non-trivial changes 👍

Great idea @zariiii9003!

lumagi · 2023-03-07T19:31:33Z

@hardbyte Thank you for the invitation, I appreciate it! And thanks for the recommendation @zariiii9003 ;)

lumagi · 2023-03-07T20:09:59Z

Regarding the naming of the enum, I think I like Protocol. But I would like make it a bit more specific like CANProtocol, otherwise this might clash with a different symbol when importing it into the module scope.

Edit: Maybe BusProtocol would fit better with the naming.

zariiii9003 · 2023-03-07T22:38:24Z

Here is an interesting sentence

The third generation of the CAN data link layer supports all three protocol types (Classical CAN, CAN FD, and CAN XL)

So it is either "DataLinkLayer" or "Protocol". I'm not sure which one is technically correct in our case. I prefer CanProtocol over BusProtocol btw if we take the protocol option.

lumagi · 2023-03-10T21:39:02Z

Ok, to me either one would be fine: CANLinkLayer, CANProtocol. But I wouldn't want to make the names too long.

lumagi · 2023-03-12T12:16:42Z

can/interfaces/udp_multicast/bus.py

-        super().__init__(channel, **kwargs)
+        super().__init__(
+            channel, **kwargs, protocol=CANProtocol.CAN_FD if fd else CANProtocol.CAN_20
+        )

        self.is_fd = fd


@zariiii9003 Would you consider this is_fd flag as a public attribute that would lead to a breaking API change if it were removed? With the protocol field it's technically redundant and could be removed.

I guess we could replace it with a property and raise a DeprecationWarning with a removal scheduled for version 5.

zariiii9003 · 2023-03-12T16:43:15Z

Ok, to me either one would be fine: CANLinkLayer, CANProtocol. But I wouldn't want to make the names too long.

I just looked through the rest of the code, can you rename it to CanProtocol? That's more in line with the rest of our code:

lumagi · 2023-03-12T20:26:31Z

Done

The property is scheduled for removal in v5.0

The property is superseded by BusABC.protocol and scheduled for removal in version 5.0.

lumagi · 2023-04-01T16:12:09Z

Yes sure, I don't mind. I only have a couple Bus implementations left, so this can probably change state away from draft soon. But I imagine there's gonna be some feedback :D

# Conflicts: # can/__init__.py # can/interfaces/canalystii.py # can/interfaces/cantact.py # can/interfaces/etas/__init__.py # can/interfaces/gs_usb.py # can/interfaces/ics_neovi/neovi_bus.py # can/interfaces/iscan.py # can/interfaces/ixxat/canlib_vcinpl.py # can/interfaces/ixxat/canlib_vcinpl2.py # can/interfaces/kvaser/canlib.py # can/interfaces/neousys/neousys.py # can/interfaces/nican.py # can/interfaces/nixnet.py # can/interfaces/pcan/pcan.py # can/interfaces/robotell.py # can/interfaces/serial/serial_can.py # can/interfaces/slcan.py # can/interfaces/systec/ucanbus.py # can/interfaces/udp_multicast/bus.py # can/interfaces/usb2can/usb2canInterface.py # can/interfaces/vector/canlib.py # test/serial_test.py # test/test_interface_canalystii.py

zariiii9003 · 2023-04-01T22:00:43Z

I resolved all the merge conflicts from #1551. I hope i didn't break anything.

lumagi · 2023-04-02T17:20:13Z

Thanks for resolving the conflicts! And also the rogue black test - It was not able to come to terms with my local black version.

zariiii9003 · 2023-04-02T21:46:13Z

Thanks for resolving the conflicts! And also the rogue black test - It was not able to come to terms with my local black version.

You could install the correct versions with pip install -r requirements-lint.txt

lumagi · 2023-04-13T06:26:23Z

@zariiii9003 I think I'm through with all the implementations, if you would like to take a look.

can/bus.py

The attribute is now set as class attribute with default value and can be overridden in the subclass constructor.

zariiii9003

Looks great! The enum comparisons should be fixed though 😄

can/interfaces/pcan/pcan.py

can/interfaces/udp_multicast/bus.py

can/interfaces/vector/canlib.py

Fix property access and enum comparison Co-authored-by: zariiii9003 <[email protected]>

lumagi · 2023-05-12T05:42:17Z

Great, thanks for the review! How many reviewers do you normally have for greater changes like this one?

zariiii9003 · 2023-05-13T23:07:16Z

Great, thanks for the review! How many reviewers do you normally have for greater changes like this one?

At some point 2 reviews were required. But we had more active maintainers at that point. I will merge this after a 4.2.1 bugfix release to avoid backporting headaches.

lumagi · 2023-10-12T08:41:11Z

@zariiii9003 When do you think this will make it into a release? Will this have to wait until a major release or will this go into the next minor? To be honest, I am quite looking forward to having this feature available :D

zariiii9003 · 2023-10-12T09:11:18Z

@lumagi I was hoping to include #1611. But i could release 4.3 anytime. I'll try to get it ready by the end of the week.
You could accelerate it by updating the changelog to include all relevant changes since 4.2.2 😄

lumagi · 2023-10-12T13:22:01Z

Sure, I can take care of that. I'll probably get around to doing that over the weekend.

Implement is_fd property for BusABC and PCANBus

b2ec42c

lumagi added 3 commits March 10, 2023 22:39

Implement enum to represent CAN protocol

059f33a

Implement CANProtocol for VirtualBus

07defb0

Implement CANProtocol for UDPMulticastBus

26fe9d4

lumagi force-pushed the feature/bus_fd_field branch from 14cba14 to 26fe9d4 Compare March 12, 2023 12:13

lumagi commented Mar 12, 2023

View reviewed changes

lumagi added 2 commits March 12, 2023 13:22

Implement CANProtocol for the CANalystIIBus

091572b

Implement CANProtocol for the slcanBus

56e40d2

Rename CANProtocol to CanProtocol

642e711

lumagi added 8 commits March 19, 2023 11:37

Reimplement PcanBus.fd attribute as read-only property

c6da1c4

The property is scheduled for removal in v5.0

Reimplement UdpMulticastBus.is_fd attribute as read-only property

da89d35

The property is superseded by BusABC.protocol and scheduled for removal in version 5.0.

Implement CanProtocol for robotellBus

51301fe

Implement CanProtocol for NicanBus

55e349a

Implement CanProtocol for IscanBus

76465e4

Implement CanProtocol for CantactBus

cdbfa13

Fix sphinx reference to CanProtocol

9ad3dae

Implement CanProtocol for GsUsbBus

652d5cc

lumagi force-pushed the feature/bus_fd_field branch from 9528af4 to 652d5cc Compare March 19, 2023 11:08

lumagi added 2 commits March 19, 2023 12:19

Implement CanProtocol for NiXNETcanBus

a20af38

Implement CanProtocol for EtasBus

d6fabc6

Implement CanProtocol for SocketcanBus

6b53ef4

lumagi force-pushed the feature/bus_fd_field branch from 40f9ea8 to 6b53ef4 Compare April 2, 2023 17:53

lumagi added 2 commits April 4, 2023 20:57

Permit passthrough of protocol field for SocketCanDaemonBus

5ac9814

Implement CanProtocol for SeeedBus

6914cba

lumagi force-pushed the feature/bus_fd_field branch from a290a46 to 6914cba Compare April 4, 2023 19:16

lumagi marked this pull request as ready for review April 13, 2023 06:25

lumagi requested a review from zariiii9003 April 13, 2023 06:25

Merge branch 'develop' into feature/bus_fd_field

03504b4

zariiii9003 reviewed Apr 16, 2023

View reviewed changes

can/bus.py Outdated Show resolved Hide resolved

Remove CanProtocol attribute from BusABC constructor

4fe9aa9

The attribute is now set as class attribute with default value and can be overridden in the subclass constructor.

zariiii9003 approved these changes May 11, 2023

View reviewed changes

lumagi and others added 3 commits May 12, 2023 07:17

Apply suggestions from code review

7541cd5

Fix property access and enum comparison Co-authored-by: zariiii9003 <[email protected]>

Fix syntax error

0500799

Fix more enum comparisons against BusABC.protocol

3047263

zariiii9003 changed the title ~~Add FD property to BusABC to determine CAN-FD support~~ Add protocol property to BusABC to determine active CAN Protocol May 15, 2023

zariiii9003 merged commit 09213b1 into hardbyte:develop May 15, 2023

This was referenced Jun 30, 2023

remove python-can >=v4.0.0 as dependency christiansandberg/canopen#318

Closed

Check compatibility with python-can 4.0.0 christiansandberg/canopen#296

Closed

zariiii9003 mentioned this pull request Dec 5, 2023

In EtasBus _apply_filters, self._oci_filters is not defined when EtasBus init called with version 4.3.0 #1693

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `protocol` property to BusABC to determine active CAN Protocol #1532

Add `protocol` property to BusABC to determine active CAN Protocol #1532

lumagi commented Mar 5, 2023 •

edited

Loading

zariiii9003 commented Mar 5, 2023

lumagi commented Mar 6, 2023

zariiii9003 commented Mar 6, 2023

hardbyte commented Mar 7, 2023

lumagi commented Mar 7, 2023

lumagi commented Mar 7, 2023 •

edited

Loading

zariiii9003 commented Mar 7, 2023

lumagi commented Mar 10, 2023 •

edited

Loading

lumagi Mar 12, 2023

zariiii9003 Mar 12, 2023

zariiii9003 commented Mar 12, 2023

lumagi commented Mar 12, 2023

lumagi commented Apr 1, 2023

zariiii9003 commented Apr 1, 2023

lumagi commented Apr 2, 2023

zariiii9003 commented Apr 2, 2023

lumagi commented Apr 13, 2023

zariiii9003 left a comment

lumagi commented May 12, 2023

zariiii9003 commented May 13, 2023

lumagi commented Oct 12, 2023

zariiii9003 commented Oct 12, 2023

lumagi commented Oct 12, 2023

Add protocol property to BusABC to determine active CAN Protocol #1532

Add protocol property to BusABC to determine active CAN Protocol #1532

Conversation

lumagi commented Mar 5, 2023 • edited Loading

zariiii9003 commented Mar 5, 2023

lumagi commented Mar 6, 2023

zariiii9003 commented Mar 6, 2023

hardbyte commented Mar 7, 2023

lumagi commented Mar 7, 2023

lumagi commented Mar 7, 2023 • edited Loading

zariiii9003 commented Mar 7, 2023

lumagi commented Mar 10, 2023 • edited Loading

lumagi Mar 12, 2023

Choose a reason for hiding this comment

zariiii9003 Mar 12, 2023

Choose a reason for hiding this comment

zariiii9003 commented Mar 12, 2023

lumagi commented Mar 12, 2023

lumagi commented Apr 1, 2023

zariiii9003 commented Apr 1, 2023

lumagi commented Apr 2, 2023

zariiii9003 commented Apr 2, 2023

lumagi commented Apr 13, 2023

zariiii9003 left a comment

Choose a reason for hiding this comment

lumagi commented May 12, 2023

zariiii9003 commented May 13, 2023

lumagi commented Oct 12, 2023

zariiii9003 commented Oct 12, 2023

lumagi commented Oct 12, 2023

Add `protocol` property to BusABC to determine active CAN Protocol #1532

Add `protocol` property to BusABC to determine active CAN Protocol #1532

lumagi commented Mar 5, 2023 •

edited

Loading

lumagi commented Mar 7, 2023 •

edited

Loading

lumagi commented Mar 10, 2023 •

edited

Loading