Merge socketcan_native and socketcan_ctypes into one by christiansandberg · Pull Request #326 · hardbyte/python-can

christiansandberg · 2018-06-12T19:28:07Z

Since it takes a lot of effort to maintain both implementations I thought it might be a good idea to merge them into one. I took the native implementation, removed reliance on socket constants and provided a libc fallback for socket.bind and socket.connect.

Will update docs.

felixdivo · 2018-06-13T07:01:35Z

Hm, interesting! This would actually remove some more edge cases/ugly clutter all over the library. I will have a look at it the next days.

felixdivo

I think you should rebase you branch first, there are some pending changes that already remove the subprocess32 module.

In _detect_available_configs(): The interface should be socketcan, not socketcan_native.

felixdivo · 2018-06-14T10:00:07Z

test/back2back_test.py

-
-        message = can.Message(arbitration_id=0x4321, data=[1, 2, 3], extended_id=True)
-        self.bus1.send(message)
+class BasicTestSocketCan(Back2BackTestCase):


felixdivo · 2018-06-14T10:03:50Z

can/interface.py

 from .util import load_config
 from .interfaces import BACKENDS

+from can.interfaces.socketcan.socketcan import 
8000
CyclicSendTask, MultiRateCyclicSendTask


The module name socketcan.socketcan is a bit redundant.

Agree. Do you have a suggestion for a new name?

Uhm; impl? Not really better though ...

Why not just hoist them into can.interfaces.socketcan?
In can.interfaces.socketcan.__init__ we can import the classes and functions that make up the implementation

Do you mean that we should make the current socketcan.py init.py?

Nah (although that would work), I was just more commenting that no matter what file the classes end up in we should expose them via can.interfaces.socketcan e.g., can.interfaces.socketcan.CyclicSendTask.

That’s already done so this is more for internal architecture.

felixdivo · 2018-06-14T10:04:40Z

can/interface.py

            result += available

    return result
-


Good to see this being cleaned up

felixdivo · 2018-06-14T10:07:48Z

can/interfaces/socketcan/__init__.py

-from can.interfaces.socketcan import socketcan_constants as constants
-from can.interfaces.socketcan.socketcan_ctypes import SocketcanCtypes_Bus
-from can.interfaces.socketcan.socketcan_native import SocketcanNative_Bus
+from can.interfaces.socketcan import constants


Should these really be "public"?

I don't see a reason but since it was exported before I didn't want to break anything.

Hm. But this really is just an implementation detail that nobody should care about. I doubt we would touch a lot of code with that change.

Sure, I'll remove it. Since the module is now named "constants" it will work anyway.

felixdivo · 2018-06-14T10:08:32Z

can/interfaces/socketcan/socketcan.py

 #!/usr/bin/env python
 # coding: utf-8
-
-"""


Maybe keep a note that this was merged?

felixdivo · 2018-06-14T10:13:49Z

can/interfaces/socketcan/socketcan.py

+        addr = get_addr(s, channel)
+        error = libc.connect(s.fileno(), addr, len(addr))
+        if error < 0:
+            raise can.CanError('Could not connect to socket')


Maybe also print some better error message. See error_code_to_str(exc.errno).

felixdivo · 2018-06-14T10:19:03Z

can/util.py

-    # this is done later too but better safe than sorry
-    if config['interface'] == 'socketcan':
-        config['interface'] = choose_socketcan_implementation()
+    # deprecated socketcan types


Maybe print a warning to make users switch to simply `socketcan´. Should this eventually be removed?

Good idea. Yes we should eventually remove this but first a logged warning is good, then maybe a DeprecationWarning.

Could you please add a comment like this one 07be6b5

felixdivo · 2018-06-14T10:19:56Z

can/interfaces/socketcan/socketcan.py


    def _recv_internal(self, timeout):
-        if timeout:
+        if timeout is not None:


A timeout of 0 should not try to invoke select(), or should it?

It should, because otherwise a timeout of 0 will go directly to recv which will block with infinite timeout.

felixdivo

Updated review.

felixdivo · 2018-06-14T10:22:41Z

can/interfaces/socketcan/socketcan.py

+        if error < 0:
+            raise can.CanError('Could not connect to socket')
    except OSError as e:
        log.error("Couldn't connect a broadcast manager socket")


Maybe also print some better error message. See error_code_to_str(exc.errno).

Won't that information be in the exception that is raised after?

Yeah. See below.

felixdivo · 2018-06-14T10:23:12Z

can/interfaces/socketcan/socketcan.py

+        addr = get_addr(sock, channel)
+        error = libc.bind(sock.fileno(), addr, len(addr))
+        if error < 0:
+            raise can.CanError('Could not bind socket')


Maybe also print some better error message. See error_code_to_str(exc.errno).

felixdivo · 2018-06-14T10:23:42Z

can/interfaces/socketcan/socketcan.py

+                                   CAN_RAW_RECV_OWN_MSGS,
                                   struct.pack('i', receive_own_messages))
        except socket.error as e:
            log.error("Could not receive own messages (%s)", e)


Isn't that also in the exception message?

Ah right. It creates something like: socket.error: [Errno 107] Transport endpoint is not connected. This seems to be only required with the libc fallback.

felixdivo · 2018-06-14T10:32:14Z

Sorry for the many comments, but this really nice PR touches quite a bit of code / code structure.

Once everything code-related is cleaned up, we should also update the docs where appropriate.

felixdivo · 2018-06-15T16:38:38Z

socketcan.rst should still mention that filters and periodic sending are quite efficient because of in-kernel handling of that tasks. You could just copy an explanation together from the deleted parts of 6ce8565.

Support broadcast channel

felixdivo · 2018-06-15T21:13:37Z

can/interfaces/socketcan/socketcan.py

        :param str channel:
            The can interface name with which to create this bus. An example channel
            would be 'vcan0' or 'can0'.
+            An empty string '' will receive messages from all channels.


And what about sending? Is it supported on both versions?

No not currently. But it shouldn't require too much work so I'll fix that.

felixdivo · 2018-06-15T21:16:17Z

can/interfaces/socketcan/socketcan.py

+            if self.channel == "" and HAS_NATIVE_SUPPORT and msg.channel:
                self.socket.sendto(data, (msg.channel, ))
+            else:
+                self.socket.send(data)


Shouldn't we use sendall(), since send() might not transmit everything?

The default behaviour for all other interfaces and previously for socketcan is to not block on send(). If it can't be transmitted, it should be raised as an exception and the application can decide if it wants to try again, move on to the next message or signal as an error. The most common scenario is that something is wrong with the bus and no nodes are ACK:ing your messages. If we would be using sendall then the application will just freeze.

I added the timeout argument for making send blocking, but usually you want to give up after a certain time. One problem with my change will arise if it is possible to queue only part of a message. Then we want to go back to select and loop until all bytes are sent, but only for the timeout period. I'll give that a go.

felixdivo · 2018-06-15T21:17:47Z

can/interfaces/socketcan/socketcan.py

        try:
-            self.socket.sendall(build_can_frame(msg))
+            if self.channel == "" and HAS_NATIVE_SUPPORT and msg.channel:
+                self.socket.sendto(data, (msg.channel, ))


Return the number of bytes sent.

I guess we should make sure the rest gets sent as well (in a loop or handled completely in a auxiliary method) if the returned number of bytes is less than len(data).

Improved timeout handling

christiansandberg · 2018-06-16T20:23:06Z

Sorry for all the commits. I'll squash them when merging. Currently I have to push the ch 38BA anges in order to test them on the Linux machine. I should have done the broadcast channel thing later but it seemed simpler at first. Now it seems to finally work however.

felixdivo · 2018-06-17T11:10:52Z

can/interfaces/socketcan/socketcan.py

+            if not ready:
+                # Timeout
+                break
+            sent = self._send_once(data, msg.channel)


This is a nice solution! But maybe it would be more efficient to use socket.sendall() when there is native support?

The problem is that sendall doesn't have a timeout argument. And it is probably very unusual, if even impossible, that a message could be only partially cued. For most cases it will succeed on first try, or it will fail completely on first select.

felixdivo

Does sending to all channels work now? If not, it should be implemented or a note / issue should be added. That's all I see that needs to be done.

christiansandberg · 2018-06-20T10:32:41Z

Do you mean if broadcast channel is used? Then you currently need to address each message to a specific channel, otherwise it will fail. I don't think there is a way to send to all channels as far as I can see. I can add a note about that.

Add notes about deprecation of old bustypes Add note about sending to broadcast bus setsockopt should supports integers

Merge socketcan_native and socketcan_ctypes into one

dbe66f6

christiansandberg added enhancement major backend:socketcan os:linux labels Jun 12, 2018

christiansandberg self-assigned this Jun 12, 2018

christiansandberg requested a review from felixdivo June 12, 2018 19:28

felixdivo requested changes Jun 14, 2018

View reviewed changes

christiansandberg added 6 commits June 14, 2018 22:05

Update docs

6ce8565

Cleanups and review changes

6b6a7b9

Merge remote-tracking branch 'hardbyte/develop' into socketcan-merge

44c17ff

Fix tests

d766e68

Fix bug

79d47cd

Fix TestThreadSafeBus

d09f9e6

christiansandberg added 2 commits June 15, 2018 22:04

Docs and implementation changes

1ebe388

Support broadcast channel

Support broadcast channel for periodic tasks

8ad5efa

felixdivo reviewed Jun 15, 2018

View reviewed changes

christiansandberg added 5 commits June 16, 2018 10:08

Add support for broadcast in Python 2.7

ad62359

Improved timeout handling

Handle frame data as bytes

8f935aa

Add support for recvfrom in Python 2.7

10d53c1

Another try

cb48ae8

Channel should not be bytes type

6dbbf4a

felixdivo reviewed Jun 17, 2018

View reviewed changes

felixdivo approved these changes Jun 20, 2018

View reviewed changes

christiansandberg added 2 commits June 24, 2018 20:48

Address some review comments and minor adjustments

e85b35c

Add notes about deprecation of old bustypes Add note about sending to broadcast bus setsockopt should supports integers

Merge remote-tracking branch 'hardbyte/develop' into socketcan-merge

fb6983f

christiansandberg merged commit 66540e3 into hardbyte:develop Jun 26, 2018

hardbyte mentioned this pull request Jun 30, 2018

Merge develop into master for release 2.2.0 #344

Merged

Conversation

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment