diff --git a/hal/src/main/native/athena/HAL.cpp b/hal/src/main/native/athena/HAL.cpp index b8a9630215..0f1f33f3d4 100644 --- a/hal/src/main/native/athena/HAL.cpp +++ b/hal/src/main/native/athena/HAL.cpp @@ -26,7 +26,6 @@ #include "HAL/DriverStation.h" #include "HAL/Errors.h" #include "HAL/Notifier.h" -#include "HAL/cpp/NotifierInternal.h" #include "HAL/handles/HandlesInternal.h" #include "ctre/ctre.h" #include "visa/visa.h" @@ -355,6 +354,7 @@ HAL_Bool HAL_Initialize(int32_t timeout, int32_t mode) { int32_t status = 0; HAL_BaseInitialize(&status); + if (status != 0) return false; HAL_InitializeDriverStation(); diff --git a/hal/src/main/native/athena/Notifier.cpp b/hal/src/main/native/athena/Notifier.cpp index 4de559f4a0..7d48f78da7 100644 --- a/hal/src/main/native/athena/Notifier.cpp +++ b/hal/src/main/native/athena/Notifier.cpp @@ -11,13 +11,12 @@ #include // For std::atexit() #include -#include +#include #include #include "HAL/ChipObject.h" #include "HAL/Errors.h" #include "HAL/HAL.h" -#include "HAL/cpp/NotifierInternal.h" #include "HAL/cpp/make_unique.h" #include "HAL/handles/UnlimitedHandleResource.h" @@ -25,7 +24,6 @@ using namespace hal; static const int32_t kTimerInterruptNumber = 28; -static wpi::mutex notifierInterruptMutex; static wpi::mutex notifierMutex; static std::unique_ptr notifierAlarm; static std::unique_ptr notifierManager; @@ -34,100 +32,41 @@ static uint64_t closestTrigger = UINT64_MAX; namespace { struct Notifier { - std::shared_ptr prev, next; - void* param; - HAL_NotifierProcessFunction process; uint64_t triggerTime = UINT64_MAX; - HAL_NotifierHandle handle; - bool threaded; -}; - -// Safe thread to allow callbacks to run on their own thread -class NotifierThread : public wpi::SafeThread { - public: - void Main() { - std::unique_lock lock(m_mutex); - while (m_active) { - m_cond.wait(lock, [&] { return !m_active || m_notify; }); - if (!m_active) break; - m_notify = false; - uint64_t currentTime = m_currentTime; - HAL_NotifierHandle handle = m_handle; - HAL_NotifierProcessFunction process = m_process; - lock.unlock(); // don't hold mutex during callback execution - process(currentTime, handle); - lock.lock(); - } - } - - bool m_notify = false; - HAL_NotifierHandle m_handle = HAL_kInvalidHandle; - HAL_NotifierProcessFunction m_process; - uint64_t m_currentTime; -}; - -class NotifierThreadOwner : public wpi::SafeThreadOwner { - public: - void SetFunc(HAL_NotifierProcessFunction process, void* param) { - auto thr = GetThread(); - if (!thr) return; - thr->m_process = process; - m_param = param; - } - - void Notify(uint64_t currentTime, HAL_NotifierHandle handle) { - auto thr = GetThread(); - if (!thr) return; - thr->m_currentTime = currentTime; - thr->m_handle = handle; - thr->m_notify = true; - thr->m_cond.notify_one(); - } - - void* m_param; + uint64_t triggeredTime = UINT64_MAX; + bool active = true; + wpi::mutex mutex; + wpi::condition_variable cond; }; } // namespace -static std::shared_ptr notifiers; static std::atomic_flag notifierAtexitRegistered = ATOMIC_FLAG_INIT; static std::atomic_int notifierRefCount{0}; using namespace hal; -static UnlimitedHandleResource - notifierHandles; - -// Internal version of updateAlarm used during the alarmCallback when we know -// that the pointer is a valid pointer. This function is synchronized by the -// caller locking notifierMutex. -void updateNotifierAlarmInternal(std::shared_ptr notifierPointer, - uint64_t triggerTime, int32_t* status) { - auto notifier = notifierPointer; - // no need for a null check, as this must always be a valid pointer. - notifier->triggerTime = triggerTime; - bool wasActive = (closestTrigger != UINT64_MAX); - - if (!notifierInterruptMutex.try_lock() || notifierRefCount == 0 || - !notifierAlarm) - return; - - // Update alarm time if closer than current. - if (triggerTime < closestTrigger) { - closestTrigger = triggerTime; - // Simply truncate the hardware trigger time to 32-bit. - notifierAlarm->writeTriggerTime(static_cast(triggerTime), status); +class NotifierHandleContainer + : public UnlimitedHandleResource { + public: + ~NotifierHandleContainer() { + ForEach([](HAL_NotifierHandle handle, Notifier* notifier) { + { + std::lock_guard lock(notifier->mutex); + notifier->triggerTime = UINT64_MAX; + notifier->triggeredTime = 0; + notifier->active = false; + } + notifier->cond.notify_all(); // wake up any waiting threads + }); } - // Enable the alarm. The hardware disables itself after each alarm. - if (!wasActive) notifierAlarm->writeEnable(true, status); +}; - notifierInterruptMutex.unlock(); -} +static NotifierHandleContainer notifierHandles; static void alarmCallback(uint32_t, void*) { - std::unique_lock sync(notifierMutex); - + std::lock_guard sync(notifierMutex); int32_t status = 0; uint64_t currentTime = 0; @@ -135,22 +74,26 @@ static void alarmCallback(uint32_t, void*) { closestTrigger = UINT64_MAX; // process all notifiers - std::shared_ptr notifier = notifiers; - while (notifier) { - if (notifier->triggerTime != UINT64_MAX) { - if (currentTime == 0) currentTime = HAL_GetFPGATime(&status); - if (notifier->triggerTime < currentTime) { - notifier->triggerTime = UINT64_MAX; - auto process = notifier->process; - auto handle = notifier->handle; - sync.unlock(); - process(currentTime, handle); - sync.lock(); - } else if (notifier->triggerTime < closestTrigger) { - updateNotifierAlarmInternal(notifier, notifier->triggerTime, &status); - } + notifierHandles.ForEach([&](HAL_NotifierHandle handle, Notifier* notifier) { + if (notifier->triggerTime == UINT64_MAX) return; + if (currentTime == 0) currentTime = HAL_GetFPGATime(&status); + std::unique_lock lock(notifier->mutex); + if (notifier->triggerTime < currentTime) { + notifier->triggerTime = UINT64_MAX; + notifier->triggeredTime = currentTime; + lock.unlock(); + notifier->cond.notify_all(); + } else if (notifier->triggerTime < closestTrigger) { + closestTrigger = notifier->triggerTime; } - notifier = notifier->next; + }); + + if (closestTrigger != UINT64_MAX) { + // Simply truncate the hardware trigger time to 32-bit. + notifierAlarm->writeTriggerTime(static_cast(closestTrigger), + &status); + // Enable the alarm. The hardware disables itself after each alarm. + notifierAlarm->writeEnable(true, &status); } } @@ -159,30 +102,14 @@ static void cleanupNotifierAtExit() { notifierManager = nullptr; } -static void threadedNotifierHandler(uint64_t currentTimeInt, - HAL_NotifierHandle handle) { - // Grab notifier and get handler param - auto notifier = notifierHandles.Get(handle); - if (!notifier) return; - auto notifierPointer = notifier->param; - if (notifierPointer == nullptr) return; - NotifierThreadOwner* owner = - static_cast(notifierPointer); - owner->Notify(currentTimeInt, handle); -} - extern "C" { -HAL_NotifierHandle HAL_InitializeNotifierNonThreadedUnsafe( - HAL_NotifierProcessFunction process, void* param, int32_t* status) { - if (!process) { - *status = NULL_PARAMETER; - return 0; - } +HAL_NotifierHandle HAL_InitializeNotifier(int32_t* status) { if (!notifierAtexitRegistered.test_and_set()) std::atexit(cleanupNotifierAtExit); + if (notifierRefCount.fetch_add(1) == 0) { - std::lock_guard sync(notifierInterruptMutex); + std::lock_guard sync(notifierMutex); // create manager and alarm if not already created if (!notifierManager) { notifierManager = std::make_unique( @@ -193,68 +120,43 @@ HAL_NotifierHandle HAL_InitializeNotifierNonThreadedUnsafe( if (!notifierAlarm) notifierAlarm.reset(tAlarm::create(status)); } - std::lock_guard sync(notifierMutex); std::shared_ptr notifier = std::make_shared(); HAL_NotifierHandle handle = notifierHandles.Allocate(notifier); if (handle == HAL_kInvalidHandle) { *status = HAL_HANDLE_ERROR; return HAL_kInvalidHandle; } - // create notifier structure and add to list - notifier->next = notifiers; - if (notifier->next) notifier->next->prev = notifier; - notifier->param = param; - notifier->process = process; - notifier->handle = handle; - notifier->threaded = false; - notifiers = notifier; return handle; } -HAL_NotifierHandle HAL_InitializeNotifier(HAL_NotifierProcessFunction process, - void* param, int32_t* status) { - NotifierThreadOwner* notify = new NotifierThreadOwner; - notify->Start(); - notify->SetFunc(process, param); - - auto notifierHandle = HAL_InitializeNotifierNonThreadedUnsafe( - threadedNotifierHandler, notify, status); - - if (notifierHandle == HAL_kInvalidHandle || *status != 0) { - delete notify; - return HAL_kInvalidHandle; - } - +void HAL_StopNotifier(HAL_NotifierHandle notifierHandle, int32_t* status) { auto notifier = notifierHandles.Get(notifierHandle); - if (!notifier) { - return HAL_kInvalidHandle; - } - notifier->threaded = true; + if (!notifier) return; - return notifierHandle; + { + std::lock_guard lock(notifier->mutex); + notifier->triggerTime = UINT64_MAX; + notifier->triggeredTime = 0; + notifier->active = false; + } + notifier->cond.notify_all(); // wake up any waiting threads } void HAL_CleanNotifier(HAL_NotifierHandle notifierHandle, int32_t* status) { + auto notifier = notifierHandles.Free(notifierHandle); + if (!notifier) return; + + // Just in case HAL_StopNotifier() wasn't called... { - std::lock_guard sync(notifierMutex); - auto notifier = notifierHandles.Get(notifierHandle); - if (!notifier) return; - - // remove from list - if (notifier->prev) notifier->prev->next = notifier->next; - if (notifier->next) notifier->next->prev = notifier->prev; - if (notifiers == notifier) notifiers = notifier->next; - notifierHandles.Free(notifierHandle); - - if (notifier->threaded) { - NotifierThreadOwner* owner = - static_cast(notifier->param); - delete owner; - } + std::lock_guard lock(notifier->mutex); + notifier->triggerTime = UINT64_MAX; + notifier->triggeredTime = 0; + notifier->active = false; } + notifier->cond.notify_all(); if (notifierRefCount.fetch_sub(1) == 1) { - std::lock_guard sync(notifierInterruptMutex); + std::lock_guard sync(notifierMutex); // if this was the last notifier, clean up alarm and manager if (notifierAlarm) { notifierAlarm->writeEnable(false, status); @@ -268,32 +170,50 @@ void HAL_CleanNotifier(HAL_NotifierHandle notifierHandle, int32_t* status) { } } -void* HAL_GetNotifierParam(HAL_NotifierHandle notifierHandle, int32_t* status) { - auto notifier = notifierHandles.Get(notifierHandle); - if (!notifier) return nullptr; - if (notifier->threaded) { - // If threaded, return thread param rather then notifier param - NotifierThreadOwner* owner = - static_cast(notifier->param); - return owner->m_param; - } - return notifier->param; -} - void HAL_UpdateNotifierAlarm(HAL_NotifierHandle notifierHandle, uint64_t triggerTime, int32_t* status) { - std::lock_guard sync(notifierMutex); - auto notifier = notifierHandles.Get(notifierHandle); if (!notifier) return; - updateNotifierAlarmInternal(notifier, triggerTime, status); + + { + std::lock_guard lock(notifier->mutex); + notifier->triggerTime = triggerTime; + notifier->triggeredTime = UINT64_MAX; + } + + std::lock_guard sync(notifierMutex); + // Update alarm time if closer than current. + if (triggerTime < closestTrigger) { + bool wasActive = (closestTrigger != UINT64_MAX); + closestTrigger = triggerTime; + // Simply truncate the hardware trigger time to 32-bit. + notifierAlarm->writeTriggerTime(static_cast(closestTrigger), + status); + // Enable the alarm. + if (!wasActive) notifierAlarm->writeEnable(true, status); + } } -void HAL_StopNotifierAlarm(HAL_NotifierHandle notifierHandle, int32_t* status) { - std::lock_guard sync(notifierMutex); +void HAL_CancelNotifierAlarm(HAL_NotifierHandle notifierHandle, + int32_t* status) { auto notifier = notifierHandles.Get(notifierHandle); if (!notifier) return; - notifier->triggerTime = UINT64_MAX; + + { + std::lock_guard lock(notifier->mutex); + notifier->triggerTime = UINT64_MAX; + } +} + +uint64_t HAL_WaitForNotifierAlarm(HAL_NotifierHandle notifierHandle, + int32_t* status) { + auto notifier = notifierHandles.Get(notifierHandle); + if (!notifier) return 0; + std::unique_lock lock(notifier->mutex); + notifier->cond.wait(lock, [&] { + return !notifier->active || notifier->triggeredTime != UINT64_MAX; + }); + return notifier->active ? notifier->triggeredTime : 0; } } // extern "C" diff --git a/hal/src/main/native/athena/SPI.cpp b/hal/src/main/native/athena/SPI.cpp index 96e0b0cfed..de86e66d5e 100644 --- a/hal/src/main/native/athena/SPI.cpp +++ b/hal/src/main/native/athena/SPI.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -515,13 +516,7 @@ void HAL_SetSPIHandle(HAL_SPIPort port, int32_t handle) { } } -static void spiAccumulatorProcess(uint64_t currentTime, - HAL_NotifierHandle handle) { - int32_t status = 0; - auto param = HAL_GetNotifierParam(handle, &status); - if (param == nullptr) return; - SPIAccumulator* accum = static_cast(param); - +static void spiAccumulatorProcess(uint64_t currentTime, SPIAccumulator* accum) { // perform SPI transaction uint8_t resp_b[4]; HAL_TransactionSPI(accum->port, accum->cmd, resp_b, accum->xferSize); @@ -564,7 +559,7 @@ static void spiAccumulatorProcess(uint64_t currentTime, // handle timer slip if (accum->triggerTime < currentTime) accum->triggerTime = currentTime + accum->period; - status = 0; + int32_t status = 0; HAL_UpdateNotifierAlarm(accum->notifier, accum->triggerTime, &status); } @@ -623,10 +618,18 @@ void HAL_InitSPIAccumulator(HAL_SPIPort port, int32_t period, int32_t cmd, accum->bigEndian = bigEndian; accum->port = port; if (!accum->notifier) { - accum->notifier = - HAL_InitializeNotifier(spiAccumulatorProcess, accum, status); + accum->notifier = HAL_InitializeNotifier(status); accum->triggerTime = HAL_GetFPGATime(status) + period; if (*status != 0) return; + std::thread thr([=] { + int32_t status2 = 0; + while (status2 == 0) { + uint64_t curTime = HAL_WaitForNotifierAlarm(accum->notifier, &status2); + if (curTime == 0 || status2 != 0) break; + spiAccumulatorProcess(curTime, accum); + } + }); + thr.detach(); HAL_UpdateNotifierAlarm(accum->notifier, accum->triggerTime, status); } } diff --git a/hal/src/main/native/include/HAL/Notifier.h b/hal/src/main/native/include/HAL/Notifier.h index f269697887..27ff98e20c 100644 --- a/hal/src/main/native/include/HAL/Notifier.h +++ b/hal/src/main/native/include/HAL/Notifier.h @@ -15,16 +15,16 @@ extern "C" { #endif -typedef void (*HAL_NotifierProcessFunction)(uint64_t currentTime, - HAL_NotifierHandle handle); - -HAL_NotifierHandle HAL_InitializeNotifier(HAL_NotifierProcessFunction process, - void* param, int32_t* status); +HAL_NotifierHandle HAL_InitializeNotifier(int32_t* status); +void HAL_StopNotifier(HAL_NotifierHandle notifierHandle, int32_t* status); void HAL_CleanNotifier(HAL_NotifierHandle notifierHandle, int32_t* status); -void* HAL_GetNotifierParam(HAL_NotifierHandle notifierHandle, int32_t* status); void HAL_UpdateNotifierAlarm(HAL_NotifierHandle notifierHandle, uint64_t triggerTime, int32_t* status); -void HAL_StopNotifierAlarm(HAL_NotifierHandle notifierHandle, int32_t* status); +void HAL_CancelNotifierAlarm(HAL_NotifierHandle notifierHandle, + int32_t* status); +uint64_t HAL_WaitForNotifierAlarm(HAL_NotifierHandle notifierHandle, + int32_t* status); + #ifdef __cplusplus } // extern "C" #endif diff --git a/hal/src/main/native/include/HAL/cpp/NotifierInternal.h b/hal/src/main/native/include/HAL/cpp/NotifierInternal.h deleted file mode 100644 index 7d0cc59f66..0000000000 --- a/hal/src/main/native/include/HAL/cpp/NotifierInternal.h +++ /dev/null @@ -1,15 +0,0 @@ -/*----------------------------------------------------------------------------*/ -/* Copyright (c) 2017 FIRST. All Rights Reserved. */ -/* Open Source Software - may be modified and shared by FRC teams. The code */ -/* must be accompanied by the FIRST BSD license file in the root directory of */ -/* the project. */ -/*----------------------------------------------------------------------------*/ - -#pragma once - -#include "HAL/Types.h" - -extern "C" { -HAL_NotifierHandle HAL_InitializeNotifierNonThreadedUnsafe( - HAL_NotifierProcessFunction process, void* param, int32_t* status); -} // extern "C" diff --git a/hal/src/main/native/include/HAL/handles/UnlimitedHandleResource.h b/hal/src/main/native/include/HAL/handles/UnlimitedHandleResource.h index cc5d9c45ad..0cff63956b 100644 --- a/hal/src/main/native/include/HAL/handles/UnlimitedHandleResource.h +++ b/hal/src/main/native/include/HAL/handles/UnlimitedHandleResource.h @@ -10,6 +10,7 @@ #include #include +#include #include #include @@ -44,9 +45,16 @@ class UnlimitedHandleResource : public HandleBase { THandle Allocate(std::shared_ptr structure); std::shared_ptr Get(THandle handle); - void Free(THandle handle); + /* Returns structure previously at that handle (or nullptr if none) */ + std::shared_ptr Free(THandle handle); void ResetHandles() override; + /* Calls func(THandle, TStruct*) for each handle. Note this holds the + * global lock for the entirety of execution. + */ + template + void ForEach(Functor func); + private: std::vector> m_structures; wpi::mutex m_handleMutex; @@ -81,12 +89,13 @@ UnlimitedHandleResource::Get(THandle handle) { } template -void UnlimitedHandleResource::Free( - THandle handle) { +std::shared_ptr +UnlimitedHandleResource::Free(THandle handle) { int16_t index = getHandleTypedIndex(handle, enumValue, m_version); std::lock_guard sync(m_handleMutex); - if (index < 0 || index >= static_cast(m_structures.size())) return; - m_structures[index].reset(); + if (index < 0 || index >= static_cast(m_structures.size())) + return nullptr; + return std::move(m_structures[index]); } template @@ -99,4 +108,19 @@ void UnlimitedHandleResource::ResetHandles() { } HandleBase::ResetHandles(); } + +template +template +void UnlimitedHandleResource::ForEach( + Functor func) { + std::lock_guard sync(m_handleMutex); + size_t i; + for (i = 0; i < m_structures.size(); i++) { + if (m_structures[i] != nullptr) { + func(static_cast(createHandle(i, enumValue, m_version)), + m_structures[i].get()); + } + } +} + } // namespace hal diff --git a/hal/src/main/native/sim/Notifier.cpp b/hal/src/main/native/sim/Notifier.cpp index afb3822a29..b9703e5713 100644 --- a/hal/src/main/native/sim/Notifier.cpp +++ b/hal/src/main/native/sim/Notifier.cpp @@ -8,9 +8,9 @@ #include "HAL/Notifier.h" #include -#include -#include +#include +#include #include #include "HAL/HAL.h" @@ -19,138 +19,128 @@ #include "HAL/handles/UnlimitedHandleResource.h" namespace { -class NotifierThread : public wpi::SafeThread { - public: - void Main() { - int32_t status = 0; - std::unique_lock lock(m_mutex); - while (m_active) { - startNotifierLoop: - double waitTime = m_waitTime * 1e-6; - if (!m_running) { - status = 0; - waitTime = (HAL_GetFPGATime(&status) * 1e-6) + 1000.0; - // If not running, wait 1000 seconds - } - - auto timeoutTime = - hal::fpga_clock::epoch() + std::chrono::duration(waitTime); - // auto timeoutTime = std::chrono::steady_clock::now() + - // std::chrono::duration(1.0); - m_cond.wait_until(lock, timeoutTime); - if (m_updatedAlarm) { - m_updatedAlarm = false; - goto startNotifierLoop; - } - if (!m_running) continue; - if (!m_active) break; - m_running = false; - int32_t status = 0; - uint64_t currentTime = HAL_GetFPGATime(&status); - HAL_NotifierHandle handle = m_handle; - HAL_NotifierProcessFunction process = m_process; - lock.unlock(); // don't hold mutex during callback execution - process(currentTime, handle); - m_updatedAlarm = false; - lock.lock(); - } - } - - HAL_NotifierHandle m_handle = HAL_kInvalidHandle; - HAL_NotifierProcessFunction m_process; - uint64_t m_waitTime; - bool m_updatedAlarm = false; - bool m_running = false; -}; - -class NotifierThreadOwner : public wpi::SafeThreadOwner { - public: - void SetFunc(HAL_NotifierProcessFunction process, HAL_NotifierHandle handle) { - auto thr = GetThread(); - if (!thr) return; - thr->m_process = process; - thr->m_handle = handle; - } - - void UpdateAlarm(uint64_t triggerTime) { - auto thr = GetThread(); - if (!thr) return; - thr->m_waitTime = triggerTime; - thr->m_running = true; - thr->m_updatedAlarm = true; - thr->m_cond.notify_one(); - } - - void StopAlarm() { - auto thr = GetThread(); - if (!thr) return; - thr->m_running = false; - } -}; struct Notifier { - std::unique_ptr thread; - void* param; + uint64_t waitTime; + bool updatedAlarm = false; + bool active = true; + bool running = false; + wpi::mutex mutex; + wpi::condition_variable cond; }; } // namespace using namespace hal; -static UnlimitedHandleResource - notifierHandles; +class NotifierHandleContainer + : public UnlimitedHandleResource { + public: + ~NotifierHandleContainer() { + ForEach([](HAL_NotifierHandle handle, Notifier* notifier) { + { + std::lock_guard lock(notifier->mutex); + notifier->active = false; + notifier->running = false; + } + notifier->cond.notify_all(); // wake up any waiting threads + }); + } +}; + +static NotifierHandleContainer notifierHandles; extern "C" { -HAL_NotifierHandle HAL_InitializeNotifierNonThreadedUnsafe( - HAL_NotifierProcessFunction process, void* param, int32_t* status) { - return HAL_InitializeNotifier(process, param, status); -} - -HAL_NotifierHandle HAL_InitializeNotifier(HAL_NotifierProcessFunction process, - void* param, int32_t* status) { - if (!process) { - *status = NULL_PARAMETER; - return 0; - } - +HAL_NotifierHandle HAL_InitializeNotifier(int32_t* status) { std::shared_ptr notifier = std::make_shared(); HAL_NotifierHandle handle = notifierHandles.Allocate(notifier); if (handle == HAL_kInvalidHandle) { *status = HAL_HANDLE_ERROR; return HAL_kInvalidHandle; } - - notifier->thread = std::make_unique(); - - notifier->thread->Start(); - notifier->thread->SetFunc(process, handle); - - notifier->param = param; - return handle; } -void HAL_CleanNotifier(HAL_NotifierHandle notifierHandle, int32_t* status) { +void HAL_StopNotifier(HAL_NotifierHandle notifierHandle, int32_t* status) { auto notifier = notifierHandles.Get(notifierHandle); if (!notifier) return; - notifier->thread->StopAlarm(); - notifierHandles.Free(notifierHandle); + + { + std::lock_guard lock(notifier->mutex); + notifier->active = false; + notifier->running = false; + } + notifier->cond.notify_all(); } -void* HAL_GetNotifierParam(HAL_NotifierHandle notifierHandle, int32_t* status) { - auto notifier = notifierHandles.Get(notifierHandle); - if (!notifier) return nullptr; - return notifier->param; + +void HAL_CleanNotifier(HAL_NotifierHandle notifierHandle, int32_t* status) { + auto notifier = notifierHandles.Free(notifierHandle); + if (!notifier) return; + + // Just in case HAL_StopNotifier() wasn't called... + { + std::lock_guard lock(notifier->mutex); + notifier->active = false; + notifier->running = false; + } + notifier->cond.notify_all(); } + void HAL_UpdateNotifierAlarm(HAL_NotifierHandle notifierHandle, uint64_t triggerTime, int32_t* status) { auto notifier = notifierHandles.Get(notifierHandle); if (!notifier) return; - notifier->thread->UpdateAlarm(triggerTime); + + { + std::lock_guard lock(notifier->mutex); + notifier->waitTime = triggerTime; + notifier->running = true; + notifier->updatedAlarm = true; + } + + // We wake up any waiters to change how long they're sleeping for + notifier->cond.notify_all(); } -void HAL_StopNotifierAlarm(HAL_NotifierHandle notifierHandle, int32_t* status) { + +void HAL_CancelNotifierAlarm(HAL_NotifierHandle notifierHandle, + int32_t* status) { auto notifier = notifierHandles.Get(notifierHandle); if (!notifier) return; - notifier->thread->StopAlarm(); + + { + std::lock_guard lock(notifier->mutex); + notifier->running = false; + } +} + +uint64_t HAL_WaitForNotifierAlarm(HAL_NotifierHandle notifierHandle, + int32_t* status) { + auto notifier = notifierHandles.Get(notifierHandle); + if (!notifier) return 0; + + std::unique_lock lock(notifier->mutex); + while (notifier->active) { + double waitTime; + if (!notifier->running) { + waitTime = (HAL_GetFPGATime(status) * 1e-6) + 1000.0; + // If not running, wait 1000 seconds + } else { + waitTime = notifier->waitTime * 1e-6; + } + + auto timeoutTime = + hal::fpga_clock::epoch() + std::chrono::duration(waitTime); + notifier->cond.wait_until(lock, timeoutTime); + if (notifier->updatedAlarm) { + notifier->updatedAlarm = false; + continue; + } + if (!notifier->running) continue; + if (!notifier->active) break; + notifier->running = false; + return HAL_GetFPGATime(status); + } + return 0; } } // extern "C" diff --git a/wpilibc/src/main/native/cpp/Notifier.cpp b/wpilibc/src/main/native/cpp/Notifier.cpp index abba304a05..ec3a1b56ac 100644 --- a/wpilibc/src/main/native/cpp/Notifier.cpp +++ b/wpilibc/src/main/native/cpp/Notifier.cpp @@ -15,8 +15,6 @@ using namespace frc; -wpi::mutex Notifier::m_destructorMutex; - /** * Create a Notifier for timer event notification. * @@ -28,8 +26,31 @@ Notifier::Notifier(TimerEventHandler handler) { wpi_setWPIErrorWithContext(NullParameter, "handler must not be nullptr"); m_handler = handler; int32_t status = 0; - m_notifier = HAL_InitializeNotifier(&Notifier::Notify, this, &status); + m_notifier = HAL_InitializeNotifier(&status); wpi_setErrorWithContext(status, HAL_GetErrorMessage(status)); + + m_thread = std::thread([=] { + for (;;) { + int32_t status = 0; + HAL_NotifierHandle notifier = m_notifier.load(); + if (notifier == 0) break; + uint64_t curTime = HAL_WaitForNotifierAlarm(notifier, &status); + if (curTime == 0 || status != 0) break; + + TimerEventHandler handler; + { + std::lock_guard sync(m_processMutex); + handler = m_handler; + if (m_periodic) { + m_expirationTime += m_period; + UpdateAlarm(); + } + } + + // call callback + if (handler) handler(); + } + }); } /** @@ -39,14 +60,13 @@ Notifier::~Notifier() { int32_t status = 0; // atomically set handle to 0, then clean HAL_NotifierHandle handle = m_notifier.exchange(0); - HAL_CleanNotifier(handle, &status); + HAL_StopNotifier(handle, &status); wpi_setErrorWithContext(status, HAL_GetErrorMessage(status)); - /* Acquire the mutex; this makes certain that the handler is not being - * executed by the interrupt manager. - */ - std::lock_guard lockStatic(Notifier::m_destructorMutex); - std::lock_guard lock(m_processMutex); + // Join the thread to ensure the handler has exited. + if (m_thread.joinable()) m_thread.join(); + + HAL_CleanNotifier(handle, &status); } /** @@ -55,37 +75,21 @@ Notifier::~Notifier() { void Notifier::UpdateAlarm() { int32_t status = 0; // Return if we are being destructed, or were not created successfully - if (m_notifier == 0) return; + auto notifier = m_notifier.load(); + if (notifier == 0) return; HAL_UpdateNotifierAlarm( - m_notifier, static_cast(m_expirationTime * 1e6), &status); + notifier, static_cast(m_expirationTime * 1e6), &status); wpi_setErrorWithContext(status, HAL_GetErrorMessage(status)); } /** - * Notify is called by the HAL layer. We simply need to pass it through to - * the user handler. + * Change the handler function. + * + * @param handler Handler */ -void Notifier::Notify(uint64_t currentTimeInt, HAL_NotifierHandle handle) { - Notifier* notifier; - { - // Lock static mutex to grab the notifier param - std::lock_guard lock(Notifier::m_destructorMutex); - int32_t status = 0; - auto notifierPointer = HAL_GetNotifierParam(handle, &status); - if (notifierPointer == nullptr) return; - notifier = static_cast(notifierPointer); - notifier->m_processMutex.lock(); - } - - if (notifier->m_periodic) { - notifier->m_expirationTime += notifier->m_period; - notifier->UpdateAlarm(); - } - - auto handler = notifier->m_handler; - - if (handler) handler(); - notifier->m_processMutex.unlock(); +void Notifier::SetHandler(TimerEventHandler handler) { + std::lock_guard sync(m_processMutex); + m_handler = handler; } /** @@ -132,11 +136,6 @@ void Notifier::StartPeriodic(double period) { */ void Notifier::Stop() { int32_t status = 0; - HAL_StopNotifierAlarm(m_notifier, &status); + HAL_CancelNotifierAlarm(m_notifier, &status); wpi_setErrorWithContext(status, HAL_GetErrorMessage(status)); - - // Wait for a currently executing handler to complete before returning from - // Stop() - std::lock_guard lockStatic(Notifier::m_destructorMutex); - std::lock_guard lock(m_processMutex); } diff --git a/wpilibc/src/main/native/include/Notifier.h b/wpilibc/src/main/native/include/Notifier.h index a10f5ac0b9..2349c196c7 100644 --- a/wpilibc/src/main/native/include/Notifier.h +++ b/wpilibc/src/main/native/include/Notifier.h @@ -11,6 +11,7 @@ #include #include +#include #include #include @@ -36,23 +37,18 @@ class Notifier : public ErrorBase { Notifier(const Notifier&) = delete; Notifier& operator=(const Notifier&) = delete; + void SetHandler(TimerEventHandler handler); void StartSingle(double delay); void StartPeriodic(double period); void Stop(); private: - // Update the HAL alarm + // update the HAL alarm void UpdateAlarm(); - - // HAL callback - static void Notify(uint64_t currentTimeInt, HAL_NotifierHandle handle); - - // Used to constrain execution between destructors and callback - static wpi::mutex m_destructorMutex; - - // Held while updating process information + // the thread waiting on the HAL alarm + std::thread m_thread; + // held while updating process information wpi::mutex m_processMutex; - // HAL handle, atomic for proper destruction std::atomic m_notifier{0}; diff --git a/wpilibj/src/main/java/edu/wpi/first/wpilibj/Notifier.java b/wpilibj/src/main/java/edu/wpi/first/wpilibj/Notifier.java index 2dda4c43cb..bc6e73d53f 100644 --- a/wpilibj/src/main/java/edu/wpi/first/wpilibj/Notifier.java +++ b/wpilibj/src/main/java/edu/wpi/first/wpilibj/Notifier.java @@ -14,88 +14,52 @@ import edu.wpi.first.wpilibj.hal.NotifierJNI; public class Notifier { - private static class Process implements NotifierJNI.NotifierJNIHandlerFunction { - // The lock for the process information. - private final ReentrantLock m_processLock = new ReentrantLock(); - // The C pointer to the notifier object. We don't use it directly, it is - // just passed to the JNI bindings. - AtomicInteger m_notifier = new AtomicInteger(); - // The time, in microseconds, at which the corresponding handler should be - // called. Has the same zero as Utility.getFPGATime(). - private double m_expirationTime = 0; - // The handler passed in by the user which should be called at the - // appropriate interval. - private Runnable m_handler; - // Whether we are calling the handler just once or periodically. - private boolean m_periodic = false; - // If periodic, the period of the calling; if just once, stores how long it - // is until we call the handler. - private double m_period = 0; - // Lock on the handler so that the handler is not called before it has - // completed. This is only relevant if the handler takes a very long time - // to complete (or the period is very short) and when everything is being - // destructed. - private final ReentrantLock m_handlerLock = new ReentrantLock(); + // The thread waiting on the HAL alarm. + private final Thread m_thread; + // The lock for the process information. + private final ReentrantLock m_processLock = new ReentrantLock(); + // The C pointer to the notifier object. We don't use it directly, it is + // just passed to the JNI bindings. + private final AtomicInteger m_notifier = new AtomicInteger(); + // The time, in microseconds, at which the corresponding handler should be + // called. Has the same zero as Utility.getFPGATime(). + private double m_expirationTime = 0; + // The handler passed in by the user which should be called at the + // appropriate interval. + private Runnable m_handler; + // Whether we are calling the handler just once or periodically. + private boolean m_periodic = false; + // If periodic, the period of the calling; if just once, stores how long it + // is until we call the handler. + private double m_period = 0; - Process(Runnable run) { - m_handler = run; - m_notifier.set(NotifierJNI.initializeNotifier(this)); - } - - @Override - @SuppressWarnings("NoFinalizer") - protected void finalize() { - int handle = m_notifier.getAndSet(0); - NotifierJNI.cleanNotifier(handle); - m_handlerLock.lock(); - } - - /** - * Update the alarm hardware to reflect the next alarm. - */ - private void updateAlarm() { - NotifierJNI.updateNotifierAlarm(m_notifier.get(), (long) (m_expirationTime * 1e6)); - } - - /** - * Handler which is called by the HAL library; it handles the subsequent calling of the user - * handler. - */ - @Override - public void apply(long time) { - m_processLock.lock(); - if (m_periodic) { - m_expirationTime += m_period; - updateAlarm(); - } - - m_handlerLock.lock(); - m_processLock.unlock(); - - m_handler.run(); - m_handlerLock.unlock(); - } - - public void start(double period, boolean periodic) { - synchronized (m_processLock) { - m_periodic = periodic; - m_period = period; - m_expirationTime = Utility.getFPGATime() * 1e-6 + period; - updateAlarm(); + @Override + @SuppressWarnings("NoFinalizer") + protected void finalize() { + int handle = m_notifier.getAndSet(0); + NotifierJNI.stopNotifier(handle); + // Join the thread to ensure the handler has exited. + if (m_thread.isAlive()) { + try { + m_thread.interrupt(); + m_thread.join(); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); } } - - public void stop() { - NotifierJNI.stopNotifierAlarm(m_notifier.get()); - - // Wait for a currently executing handler to complete before returning - // from stop() - m_handlerLock.lock(); - m_handlerLock.unlock(); - } + NotifierJNI.cleanNotifier(handle); } - private Process m_process; + /** + * Update the alarm hardware to reflect the next alarm. + */ + private void updateAlarm() { + int notifier = m_notifier.get(); + if (notifier == 0) { + return; + } + NotifierJNI.updateNotifierAlarm(notifier, (long) (m_expirationTime * 1e6)); + } /** * Create a Notifier for timer event notification. @@ -104,7 +68,53 @@ public class Notifier { * or StartPeriodic. */ public Notifier(Runnable run) { - m_process = new Process(run); + m_handler = run; + m_notifier.set(NotifierJNI.initializeNotifier()); + + m_thread = new Thread(() -> { + while (!Thread.interrupted()) { + int notifier = m_notifier.get(); + if (notifier == 0) { + break; + } + long curTime = NotifierJNI.waitForNotifierAlarm(notifier); + if (curTime == 0) { + break; + } + + Runnable handler = null; + m_processLock.lock(); + try { + handler = m_handler; + if (m_periodic) { + m_expirationTime += m_period; + updateAlarm(); + } + } finally { + m_processLock.unlock(); + } + + if (handler != null) { + handler.run(); + } + } + }); + m_thread.setDaemon(true); + m_thread.start(); + } + + /** + * Change the handler function. + * + * @param handler Handler + */ + public void setHandler(Runnable handler) { + m_processLock.lock(); + try { + m_handler = handler; + } finally { + m_processLock.unlock(); + } } /** @@ -114,7 +124,15 @@ public class Notifier { * @param delay Seconds to wait before the handler is called. */ public void startSingle(double delay) { - m_process.start(delay, false); + m_processLock.lock(); + try { + m_periodic = false; + m_period = delay; + m_expirationTime = Utility.getFPGATime() * 1e-6 + delay; + updateAlarm(); + } finally { + m_processLock.unlock(); + } } /** @@ -126,7 +144,15 @@ public class Notifier { * method. */ public void startPeriodic(double period) { - m_process.start(period, true); + m_processLock.lock(); + try { + m_periodic = true; + m_period = period; + m_expirationTime = Utility.getFPGATime() * 1e-6 + period; + updateAlarm(); + } finally { + m_processLock.unlock(); + } } /** @@ -135,6 +161,6 @@ public class Notifier { * registered handler is in progress, this function will block until the handler call is complete. */ public void stop() { - m_process.stop(); + NotifierJNI.cancelNotifierAlarm(m_notifier.get()); } } diff --git a/wpilibj/src/main/java/edu/wpi/first/wpilibj/hal/NotifierJNI.java b/wpilibj/src/main/java/edu/wpi/first/wpilibj/hal/NotifierJNI.java index f1954459c9..a5ba64ee0c 100644 --- a/wpilibj/src/main/java/edu/wpi/first/wpilibj/hal/NotifierJNI.java +++ b/wpilibj/src/main/java/edu/wpi/first/wpilibj/hal/NotifierJNI.java @@ -14,17 +14,16 @@ package edu.wpi.first.wpilibj.hal; * class, which corresponds to the C++ Notifier class, should be used. */ public class NotifierJNI extends JNIWrapper { - /** - * Callback function. - */ - public interface NotifierJNIHandlerFunction { - void apply(long curTime); - } - /** * Initializes the notifier. */ - public static native int initializeNotifier(NotifierJNIHandlerFunction func); + public static native int initializeNotifier(); + + /** + * Wakes up the waiter with time=0. Note: after this function is called, all + * calls to waitForNotifierAlarm() will immediately start returning 0. + */ + public static native void stopNotifier(int notifierHandle); /** * Deletes the notifier object when we are done with it. @@ -32,12 +31,19 @@ public class NotifierJNI extends JNIWrapper { public static native void cleanNotifier(int notifierHandle); /** - * Sets the notifier to call the callback in another triggerTime microseconds. + * Sets the notifier to wakeup the waiter in another triggerTime microseconds. */ public static native void updateNotifierAlarm(int notifierHandle, long triggerTime); /** - * Tells the notifier to stop calling the callback. + * Cancels any pending wakeups set by updateNotifierAlarm(). Does NOT wake + * up any waiters. */ - public static native void stopNotifierAlarm(int notifierHandle); + public static native void cancelNotifierAlarm(int notifierHandle); + + /** + * Block until woken up by an alarm (or stop). + * @return Time when woken up. + */ + public static native long waitForNotifierAlarm(int notifierHandle); } diff --git a/wpilibj/src/main/native/cpp/NotifierJNI.cpp b/wpilibj/src/main/native/cpp/NotifierJNI.cpp index 0bbccfda26..b55bddcb54 100644 --- a/wpilibj/src/main/native/cpp/NotifierJNI.cpp +++ b/wpilibj/src/main/native/cpp/NotifierJNI.cpp @@ -9,15 +9,9 @@ #include #include #include -#include -#include -#include -#include #include "HALUtil.h" #include "HAL/cpp/Log.h" #include "edu_wpi_first_wpilibj_hal_NotifierJNI.h" -#include "support/SafeThread.h" -#include "HAL/cpp/NotifierInternal.h" using namespace frc; @@ -30,137 +24,49 @@ TLogLevel notifierJNILogLevel = logWARNING; else \ Log().Get(level) -// Thread where callbacks are actually performed. -// -// JNI's AttachCurrentThread() creates a Java Thread object on every -// invocation, which is both time inefficient and causes issues with Eclipse -// (which tries to keep a thread list up-to-date and thus gets swamped). -// -// Instead, this class attaches just once. When a hardware notification -// occurs, a condition variable wakes up this thread and this thread actually -// makes the call into Java. -// -// We don't want to use a FIFO here. If the user code takes too long to -// process, we will just ignore the redundant wakeup. -class NotifierThreadJNI : public wpi::SafeThread { - public: - void Main(); - - bool m_notify = false; - jobject m_func = nullptr; - jmethodID m_mid; - uint64_t m_currentTime; -}; - -class NotifierJNI : public wpi::SafeThreadOwner { - public: - void SetFunc(JNIEnv *env, jobject func, jmethodID mid); - - void Notify(uint64_t currentTime) { - auto thr = GetThread(); - if (!thr) return; - thr->m_currentTime = currentTime; - thr->m_notify = true; - thr->m_cond.notify_one(); - } -}; - -void NotifierJNI::SetFunc(JNIEnv *env, jobject func, jmethodID mid) { - auto thr = GetThread(); - if (!thr) return; - // free global reference - if (thr->m_func) env->DeleteGlobalRef(thr->m_func); - // create global reference - thr->m_func = env->NewGlobalRef(func); - thr->m_mid = mid; -} - -void NotifierThreadJNI::Main() { - JNIEnv *env; - JavaVMAttachArgs args; - args.version = JNI_VERSION_1_2; - args.name = const_cast("Notifier"); - args.group = nullptr; - jint rs = - jvm->AttachCurrentThreadAsDaemon(reinterpret_cast(&env), &args); - if (rs != JNI_OK) return; - - std::unique_lock lock(m_mutex); - while (m_active) { - m_cond.wait(lock, [&] { return !m_active || m_notify; }); - if (!m_active) break; - m_notify = false; - if (!m_func) continue; - jobject func = m_func; - jmethodID mid = m_mid; - uint64_t currentTime = m_currentTime; - lock.unlock(); // don't hold mutex during callback execution - env->CallVoidMethod(func, mid, static_cast(currentTime)); - if (env->ExceptionCheck()) { - env->ExceptionDescribe(); - env->ExceptionClear(); - } - lock.lock(); - } - - // free global reference - if (m_func) env->DeleteGlobalRef(m_func); - - jvm->DetachCurrentThread(); -} - -void notifierHandler(uint64_t currentTimeInt, HAL_NotifierHandle handle) { - int32_t status = 0; - auto param = HAL_GetNotifierParam(handle, &status); - if (param == nullptr) return; - (static_cast(param))->Notify(currentTimeInt); -} - extern "C" { /* * Class: edu_wpi_first_wpilibj_hal_NotifierJNI * Method: initializeNotifier - * Signature: (Ljava/lang/Runnable;)I + * Signature: ()I */ JNIEXPORT jint JNICALL Java_edu_wpi_first_wpilibj_hal_NotifierJNI_initializeNotifier( - JNIEnv *env, jclass, jobject func) { + JNIEnv *env, jclass) { NOTIFIERJNI_LOG(logDEBUG) << "Calling NOTIFIERJNI initializeNotifier"; - jclass cls = env->GetObjectClass(func); - if (cls == 0) { - NOTIFIERJNI_LOG(logERROR) << "Error getting java class"; - assert(false); - return 0; - } - jmethodID mid = env->GetMethodID(cls, "apply", "(J)V"); - if (mid == 0) { - NOTIFIERJNI_LOG(logERROR) << "Error getting java method ID"; - assert(false); - return 0; - } - - // each notifier runs in its own thread; this is so if one takes too long - // to execute, it doesn't keep the others from running - NotifierJNI *notify = new NotifierJNI; - notify->Start(); - notify->SetFunc(env, func, mid); int32_t status = 0; - HAL_NotifierHandle notifierHandle = - HAL_InitializeNotifierNonThreadedUnsafe(notifierHandler, notify, &status); + HAL_NotifierHandle notifierHandle = HAL_InitializeNotifier(&status); NOTIFIERJNI_LOG(logDEBUG) << "Notifier Handle = " << notifierHandle; NOTIFIERJNI_LOG(logDEBUG) << "Status = " << status; if (notifierHandle <= 0 || !CheckStatusForceThrow(env, status)) { - // something went wrong in HAL, clean up - delete notify; + return 0; // something went wrong in HAL } return (jint)notifierHandle; } +/* + * Class: edu_wpi_first_wpilibj_hal_NotifierJNI + * Method: stopNotifier + * Signature: (I)V + */ +JNIEXPORT void JNICALL +Java_edu_wpi_first_wpilibj_hal_NotifierJNI_stopNotifier( + JNIEnv *env, jclass cls, jint notifierHandle) { + NOTIFIERJNI_LOG(logDEBUG) << "Calling NOTIFIERJNI stopNotifier"; + + NOTIFIERJNI_LOG(logDEBUG) << "Notifier Handle = " << notifierHandle; + + int32_t status = 0; + HAL_StopNotifier((HAL_NotifierHandle)notifierHandle, &status); + NOTIFIERJNI_LOG(logDEBUG) << "Status = " << status; + CheckStatus(env, status); +} + /* * Class: edu_wpi_first_wpilibj_hal_NotifierJNI * Method: cleanNotifier @@ -173,12 +79,9 @@ JNIEXPORT void JNICALL Java_edu_wpi_first_wpilibj_hal_NotifierJNI_cleanNotifier( NOTIFIERJNI_LOG(logDEBUG) << "Notifier Handle = " << notifierHandle; int32_t status = 0; - NotifierJNI *notify = - (NotifierJNI *)HAL_GetNotifierParam((HAL_NotifierHandle)notifierHandle, &status); HAL_CleanNotifier((HAL_NotifierHandle)notifierHandle, &status); NOTIFIERJNI_LOG(logDEBUG) << "Status = " << status; CheckStatus(env, status); - delete notify; } /* @@ -203,20 +106,42 @@ Java_edu_wpi_first_wpilibj_hal_NotifierJNI_updateNotifierAlarm( /* * Class: edu_wpi_first_wpilibj_hal_NotifierJNI - * Method: stopNotifierAlarm + * Method: cancelNotifierAlarm * Signature: (I)V */ JNIEXPORT void JNICALL -Java_edu_wpi_first_wpilibj_hal_NotifierJNI_stopNotifierAlarm( +Java_edu_wpi_first_wpilibj_hal_NotifierJNI_cancelNotifierAlarm( JNIEnv *env, jclass cls, jint notifierHandle) { - NOTIFIERJNI_LOG(logDEBUG) << "Calling NOTIFIERJNI stopNotifierAlarm"; + NOTIFIERJNI_LOG(logDEBUG) << "Calling NOTIFIERJNI cancelNotifierAlarm"; NOTIFIERJNI_LOG(logDEBUG) << "Notifier Handle = " << notifierHandle; int32_t status = 0; - HAL_StopNotifierAlarm((HAL_NotifierHandle)notifierHandle, &status); + HAL_CancelNotifierAlarm((HAL_NotifierHandle)notifierHandle, &status); NOTIFIERJNI_LOG(logDEBUG) << "Status = " << status; CheckStatus(env, status); } +/* + * Class: edu_wpi_first_wpilibj_hal_NotifierJNI + * Method: waitForNotifierAlarm + * Signature: (I)J + */ +JNIEXPORT jlong JNICALL +Java_edu_wpi_first_wpilibj_hal_NotifierJNI_waitForNotifierAlarm( + JNIEnv *env, jclass cls, jint notifierHandle) { + NOTIFIERJNI_LOG(logDEBUG) << "Calling NOTIFIERJNI waitForNotifierAlarm"; + + NOTIFIERJNI_LOG(logDEBUG) << "Notifier Handle = " << notifierHandle; + + int32_t status = 0; + uint64_t time = + HAL_WaitForNotifierAlarm((HAL_NotifierHandle)notifierHandle, &status); + NOTIFIERJNI_LOG(logDEBUG) << "Status = " << status; + NOTIFIERJNI_LOG(logDEBUG) << "Time = " << time; + CheckStatus(env, status); + + return (jlong)time; +} + } // extern "C"