Skip to content

Conversation

@christiansandberg
Copy link
Collaborator

Fixes#353.

@codecov
Copy link

codecovbot commented Jul 9, 2018

Codecov Report

Merging #355 into develop will decrease coverage by 0.04%.
The diff coverage is 25%.

Impacted file tree graph

@@ Coverage Diff @@## develop #355 +/- ## =========================================== - Coverage 58.6% 58.55% -0.05%  =========================================== Files 54 54 Lines 4215 4218 +3 =========================================== Hits 2470 2470 - Misses 1745 1748 +3
Impacted FilesCoverage Δ
can/interfaces/vector/vxlapi.py5.03% <0%> (-0.04%)⬇️
can/interfaces/vector/canlib.py14.07% <33.33%> (-0.14%)⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d0f4a6...bc69537. Read the comment docs.

@tpwrules
Copy link

I'm not sure this is the complete solution. This solution only detects when the channel is configured correctly in the Vector API, but its hardware is not present. If the user specifies an invalid channel (negative or too big) or invalid app_name, the xlGetApplConfig raises an exception with XL_ERROR.

In fact I'm not really sure I like the error handling in general in this library. Having a generic CanError and VectorError (and whatever other bus types have) with strings as errors is kind of useless when you want to be able to tell the user to correctly configure the Vector stuff, that their interface isn't plugged in, or even something like "transmit queue full". It also makes changing library versions or bus types not really possible. I haven't looked into it in enough detail to see what changing error behavior would entail or how that would look, but I think it needs serious consideration. Sorry for this rant, I'm not too sure where to put it.

For reference, here's the code I put in my program to detect the important cases when connecting:
https://github.com/tpwrules/emdash/blob/48eb9b303af6de961d3804a3128fd405dbcd18f3/boot/bootload.py#L105-L130

@christiansandberg
Copy link
CollaboratorAuthor

I didn't want to use VectorError as it is a wrapper around XLstatus that most functions returns. For some reason this function uses a whole different error handling mechanism that the API documentation doesn't even care to mention. I've changed the implementation to "simulate" a XL_ERR_HW_NOT_PRESENT but IMO it shouldn't be our responsibility to fix broken API's.

Regarding the general error handling in python-can I agree that it can be improved a lot. Right now there isn't any portable way to know what's wrong. Perhaps there should be more general subclasses of CanError for raising specific CAN related errors. It won't cover 100% of all things that can go wrong so you still might need to check for specific error codes anyway. I'll create a separate issue for this.

@tpwrules
Copy link

For the record, I am not insisting you simulate the XL_ERR_HW_NOT_PRESENT. If you prefer to raise a CanError for now, I think that's okay because I can still differentiate that. But that should be reconsidered with future error handling improvements.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@christiansandberg@tpwrules