Suservo

[hartytp]

ARTIQ Pull Request

Description of Changes

Fix assorted SUServo issues:

1. Sign extend coefficients we read back (they are stored in the LSB of int32s)
2. Fix writing of coefficients to servo memory
3. flake8 and doc fixes
4. increase delay in profile register read back to avoid underflows
5. apply bit masks to avoid corrupting servo memory

Related Issue

Closes #1258, #1256

Type of Changes

	Type
✓	🐛 Bug fix
✓	📜 Docs

Steps (Choose relevant, delete irrelevant before submitting)

All Pull Requests

• Use correct spelling and grammar.
• Update RELEASE_NOTES.md if there are noteworthy changes, especially if there are changes to existing APIs.
• Close/update issues.
• Check the copyright situation of your changes and sign off your patches (git commit --signoff, see copyright).

Code Changes

• Run flake8 to check code style (follow PEP-8 style). flake8 has issues with parsing Migen/gateware code, ignore as necessary.
• Test your changes or have someone test them. Mention what was tested and how.
• Add and check docstrings and comments

Tested using the following script under artiq5.0.dev:

import numpy.random as rand

from artiq.experiment import *
from artiq.coredevice.suservo import COEFF_WIDTH

class Startup(EnvExperiment):
    def build(self):
        self.setattr_device("core")
        self.setattr_device("aom_servo")
        self.setattr_device("aom_397_doppler")
        self.offset = -0.00817

    def run(self):

        coeff_max = 1 << COEFF_WIDTH - 1

        self.ftw = self.pow_ = self.offs = 0

        for i in range(1000):

            self.a1 = rand.randint(low=-coeff_max, high=coeff_max)
            self.b1 = rand.randint(low=-coeff_max, high=coeff_max)
            self.b0 = rand.randint(low=-coeff_max, high=coeff_max)
            self.adc = rand.randint(low=0, high=8)
            self.dly = rand.randint(low=0, high=256)
            self.profile = rand.randint(low=0, high=8)
            self.freq = rand.random() * 400*MHz
            self.phase = (rand.random() - 0.5) * 2
            self.offset = (rand.random() - 0.5) * 2

            self.krun()
        print("pased")

    @kernel
    def krun(self):
        dds = self.aom_servo.dds0
        self.ftw = dds.frequency_to_ftw(self.freq)
        self.pow_ = dds.turns_to_pow(self.phase)
        self.offs = int(round(self.offset*(1 << COEFF_WIDTH - 1)))

        self.core.break_realtime()

        self.aom_397_doppler.set_dds_mu(self.profile,
                                        self.ftw,
                                        self.offs,
                                        self.pow_)
        delay(10*us)
        self.aom_397_doppler.set_iir_mu(self.profile,
                                        self.adc,
                                        self.a1,
                                        self.b0,
                                        self.b1,
                                        self.dly)

        data = [0] * 8
        self.aom_397_doppler.get_profile_mu(self.profile, data)

        assert self.ftw >> 16 == data[0]
        assert self.b1 == data[1]
        assert self.pow_ == data[2]
        assert self.adc | (self.dly << 8) == data[3]
        assert self.offs == data[4]
        assert self.a1 == data[5]
        assert self.ftw & 0xffff == data[6]
        assert self.b0 == data[7]

[hartytp]

@jordens any feedback on this PR, or am I okay to merge?

[jordens]

Not so fast. This mixes up a lot of things. Please consider splitting it. Also you still need to check how much of this is a regression from 4.0. If this is just a side effect of a bigger regressions, then it should be approached differently. As I said, it's not just the compiler changes.

[hartytp]

@jordens ack. I'll split this into smaller PRs.

AFAICT at least some elements of PR are straight forward bug fixes, where there is a clear issue with the current code and a clear-cut solution to it modulo stylistic concerns (for example, eb2d712). In these cases, it shouldn't be necessary to demonstrate symptoms with a different version of ARTIQ to merge the bug fix. So, let's get those aspects merged first and then discuss the less clear-cut elements of the PR.