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

Instream improvements #78

Merged
merged 36 commits into from
Jun 21, 2023
Merged

Instream improvements #78

merged 36 commits into from
Jun 21, 2023

Conversation

Neverhorst
Copy link
Member

@Neverhorst Neverhorst commented Jun 5, 2023

Description

Expands and improves the DataInStreamInterface in various ways:

  • Added SampleTiming Enum to constraints that specifies one of three timing modes:
    1. CONSTANT: The sample rate is deterministic and constant. Each sample in the stream has a fixed timing determined by the sample rate.
    2. TIMESTAMP: The sample rate is just a hint for the hardware but can not be considered constant. The hardware will provide a numpy.float64 timestamp in seconds from the start of the stream for each sample. This requires an additional timestamp buffer array in addition to the normal channel sample buffer.
    3. RANDOM: The sample rate is just a hint for the hardware but can not be considered constant. There is no deterministic time correlation between samples, except that they are acquired one after another.
  • Removed return values from interface methods as agreed on as general practice for new/revised modules.
  • Comprehensive exception handling and meaningful error messages
  • Cleaned-up settings configuration

InStreamDummy, NIXSeriesInStreamer, TimeSeriesReaderLogic and TimeSeriesGui modules have been adapted to the interface changes.

Rudimentary data saving has been implemented in the TimeSeriesReaderLogic module.

Sample generation in InStreamDummy module has been improved and new SampleTiming modes have been implemented (choose via ConfigOption)

Motivation and Context

The main feature was the introduction of the SampleTiming into the interface. Several new projects were missing a possibility to use the streaming interface with non-uniform sampling.

See also Issue #59

How Has This Been Tested?

Extensively tested with dummy module and time series toolchain.
Needs further testing with NI X-series hardware.

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.

@Neverhorst Neverhorst added bug Something isn't working enhancement New feature or request labels Jun 5, 2023
… dialog. The user can choose between a fixed number of fractional digits or automatic scaling with SI prefix and 5 fractional digits.
@qku
Copy link
Contributor

qku commented Jun 7, 2023

Awesome @Neverhorst ! I am happy that we are going with floats for the timestamps. I played around with np.datetime64 but found handling different units to be a nightmare.

@NicolasStaudenmaier
Copy link
Contributor

I have tested the NIXSeriesInStreamer using NI USB-6343 with one practical digital channel. Additionally, I have configured several analog and digital channels displaying only noise. Everything looks good to me. Data saving works well as it is supposed in this minimal form.

@qku
Copy link
Contributor

qku commented Jun 15, 2023

I also tested the NIXSeriesInStreamer using NI USB-6343 with a single digital channel (connected to a SPCM). Everything seems to work fine with the GUI and logic. I was able to record and save data. I must note though that an installation of qudi-core from source was neccesary due to changes with ScalarConstraint. A 1.3.1 release might be useful here.

In addition, I am testing this with the wavemeter implementation I am working on right now. Almost everything seems to work fine here including multiple channels. However, I had to remove a with _lock from the callback function to make it work, otherwise it freezes when used with time_series_reader_logic. This wasn't the case before these instreamer changes, but changes in the wavemeter implementation itself might also have caused this bug.

@Neverhorst
Copy link
Member Author

Thank you for testing.
Yes, the recent changes in qudi-core are needed, but until now none of the maintained dependent repos were relying on it, so it was just the current dev version. Once this PR merges, I will release a new qudi-core version and update the qudi-iqo-modules package dependencies.

Apart from that I'm still not satisfied with the buffer requirements in the DataInStreamInterface. In most "normal" multichannel buffer/streaming environment the memory layout of a sample buffer is channel-interleaved, meaning each sample is grouped by acquisition time and not by channel, i.e.:

+-----------------------------------------+
| Ch1 | Ch2 | Ch3 | Ch1 | Ch2 | Ch3 | ... | 
+-----------------------------------------+

This is the preferred way for data streams because you can simply append new samples to such an array without copying samples around. The current version of nidaqmx unfortunately has the other option (grouping by channel) hardcoded which needs rearrangement of a buffer each time new samples are acquired. Since the C-API of NI supports both buffer options (the Python wrapper just drops the second possibility) I really would like to make "grouping-by-acquisition" the required buffer format in the interface. It just is much more efficient and "standard".

- Moved everything to handle contiguous C-style buffers with multiplexed channel layout for more efficient memory handling.
@qku
Copy link
Contributor

qku commented Jun 20, 2023

The two most recent commits seem to have broken something - I get a zero cts/s reading with our nidaq even though there should be around 1 kHz of counts. The analog channels work for me, at least they display the same noise as the nidaq test panel does. Can you confirm that with your nidaq @NicolasStaudenmaier ?

@NicolasStaudenmaier
Copy link
Contributor

The two most recent commits seem to have broken something - I get a zero cts/s reading with our nidaq even though there should be around 1 kHz of counts. The analog channels work for me, at least they display the same noise as the nidaq test panel does. Can you confirm that with your nidaq @NicolasStaudenmaier ?

Yes, I can confirm. If I have more than one digital channel the data of the first is displayed as the second displayed digital channel. This only of the first digital channel is actually displayed, if it is not displayed its data is not shown anywhere.
Furthermore, when I have configured analog channels as well but want to display only the digital one(s), then I get a "Getting samples from streamer failed" error. Still buffer handling ...

Getting samples from streamer failed. Stopping streamer.

ValueError: could not broadcast input array from shape (2,) into shape (1,)
Traceback (most recent call last):

  File "d:\users\thomas\documents\git\qudi-env\qudi-iqo-modules\src\qudi\hardware\ni_x_series\ni_x_series_in_streamer.py", line 450, in read_data_into_buffer
    data_buffer[channel_offset:total_samples:channel_count] = self.__tmp_buffer[

@Neverhorst
Copy link
Member Author

Neverhorst commented Jun 20, 2023

Yeah, digital channels are difficult/impossible to debug with a simulated device due to our weird way of setting up the task, so thanks for the input. I can not check if the digital channel output shown in the instreamer makes sense, but at least the error you reported @NicolasStaudenmaier is gone now. I suspect it was the cause of all the trouble.

@NicolasStaudenmaier
Copy link
Contributor

Yes, for me now it works as I would expect it and as it was before the two commits of yesterday.

@qku
Copy link
Contributor

qku commented Jun 20, 2023

For me as well! I can read out one digital and one analog channel simultaneously without issue. Awesome!

@Neverhorst
Copy link
Member Author

@qku @NicolasStaudenmaier ,
happy to hear. 👍
Is it ready to merge in your eyes? If so, please approve.

@qku
Copy link
Contributor

qku commented Jun 21, 2023

Yes, it is ready to merge in my eyes.

Copy link
Contributor

@NicolasStaudenmaier NicolasStaudenmaier left a comment

Choose a reason for hiding this comment

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

With testing by @qku and my side and fixing the bugs as of the discussion, looks ready to merge now.

- Raised package requirement to `qudi-core>=1.4.0`
@Neverhorst Neverhorst merged commit 14ba0c7 into main Jun 21, 2023
@Neverhorst Neverhorst deleted the instream-improvements branch June 21, 2023 09:09
LukePiewalker added a commit that referenced this pull request Oct 2, 2023
…ummy from number_of_samples to samples_per_channel. I assume this was overseen in PR #78 and the only real hardware in main (NIXSeriesInStreamer) is actually already written that way.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants