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

Add feature to detect socketcand beacon #1687

Merged
merged 17 commits into from
Jan 12, 2024

Conversation

faisal-shah
Copy link
Contributor

This implements the client side service discovery for socketcand:
https://github.com/dschanoeh/socketcand/blob/master/doc/protocol.md#service-discovery

Need some feedback on how to enable this option. In the PR, I've proposed a way that makes the happy path as easy as possible ... though it is a breaking change for the SocketCanDaemonBus class.

Happy path:

bus = SocketCanDaemonBus(None) # Auto detect socketcand server, and connect to first available CAN bus
  1. If a host address (hostname,port) is provided, it works the same way as before.

  2. If a host address (hostname,port) is not provided, it activates detect_beacon(). The user can optionally provide an address to listen for the beacon on a specific interface and port. If not provided, it listens on all interfaces at the default port 42000. If channel is None, it selects the first channel in the channel list provided in the beacon.

@zariiii9003
Copy link
Collaborator

Regarding the API, i'd suggest to leave the actual SocketCanDaemonBus class untouched.
The detect_beacon function could work similar to can.detect_available_configs() and return a list of dictionaries like this:

cfg_list: list[AutoDetectedConfig] = detect_beacon()
bus = SocketCanDaemonBus(**cfg_list[0])

A dictionary should contain all necessary keywords to instantiate the SocketCanDaemonBus.

@faisal-shah
Copy link
Contributor Author

I'll be testing this in a couple different environments, and will report back.

@faisal-shah
Copy link
Contributor Author

Alright, tested it on both windows and linux. Good to go from my side. Please review for merge.

can/interfaces/socketcand/socketcand.py Outdated Show resolved Hide resolved
can/interfaces/socketcand/socketcand.py Outdated Show resolved Hide resolved
can/interfaces/socketcand/socketcand.py Outdated Show resolved Hide resolved
can/interfaces/socketcand/socketcand.py Show resolved Hide resolved
can/interfaces/socketcand/socketcand.py Outdated Show resolved Hide resolved
@faisal-shah
Copy link
Contributor Author

All review comments addressed.

@zariiii9003
Copy link
Collaborator

You should mention it in the docs, or nobody will know about it 😄
https://python-can--1687.org.readthedocs.build/en/1687/interfaces/socketcand.html

@faisal-shah
Copy link
Contributor Author

faisal-shah commented Dec 5, 2023

You should mention it in the docs, or nobody will know about it 😄 https://python-can--1687.org.readthedocs.build/en/1687/interfaces/socketcand.html

Done.

Edit ... docstring is there. Not familiar with the documentation on this project, and how it's built. Lemee look into it and see if I can make it pop up in the right place. Pointers appreciated in case anyone's looking.

@zariiii9003
Copy link
Collaborator

You need to add .. autofunction:: can.interfaces.socketcand.detect_beacon somewhere before you can reference it.

@faisal-shah
Copy link
Contributor Author

You need to add .. autofunction:: can.interfaces.socketcand.detect_beacon somewhere before you can reference it.

Done. Docs are passing now.

@faisal-shah
Copy link
Contributor Author

@zariiii9003 I've noticed sometimes tests will spuriously fail. I made an empty commit to trigger the tests and all are passing now. Let me know if any more changes are required to merge this. Appreciate the pointers in working with the doc system.

Copy link
Collaborator

@zariiii9003 zariiii9003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still a few minor issues. I'll merge after you fix these. Thank you for your patience 👍

can/interfaces/socketcand/socketcand.py Outdated Show resolved Hide resolved
can/interfaces/socketcand/socketcand.py Outdated Show resolved Hide resolved
can/interfaces/socketcand/socketcand.py Outdated Show resolved Hide resolved
can/interfaces/socketcand/socketcand.py Outdated Show resolved Hide resolved
faisal-shah and others added 2 commits January 4, 2024 09:57
Co-authored-by: zariiii9003 <52598363+zariiii9003@users.noreply.github.com>
Co-authored-by: zariiii9003 <52598363+zariiii9003@users.noreply.github.com>
@faisal-shah
Copy link
Contributor Author

There are still a few minor issues. I'll merge after you fix these. Thank you for your patience 👍

Hope I got them all. Thanks for your maintainership.

Copy link
Collaborator

@zariiii9003 zariiii9003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@zariiii9003 zariiii9003 merged commit bcd2ee5 into hardbyte:main Jan 12, 2024
30 checks passed
@FedericoSpada
Copy link
Contributor

Hi All,
I've just updated from version 4.3.1 to 4.4.2 and noticed that dongle autodetection went from lasting 500ms to 4,5s. After some tests, I've found that this change is the main reason for the big increase in duration. Isn't there a better way to implement this detection? Perhaps avoiding it altogether if some drivers aren't installed? Or maybe it could be disabled by default and you need to force the search for this particular interface so that old codes will still run fast without having to update the instruction with something like this:

image

Thanks in advance, greetings.

Sign up for free to 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.

4 participants