Skip to content

fix iir clamping - #29

Open
nkrackow wants to merge 6 commits into
quartiq:masterfrom
nkrackow:nk/fix_iir_clamping
Open

fix iir clamping#29
nkrackow wants to merge 6 commits into
quartiq:masterfrom
nkrackow:nk/fix_iir_clamping

Conversation

@nkrackow

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor
fromartiq.coredeviceimportphaserfromartiq.experimentimport*classTest(EnvExperiment):
defbuild(self):
self.setattr_device("core")
self.setattr_device("core_dma")
self.phaser=self.get_device("phaser0")
@kerneldefrun(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 inputsch.oscillator[0].set_frequency(100*MHz)
ch.oscillator[0].set_amplitude_phase(1.0, 0.0)
profile=0ch.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
Copy Markdown
Member

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.

@jordensjordens left a comment

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.

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

@nkrackow

Copy link
Copy Markdown
ContributorAuthor

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

Copy link
Copy Markdown
ContributorAuthor

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.

@jordens

Copy link
Copy Markdown
Member

Are those new c and m widths those of the actual DSP primitive? And does it infer in the same it did before?

Sign up for freeto 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

@nkrackow@jordens