-
Notifications
You must be signed in to change notification settings - Fork 33
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
base: master
Are you sure you want to change the base?
Interface improvements #123
Conversation
- 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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
.
from_serial
andfrom_udp
that combine instance creation and connection__enter__
and__exit__
methods to allow using in a context manager (initialize
on entry,close
connection on exit)examples/simplePing360Example.py
tools/ping360-simulation.py
scriptset_range
functionality for Ping360 #122setup.py
update to the end