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

Autoconnect object #89

Merged
merged 24 commits into from
Nov 7, 2022
Merged

Autoconnect object #89

merged 24 commits into from
Nov 7, 2022

Conversation

carlsondev
Copy link
Collaborator

When added, this object will fetch a list of available serial devices and (optionally) server paths that have a MAV SDK server hosted on them.

Still needs to be tested, and the code might be able to be cleaned up a little. Some of the methods are public because pylint requires at least two public methods in a class.

Closes #55

@carlsondev carlsondev added enhancement New feature or request dont-merge For a pull request that should not be merged yet. Remove when it should be reviewed for merge. labels Oct 28, 2022
Copy link
Collaborator

@justincdavis justincdavis left a comment

Choose a reason for hiding this comment

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

No changes are needed to be made, but could be nice to use cached_property on mavsdk_servers as an instance method or something. Could make it so everything is private (since I don't see a need for some methods to be public)

pteranodon/autoconnect.py Outdated Show resolved Hide resolved
pteranodon/autoconnect.py Outdated Show resolved Hide resolved
pteranodon/autoconnect.py Outdated Show resolved Hide resolved
pteranodon/autoconnect.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
pteranodon/autoconnect.py Outdated Show resolved Hide resolved
@justincdavis
Copy link
Collaborator

Also this class should end up in the utils folder that the one branch provides. I am going to merge branch to main as I right this.

@justincdavis justincdavis added this to the Alpha v1.0.0 milestone Oct 31, 2022
@carlsondev
Copy link
Collaborator Author

@justincdavis Okay, we should be good. Can you review _get_server_value to ensure the server "paths" I am returning are in the valid format for MAV SDK to use? Specifically the serial path (currently it'll just return /dev/tty-some-serial)

Copy link
Collaborator

@justincdavis justincdavis left a comment

Choose a reason for hiding this comment

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

Only change required is >= for requirements.

See note on other thing, but even without tests I am fine merging

requirements.txt Outdated Show resolved Hide resolved
pteranodon/autoconnect.py Outdated Show resolved Hide resolved
@justincdavis
Copy link
Collaborator

@carlsondev Looks good to me, I had one comment about the requirements. If you want to do the same for numpy and grpcio at the same time that would be great :)

Tests not required to merge :)

@justincdavis
Copy link
Collaborator

@carlsondev Can this get used if the user does not pass an address to AbstractDrone OR if the connection lasts too long we use this instead? That would be dandy

…ature/autoconnect-#55

# Conflicts:
#	pteranodon/server_detector.py
#	requirements.txt
@carlsondev
Copy link
Collaborator Author

Can you elaborate on the "if the connection lasts too long we use this instead"?

I am interested in adding it if the user doesn't pass an address, but I would also be worried that it could cause some bugs/unexcepted behavior for end users. Since I return a list with no priority if multiple come back, there is no way to know which one, and assuming that is not a problem, the user might have the drone auto-connect without their knowledge and not think about it until it goes wrong, at which point it's not clear where to start.

I think it is best (at least for the first question) to have the users need to define that themselves. It's easy enough.

detect = ServerDetector()
servers = detect.get_mavsdk_servers()

@justincdavis
Copy link
Collaborator

I think that is a fair point, can we have it use auto-connect if nothing is specified but maybe we can give a WARNING in the logger for auto-connect behavior. Also we can have a boolean flag for whether or not to use it.

Do these sound reasonable? @carlsondev

Copy link
Collaborator

@justincdavis justincdavis left a comment

Choose a reason for hiding this comment

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

Changes for uvloop checking, if the user has it we should use it

Changes for logging vs printing - up to you for implementation, but we have a logger instance in AbstractDrone so either an instance or direct calls to logging would be my view. But up to you

I had some suggested changes otherwise

pteranodon/server_detector.py Outdated Show resolved Hide resolved
pteranodon/server_detector.py Outdated Show resolved Hide resolved
pteranodon/server_detector.py Outdated Show resolved Hide resolved
pteranodon/server_detector.py Outdated Show resolved Hide resolved
pteranodon/server_detector.py Outdated Show resolved Hide resolved
pteranodon/server_detector.py Outdated Show resolved Hide resolved
@carlsondev
Copy link
Collaborator Author

I think that is a fair point, can we have it use auto-connect if nothing is specified but maybe we can give a WARNING in the logger for auto-connect behavior. Also we can have a boolean flag for whether or not to use it.

Do these sound reasonable? @carlsondev

I think a warning would be helpful. If we are going to add a boolean flag, though... why not have them do it manually? Would the flag be in the initializer?

@carlsondev
Copy link
Collaborator Author

I moved the logger methods to ServerDetector so that both ServerDetector and AbstractDrone could use them without there being circular imports. I don't love it, they would probably be best in a util file (since they are static anyway).

@justincdavis justincdavis removed the dont-merge For a pull request that should not be merged yet. Remove when it should be reviewed for merge. label Nov 7, 2022
@justincdavis justincdavis merged commit 4d3b46f into main Nov 7, 2022
@justincdavis justincdavis deleted the feature/autoconnect-#55 branch November 7, 2022 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-Connect
2 participants