add suservo CPLD#2862
Conversation
| """ | ||
| Needed to avoid polymorphic issues when using SUServo and regular Urukuls in the same experiment. | ||
| """ | ||
| rb_len = 8 |
There was a problem hiding this comment.
This seems related to phase-coherent suservo support and probably shouldn't be in this PR.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@dnadlinger Your PR to fix the bug in the legacy compiler would be appreciated.
There was a problem hiding this comment.
And adding hacks/workarounds is never "cleaning up".
There was a problem hiding this comment.
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.
| 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, | ||
| ) No newline at end of file |
There was a problem hiding this comment.
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.
|
For context, see #2112 and several similar issues/forum threrads. Our usual out-of-tree workaround doesn't easily translate, because the constructor now creates |
3 participants
With introduction of support for
ProtoRev9incoredevice.urukul, workaround for using both SUServo and normal Urukul devices in the same experiment no longer working due to polymorphism issues.Introducing a suservo specific
CPLDand providing class attributes for bothProtoRev8andProtoRev9incoredevice.suservoallows for using both SUServo and normal Urukul devices again.