Skip to content

NIDAQ: Fix bug that caused the NIDAQ adapter to crash when running the MDA wizard. #980

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.

Sign up for GitHub

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

Merged
nicost merged 2 commits into from
Aug 4, 2026
Merged

NIDAQ: Fix bug that caused the NIDAQ adapter to crash when running the MDA wizard. #980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
295e240
NIDAQ: Fix bug that caused the NIDAQ adapter to crash when
nicost Aug 4, 2026
55fb812
NIDAQ: Address CoPilot's comments.
nicost Aug 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
214 changes: 171 additions & 43 deletions DeviceAdapters/NIDAQ/NIDAQ.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,30 +320,78 @@ int NIDAQHub::InitializeImpl()
}


void NIDAQHub::DestroyMonitoringThread(InputMonitoringThread*& t)
{
if (t == 0)
return;
t->Stop();
t->Join();
delete t;
t = 0;
}


void NIDAQHub::DestroyMonitoringThread(TraceMonitoringThread*& t)
{
if (t == 0)
return;
t->Stop();
t->Join();
delete t;
t = 0;
}


int NIDAQHub::Shutdown()
{
// Wrapper: see NIDAQHub::Initialize(). Shutdown() is also called from
// ~NIDAQHub(), where an escaping exception means immediate std::terminate.
try
{
return ShutdownImpl();
}
catch (const std::exception& e)
{
LogMessage(std::string("EXCEPTION in NIDAQHub::Shutdown: ") +
typeid(e).name() + ": " + e.what());
return DEVICE_ERR;
}
catch (...)
{
LogMessage("Unknown (non-standard) C++ exception in NIDAQHub::Shutdown");
return DEVICE_ERR;
}
}


int NIDAQHub::ShutdownImpl()
{
if (!initialized_)
return DEVICE_OK;

mThread_->Stop();
mThread_->wait();
delete mThread_;
// Destroy both threads first, so that no worker can still be running
// (and dereferencing this hub) while the members below are torn down.
//
// Order matters: the trace thread must go first. On exit its svc() calls
// hub_->FinishTrace(), which dereferences mThread_, so destroying mThread_
// while the trace thread is still running is a null dereference.
DestroyMonitoringThread(tThread_);
DestroyMonitoringThread(mThread_);

int err = StopTask(aoTask_);

physicalAOChannels_.clear();
aoChannelSequences_.clear();

if (doHub8_ != 0)
delete doHub8_;
else if (doHub16_ != 0)
delete doHub16_;
else if (doHub32_ != 0)
delete doHub32_;

tThread_->Stop();
tThread_->wait();
delete tThread_;
// Independent deletes: the previous if/else-if chain leaked doHub16_ and
// doHub32_ whenever doHub8_ was non-null, which is the normal case since
// InitializeImpl() allocates all three together. Deleting null is a no-op.
delete doHub8_;
doHub8_ = 0;
delete doHub16_;
doHub16_ = 0;
delete doHub32_;
doHub32_ = 0;

initialized_ = false;
return err;
Expand Down Expand Up @@ -964,19 +1012,15 @@ int NIDAQHub::StartAIMeasuringForPort(NIAnalogInputPort* port)
int err;
if (measuringTrace_)
{
tThread_->Stop();
tThread_->wait();
delete tThread_;
DestroyMonitoringThread(tThread_);

tThread_ = new TraceMonitoringThread(this);
err = tThread_->Start(GetPhysicalChannelListForMeasuring(physicalAIChannels_), expectedMinVoltsIn_,
expectedMaxVoltsIn_, (float) traceFrequency_, traceAmount_, (int) physicalAIChannels_.size());
}
else
{
mThread_->Stop();
mThread_->wait();
delete mThread_;
DestroyMonitoringThread(mThread_);

mThread_ = new InputMonitoringThread(this);
err = mThread_->Start(GetPhysicalChannelListForMeasuring(physicalAIChannels_), expectedMinVoltsIn_, expectedMaxVoltsIn_);
Expand All @@ -998,9 +1042,7 @@ int NIDAQHub::StopAIMeasuringForPort(NIAnalogInputPort* port)

if (measuringTrace_)
{
tThread_->Stop();
tThread_->wait();
delete tThread_;
DestroyMonitoringThread(tThread_);

tThread_ = new TraceMonitoringThread(this);
int err = DEVICE_OK;
Expand All @@ -1012,9 +1054,7 @@ int NIDAQHub::StopAIMeasuringForPort(NIAnalogInputPort* port)
}
else
{
mThread_->Stop();
mThread_->wait();
delete mThread_;
DestroyMonitoringThread(mThread_);

mThread_ = new InputMonitoringThread(this);

Expand Down Expand Up @@ -1062,11 +1102,15 @@ std::string NIDAQHub::GetPhysicalChannelListForMeasuring(std::vector<NIAnalogInp
int NIDAQHub::StartTrace()
{
measuringTrace_ = true;
mThread_->Stop();
mThread_->wait();

// Destroy the trace thread before the input thread: a trace thread that is
// still running calls hub_->FinishTrace() on exit, which dereferences
// mThread_. (This was previously a bare "delete tThread_", which destroyed
// a possibly still running thread without stopping or joining it first.)
DestroyMonitoringThread(tThread_);
DestroyMonitoringThread(mThread_);
mThread_ = new InputMonitoringThread(this);

delete tThread_;
tThread_ = new TraceMonitoringThread(this);
int err = tThread_->Start(GetPhysicalChannelListForMeasuring(physicalAIChannels_), expectedMinVoltsIn_,
expectedMaxVoltsIn_, (float) traceFrequency_, traceAmount_, (int) physicalAIChannels_.size());
Expand All @@ -1077,12 +1121,12 @@ int NIDAQHub::StartTrace()

int NIDAQHub::StopTrace()
{
tThread_->Stop();
tThread_->wait();
DestroyMonitoringThread(tThread_);
tThread_ = new TraceMonitoringThread(this);
measuringTrace_ = false;


delete mThread_;
// Previously a bare "delete mThread_" (see StartTrace()).
DestroyMonitoringThread(mThread_);
mThread_ = new InputMonitoringThread(this);
int err = DEVICE_OK;
if (physicalAIChannels_.size() > 0)
Expand All @@ -1092,6 +1136,9 @@ int NIDAQHub::StopTrace()
}


// Called from TraceMonitoringThread::svc(), i.e. on the trace thread itself.
// It must therefore never stop or join tThread_: doing so would make the trace
// thread join itself and deadlock. Only mThread_ may be touched here.
int NIDAQHub::FinishTrace()
{
measuringTrace_ = false;
Expand Down Expand Up @@ -1228,10 +1275,8 @@ int NIDAQHub::OnExpectedMaxVoltsIn(MM::PropertyBase* pProp, MM::ActionType eAct)
double temp_max = 5.0;
pProp->Get(temp_max);
expectedMaxVoltsIn_ = (float) temp_max;

mThread_->Stop();
mThread_->wait();
delete mThread_;

DestroyMonitoringThread(mThread_);
mThread_ = new InputMonitoringThread(this);
if (physicalAIChannels_.size() > 1)
mThread_->Start(GetPhysicalChannelListForMeasuring(physicalAIChannels_), expectedMinVoltsIn_, expectedMaxVoltsIn_);
Expand All @@ -1253,9 +1298,7 @@ int NIDAQHub::OnExpectedMinVoltsIn(MM::PropertyBase* pProp, MM::ActionType eAct)
pProp->Get(temp_min);
expectedMinVoltsIn_ = (float) temp_min;

mThread_->Stop();
mThread_->wait();
delete mThread_;
DestroyMonitoringThread(mThread_);
mThread_ = new InputMonitoringThread(this);
if (physicalAIChannels_.size() > 1)
mThread_->Start(GetPhysicalChannelListForMeasuring(physicalAIChannels_), expectedMinVoltsIn_, expectedMaxVoltsIn_);
Expand Down Expand Up @@ -1339,7 +1382,7 @@ NIDAQDOHub<Tuint>::NIDAQDOHub(NIDAQHub* hub) : diTask_(0), doTask_(0), hub_(hub)
{
if (typeid(Tuint) == typeid(uInt8))
portWidth_ = 8;
else if (typeid(Tuint) == typeid(uInt8))
else if (typeid(Tuint) == typeid(uInt16))
portWidth_ = 16;
else if (typeid(Tuint) == typeid(uInt32))
portWidth_ = 32;
Expand Down Expand Up @@ -1678,6 +1721,7 @@ template class NIDAQDOHub<uInt32>;

InputMonitoringThread::InputMonitoringThread(NIDAQHub* hub) :
stop_(false),
started_(false),
aiTask_(NULL)
{
hub_ = hub;
Expand All @@ -1687,7 +1731,32 @@ InputMonitoringThread::InputMonitoringThread(NIDAQHub* hub) :
InputMonitoringThread::~InputMonitoringThread()
{
Stop();
wait();
// Join rather than letting ~MMDeviceThreadBase detach: svc() dereferences
// hub_, so a detached worker outliving the hub is a use-after-free.
Join();
}


void InputMonitoringThread::Join()
{
if (started_)
{
// Clear first: if wait() throws anyway, a later Join() must not retry.
started_ = false;
wait();
}
}


// Clear aiTask_ and reset the handle, for use on Start()'s error paths. The
// task was already created there, so simply returning would leak it.
void InputMonitoringThread::ClearTask()
{
if (aiTask_ != NULL)
{
DAQmxClearTask(aiTask_);
aiTask_ = NULL;
}
}


Expand All @@ -1696,13 +1765,22 @@ int InputMonitoringThread::Start(std::string AIChannelList, float minVal, float
stop_ = false;
int err = DAQmxCreateTask("AnalogInputReadTask", &aiTask_);
if (err != DEVICE_OK)
{
// DAQmx leaves the handle unspecified on failure; do not clear it.
aiTask_ = NULL;
return err;

}

err = DAQmxCreateAIVoltageChan(aiTask_, AIChannelList.c_str(), "", DAQmx_Val_RSE, minVal, maxVal, DAQmx_Val_Volts, NULL);
if (err != DEVICE_OK)
{
ClearTask();
return err;
}

activate();
// Only after activate() returns without throwing is there a thread to join.
started_ = true;
return DEVICE_OK;
}

Expand Down Expand Up @@ -1730,6 +1808,7 @@ int InputMonitoringThread::svc()

TraceMonitoringThread::TraceMonitoringThread(NIDAQHub* hub) :
stop_(false),
started_(false),
totalAmount_(0),
numberOfChannels_(0),
CSVheader_("")
Expand All @@ -1742,7 +1821,31 @@ TraceMonitoringThread::TraceMonitoringThread(NIDAQHub* hub) :
TraceMonitoringThread::~TraceMonitoringThread()
{
Stop();
wait();
// See ~InputMonitoringThread().
Join();
}


void TraceMonitoringThread::Join()
{
if (started_)
{
// Clear first: if wait() throws anyway, a later Join() must not retry.
started_ = false;
wait();
}
}


// See InputMonitoringThread::ClearTask(). DAQmxClearTask() also stops a task
// that was already started, so this is valid after DAQmxStartTask() too.
void TraceMonitoringThread::ClearTask()
{
if (aiTask_ != NULL)
{
DAQmxClearTask(aiTask_);
aiTask_ = NULL;
}
}


Expand All @@ -1751,39 +1854,64 @@ int TraceMonitoringThread::Start(std::string AIChannelList, float minVal, float
stop_ = false;
int err = DAQmxCreateTask("AnalogInputReadTask", &aiTask_);
if (err != DEVICE_OK)
{
// DAQmx leaves the handle unspecified on failure; do not clear it.
aiTask_ = NULL;
return err;
}

// Every error path below must clear aiTask_, which was created above.
CSVheader_ = "Time, " + AIChannelList;
err = DAQmxCreateAIVoltageChan(aiTask_, AIChannelList.c_str(), "", DAQmx_Val_RSE, minVal, maxVal, DAQmx_Val_Volts, NULL);
if (err != DEVICE_OK)
{
ClearTask();
return err;
}

timestep_ = 1 / frequency;
err = DAQmxSetSampClkRate(aiTask_, frequency);
if (err != DEVICE_OK)
{
ClearTask();
return err;
}

err = DAQmxSetSampQuantSampMode(aiTask_, DAQmx_Val_FiniteSamps);
if (err != DEVICE_OK)
{
ClearTask();
return err;
}

totalAmount_ = numberOfSamples;
err = DAQmxSetSampQuantSampPerChan(aiTask_, numberOfSamples);
if (err != DEVICE_OK)
{
ClearTask();
return err;
}

err = DAQmxSetSampTimingType(aiTask_, DAQmx_Val_SampClk);
if (err != DEVICE_OK)
{
ClearTask();
return err;
}

path_ += boost::posix_time::to_iso_string(boost::posix_time::second_clock::local_time()) + ".csv";
numberOfChannels_ = numberOfChannels;

err = DAQmxStartTask(aiTask_);
if (err != DEVICE_OK)
{
ClearTask();
return err;
}

activate();
// Only after activate() returns without throwing is there a thread to join.
started_ = true;
return DEVICE_OK;
}

Expand Down
Loading