Software filtering for all Buses by felixdivo · Pull Request #277 · hardbyte/python-can

felixdivo · 2018-03-06T22:35:08Z

This basically implements #157 (with some small changes).

The BusABC class is now finished, and I want to ask whether that is looking good. If it is, I will adjust the other interfaces one by one to make it work with this new frontend. It shouldn't be too much work though. It would be nice if someone could comment on the docstrings / the semantics of the methods.

Well, and the tests will obviously fail for now.

I also did some refactoring in interface.py so it will be quite easy to add support for #51 later.

Closes #157. Closes #104.

…tributes)

christiansandberg

Try not to change unrelated lines in a pull request. Makes it more difficult to review.

christiansandberg · 2018-03-07T18:28:49Z

can/bus.py

+        * :meth:`~can.BusABC.shutdown` to override how the bus should
+          shut down, in which case the class has to either call through
+          `super().shutdown()` or call :meth:`~can.BusABC.flush_tx_buffer`
+          on its own


Flushing buffer is usually unnecessary.

See below on the discussion of def shutdown(): ....

christiansandberg · 2018-03-07T18:36:24Z

can/bus.py

+            # try next one only if there still is time, and with reduced timeout
+            else:
+
+                timeout = timeout - (time() - start)


Is this even correct? Maybe something like time_left = timeout - (time() - start) and use time_left in the _recv_internal call. Then you also need to set time_left = timeout in the beginning.

Yes, you are right of course.

christiansandberg · 2018-03-07T18:39:28Z

can/bus.py


        :return: A started task instance
-        :rtype: can.CyclicSendTaskABC
+        :rtype: :class:`can.CyclicSendTaskABC`


Don't think you need to change this. See Sphinx docs.

christiansandberg · 2018-03-07T18:44:34Z

can/bus.py

        """
-        raise NotImplementedError("Trying to set_filters on unsupported bus")
+
+        # TODO: add unit testing for this method


Also if filters is empty then this should always return True.

christiansandberg · 2018-03-07T18:47:08Z

can/bus.py

+            # then check for the mask and id
+            can_id = can_filter['can_id']
+            can_mask = can_filter['can_mask']
+            if msg.arbitration_id & can_mask == can_id & can_mask:


It could be faster to use (can_id ^ msg.arbitration_id) & can_mask == 0 but is harder to read.

I will replace it and leave a comment explaining it.

christiansandberg · 2018-03-07T18:48:50Z

can/bus.py

        """
        Called to carry out any interface specific cleanup required
-        in shutting down a bus.
+        in shutting down a bus. Calls :meth:`~can.BusABC.flush_tx_buffer`.


I don't know the reason for this. Maybe remove it. Is there any interface using it?

Well, ixxat, kavaser, nican and vector all implement flush_tx_buffer(). But none of the interfaces calls the method, at least none of the internal ones. I propose we remove the call to flush_tx_buffer() from shutdown() and remove the comment I added.

See 6a4aa5b.

christiansandberg · 2018-03-07T18:54:34Z

can/bus.py

+        """
+        Read a message from the bus and tell whether it was filtered.
+
+        :raise: :class:`can.CanError`


Use :raises can.Error: instead. See Sphinx.

christiansandberg · 2018-03-07T19:01:18Z

can/bus.py

+                else:
+                    return None
+
+    @abstractmethod


Don't make this abstract or it will brake older external implementations. If an interface implements recv() directly then this will also work.

You are right, I changed it.

felixdivo · 2018-04-04T14:00:25Z

Shall I change all internal interfaces to work with this new ABC, or shall I change the ABC first?

felixdivo · 2018-05-01T17:29:02Z

@hardbyte What do you think?

felixdivo · 2018-05-01T17:30:36Z

felixdivo · 2018-05-16T11:17:14Z

@christiansandberg @hardbyte I finished this PR as far as I know. The changing of all 13 backend interfaces as well as the core API has lead to many changes that ware required all over the place, along with a lot of cleanup effort that went into this. The goal of this PR is to unify the API more than it was previously the case, which was quite messy. But all unittests now run together with the new one I added.

hardbyte

Fantastic work @felixdivo - a couple of documentation related things I think can be improved before merging.

hardbyte · 2018-05-27T12:20:39Z

can/bus.py

+        """Construct and open a CAN bus instance of the specified type.
+
+        Subclasses should call though this one with all parameters as
+        it applies filters (for now).


Keep in mind that a lot of the code comments have lasted many many years so when people read "for now" they won't easily have any idea if that is talking of change to come soon... or was an idea the developer had 5 years earlier...

hardbyte · 2018-05-27T12:21:04Z

can/bus.py


 """
-Contains the ABC bus implementation.
+Contains the ABC bus implementation and it's documentation.


nit: it's "its"

hardbyte · 2018-05-27T12:30:02Z

can/bus.py

        """Block waiting for a message from the Bus.

-        :param float timeout: Seconds to wait for a message.
+        If the concrete bus does not override it, this method makes sure


This is reading like library developer documentation - not user documentation.

This class along with the Message data class is essentially the primary interface to python-can. The front page of the docs links directly to API Docs -> Bus. I think this needs to be moved into an interface developers section of the docs.

hardbyte · 2018-05-27T12:37:40Z

can/interfaces/ics_neovi/neovi_bus.py


 class ICSApiError(CanError):
+    """
+    TODO add docs


Yes please - or remove the docstring

… pages

hardbyte · 2018-05-27T23:35:15Z

doc/development.rst

  makes Sphinx complain about more subtle problems.


+Creating a new interface/backend


Thanks for writing this! 🥇

felixdivo added 4 commits March 2, 2018 22:05

factored out some common import functionality

088423d

cleaning up interface.py

828b10f

make class Bus in interface.py extend BusABC (IDEs can now see the at…

405d569

…tributes)

changed BusABC to allow for software filtering and added documentation

e3c5910

felixdivo added enhancement major proposal work-in-progress labels Mar 6, 2018

felixdivo self-assigned this Mar 6, 2018

felixdivo requested review from christiansandberg and hardbyte March 6, 2018 22:35

felixdivo added the help wanted label Mar 6, 2018

christiansandberg reviewed Mar 7, 2018

View reviewed changes

felixdivo added 4 commits March 8, 2018 15:39

fix proposed changes in PR

a5208dd

allow older implementations of BusABC.revc() to continue working

d829915

remove unused call to BusABC.flush_tx_buffer()

6a4aa5b

add some unit testing for BusABC._matches_filters()

288c8b1

felixdivo mentioned this pull request Apr 17, 2018

Unable to set filters for Vector CancaseXL #299

Closed

Merge branch 'develop' into software-filtering

a973d73

felixdivo and others added 2 commits May 14, 2018 16:54

Merge branch 'develop' into software-filtering

92037c4

more docs

f097d93

felixdivo removed the proposal label May 14, 2018

felixdivo added 4 commits May 14, 2018 19:09

more docs

90a823b

added very basic test for message filtering

3e12e3c

fix import

a831e48

switch virtaul interface to new receive method

ed1ba8b

felixdivo added 12 commits May 16, 2018 11:08

removed problemativ command line argument on AppVeyor

b3df2d7

better error handling in socketcan and correct filtering for native one

f1d0a5e

fixes for PCAN

43258be

new methods for usb2can

2a71dfe

added new mthods to slcan

79f2c4c

small fixes

dec258e

new methods for nican backend

d61c744

better imports in IXXAT

c7607df

new methods for IXXAT interface

ab4b3a0

new methods for iscan

0b6d3ca

bugfixes, simplifications, optimizations & new methods for serial can

0bb21e6

small fix for IXXAT import on Python 2

f5ea870

felixdivo removed the work-in-progress label May 16, 2018

felixdivo mentioned this pull request May 16, 2018

Adds canal interface #245

Closed

felixdivo requested a review from christiansandberg May 16, 2018 11:13

Merge branch 'develop' into software-filtering

947363b

felixdivo mentioned this pull request May 22, 2018

Feature: Additional thread safe bus instance wrapper #304

Closed

hardbyte approved these changes May 27, 2018

View reviewed changes

hardbyte added this to the 2.2 Release milestone May 27, 2018

felixdivo and others added 4 commits May 28, 2018 00:13

small doc fixes

e1e6931

added "Creating a new interface/backend" section to the documentation…

51eae1c

… pages

moved some docs of the BusABC to the development section

6a527fb

Merge branch 'develop' into software-filtering

9c4e10a

felixdivo mentioned this pull request May 27, 2018

Add a general timeout for tests #313

Merged

hardbyte reviewed May 27, 2018

View reviewed changes

felixdivo merged commit 4ba482f into develop May 28, 2018

felixdivo deleted the software-filtering branch May 28, 2018 05:45

pierreluctg mentioned this pull request Jun 12, 2018

Setting _filters to None when filter Iterator is empty #325

Merged

hardbyte mentioned this pull request Jun 30, 2018

Merge develop into master for release 2.2.0 #344

Merged

		makes Sphinx complain about more subtle problems.


		Creating a new interface/backend

Conversation

felixdivo commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christiansandberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixdivo Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixdivo commented Apr 4, 2018

Uh oh!

felixdivo commented May 1, 2018

Uh oh!

felixdivo commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixdivo commented May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hardbyte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

felixdivo commented Mar 6, 2018 •

edited

Loading

felixdivo Mar 8, 2018 •

edited

Loading

felixdivo commented May 1, 2018 •

edited

Loading

felixdivo commented May 16, 2018 •

edited

Loading