search

DiCarloLab-Delft / PycQED_py3

Python3 version of PycQED using QCoDeS as backend
MIT License
67 stars 40 forks source link

Add DIO calibration routines for CC->SHFQA communication #701

Closed martinr-zi closed 2 years ago

martinr-zi commented 2 years ago

Changes proposed in this pull request:

wvlothuizen commented 2 years ago

This PR breaks CI even further than it was for Python 3.6 (which still is the current version as of today), since zhinst-deviceutils doesn't seem to be available for 3.6

I think the change to CC.py needs to be reverted, since it introduces a dependency between drivers. Preferably, all necessary knowledge on DIO mode details should come frompycqed/instrument_drivers/library/DIO.py

wvlothuizen commented 2 years ago

@martinr-zi just tagging you: don't know whether comments on closed PR's like above trigger notification

martinr-zi commented 2 years ago

The dependency between drivers you are referring to has been there all along, only in its worst form from a design perspective - the function output_dio_calibration_data in CC.py is quite full of hardcoded magic numbers that are hardware specific. For the SHFQA, this dependency is fully explicit, which IMO is actually an improvement. Not only because it actually lays out the actual dependencies programatically, but also because it reduces potential errors - things specified explicitly in one place are much easier to maintain than matching programatically unrelated magic numbers split over different files.

I do not have the necessary knowledge to comment on the idea of putting DIO mode knowledge into the DIO library.

In any case, I believe encapsulating DIO communication in a classes separate from drivers is a quite substantial refactoring effort out of scope of this PR, which focuses on getting things working with the SHFQA in the same way as is done for other devices.

wvlothuizen commented 2 years ago

A very practical and pressing problem is that this PR breaks the CC driver on Python3.6 (because of the indirect dependence on zhinst-deviceutils), which is the version currently used in normal lab setups. Although the switch to 3.7 is planned, changes to branch 'develop' cannot require 3.7 today.

My proposal is revert this PR, and move the changes to a new branch (and accompanying PR) dedicated to SHFQA development (e.g. feature/shfqa). I guess by the time SHF support is finished, the move to 3.7 will have happened too. A discussion on how to bring DIO knowledge into the CC driver (and other drivers) can then take place within the context of that new PR.

martinr-zi commented 2 years ago

Now I understand, sorry. See my proposed fix in https://github.com/DiCarloLab-Delft/PycQED_py3/pull/705.