-
Notifications
You must be signed in to change notification settings - Fork 189
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
Zotino #969
Zotino #969
Conversation
…interface Add Zotino driver and add to opticlock target for Kasli Test Zotino on hw: - Verify all timings on the hardware with a scope - Verify that we can correctly set and read back all registers in a loop (checks for SI and driver issues) - check we can set LEDs correctly - check calibration routine + all si unit functions with a good DVM - look at DAC transitions on a scope (while triggering of a TTL) on persist to check there are no LDAC glitches etc All looks good. To do: update examples and e.g. KC705 device db.
|
||
|
||
@portable | ||
def voltage_to_mu(voltage, offset_dacs=8192, vref=5.): |
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.
I think that calling both units of time and voltage mu
is going to cause confusion at some point.
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.
I take your point, but we currently use mu for all units even freq phi and amp for dds. So this is just being consistent (which is the least confusing thing to do imo).
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.
IMO mu
is ok for an opaque quantity of machine units independent of what the quantity describes if the corresponding SI unit is always clear from the context and it's not used as a variable name (mu
itself is not ok, but voltage_{to_}_mu
is fine).
chip_select=1, div_write=4, div_read=8, vref=5., | ||
offset_dacs=8192, core="core"): | ||
self.bus = dmgr.get(spi_device) | ||
self.ldac = dmgr.get(ldac_device) |
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.
@hartytp Did you eventually deem it unnecessary to support configurations without LDAC?
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.
It can simply be:
class DummyTTL:
@portable
def on(self):
pass
@portable
def off(self):
pass
....
if ldac_device is None:
self.ldac = DummyTTL()
else:
self.ldac = dmgr.get(ldac_device)
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.
True that works, but it seems a but heavyweight for me unless there is a concrete use case for it - I'm not aware of one. My approach is to start simple and add that kind of feature when a need is expressed.
If you want to add this during merge I don't object.
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.
Just need to check that e.g. NIST don't need it... @dhslichter ?
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.
Assuming no major objections, would you mind finishing this off from here? This is now all working on the hw and I'd like to prioritize analogy tests on Zotino.
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.
Ok.
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.
@sbourdeauducq thinking about this more: using this without ldac is probably a rare use case, so let's not clutter up the driver with it if we can avoid it. What about having a general dummy ttl that the device manager can provide for this kind of case?
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.
@sbourdeauducq @hartytp we have some DACs that we use without LDAC (it's tied low on the board). We like having this option because then you free a TTL line for other uses.
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.
That option was subsequently added back in.
5ca5946
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.
Thanks @jordens!
chip_select=_SPI_CS_DAC, div_write=div_write, | ||
div_read=div_read, core=core) | ||
|
||
@ kernel |
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.
no space
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.
Thanks! Will fix
from artiq.coredevice import spi2 as spi | ||
from artiq.coredevice.ad53xx import SPI_AD53XX_CONFIG, AD53xx | ||
|
||
_SPI_DAC_CONFIG = SPI_AD53XX_CONFIG |
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.
I don't think this redefinition helps.
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.
Yes let's scrap it. Remnant f old version of code. Will fix.
There should be an entry in RELEASE_NOTES about the rename to |
Yes. Will do once we agree that the changes are ok. |
In general commit messages should be as concise and readable as possible. That means including the area where the work was done. "fix core device addr." is not decipherable. And if at all possible, that commit and the change it reverts should not appear in the history. |
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.
Will fix and tweak.
Testing done. Other points ack. |
We should probably write up a check list for pull requests in the contribution guidelines. |
Sounds like a good idea. FWIW, that wasn't intended as merge-ready code, but more of a design sketch showing the interface and timings I wanted. Thanks a lot @jordens and @sbourdeauducq for finishing it off and turning it into high-quality code! |
No description provided.