Merge the CAN viewer terminal application into python-can #390

Lauszus · 2018-08-11T21:18:35Z

See: #370 and Lauszus/python_can_viewer#2

codecov · 2018-08-11T21:24:40Z

Codecov Report

Merging #390 into develop will increase coverage by 1.78%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop     #390      +/-   ##
===========================================
+ Coverage    60.15%   61.94%   +1.78%     
===========================================
  Files           54       55       +1     
  Lines         4337     4551     +214     
===========================================
+ Hits          2609     2819     +210     
- Misses        1728     1732       +4

See: hardbyte#370 and Lauszus/python_can_viewer#2

christiansandberg

Great addition! Unfortunately I developed an almost identical application, but yours is much better structured than mine so I might contribute with some of my features later.

christiansandberg · 2018-08-12T11:55:12Z

doc/scripts.rst

+                            Specify the backend CAN interface to use. (default: "socketcan")
+      --ignore-canopen      Do not print CANopen information
+
+Shortcuts


Would be helpful to print these in the program too, maybe at the bottom.

christiansandberg · 2018-08-12T11:59:00Z

can/scripts/viewer.py

+        self.y, self.x = self.stdscr.getmaxyx()
+
+        # Do not wait for key inputs and disable the cursor
+        self.stdscr.nodelay(True)


The program uses 100% CPU resources because there is no place for it to wait. Either put a small timeout on key input (like 10-100 ms or so) or a timeout on bus.recv() (or both). Lowering the update rate of the window could also help reading values that change quickly, but might result in buffer overflow on some interfaces.

christiansandberg · 2018-08-12T12:01:03Z

can/scripts/viewer.py

+def main():  # pragma: no cover
+    parsed_args, can_filters, data_structs, ignore_canopen = parse_args(sys.argv[1:])
+
+    config = {}


Put single_handle=True here for Kvaser, otherwise we won't be able to see messages sent from the local machine.

christiansandberg · 2018-08-12T12:03:21Z

can/scripts/viewer.py

+
+class CanViewer:
+
+    def __init__(self, stdscr, bus, data_structs, ignore_canopen, testing=False):


You could also add curses.use_default_colors() somewhere to allow the terminal window to choose a fitting background instead of always black.

christiansandberg · 2018-08-12T12:07:20Z

can/scripts/viewer.py

+
+from can import __version__
+
+# CANopen function codes


Python-can is meant to be protocol agnostic, so if we support CANopen then we will have to prepare to support many different protocols in the future as well. Perhaps it would be a good idea to make it possible to write plugins for decoding messages. There are a lot of proprietary CAN protocols out there and this would allow the most flexibility. We could also link to third party packages from here so it is easy to find them.

I agree that the viewer should remain protocol agnostic. Can we aim to support multiple CAN protocols without the user having to change any python-can code?

Your separate config file for --decode looks close for a naive (no custom protocol code) version? Could add a display name per frame id.

… so the application does not use 100 % of the CPU resources, added "single_handle=True" so it is compatible with the Kvaser interface and let curses choose the background color automatically

…ment text, so there are no unwanted spaces after the text

Lauszus · 2018-08-12T13:42:27Z

@christiansandberg I believe 7a8ca77 should address all of your comments.

…thod in 7a8ca77

Lauszus · 2018-08-12T14:05:19Z

Seems like Travis is having connection issues. Can someone with admin privileges please restart the Python 3.5 and pypy3.5 build?

christiansandberg · 2018-08-12T12:24:50Z

can/scripts/viewer.py

+
+    # Convert it into raw integer values and then pack the data
+    @staticmethod
+    def pack_data(cmd, cmd_to_struct, *args):  # type: (int, Dict, Union[*float, *int]) -> bytes


What is this used for?

The unpack_data and pack_data are coming from another project of mine. I use it for the test. I have moved into the test, as it is unused in the viewer.

christiansandberg · 2018-08-12T12:32:05Z

can/scripts/viewer.py

+            # Increment frame counter
+            self.ids[key]['count'] += 1
+
+        # Sort frames based on the CAN-Bus ID if a new frame was added


Other tools like CANalyzer tend to add new messages at the bottom as that does not disturb the user while reading the list. We could sort the list by pressing 's' for example.

christiansandberg · 2018-08-12T12:34:08Z

can/scripts/viewer.py

+            # Generate data string
+            data_string = ''
+            if msg.dlc > 0:
+                data_string = ' '.join('{:02X}'.format(x) for x in msg.data)


Might set data_string to something like "Remote frame" if it is.

I can't see a clean way to implement this. Maybe use a different color?

can/scripts/viewer.py

christiansandberg · 2018-08-12T17:47:46Z

can/scripts/viewer.py

+
+    optional.add_argument('-i', '--interface', dest='interface',
+                          help='''R|Specify the backend CAN interface to use. (default: "socketcan")''',
+                          choices=sorted(can.VALID_INTERFACES), default='socketcan')


Should default to use config files like the regular API.

christiansandberg · 2018-08-13T18:07:30Z

can/scripts/viewer.py

+                self.redraw_screen()
+
+            # Sleep 0.1 ms, so the application does not use 100 % of the CPU resources
+            time.sleep(.1 / 1000.)


I think it's better to put a longer timeout in the recv method instead. By sleeping between each message you risk losing messages eventually. A timeout will only allow the program to rest when the receive queue has been cleared.

felixdivo · 2018-08-15T14:30:38Z

I restarted the Travis CI builds and they now run flawlessly. You may also add a note on this merge in python-can/doc/history.rst. ;-)

…er dropped, sort frame by pressing 's' instead of doing it automatically and moved "pack_data" into the test

Lauszus · 2018-08-17T15:43:26Z

@christiansandberg please see: 33ffd6c

Lauszus · 2018-08-17T15:50:42Z

@felixdivo please see: 08249da

felixdivo · 2018-08-17T15:56:09Z

@Lauszus Your addition in the history section in 08249da looks good. 😃

Lauszus · 2018-08-28T14:37:41Z

@christiansandberg anymore corrections?

felixdivo · 2018-09-06T15:14:03Z

Please see #406. I fixed the bug with the scripts not being found. Because of this, I had to place the scripts in the python-can/can/ folder, and not inside the python-can/can/scripts/ folder.

Also, this can be used to print the help section when no arguments were given:

# [...]
import sys
# [...]

def main():
    # [...]

    # print help message when no arguments were given
    if len(sys.argv) < 2:
        parser.print_help(sys.stderr)
        import errno
        raise SystemExit(errno.EINVAL)

# [...]

hardbyte

I agree with @christiansandberg here that the viewer should remain protocol agnostic. I suggest removing the CANOPEN specific decoding information into a separate module.

Ideally can we aim to support multiple CAN protocols without the user having to change any python-can library code?

Your separate config file for --decode looks close for a naive version? (no custom protocol code) Consider adding a display name per frame id.

edit Also this is a fantastic addition and I think it will make the library much more useful!

# Conflicts: # can/scripts/__init__.py

Lauszus · 2018-09-09T15:47:22Z

Just moved the script back into the CAN module. Will remove the CANopen code later tonight.

…g how the decoding looks like

Lauszus · 2018-09-09T19:35:38Z

The CANopen code is now removed and the documentation is updated :)

Lauszus · 2018-09-09T19:37:01Z

Btw I have considered changing the syntax of the -d argument, so it is comma separated. Not sure if it's worth doing or I should just stick with colons?

christiansandberg · 2018-09-09T19:58:38Z

The decoding part could also be moved to a plugin using a database instead of specifying everything on the command line. It could accomplished using canmatrix.

Lauszus · 2018-09-09T20:03:39Z

@christiansandberg I already allow giving the decoding as a file, but you could add support for any kind of format in the future if you like.

Lauszus · 2018-09-12T08:22:54Z

@hardbyte anything holding this back?

hardbyte

All good by me.

Thanks for taking on all the feedback.

Lauszus force-pushed the viewer branch from b4977ce to 97e5934 Compare August 11, 2018 21:24

Merge the CAN viewer terminal application into python-can

79d6970

See: hardbyte#370 and Lauszus/python_can_viewer#2

Lauszus force-pushed the viewer branch 2 times, most recently from 2081d02 to 79d6970 Compare August 11, 2018 22:22

christiansandberg requested changes Aug 12, 2018

View reviewed changes

Lauszus added 2 commits August 12, 2018 15:38

Print shortcuts in the usage page, sleep for 0.1 ms in the main loop,…

7a8ca77

… so the application does not use 100 % of the CPU resources, added "single_handle=True" so it is compatible with the Kvaser interface and let curses choose the background color automatically

Just erase the screen instead of clearing it and reformatted the argu…

65ee6be

…ment text, so there are no unwanted spaces after the text

Forgot to patch curses.use_default_colors() and the stdscr.erase() me…

2e42612

…thod in 7a8ca77

christiansandberg reviewed Aug 12, 2018

View reviewed changes

christiansandberg reviewed Aug 13, 2018

View reviewed changes

felixdivo added this to the 2.3 Release milestone Aug 15, 2018

felixdivo assigned Lauszus and christiansandberg Aug 15, 2018

felixdivo added enhancement major api work-in-progress labels Aug 15, 2018

Color error frames red, set timeout in a better way so frames are nev…

33ffd6c

…er dropped, sort frame by pressing 's' instead of doing it automatically and moved "pack_data" into the test

Added my contributions to history.rst

08249da

Merge branch 'develop' into viewer

315ebb2

Merge branch 'develop' into viewer

09d2056

felixdivo mentioned this pull request Sep 1, 2018

Problem with new location of scripts #392

Closed

hardbyte requested changes Sep 8, 2018

View reviewed changes

Lauszus added 3 commits September 9, 2018 17:13

Merge branch 'develop' into viewer

a8e74fe

# Conflicts: # can/scripts/__init__.py

Put the viewer script back into the CAN module

9b397d1

Print help message when no arguments are given

2e5de36

Lauszus added 3 commits September 9, 2018 21:06

Removed all CANopen code

5981fcd

Fixed Python 2 compatibility

2ff8c5c

Removed CANopen from the documentation and take new screenshot showin…

687c925

…g how the decoding looks like

Removed unused import

63d6d56

christiansandberg approved these changes Sep 9, 2018

View reviewed changes

Merge branch 'develop' into viewer

ca8e05a

felixdivo approved these changes Sep 15, 2018

View reviewed changes

felixdivo removed the work-in-progress label Sep 15, 2018

hardbyte approved these changes Sep 17, 2018

View reviewed changes

felixdivo merged commit 5a9e6eb into hardbyte:develop Sep 17, 2018

felixdivo mentioned this pull request Sep 17, 2018

Merging the Python CAN viewer tool into the python-can library Lauszus/python_can_viewer#2

Closed

Lauszus added a commit to Lauszus/python_can_viewer that referenced this pull request Sep 17, 2018

Backported changes from hardbyte/python-can#390

0b4625c

Lauszus deleted the viewer branch September 24, 2018 18:29


		class CanViewer:

		def __init__(self, stdscr, bus, data_structs, ignore_canopen, testing=False):

Merge the CAN viewer terminal application into python-can #390

Merge the CAN viewer terminal application into python-can #390

Uh oh!

Conversation

Lauszus commented Aug 11, 2018

Uh oh!

codecov bot commented Aug 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

christiansandberg 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!

Choose a reason for hiding this comment

Uh oh!

Lauszus commented Aug 12, 2018

Uh oh!

Lauszus commented Aug 12, 2018

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixdivo commented Aug 15, 2018

Uh oh!

Lauszus commented Aug 17, 2018

Uh oh!

Lauszus commented Aug 17, 2018

Uh oh!

felixdivo commented Aug 17, 2018

Uh oh!

Lauszus commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

hardbyte left a comment • 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!

Lauszus commented Sep 9, 2018

Uh oh!

Lauszus commented Sep 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lauszus commented Sep 9, 2018

Uh oh!

christiansandberg commented Sep 9, 2018

Uh oh!

Lauszus commented Sep 9, 2018

Uh oh!

Lauszus commented Sep 12, 2018

Uh oh!

hardbyte left a comment

codecov bot commented Aug 11, 2018 •

edited

Loading

Lauszus commented Aug 28, 2018 •

edited

Loading

felixdivo commented Sep 6, 2018 •

edited

Loading

hardbyte left a comment •

edited

Loading

Lauszus commented Sep 9, 2018 •

edited

Loading