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

Spectrum instrumentation ADC as fast_counter hardware #131

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

takuya-ulm
Copy link
Contributor

@takuya-ulm takuya-ulm commented May 13, 2024

Description

AD converter from spectrum instrumentation is implemented as fast_counter hardware.

Motivation and Context

AD card from spectrum instrumentation is newly implemented.
The class structure is organized for robustness.

How Has This Been Tested?

This hardware is tested in the pulsed toolchain.
This change does not affect the rest of the code.

Screenshots (only if appropriate, delete if not):

Types of changes

  • Bug fix
  • New feature
  • Breaking change (Causes existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • I have documented my changes in /docs/changelog.md.
  • My change requires additional/updated documentation.
  • I have updated the documentation accordingly.
  • I have added/updated the config example for any module docstrings as necessary.
  • I have checked that the change does not contain obvious errors
    (syntax, indentation, mutable default values, etc.).
  • I have tested my changes using 'Load all modules' on the default dummy configuration.
  • All changed Jupyter notebooks have been stripped of their output cells.

@takuya-ulm takuya-ulm marked this pull request as ready for review May 14, 2024 15:41
Copy link
Contributor

@TobiasSpohn TobiasSpohn left a comment

Choose a reason for hiding this comment

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

Hi Takuya,

Great code. I added some small suggestions in the comments.
In my comments about the while-loops, making the thread unresponsive: It might be okay to leave it as is, if the time spent in the loop is very short, so the thread is not blocked for too long.

@Neverhorst
Copy link
Member

Where does the pyspcm dependency come from? It is not listed in setup.py.
Is this derivative work of this project? If so, this needs proper attribution and copyright/license disclaimers.

@takuya-ulm
Copy link
Contributor Author

Where does the pyspcm dependency come from? It is not listed in setup.py. Is this derivative work of this project? If so, this needs proper attribution and copyright/license disclaimers.

Spectrum instrumentation originally distributed some python codes with an USB stick. Recently, they shared the codes on github with MIT license:
https://github.com/SpectrumInstrumentation/spcm
Now I added this package in setup.py and modified the imports.

Copy link
Contributor Author

@takuya-ulm takuya-ulm left a comment

Choose a reason for hiding this comment

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

@TobiasSpohn

Thank you for your reviewing. I modified the codes and replied to your comments.
Can you have a look on them?

Copy link
Contributor

@TobiasSpohn TobiasSpohn left a comment

Choose a reason for hiding this comment

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

Thanks for the modified code, it looks good from my side. I only have the one comment in time_series_gui.py, which I think might be a syntax error.

@@ -326,7 +326,7 @@ def update_channel_settings(self, enabled, averaged):
units=different_units[0])
self._mw.trace_plot_widget.setLabel('right',
self._channels_per_axis[1][0],
units=different_units[1])
rev units=different_units[1])
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this "rev" supposed to be here?


If the hardware does not support these features, the values should be None
"""
with self._threadlock:
Copy link
Contributor

@timoML timoML Jun 27, 2024

Choose a reason for hiding this comment

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

This thread lock looks sketchy. It is not taken when writing to the same data in DataProcess.

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.

4 participants