Skip to content

Conversation

nkrackow
Copy link
Contributor

The clamping mechanism in the IIR was broken. It would clamp the positive side to positive fullscale but actually flip a negative output to positive fullsacale instead of clamping at 0 as intended originally.

The problem was that apparently the negative words in the DSP registers don't get their sign bit extended to the full register sizes. Therefore the MSB of the output never changed. I added manual replication of the sign bit to fill the registers. Now both sides of clamping work as intended.

DSP usage and timing didn't change.

@nkrackow
Copy link
Contributor Author

from artiq.coredevice import phaser
from artiq.experiment import *

class Test(EnvExperiment):
    def build(self):
        self.setattr_device("core")
        self.setattr_device("core_dma")
        self.phaser = self.get_device("phaser0")

    @kernel
    def run(self):
        self.core.reset()
        self.phaser.init()
        ch = self.phaser.channel[0]
        ch.set_att(0.0)
        self.phaser.write8(phaser.PHASER_ADDR_ADC_CFG, 0b1) # +- 1V inputs
        ch.oscillator[0].set_frequency(100 * MHz)
        ch.oscillator[0].set_amplitude_phase(1.0, 0.0)
        profile = 0
        ch.set_iir(profile=profile, kp=0.0, ki=0., g=0., y_offset=-0.2)
        ch.set_servo(profile=profile, enable=1, hold=0)

leads to full output with current master, no output (as expected) with this PR.

@jordens
Copy link
Member

jordens commented Aug 29, 2025

Standard Migen assignment does sign extensin. Please find a way to use that without manual sign extension. This might also be related to one of the migen issues.

Copy link
Member

@jordens jordens left a comment

Choose a reason for hiding this comment

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

Please find a way to do this with less code and especially without manual sign extension.

@nkrackow
Copy link
Contributor Author

Length-matching the inputs works. Maybe related to m-labs/migen#102?

This made me realize the following:
Before the IIR offset was not fixed-point aligned to the coefficients/data and therefore effectively didn't get the a0 gain. In my latest change this is the case and therefore we'd have to take this into account when calculating the effective offset. Until now we defined this only as the coefficient shift. Now it would include the offset.

This would have the downside that it further reduces effective resolution of the setpoint when used as a PI controller in a realistic scenario. There you already often get effective offsets of eg. 25 when stabilizing to half positive input range.

@nkrackow nkrackow requested a review from jordens September 3, 2025 13:46
@nkrackow
Copy link
Contributor Author

In order to not complicate things I reverted to simply hardcoding the correct input sizes. This fixes the problem in a non-invasive way and without changes necessary elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants