-
Notifications
You must be signed in to change notification settings - Fork 31
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
DecaDAC driver #742
base: master
Are you sure you want to change the base?
DecaDAC driver #742
Conversation
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.
Overall code structure looks good :)
I made a few remarks but I guess You'll need to change some of it anyways for the code to work.
|
||
|
||
def get_ascii(self): | ||
command = 'B' + str(self._channel[0]) + ';C' + str(self._channel[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.
Use the python format string literals where possible. It makes reading the ASCII commands in the code easier and is generally faster.
command = f"D{self._channel[0]};C{self._channel[1]};"
# vs
command = 'B' + str(self._channel[0]) + ';C' + str(self._channel[1]) + ';'
# although the best would be
command = f"D{self.block};C{self.channel};"
raise TypeError( | ||
'Declared port must be a string. Usually in the form of COM + n, with n being the port number') | ||
self._ser = serial.Serial(port, 9600, timeout=0) | ||
self._channel_voltage_location = {1: 1545, |
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.
Put this information in a global constant with an all capital name. And it looks like the formula is
VOLT_LOCATION_OFFSET = 1545
def get_channel_voltage_location(channel: int) -> int:
return VOLT_LOCATION_OFFSET + channel * 6
command += '}' ''' | ||
self._block_position = 1 | ||
self._iteration_count = 0 | ||
channels = list(to_waveform(program).defined_channels) |
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.
defined_channels
is a set of channel names. The channels
argument gives you the "inverse" mapping of channel name to channel index.
channel_idx_to_name = {idx: name
for idx, name in channels
if name is not None}
command += ch + ';D' + str(voltage) + ';' | ||
command += '$' + str(int(loop.waveform.duration / 1000)) + ';' | ||
command += '}' ''' | ||
self._block_position = 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.
These variables and many of the following code and methods probably belong into a seperate DecaDACProgram
class.
It is totally fine to have it here for development hacking :)
def __init__(self, port: str, identifier='DecaDAC',): | ||
super().__init__(identifier=identifier) | ||
|
||
if port is not str: |
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.
not isinstance(port, str)
It is probably the best to take an established serial connetion as __init__
argument and use a @classmethod
or functools.singledispatchmethod
to connect if required.
Added the DecaDAC driver. Ignore all the commented blocks. It's mostly there in case I want to reverse some of the changes I've made. I'll delete these in a later stage