General exception handling (+ changes to the ixxat interface) #562

marcel-kanter · 2019-04-30T10:01:44Z

A starting point for the unification of the exception handling.
Includes some improvements for the ixxat interface (the improvements use the new exceptions).
Please pull this into new branch, to keep the develop working until this work is applied to all the interfaces/classes/etc...

Closes #560
Addresses #356

Started to use the new exception types.

…d. The method returns True on success and False on timeout, if the user wanted an timeout.

marcel-kanter · 2019-04-30T10:06:06Z

Checks fail, since the ixxat interface cannot be loaded on the test environment.

felixdivo · 2019-04-30T18:35:29Z

I will create a new branch for this and point the PR to it.

(Done)

can/interfaces/ixxat/canlib.py

test/test_interface_ixxat.py

codecov · 2019-04-30T19:17:29Z

Codecov Report

Merging #562 into unified-exceptions will increase coverage by <.01%.
The diff coverage is 46.66%.

@@                  Coverage Diff                   @@
##           unified-exceptions     #562      +/-   ##
======================================================
+ Coverage               64.03%   64.04%   +<.01%     
======================================================
  Files                      63       64       +1     
  Lines                    5700     5715      +15     
======================================================
+ Hits                     3650     3660      +10     
- Misses                   2050     2055       +5

hardbyte

Thanks so much for taking up the challenge.

can/exceptions.py

hardbyte · 2019-05-02T06:39:05Z

can/exceptions.py

+class CanOperationError(CanError):
+	""" Indicates an error while operation.
+	"""
+	pass


nit: newline at end of file please

Oups. Will be fixed.

can/exceptions.py

hardbyte · 2019-05-02T06:40:38Z

can/interfaces/ixxat/canlib.py

        # Real return value is an unsigned long
        result = ctypes.c_ulong(result).value

+    #print(hex(result), function)


nit: remove

hardbyte · 2019-05-02T06:49:59Z

can/interfaces/ixxat/canlib.py

-
+        """
+        Sends a message on the bus. The interface may buffer the message.
+        returns True on success or when timeout is None


Please use the sphinx syntax we use elsewhere in the code base.

See for example https://github.com/hardbyte/python-can/blob/develop/can/interfaces/socketcan/socketcan.py#L573-L596

hardbyte · 2019-05-02T06:56:51Z

can/interfaces/ixxat/canlib.py

+        Sends a message on the bus. The interface may buffer the message.
+        returns True on success or when timeout is None
+        returns False on timeout (when timeout is not None)
+        raises CanOperationError


Suggest we raise an exception on timeout rather return a boolean.

Hmm. Thats a good point. Maybe our way thinking about Exceptions differs a bit. This method is a good example for "fine-adjusting" the handling/rasising of exceptions.

We have two cases: With or without timeout
First without timeout: Thats easy. The user don't want a timeout, so any timeout is a case that should not happen -> exceptional behavior which needs special treatment.
Second with timeout: Thats the point. The user wanted a timeout, so he/she accepts any timeout (maybe don't cares about it - recovering from a fault situation might be a situation were this mades much sense. the fault may still be present and the other node not reachable). So is an timeout an exceptional behavior? Not really. (But i still need to clearify if an timeout may occur because of exceptional behavior of the driver. Then this can get complicated.)

In any case the behavior should be consistent between interfaces. If we change to return a boolean instead of raising an exception then other interfaces must change too.

My original intention was that if a timeout is specified, the developer really wants to make sure the message is sent or queued successfully. If not then something is wrong and the program may be stopped or it can try to retransmit later.

Ok, I'll stick to the more pythonic raise-an-exception way.
BUT. Should we generally use the built-in TimeoutError for timeouts or sub-class CanError?
I prefer the built-in.

Would there be an issue inheriting from both? I agree that our specific exception e.g. CanBusWriteTimeout should inherit from TimeoutError and I imagine it should work also inheriting from our generic CanError exception?

I think there would be no problem. But i would call this class CanTimeoutError and use it generally for timeout errors, not only for send.

On the other hand I don’t think the OSErrors are supposed to be used for other than pure system level errors.

“Raised when a system function timed out at the system level. Corresponds to errno ETIMEDOUT.”

Oki. I had a look into other python packages and the common unterstanding is like christian said it. They dervie their own TimeoutError.
So we'll get an CanTimeoutError

test/test_interface_ixxat.py

hardbyte · 2019-05-02T06:58:06Z

test/test_interface_ixxat.py

+
+class SoftwareTestCase(unittest.TestCase):
+    """
+    Test cases that test the software only and do not rely on an existing/connected hardware.


Do they require any driver software to be installed?

Actually they rely on the ixxat driver installed on the maschine. Currently I'll skip the tests, if they can't be loaded (ImportError).

Still looking for a good and simple way for testing as much as possible without any driver.

You could have a look at test_systec.py, which uses unittest.mock.

They have linux drivers for x86 and AMD64 on their website.
I contacted HMS (Ixxat is a part of them) and got an beta driver for ARM64, I'm going to test them on the weekend on my ODROID.
Then we may get linux working support too.

test/test_interface_ixxat.py

can/exceptions.py

Win32 and cygwin will both load "vcimpl.dll". Added comments.

felixdivo · 2019-05-13T10:55:37Z

We should also consider a way to document this. Maybe like this hierarchy, along with the autodoc classes added to python-can/doc/api.rst#errors?

felixdivo · 2019-05-13T11:08:46Z

can/exceptions.py

+class CanBackEndError(CanError):
+	""" Indicates an error related to the backend (e.g. driver/OS/library)
+	Examples:
+	- A call to a library function results in an unexpected return value


Should we add the return code as a parameter, which is None if not explicitly set? Also applies to CanInitializationError. This way, we could unify it a little bit, and it would be opt-in for interfaces where it would be helpful to have the error codes as a user.

marcel-kanter · 2019-05-14T19:16:59Z

We should also consider a way to document this. Maybe like this hierarchy, along with the autodoc classes added to python-can/doc/api.rst#errors?

Is there an easy and fast way to build the doc locally? When I try python setup.py build_sphinx I get an error that sphinx misses some a packet programoutput

felixdivo · 2019-05-15T12:04:27Z

I never heard of that error. It just works on my machine. Could you post the part of the output that indicates the problem?

marcel-kanter · 2019-05-18T11:31:30Z

I got this output, but then I found that the missing extension is an addon to sphinx, with this addon, building works:

Warning: 'classifiers' should be a list, got type 'tuple'
C:\Program Files\Python37\lib\site-packages\setuptools\dist.py:481: UserWarning: The version specified ('3.2.0-exception-handling') is an invalid version, this
may not work as expected with newer versions of setuptools, pip, and PyPI. Please see PEP 440 for more details.
"details." % self.metadata.version
running build_sphinx
Running Sphinx v2.0.1
loading translations [en]... done

Extension error:
Could not import extension sphinxcontrib.programoutput (exception: No module named 'sphinxcontrib.programoutput')

I'll change the version to 4.0.0-dev0

marcel-kanter

According to PEP440 and the implementation in setuptools, the normalized version should be 4.0.0.dev0 (with the 0 at the end).

Change version to 4.0.0.dev0 Fix classifiers in setup.py, it should be a list.

…/python-can into exception-handling

hardbyte

I think we still have work to do with regard to common attributes our exceptions could expose but this is a great start

hardbyte

Good start, we need to sort out the optional/required exception attributes next but we can merge this into develop

hardbyte · 2019-05-19T12:11:07Z

Erm sorry that was weird, ignore my double review. Glitch in the intewebs

marcel-kanter added 5 commits April 29, 2019 21:01

Refactoring: Move exceptions to separate file.

01b9188

Added tests for ixxat interface.

8697f14

Started to use the new exception types.

Introduced CanBackEndError for exceptions related to the backend.

b9bea05

Raise an CanOperationError when an exception occurs in the send metho…

938e440

…d. The method returns True on success and False on timeout, if the user wanted an timeout.

Change intentions to 4 spaces to match remote.

07326ef

felixdivo reviewed Apr 30, 2019

View reviewed changes

can/interfaces/ixxat/canlib.py Outdated Show resolved Hide resolved

felixdivo reviewed Apr 30, 2019

View reviewed changes

can/interfaces/ixxat/canlib.py Outdated Show resolved Hide resolved

felixdivo reviewed Apr 30, 2019

View reviewed changes

test/test_interface_ixxat.py Outdated Show resolved Hide resolved

felixdivo changed the base branch from develop to unified-exceptions April 30, 2019 19:08

felixdivo assigned marcel-kanter Apr 30, 2019

Skip the test if there is an ImportError

d5be879

felixdivo added api enhancement labels Apr 30, 2019

Merged in suggested changes from pull request.

e9c7096

hardbyte requested changes May 2, 2019

View reviewed changes

felixdivo added this to the 4.0 Release milestone May 2, 2019

felixdivo reviewed May 10, 2019

View reviewed changes

can/exceptions.py Outdated Show resolved Hide resolved

Introduced CanTimeoutError.

5d40159

Win32 and cygwin will both load "vcimpl.dll". Added comments.

felixdivo changed the title ~~Exception handling~~ General exception handling (+ changes to the ixxat interface) May 13, 2019

felixdivo reviewed May 13, 2019

View reviewed changes

Change version to 4.0.0-dev

e29c162

marcel-kanter commented May 19, 2019

View reviewed changes

Fix imports

543163d

Change version to 4.0.0.dev0 Fix classifiers in setup.py, it should be a list.

hardbyte and others added 2 commits May 19, 2019 15:47

Update __init__.py

3cf73c8

Merge branch 'exception-handling' of https://github.com/marcel-kanter…

a929ffe

…/python-can into exception-handling

hardbyte approved these changes May 19, 2019

View reviewed changes

felixdivo approved these changes May 19, 2019

View reviewed changes

hardbyte approved these changes May 19, 2019

View reviewed changes

felixdivo merged commit 739e405 into hardbyte:unified-exceptions May 19, 2019

felixdivo mentioned this pull request Apr 22, 2021

Add specific exceptions #1025

Merged

felixdivo mentioned this pull request Feb 4, 2022

Update CONTRIBUTERS.txt and history.rst #1242

Closed

General exception handling (+ changes to the ixxat interface) #562

General exception handling (+ changes to the ixxat interface) #562

Uh oh!

Conversation

marcel-kanter commented Apr 30, 2019 • edited by felixdivo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcel-kanter commented Apr 30, 2019

Uh oh!

felixdivo commented Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hardbyte left a comment

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!

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!

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!

Uh oh!

felixdivo commented May 13, 2019

Uh oh!

felixdivo May 13, 2019 • 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!

marcel-kanter commented May 14, 2019

Uh oh!

felixdivo commented May 15, 2019

Uh oh!

marcel-kanter commented May 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcel-kanter left a comment

Choose a reason for hiding this comment

Uh oh!

hardbyte left a comment

Choose a reason for hiding this comment

Uh oh!

hardbyte left a comment

marcel-kanter commented Apr 30, 2019 •

edited by felixdivo

Loading

felixdivo commented Apr 30, 2019 •

edited

Loading

codecov bot commented Apr 30, 2019 •

edited

Loading

felixdivo May 13, 2019 •

edited

Loading

marcel-kanter commented May 18, 2019 •

edited

Loading