Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve python-can driver #66

Merged
merged 3 commits into from Aug 12, 2019

Conversation

@adolfogc
Copy link
Contributor

commented Aug 1, 2019

  • Fixes the issue of not having python-can installed causing an error, even though users may want to use, e.g., SocketCAN instead.
  • Allows passing CAN interface configuration information using the constructor instead of requiring the use of an INI file.
  • Corrects other minor issues.
@adolfogc

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

@pavel-kirienko In #53 (review) you said:

... here is still a room for improvement (such as writing tests, documentation, and fixing minor mistakes such as the fixed latency being only 1 microsecond which is unrealistic). ...

(Boldface was added to the original)

I'm guessing the correct value depends on the hardware we're using, but I don't know how to get the correct one to use. What is your suggestion to address this issue?

@pavel-kirienko
Copy link
Member

left a comment

I'm guessing the correct value depends on the hardware we're using, but I don't know how to get the correct one to use. What is your suggestion to address this issue?

We should specify the best-case fixed latency attainable with common high-level operating systems such as Windows. My experience indicates that the value of 100~200 microseconds fits the majority of deployments. If you are interested in the underlying technical details, see the Olson passive time synchronization (recovery) algorithm: https://april.eecs.umich.edu/pdfs/olson2010.pdf

Would you be available to implement PythonCAN support for the new PyUAVCAN rewrite? It's in the branch uavcan-v1.0 and it's totally new (no old code whatsoever).


if sys.platform.startswith('linux'):
from .socketcan import SocketCAN
else:
SocketCAN = None

try:
import can

This comment has been minimized.

Copy link
@pavel-kirienko

pavel-kirienko Aug 1, 2019

Member

Why can't we move from .python_can import PythonCAN here?

This comment has been minimized.

Copy link
@adolfogc

adolfogc Aug 10, 2019

Author Contributor

IMO it reads more explicit that we're just checking that the specific module, can, is present; also that way we also prevent a possible import error in python_can.py from being caught and ignored.

This comment has been minimized.

Copy link
@pavel-kirienko

pavel-kirienko Aug 11, 2019

Member

IDK it seems non-obvious and either I'm reading too much into it but it also looks like a leaky abstraction where you make assumptions about the underlying structure of the imported module. The general lack of a well-constructed architecture here might make my observation about the leaky abstraction here a bit inappropriate but it's still valid.

If you are certain about this then I won't insist.

This comment has been minimized.

Copy link
@adolfogc

adolfogc Aug 11, 2019

Author Contributor

I've made the change as per your request.

This comment has been minimized.

Copy link
@pavel-kirienko

pavel-kirienko Aug 11, 2019

Member

That's not quite what I meant. The import can statement is the one I am concerned about. Any chance to get rid of it?

This comment has been minimized.

Copy link
@adolfogc

adolfogc Aug 11, 2019

Author Contributor

Sorry, I misunderstood what you meant. I've moved the check to the .python_can module itself. You're right, it's cleaner that way.

@thirtytwobits

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

Why are the AppVeyor builds failing for master?

@thirtytwobits

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

OH! I just realized this should enable uavcan tools to run on OSX with the peak systems adapter. Yes please! I can haz this PR @pavel-kirienko ?

@pavel-kirienko

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Why are the AppVeyor builds failing for master?

Because there is no appveyor.yml on master. Could someone submit one, please?

OH! I just realized this should enable uavcan tools to run on OSX with the peak systems adapter. Yes please! I can haz this PR @pavel-kirienko ?

Observe that this is UAVCAN v0 though. We could really use it in UAVCAN v1.

@thirtytwobits

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

@adolfogc : if you rebase this on mainline I've merged in an appveyor.yml that will fix the checks.

Improve python-can driver
- Fixes the issue of not having python-can installed causing an error, even though users may want to use, e.g., SocketCAN instead.
- Allows passing CAN interface configuration information using the constructor instead of requiring the use of an INI file.
- Corrects other minor issues.

@adolfogc adolfogc force-pushed the adolfogc:python-can-patches branch from 6a20b2d to 0fd6537 Aug 10, 2019

@adolfogc

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

@adolfogc : if you rebase this on mainline I've merged in an appveyor.yml that will fix the checks.

Thanks @thirtytwobits

@adolfogc

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

We should specify the best-case fixed latency attainable with common high-level operating systems such as Windows. My experience indicates that the value of 100~200 microseconds fits the majority of deployments. If you are interested in the underlying technical details, see the Olson passive time synchronization (recovery) algorithm: https://april.eecs.umich.edu/pdfs/olson2010.pdf

Thanks, @pavel-kirienko, I'll check that out.

Would you be available to implement PythonCAN support for the new PyUAVCAN rewrite? It's in the branch uavcan-v1.0 and it's totally new (no old code whatsoever).

Yes, I intend to continue working on the v1 branch as well. This last month I've been focused on a course I'm attending abroad and the next one I'll be somewhat distracted because I'm moving overseas to study, so I expect not being able to work at a constant rate shortly. Is there a deadline I should keep in mind?

@pavel-kirienko

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

Is there a deadline I should keep in mind?

There is no strict deadline but it would be optimal to have it done by September so that we could begin to push people towards v1 more confidently. Perhaps we should replace master with v1 around that time, too, but that is also dependent on @thirtytwobits' ongoing work on libuavcan.

adolfogc added some commits Aug 11, 2019

Refactor code
Use try-except instead of try-except-else.
Refactor code
Move import check to .python_can module.

@pavel-kirienko pavel-kirienko merged commit 85da86d into UAVCAN:master Aug 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.