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

Extend RTO1000 driver #1410

Merged
merged 11 commits into from
Jan 15, 2019
Merged

Conversation

nyxus
Copy link
Contributor

@nyxus nyxus commented Dec 4, 2018

Hi,

I've fixed problems, extended and improved the RTO1000 driver. The main extension is the measurement submodule. This allows the sure to setup, start and clear all 8 measurements. I've tested this PR using an RTO1024 on my desk.

Changes proposed in this pull request:

Driver

  • Added submodules and parameters to do a measurement (on all 8 measurements)
  • Added error count and error next parameters
  • Added trigger modes
  • Added functions check readout if the scope is running, triggered and acquiring data.
  • Changed the opc and stop_opc functions so they do not result in errors
  • Added a warning in docstring of the High definition mode

Driver example

  • Added basic example on how to use measurement
  • Added useful function to kind of automate the process of measurements

@WilliamHPNielsen @AdriaanRol

…t and next; corrected the stop_opc and opc paramaters; Added trigger mode paramater and added docstring to High defintion mode
@codecov
Copy link

codecov bot commented Dec 4, 2018

Codecov Report

Merging #1410 into master will increase coverage by 0.03%.
The diff coverage is 84.21%.

@@            Coverage Diff             @@
##           master    #1410      +/-   ##
==========================================
+ Coverage    73.3%   73.33%   +0.03%     
==========================================
  Files          91       91              
  Lines       10189    10225      +36     
==========================================
+ Hits         7469     7499      +30     
- Misses       2720     2726       +6

@nyxus
Copy link
Contributor Author

nyxus commented Dec 10, 2018

I've made the requested changes, @sohailc can you continue your review?

@nyxus
Copy link
Contributor Author

nyxus commented Dec 13, 2018

@WilliamHPNielsen can you review this pr, please?

Copy link
Contributor

@WilliamHPNielsen WilliamHPNielsen left a comment

Choose a reason for hiding this comment

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

This looks really good and comprehensive, thank you for adding all this to the driver. I don't have the scope myself, but I trust that these settings are indeed agreeing with the instrument firmware.

I have only a very shallow request and and optional one. Shallow: could you please go through the code and fix typos in the docstrings? The word "measurement" is misspelled in many places (plus a few other typos). Optional: could you be talked into updating the example notebook for this driver with these changes? It's a bit of work, since you'd need to hook up a signal generator to the scope to recreate the existing notebook, but it would be fantastic to have these new features documented somewhere.

…ions, trigger value_mapping to validator, Fixed typos; Added measurement to the RTO1000 example program
@WilliamHPNielsen
Copy link
Contributor

@nyxus, great! I noticed that you also added some more functionality. Is the driver done now, or are you going to add more later?

@nyxus
Copy link
Contributor Author

nyxus commented Dec 21, 2018

I've made the changes as requested, including expending the example notebook. Also added a couple of other functions to the driver to readout if the scope is running, triggered and acquiring data. These functions are used to check if a measurement is possible. The example notebook also contains a useful function to kind off automate the process of measuring.

@nyxus
Copy link
Contributor Author

nyxus commented Dec 21, 2018

@WilliamHPNielsen The driver is done for now. It still misses lots of functions/parameters but I did not need them. I will create new PR(s) if that changes.

@jenshnielsen
Copy link
Collaborator

@WilliamHPNielsen is this ready to merge?

@WilliamHPNielsen
Copy link
Contributor

@jenshnielsen, yes, I believe it is (sorry for slow response).

@jenshnielsen jenshnielsen merged commit 3b5e79d into microsoft:master Jan 15, 2019
giulioungaretti pushed a commit that referenced this pull request Jan 15, 2019
Merge: 6c5440a 0b1fb2e
Author: Jens Hedegaard Nielsen <jenshnielsen@gmail.com>

    Merge pull request #1410 from nyxus/ImproveRTO1000Driver
Sign up for free to 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.

None yet

4 participants