SUServo coefficient readback

[hartytp]

Bug Report

One-Line Summary

SUServo IIR coefficient read back does not work

Steps to Reproduce

a1 = -1
profile = 0
addr = 5
base = (self.aom_397_doppler.servo_channel << 8) | (profile << 3)
self.aom_servo.write(base + addr, a1)
readback = self.aom_servo.read(base + addr)

test = [0]*32
for i in range(32):
    test[i] = 1 if ((readback << i) & (1<<31)) != 0 else 0
print(test)

Expected Behavior

The number we read back should match the number we write.

Actual (undesired) Behavior

Readback seems to be: (((base+addr) | WE) >> 8) | ((addr << 5) & (1 << COEFF_WIDTH) - 1 ))

i.e. the 5 LSB of the number we read back are (((base+addr) | WE) >> 8) and the upper 13 bits are the 13 LSB of the coefficent.

Appears to affect the a0 and b1 coefficients equally.

Your System

Artiq 5.0.dev

[hartytp]

@jordens I don't understand this: https://github.com/hartytp/artiq/blob/f70d6913bda8bd4113d45246639137a8e0fdb9de/artiq/gateware/rtio/phy/servo.py#L94

Doesn't self.rtlink.o.data contain both an address and data? Shouldn't this be something like:

coeff_data = signal(w.coeff)
self.comb += coeff_data.eq(self.rtlink.o.data[:w.coeff])
self.comb += m_coeff.dat_w.eq(Cat(coeff_data, coeff_data))

[hartytp]

i.e. won't the following fail:

assert len(self.rtlink.o.data) == w.coeff

[jordens]

@hartytp Yes that assert would fail since the new rtio layout. See https://github.com/m-labs/artiq/pull/1266#issuecomment-461424074