SUServo: Coherent phase tracking mode #1467

pmldrmota · 2020-06-16T19:19:01Z

ARTIQ Pull Request

Description of Changes

SUServo upgrade:

Refactoring of the pipelined IIR processing core, accepting any number of ADC input channels and DDS output channels with minimal ressource usage. (Previously restricted to a power of 2.)
Using half-duplex SPI mode for readback from Urukul CPLDs in NU-mode (DIP switches configured for SUServo operation). To use this feature, the CPLDs require the corresponding update.
Extension of the per-channel control interface by a flag en_pt, serving to enable coherent DDS phase mode. This works by tracking the DDS-internal phase accumulator as well as the servo runtime to calculate the POW accordingly.
Reverse-compatible extension of the channel RTIO addresses for setting the reference time of a channel for coherent phase mode in real time (intended for use in DMA sequences).
To ensure coherent phase updates, the io_update line of an Urukul is converted into a SERDES output. The I/O update alignment delays can be configured by writing to servo memory. The optimal alignment delay can be found using the existing method, relying on half-duplex CPLD readback capability.
Further, to ensure coherent phase updates, the sync_in line of all Urukuls is connected to a ClockGen phy. The optimal sync settings can be found using the existing method, relying on half-duplex CPLD readback capability.
Restructuring of the servo RTIO addresses to make space for the I/O update alignment delay memory. This is done efficiently and does not increase the address width.

Related Issue

Resolves #1339

Type of Changes

	Type
✓	✨ New feature
✓	🔨 Refactoring

Steps

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 (@dnadlinger). Report is in preparation.
Add and check docstrings and comments
Check, test, and update the unittests in /artiq/test/ or gateware simulations in /artiq/gateware/test

Documentation Changes

Check, test, and update the documentation in doc/. Build documentation (cd doc/manual/; make html) to ensure no errors.

Git Logistics

Split your contribution into logically separate changes (git rebase --interactive). Merge/squash/fixup commits that just fix or amend previous commits. Remove unintended changes & cleanup. See tutorial.
Write short & meaningful commit messages. Review each commit for messages (git show).

Licensing

Credits for this also to @dnadlinger !
See copyright & licensing for more info.
ARTIQ files that do not contain a license header are copyrighted by M-Labs Limited and are licensed under LGPLv3+.

jordens

There are really good ideas and valuable work here and most of it should probably ultimately land in ARTIQ. I'm happy to review them and merge them.

But I see no way to review this PR properly. We need to ensure that it doesn't break on the dozens of installations out there. It seems to be still far away from something mergeable. As I said before you have a much better chance of getting anything through by breaking it up into small PRs that can be reasoned about merged, and tested individually. I'm not saying the existing code is perfect or clean or should not be improved and fixed. But I think if we want to do that, we need a broader plan than this PR.

For example, one could suspect that the half-duplex changes are not backwards compatible breaking all existing users unless they upgrade the CPLD gateware (and that doesn't even have a PR yet). There seems to be no assessment of compatibility here. None of the changes are mentioned or explained in the RELEASE NOTES.

jordens · 2020-07-15T07:51:54Z

artiq/gateware/eem.py

@@ -125,17 +126,17 @@ def io_qspi(eem0, eem1, iostandard="LVDS_25"):
            ),
        ]
        ttls = [(6, eem0, "io_update"),
-                (7, eem0, "dds_reset_sync_in"),
+                (7, eem0, "dds_reset_sync_in", Misc("IOB=TRUE")),


As pointed out elsewhere, this leads to

CRITICAL WARNING: [Place 30-722] Terminal 'urukul5_dds_reset_sync_in_n' has IOB constraint set to TRUE, but it is either not connected to a FLOP element or the connected FLOP element could not be brought into the I/O

which indicates a problem. Please investigate and resolve that.

We didn't get this warning. Without it, we faced the symptoms of #1382 again. This just applies the same patch (bc3b55b), which seems to work for everyone. I'm not sure why it would pose a problem here, but maybe #1384 is a better place to discuss a proper solution?

The warning shows up since that patch was applied. "Seems to work for everyone" can't be our benchmark for robustness, correctness and maintainability. Yes. Let's discuss it there.

jordens · 2020-07-15T07:52:34Z

artiq/coredevice/suservo.py

+
+    @kernel
+    def measure_io_update_alignment(self, delay_start, delay_stop):
+        """Refer to artiq.coredevice.ad9910 :meth:`measure_io_update_alignment`.


Please make an effort to adhere to the prevalent coding style.

jordens · 2020-07-15T07:52:44Z

artiq/coredevice/suservo.py

+    @kernel
+    def read32(self, addr):
+        """
+        ! This method returns only the .suservo.CPLD.rb_len least significant bits !


pmldrmota · 2020-07-29T07:44:19Z

@jordens Thank you for your feedback and agreeing to review our changes. I will break it up into even smaller pieces as you suggested.

jordens · 2020-07-29T15:47:15Z

Thanks. Please close this PR once superseded.

sbourdeauducq · 2022-09-05T02:47:37Z

AFAICT it has not been superseded?

pmldrmota · 2022-09-05T08:25:00Z

Part 1 was superseded by #1782.

Since this PR, we fixed one bug (clip indicator flag was cropped to too few bits in gateware).
Moreover, after the RISC-V intro, we had to explicitly add an ODDR at the sync_in output. This supersedes the "IOB=TRUE" property in eem.py.

I can provide the patches for these two updates.

Also, we need to check that the Urukul default profile is consistent with the new default.

Requires a sufficiently up-to-date CPLD gateware.

This is prerequisite for phase coherent operation of SUServo. The I/O update alignment delays can be configured by writing to servo memory. The optimal io_update alignment delay and sync_in delay can be found using the existing AD9910 (overloaded) methods, relying on half-duplex CPLD readback capability.

Extension of the per-channel control interface by a flag en_pt, serving to enable coherent DDS phase mode. This works by tracking the DDS-internal phase accumulator as well as the servo runtime to calculate the POW accordingly. The reference time for this calculation can be set for each output channel individually in real time.

pmldrmota · 2022-09-12T16:04:52Z

@sbourdeauducq the source branch is now rebased on m-labs/master with the experimental var-urukuls patch applied. The new commits include all known fixups.
All relevant migen tests pass, and there are no noteworthy diffs across the changed files of this PR to the Oxford-internal fork. I have not tested this exact revision on hardware.
I am planning to provide CPLD gateware for v1.4+ Urukuls on the Oxford fork soon.

…1467)

sbourdeauducq · 2022-09-13T01:40:51Z

Maybe we can copy the Urukul CPLD code into the ARTIQ repository? Then the experimental-features patches would also apply.

sbourdeauducq · 2022-09-13T01:42:32Z

Or just add the patch to nix-scripts and add a Hydra build for patched CPLD, I suppose there shouldn't be too many of those patches and it may not be worth the trouble/bloat associated with using the artiq repos for this.

pmldrmota · 2022-09-27T21:25:14Z

Maybe we can copy the Urukul CPLD code into the ARTIQ repository? Then the experimental-features patches would also apply.

My instant reaction to this was git submodule.

sbourdeauducq · 2022-09-27T22:50:03Z

git submodule

hell no

pmldrmota changed the title ~~Suservo multi phasecoherent~~ Suservo extension: arbitrary number of channels and coherent phase tracking mode Jun 16, 2020

jordens requested changes Jul 15, 2020

View reviewed changes

pmldrmota closed this Jul 29, 2020

sbourdeauducq reopened this Sep 5, 2022

pmldrmota added 3 commits September 10, 2022 09:24

Apply experimental suservo-var-urukul

2502459

Urukul CPLD half-duplex readback support in SUServo configuration.

bd849c4

Requires a sufficiently up-to-date CPLD gateware.

pmldrmota changed the title ~~Suservo extension: arbitrary number of channels and coherent phase tracking mode~~ SUServo: Coherent phase tracking mode Sep 12, 2022

pmldrmota force-pushed the suservo-multi-phasecoherent branch from 2421931 to e643e47 Compare September 12, 2022 15:49

pmldrmota added 2 commits September 12, 2022 21:33

fixup artiq_ddb_template for SU Servo

9f34725

coredevice.suservo: clean up IO update alignment measurement

2aff060

sbourdeauducq added a commit that referenced this pull request Sep 13, 2022

experimental-features: add SU Servo coherent phase tracking mode (PR #…

6085fe3

…1467)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SUServo: Coherent phase tracking mode #1467

SUServo: Coherent phase tracking mode #1467

pmldrmota commented Jun 16, 2020 •

edited

Loading

jordens left a comment

jordens Jul 15, 2020

pmldrmota Jul 28, 2020

jordens Jul 29, 2020

jordens Jul 15, 2020

jordens Jul 15, 2020

pmldrmota commented Jul 29, 2020

jordens commented Jul 29, 2020

sbourdeauducq commented Sep 5, 2022

pmldrmota commented Sep 5, 2022

pmldrmota commented Sep 12, 2022

sbourdeauducq commented Sep 13, 2022

sbourdeauducq commented Sep 13, 2022

pmldrmota commented Sep 27, 2022

sbourdeauducq commented Sep 27, 2022

SUServo: Coherent phase tracking mode #1467

Are you sure you want to change the base?

SUServo: Coherent phase tracking mode #1467

Conversation

pmldrmota commented Jun 16, 2020 • edited Loading

ARTIQ Pull Request

Description of Changes

Related Issue

Type of Changes

Steps

All Pull Requests

Code Changes

Documentation Changes

Git Logistics

Licensing

jordens left a comment

Choose a reason for hiding this comment

jordens Jul 15, 2020

Choose a reason for hiding this comment

pmldrmota Jul 28, 2020

Choose a reason for hiding this comment

jordens Jul 29, 2020

Choose a reason for hiding this comment

jordens Jul 15, 2020

Choose a reason for hiding this comment

jordens Jul 15, 2020

Choose a reason for hiding this comment

pmldrmota commented Jul 29, 2020

jordens commented Jul 29, 2020

sbourdeauducq commented Sep 5, 2022

pmldrmota commented Sep 5, 2022

pmldrmota commented Sep 12, 2022

sbourdeauducq commented Sep 13, 2022

sbourdeauducq commented Sep 13, 2022

pmldrmota commented Sep 27, 2022

sbourdeauducq commented Sep 27, 2022

pmldrmota commented Jun 16, 2020 •

edited

Loading