Skip to content
This repository was archived by the owner on Jan 18, 2026. It is now read-only.

add suservo CPLD #2862

Open
DonovanWebb wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add suservo CPLD #2862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions artiq/coredevice/suservo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,3 +564,66 @@ def set_y(self, profile, y):
raise ValueError("Invalid SUServo y-value!")
self.set_y_mu(profile, y_mu)
return y_mu


class ProtoRev8(urukul.ProtoRev8):
"""
For use with suservo CPLD to get around polymorphic issues.
"""


class ProtoRev9(urukul.ProtoRev9):
"""
For use with suservo CPLD to get around polymorphic issues.
"""


class CPLD(urukul.CPLD):
"""
Needed to avoid polymorphic issues when using SUServo and regular Urukuls in the same experiment.
"""
rb_len = 8

@dnadlinger dnadlinger Sep 27, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This seems related to phase-coherent suservo support and probably shouldn't be in this PR.

@sbourdeauducq sbourdeauducq Sep 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To be honest, this PR shouldn't exist at all. @DonovanWebb Can you just try on the nac3 branch? https://git.m-labs.hk/M-Labs/nac3/issues/664 appears to be fixed, which in theory was the last hurdle to get this to work properly. Though I have not tested it nor updated the Urukul driver yet.

@dnadlinger dnadlinger Sep 28, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nac3 isn't anything close to a drop-in replacement, and in any case this driver is for the legacy compiler only. This PR is just cleaning up after the recent ProtoRev9 changes, which were merged onto the non-nac3 branch, but broke a common use case (mixing SUServo and non-SUServo Urukuls; cf. various forum discussions).

@sbourdeauducq sbourdeauducq Sep 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@dnadlinger Your PR to fix the bug in the legacy compiler would be appreciated.

@sbourdeauducq sbourdeauducq Sep 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And adding hacks/workarounds is never "cleaning up".

@dnadlinger dnadlinger Sep 28, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's not a bug per-se, it's a long-standing limitation. It would be possible to change to "monomorphising" types per instance with some sort of merging of identical shapes, but every Python class corresponding to an ARTIQ Python type is a pretty central part of the current design. The SUServo driver does not take into account this facet of the design (at least not while keeping compatibility with the non-SUServo Urukul driver in the same kernel). This is a real-world issue, as evidenced not just by the Oxford group's, but several other labs' reports here or on the forum, so it obviously needs to be fixed.

If you want to play games about the "proper fix", I'd argue that this points to reverting the ProtoRev9 refactor instead, as it broke an important use case. Of course, the sensible solution is just to tweak the refactored driver to once again allow running the two variants side-by-side.


ProtoRev8 = ProtoRev8 # needed to avoid polymorphic issues
ProtoRev9 = ProtoRev9 # needed to avoid polymorphic issues

def __init__(
self,
dmgr,
spi_device,
io_update_device=None,
dds_reset_device=None,
sync_device=None,
sync_sel=0,
clk_sel=0,
clk_div=0,
rf_sw=0,
refclk=125e6,
att=0x00000000,
sync_div=None,
proto_rev=0x08,
core_device="core",
):
# Separate IO_UPDATE TTL output device used by SUServo core,
# if active, else by artiq.coredevice.suservo.AD9910
# :meth:`measure_io_update_alignment`.
# The urukul.CPLD driver utilises the CPLD CFG register
# option instead for pulsing IO_UPDATE of masked DDSs.
self.io_update_ttl = dmgr.get(io_update_device)
urukul.CPLD.__init__(
self,
dmgr,
spi_device,
io_update_device,
dds_reset_device,
sync_device,
sync_sel,
clk_sel,
clk_div,
rf_sw,
refclk,
att,
sync_div,
proto_rev,
core_device,
)
Comment on lines +590 to +629

@dnadlinger dnadlinger Sep 27, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Which part of this is actually necessary for this PR? It seems like this might again all be related to phase-coherent SUServo support, so it doesn't make sense to upstream at this point.

11 changes: 7 additions & 4 deletions artiq/coredevice/urukul.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -724,6 +724,9 @@ class CPLD:

kernel_invariants = {"refclk", "bus", "core", "io_update", "clk_div", "proto_rev"}

ProtoRev8 = ProtoRev8 # needed for compatibility with SUServo devices
ProtoRev9 = ProtoRev9 # needed for compatibility with SUServo devices

def __init__(self, dmgr, spi_device, io_update_device=None,
dds_reset_device=None, sync_device=None,
sync_sel=0, clk_sel=0, clk_div=0, rf_sw=0,
Expand Down Expand Up @@ -753,14 +756,14 @@ def __init__(self, dmgr, spi_device, io_update_device=None,

self.proto_rev = proto_rev
if proto_rev == STA_PROTO_REV_8:
self.version = ProtoRev8(self)
self.version = self.ProtoRev8(self)
elif proto_rev == STA_PROTO_REV_9:
self.version = ProtoRev9(self)
self.version = self.ProtoRev9(self)
else:
raise ValueError(f"Urukul unsupported proto_rev: {proto_rev}")

if self.proto_rev == STA_PROTO_REV_8:
self.cfg_reg = int64(ProtoRev8.urukul_cfg(
self.cfg_reg = int64(self.ProtoRev8.urukul_cfg(
rf_sw=rf_sw,
led=0,
profile=DEFAULT_PROFILE,
Expand All @@ -773,7 +776,7 @@ def __init__(self, dmgr, spi_device, io_update_device=None,
clk_div=clk_div,
))
else:
self.cfg_reg = int64(ProtoRev9.urukul_cfg(
self.cfg_reg = int64(self.ProtoRev9.urukul_cfg(
rf_sw=rf_sw,
led=0,
profile=(DEFAULT_PROFILE << 9)
Expand Down