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.

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

Feature/dem 804/implement missing uhfli commands #1500

Merged
merged 8 commits into from
Mar 14, 2019
Merged

Feature/dem 804/implement missing uhfli commands #1500

merged 8 commits into from
Mar 14, 2019

Conversation

qSaevar
Copy link
Contributor

@qSaevar qSaevar commented Mar 4, 2019

Changes proposed in this pull request:

  • Add support for enabling, and amplitude, for all devNNN/sigouts/ in ZIUHFL driver
  • Change name of parameters signal_outputN_enable and signal_outputN_amplitude to signalN_outputM_enable1 and signalN_outputM_amplitude in ZIUHFLI driver.

@WilliamHPNielsen

@codecov
Copy link

codecov bot commented Mar 4, 2019

Codecov Report

Merging #1500 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1500   +/-   ##
=======================================
  Coverage   73.84%   73.84%           
=======================================
  Files          92       92           
  Lines       10459    10459           
=======================================
  Hits         7723     7723           
  Misses       2736     2736

@jenshnielsen
Copy link
Collaborator

Will this not only work if you have the Multi-frequency upgrade? See #978 where i started implementing this too

@qSaevar
Copy link
Contributor Author

qSaevar commented Mar 4, 2019

@jenshnielsen that could be. I'll see if we have a device without the upgrade for further testing/developing.

@jenshnielsen
Copy link
Collaborator

@qSaevar We have one without the upgrade so I can test that side but I do not have one with the upgrade. I never finished the other pr because of I couldn't test it probably.
But if we can split the testing between us that would be great. Feel free to steal the code from that pr to detect if the feature is enabled

@astafan8 astafan8 added the driver label Mar 7, 2019
@qSaevar
Copy link
Contributor Author

qSaevar commented Mar 12, 2019

@jenshnielsen I added a check, as in your PR, to see if MF is enabled. I can test this on a device later today.

@qSaevar
Copy link
Contributor Author

qSaevar commented Mar 12, 2019

@jenshnielsen I merged the code from PR 978 into this one and tested it on a device with MF enabled and it works. I do not have a device with out MF so I can not test that,

@jenshnielsen
Copy link
Collaborator

@qSaevar Thanks, I will test it on our device without mf. Hopefully tomorrow.

Copy link
Collaborator

@jenshnielsen jenshnielsen left a comment

Choose a reason for hiding this comment

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

I have verified that this works correctly without the mf functionality.
@QCoDeS/core If anyone else wants a look please do. I have been involved in
writing some of this code originally so an independent review would be good

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

The code looks correct. (but there is generally quite some room for refactoring and readability improvements.)

@qSaevar
Copy link
Contributor Author

qSaevar commented Mar 14, 2019

@astafan8 I addressed the comments you made.

@astafan8 astafan8 merged commit ebda0a8 into microsoft:master Mar 14, 2019
@qSaevar qSaevar deleted the feature/DEM-804/Implement-missing-UHFLI-commands branch March 14, 2019 15:17
giulioungaretti pushed a commit that referenced this pull request Mar 14, 2019
Merge: f95029f 5458a09
Author: Mikhail Astafev <[email protected]>

    Merge pull request #1500 from qutech-sd/feature/DEM-804/Implement-missing-UHFLI-commands
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants