Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extend SUServo to variable number of Urukul cards #1782

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Extend SUServo to variable number of Urukul cards #1782

wants to merge 8 commits into from

Conversation

pmldrmota
Copy link
Contributor

@pmldrmota pmldrmota commented Nov 17, 2021 •
edited
Loading

ARTIQ Pull Request

Description of Changes

  • Extend IIR filter to any number of ADC input channels and DDS output channels with minimal ressource usage. (The number of channels was previously restricted to a power of 2, and to be the same for inputs and outputs.)
  • EEM support for one Sampler and a variable number of Urukuls.
  • More efficient mapping of RTIO addresses used by SUServo.
  • Fix migen simulations for python >=3.7

Related Issue

Closes #1339
Closes #1748

Type of Changes

Type
✓ ✨ New feature
✓ 🐛 Bug fix

Steps (Choose relevant, delete irrelevant before submitting)

All Pull Requests

  • Use correct spelling and grammar.
  • Update RELEASE_NOTES.rst 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
  • 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

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+.

@pmldrmota
Copy link
Contributor Author

This particular revision has not been tested in isolation, only as part of continuative code changes.
I was not able to build the docs yet due to "OSError: no library called 'cairo-2' was found".

@pmldrmota
gateware.test.suservo: Fix tests for python >=3.7
1d8a06b 
Resolves m-labs#1748.
@pmldrmota
suservo.iir: Extend to arbitrary number of in/output channels
fe09479
@pmldrmota
gateware.eem.SUServo: Extend to arbitrary number of Urukuls
61199a5
@pmldrmota
gateware.suservo.pads: Update UrukulPads for channel extension
d972393
@pmldrmota
gateware.rtio.phy.servo: Update for channel extension
01176c0
@pmldrmota
suservo: Optimise rtlink address mapping for arbitrary numbers of cha…
ffd477f 
…nnels

The coefficient memory address is typically 2 bits wider than the
address of the config, status and IIR state memories. In anticipation
of an additional rtlink destination, we subdivide the address space
into coefficient memory (1 selection bit) and otherwise four selectable
destinations (2 selection bits) which don't need those 2 bits to span
the complete address space.
@pmldrmota
coredevice.suservo: Add setter method for phase only
33a3972
@pmldrmota
Release notes: highlight variable number of Urukul cards on SUServo
0d278f2 
Signed-off-by: Peter Drmota <peter.drmota@physics.ox.ac.uk>
@sbourdeauducq
Copy link
Member

I was not able to build the docs yet due to "OSError: no library called 'cairo-2' was found".

I can provide a shell.nix that solves this issue if you want?

@sbourdeauducq
Copy link
Member

This particular revision has not been tested in isolation, only as part of continuative code changes.

Would you be able to test it, i.e. take unmodified ARTIQ, apply exactly these changes, and test carefully?

sbourdeauducq
sbourdeauducq reviewed Nov 18, 2021
View reviewed changes
RELEASE_NOTES.rst
Comment on lines -107 to -109
* To support variable numbers of Urukul cards in the future, the
``artiq.coredevice.suservo.SUServo`` constructor now accepts two device name lists,
``cpld_devices`` and ``dds_devices``, rather than four individual arguments.
Copy link
Member

Choose a reason for hiding this comment

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

This should not be removed from the ARTIQ-6 release notes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this was a mistake - these changes were not yet part of c224827 (ARTIQ-6 tag). Sorry for missing this in the last PR.

@pmldrmota
Copy link
Contributor Author

Would you be able to test it, i.e. take unmodified ARTIQ, apply exactly these changes, and test carefully?

I can test them on 11790c6 if that's alright, as our build suite doesn't build the VexRiscv yet.

@pmldrmota pmldrmota mentioned this pull request Jan 6, 2022
Closed
sbourdeauducq added a commit that referenced this pull request Sep 5, 2022
@sbourdeauducq
experimental-features: add SU Servo extension for variable number of …
411afbd 
…Urukuls (PR #1782)
@pmldrmota pmldrmota mentioned this pull request Sep 5, 2022
Open
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbourdeauducq sbourdeauducq sbourdeauducq left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@pmldrmota @sbourdeauducq