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

Interface improvements #123

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

ES-Alexander
Copy link
Contributor

@ES-Alexander ES-Alexander commented Mar 20, 2022

Sorry, something went wrong.

- added alternate constructors `from_serial` and `from_udp` that combine instance creation and connection
- added `__enter__` and `__exit__` methods to allow using in a context manager (`initialize` on entry, `close` connection on exit)
Minor update: new interface functionality (alternate constructors, context managers), but still backwards compatible.
- adds Ping360 example + simulation
Need to interactive rebase to appear after the new files actually exist
Need to overhaul so that it actually works, and is a good example.
- ideally want to use sub-parsers for serial vs udp
- needs to do something meaningful when it runs (although that could just be setting some settings and doing a full revolution or few)
- simplify to use boolean operator short-circuiting
- use super instead of explicit parent class name
Resetting can cause issues with using the sonar again until it's power cycled
Particularly relevant given we currently don't have discovery messages implemented.
Using a dedicated datastructure improves robustness (you can't forget a field, and all payloads have the same fields).
A NamedTuple adds typing (requires Python >= 3.6) and reduces memory usage and attribute access time.
 - separated data-buffer construction into its own method (`from_buffer`)
 - improved primary constructor to allow specifying message data directly
self.payload_format = self.get_payload_format()
except KeyError as e:
message_id = self.message_id
raise Exception(f"{message_id = } not recognized\n{msg_data = }") from e
Copy link
Contributor Author

Choose a reason for hiding this comment

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

f-string f'{var=}' syntax requires Python >= 3.8
-> should specify in commit message
-> should update minimum version in setup.py

Copy link
Member

Choose a reason for hiding this comment

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

We can update to 1.* version, do all the necessary syntax corrections and move to python 3.9

@@ -163,29 +159,32 @@ if __name__ == "__main__":

print("full scan in %dms, %dHz" % (1000*(tend_s - tstart_s), 400/(tend_s - tstart_s)))

# turn on auto-scan with 1 grad steps
p.control_auto_transmit(0,399,1,0)
if p.firmware_version > (3, 1, 1):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be a check against protocol version (>= 1.1.0), not firmware version (different devices supporting the protocol may have different firmware versions).

import time

class Ping360(PingDevice):
messages = definitions.Ping360Message

def initialize(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be good to include a protocol version check here, that prints a warning about auto messages not being supported if the protocol is < v1.1.0.

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