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

New Example for Time Based Synchronization #674

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MAnoushe-NI
Copy link

I've added tests applicable for this pull request

What does this Pull Request accomplish?

Adds an example for Time-Based Synchronization

Why should this Pull Request be merged?

No examples in nidaqmx python for this.

What testing has been done?

Confirmed working with cDAQ and AI Module

@zhindes
Copy link
Collaborator

zhindes commented Feb 24, 2025

I really appreciate the new example; thank you! Some requests:

  • Please create a new folder called time_based in examples/synchronization and put this here.
  • Rename this example voltage_acq_int_clk_time_start.py
  • Add a summary at the top of the example. e.g., Take the one from voltage_acq_int_clk.py and modify it according to the time-based nature of your changes

I'll make more specific code comments in the file itself.

Comment on lines +33 to +34
# Perform the measurement
perform_measurement()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Our examples usually prefer simplicity. I would remove this function and simply put your with ... task: block at the top-level

time.sleep(1)

# Read data after the task starts
data = task.read(number_of_samples_per_channel=num_samples)
Copy link
Collaborator

@zhindes zhindes Feb 24, 2025

Choose a reason for hiding this comment

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

This is a more standard read from a finite task:

Suggested change
data = task.read(number_of_samples_per_channel=num_samples)
data = task.read(READ_ALL_AVAILABLE)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note you'll need to import READ_ALL_AVAILABLE or fully specify it

Comment on lines +12 to +13
num_samples = 1000
task.timing.cfg_samp_clk_timing(1000, sample_mode=AcquisitionType.FINITE, samps_per_chan=num_samples)
Copy link
Collaborator

@zhindes zhindes Feb 24, 2025

Choose a reason for hiding this comment

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

With my suggested change to the read, this can be simplified

Suggested change
num_samples = 1000
task.timing.cfg_samp_clk_timing(1000, sample_mode=AcquisitionType.FINITE, samps_per_chan=num_samples)
task.timing.cfg_samp_clk_timing(1000, sample_mode=AcquisitionType.FINITE, samps_per_chan=1000)

Comment on lines +15 to +16
# Calculate the start time (1 minute from now)
start_time = datetime.now(timezone.utc) + timedelta(minutes=1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

1 minute is a lot...

Suggested change
# Calculate the start time (1 minute from now)
start_time = datetime.now(timezone.utc) + timedelta(minutes=1)
# Calculate the start time (10 seconds from now)
start_time = datetime.now(timezone.utc) + timedelta(seconds=10)

Comment on lines +17 to +26
print(f"Scheduled start time: {start_time}")

# Configure the time start trigger
task.triggers.start_trigger.cfg_time_start_trig(start_time)

print("Starting the task...")
task.start()

print("Waiting for the scheduled start time...")
# Wait until the scheduled start time
Copy link
Collaborator

Choose a reason for hiding this comment

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

Two things:

  1. Generally printing commentary isn't really necessary, but for something that must wait, I like it. But let's not overdo it.
  2. You don't need comments that repeat the code. I like the comment describing how the time delta stuff works out. These feel a bit less useful.
Suggested change
print(f"Scheduled start time: {start_time}")
# Configure the time start trigger
task.triggers.start_trigger.cfg_time_start_trig(start_time)
print("Starting the task...")
task.start()
print("Waiting for the scheduled start time...")
# Wait until the scheduled start time
print(f"Scheduled start time: {start_time}")
task.triggers.start_trigger.cfg_time_start_trig(start_time)
task.start()
print("Waiting for the scheduled start time...")

task.timing.cfg_samp_clk_timing(1000, sample_mode=AcquisitionType.FINITE, samps_per_chan=num_samples)

# Calculate the start time (1 minute from now)
start_time = datetime.now(timezone.utc) + timedelta(minutes=1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI: I was going to ask whether it's really necessary to specify UTC here, but then I looked up whether datetime.now() returns an "aware" datetime and now I am sad.

https://discuss.python.org/t/datetime-api-no-atomic-way-to-get-aware-current-local-datetime-when-moving-between-time-zones/55041/8
https://stackoverflow.com/questions/24281525/what-is-the-point-of-a-naive-datetime

Comment on lines +27 to +28
while datetime.now(timezone.utc) < start_time:
time.sleep(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a DAQmx API for this, Task.wait_for_valid_timestamp, so you don't need to poll.
https://github.com/ni/nidaqmx-python/blob/master/generated/nidaqmx/task/_task.py#L1028

I think you can wait for either TimestampEvent.FIRST_SAMPLE or TimestampEvent.START_TRIGGER.

I assume you are doing this to avoid getting a read timeout. FYI, another solution is to increase the read timeout to account for the scheduled start time, but that's probably less clear.

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.

3 participants