From c69b857b8dc0457e66dda502e0adf089a692d516 Mon Sep 17 00:00:00 2001 From: Yu Shan Date: Wed, 15 Nov 2023 19:12:38 +0000 Subject: [PATCH 1/2] Use timestamp from property store when get prop. Previously the timestamp in property events might come from two different sources. For property update events, the timestamp is generated from VehicleHalServer. For getProperty request, the timestamp is generated from EmulatedVehicleHal. This might cause a getProperty request which returns an older value to have a newer timestamp compared to the newer property update event. As a result, the new property update event will be ignored by the client, which should not be the case since the property is actually updated and the client should get the update. This causes flaky test cases if the client is try to subscribe, which will cause a get property request for the initial value and the client is changing the value, which will cause a new property update event at the same time. Test: atest android.hardware.automotive.vehicle@2.0-default-impl-unit-tests Bug: 311219830 Change-Id: I78613b1de30624011bd90a5d0da244f0e3d67337 --- .../include/vhal_v2_0/VehiclePropertyStore.h | 12 +++++ .../common/src/VehiclePropertyStore.cpp | 35 +++++++++++-- .../impl/vhal_v2_0/DefaultVehicleHal.cpp | 50 +++++++++++-------- .../vhal_v2_0/tests/DefaultVhalImpl_test.cpp | 40 +++++++++++++-- 4 files changed, 111 insertions(+), 26 deletions(-) diff --git a/automotive/vehicle/2.0/default/common/include/vhal_v2_0/VehiclePropertyStore.h b/automotive/vehicle/2.0/default/common/include/vhal_v2_0/VehiclePropertyStore.h index 6a02cf3cab..abbcd35f84 100644 --- a/automotive/vehicle/2.0/default/common/include/vhal_v2_0/VehiclePropertyStore.h +++ b/automotive/vehicle/2.0/default/common/include/vhal_v2_0/VehiclePropertyStore.h @@ -70,6 +70,16 @@ public: * example wasn't registered. */ bool writeValue(const VehiclePropValue& propValue, bool updateStatus); + /* + * Stores provided value. Returns true if value was written returns false if config for + * example wasn't registered. + * + * The property value's timestamp will be set to the current ElapsedRealTimeNano. + */ + bool writeValueWithCurrentTimestamp(VehiclePropValue* propValuePtr, bool updateStatus); + + std::unique_ptr refreshTimestamp(int32_t propId, int32_t areaId); + void removeValue(const VehiclePropValue& propValue); void removeValuesForProperty(int32_t propId); @@ -94,6 +104,8 @@ private: std::unordered_map mConfigs; PropertyMap mPropertyValues; // Sorted map of RecordId : VehiclePropValue. + + bool writeValueLocked(const VehiclePropValue& propValue, bool updateStatus); }; } // namespace V2_0 diff --git a/automotive/vehicle/2.0/default/common/src/VehiclePropertyStore.cpp b/automotive/vehicle/2.0/default/common/src/VehiclePropertyStore.cpp index 6087bfa526..c12904ecbf 100644 --- a/automotive/vehicle/2.0/default/common/src/VehiclePropertyStore.cpp +++ b/automotive/vehicle/2.0/default/common/src/VehiclePropertyStore.cpp @@ -15,6 +15,7 @@ */ #define LOG_TAG "VehiclePropertyStore" #include +#include #include #include "VehiclePropertyStore.h" @@ -41,9 +42,7 @@ void VehiclePropertyStore::registerProperty(const VehiclePropConfig& config, mConfigs.insert({ config.prop, RecordConfig { config, tokenFunc } }); } -bool VehiclePropertyStore::writeValue(const VehiclePropValue& propValue, - bool updateStatus) { - MuxGuard g(mLock); +bool VehiclePropertyStore::writeValueLocked(const VehiclePropValue& propValue, bool updateStatus) { if (!mConfigs.count(propValue.prop)) return false; RecordId recId = getRecordIdLocked(propValue); @@ -68,6 +67,36 @@ bool VehiclePropertyStore::writeValue(const VehiclePropValue& propValue, return true; } +bool VehiclePropertyStore::writeValue(const VehiclePropValue& propValue, bool updateStatus) { + MuxGuard g(mLock); + + return writeValueLocked(propValue, updateStatus); +} + +bool VehiclePropertyStore::writeValueWithCurrentTimestamp(VehiclePropValue* propValuePtr, + bool updateStatus) { + MuxGuard g(mLock); + + propValuePtr->timestamp = elapsedRealtimeNano(); + return writeValueLocked(*propValuePtr, updateStatus); +} + +std::unique_ptr VehiclePropertyStore::refreshTimestamp(int32_t propId, + int32_t areaId) { + MuxGuard g(mLock); + RecordId recId = getRecordIdLocked(VehiclePropValue{ + .prop = propId, + .areaId = areaId, + }); + auto it = mPropertyValues.find(recId); + if (it == mPropertyValues.end()) { + return nullptr; + } + + it->second.timestamp = elapsedRealtimeNano(); + return std::make_unique(it->second); +} + void VehiclePropertyStore::removeValue(const VehiclePropValue& propValue) { MuxGuard g(mLock); RecordId recId = getRecordIdLocked(propValue); diff --git a/automotive/vehicle/2.0/default/impl/vhal_v2_0/DefaultVehicleHal.cpp b/automotive/vehicle/2.0/default/impl/vhal_v2_0/DefaultVehicleHal.cpp index 318e9ddade..b56a1907c8 100644 --- a/automotive/vehicle/2.0/default/impl/vhal_v2_0/DefaultVehicleHal.cpp +++ b/automotive/vehicle/2.0/default/impl/vhal_v2_0/DefaultVehicleHal.cpp @@ -57,12 +57,6 @@ const VehicleAreaConfig* getAreaConfig(const VehiclePropValue& propValue, return nullptr; } -VehicleHal::VehiclePropValuePtr addTimestamp(VehicleHal::VehiclePropValuePtr v) { - if (v.get()) { - v->timestamp = elapsedRealtimeNano(); - } - return v; -} } // namespace VehicleHal::VehiclePropValuePtr DefaultVehicleHal::createVhalHeartBeatProp() { @@ -102,7 +96,7 @@ VehicleHal::VehiclePropValuePtr DefaultVehicleHal::getUserHalProp( *outStatus = StatusCode::INTERNAL_ERROR; } } - return addTimestamp(std::move(v)); + return v; } VehicleHal::VehiclePropValuePtr DefaultVehicleHal::get(const VehiclePropValue& requestedPropValue, @@ -118,13 +112,13 @@ VehicleHal::VehiclePropValuePtr DefaultVehicleHal::get(const VehiclePropValue& r if (propId == OBD2_FREEZE_FRAME) { v = getValuePool()->obtainComplex(); *outStatus = fillObd2FreezeFrame(mPropStore, requestedPropValue, v.get()); - return addTimestamp(std::move(v)); + return v; } if (propId == OBD2_FREEZE_FRAME_INFO) { v = getValuePool()->obtainComplex(); *outStatus = fillObd2DtcInfo(mPropStore, v.get()); - return addTimestamp(std::move(v)); + return v; } auto internalPropValue = mPropStore->readValueOrNull(requestedPropValue); @@ -139,7 +133,7 @@ VehicleHal::VehiclePropValuePtr DefaultVehicleHal::get(const VehiclePropValue& r } else { *outStatus = StatusCode::TRY_AGAIN; } - return addTimestamp(std::move(v)); + return v; } std::vector DefaultVehicleHal::listProperties() { @@ -486,26 +480,42 @@ VehicleHal::VehiclePropValuePtr DefaultVehicleHal::doInternalHealthCheck() { void DefaultVehicleHal::onContinuousPropertyTimer(const std::vector& properties) { auto& pool = *getValuePool(); - for (int32_t property : properties) { - VehiclePropValuePtr v; + std::vector events; if (isContinuousProperty(property)) { - auto internalPropValue = mPropStore->readValueOrNull(property); - if (internalPropValue != nullptr) { - v = pool.obtain(*internalPropValue); + const VehiclePropConfig* config = mPropStore->getConfigOrNull(property); + std::vector areaIds; + if (isGlobalProp(property)) { + areaIds.push_back(0); + } else { + for (auto& c : config->areaConfigs) { + areaIds.push_back(c.areaId); + } + } + + for (int areaId : areaIds) { + auto v = pool.obtain(*mPropStore->refreshTimestamp(property, areaId)); + if (v.get()) { + events.push_back(std::move(v)); + } } } else if (property == static_cast(VehicleProperty::VHAL_HEARTBEAT)) { // VHAL_HEARTBEAT is not a continuous value, but it needs to be updated periodically. // So, the update is done through onContinuousPropertyTimer. - v = doInternalHealthCheck(); + auto v = doInternalHealthCheck(); + if (!v.get()) { + // Internal health check failed. + continue; + } + mPropStore->writeValueWithCurrentTimestamp(v.get(), /*updateStatus=*/true); + events.push_back(std::move(v)); } else { ALOGE("Unexpected onContinuousPropertyTimer for property: 0x%x", property); continue; } - if (v.get()) { - v->timestamp = elapsedRealtimeNano(); - doHalEvent(std::move(v)); + for (VehiclePropValuePtr& event : events) { + doHalEvent(std::move(event)); } } } @@ -556,7 +566,7 @@ bool DefaultVehicleHal::isContinuousProperty(int32_t propId) const { void DefaultVehicleHal::onPropertyValue(const VehiclePropValue& value, bool updateStatus) { VehiclePropValuePtr updatedPropValue = getValuePool()->obtain(value); - if (mPropStore->writeValue(*updatedPropValue, updateStatus)) { + if (mPropStore->writeValueWithCurrentTimestamp(updatedPropValue.get(), updateStatus)) { doHalEvent(std::move(updatedPropValue)); } } diff --git a/automotive/vehicle/2.0/default/impl/vhal_v2_0/tests/DefaultVhalImpl_test.cpp b/automotive/vehicle/2.0/default/impl/vhal_v2_0/tests/DefaultVhalImpl_test.cpp index ad3f4ac822..c876836783 100644 --- a/automotive/vehicle/2.0/default/impl/vhal_v2_0/tests/DefaultVhalImpl_test.cpp +++ b/automotive/vehicle/2.0/default/impl/vhal_v2_0/tests/DefaultVhalImpl_test.cpp @@ -81,8 +81,13 @@ using ::android::hardware::automotive::vehicle::V2_0::impl::OBD2_FREEZE_FRAME; using ::android::hardware::automotive::vehicle::V2_0::impl::OBD2_FREEZE_FRAME_CLEAR; using ::android::hardware::automotive::vehicle::V2_0::impl::OBD2_FREEZE_FRAME_INFO; using ::android::hardware::automotive::vehicle::V2_0::impl::OBD2_LIVE_FRAME; +using ::android::hardware::automotive::vehicle::V2_0::impl::WHEEL_FRONT_LEFT; +using ::android::hardware::automotive::vehicle::V2_0::impl::WHEEL_FRONT_RIGHT; +using ::android::hardware::automotive::vehicle::V2_0::impl::WHEEL_REAR_LEFT; +using ::android::hardware::automotive::vehicle::V2_0::impl::WHEEL_REAR_RIGHT; using ::testing::HasSubstr; +using ::testing::UnorderedElementsAre; using VehiclePropValuePtr = recyclable_ptr; @@ -346,6 +351,38 @@ TEST_F(DefaultVhalImplTest, testSubscribe) { EXPECT_EQ(1.0f, lastEvent->value.floatValues[0]); } +TEST_F(DefaultVhalImplTest, testSubscribeContinuous_withMultipleAreaIds) { + // Clear existing events. + mEventQueue.flush(); + int propId = toInt(VehicleProperty::TIRE_PRESSURE); + + auto status = mHal->subscribe(propId, 1); + + ASSERT_EQ(StatusCode::OK, status); + + std::vector receivedEvents; + // Wait for 2 updates, each for 4 area IDs. + waitForEvents(&receivedEvents, 4 * 2); + + std::vector areasForUpdate1; + std::vector areasForUpdate2; + + for (size_t i = 0; i < receivedEvents.size(); i++) { + ASSERT_EQ(receivedEvents[i]->prop, propId); + + if (i < 4) { + areasForUpdate1.push_back(receivedEvents[i]->areaId); + } else { + areasForUpdate2.push_back(receivedEvents[i]->areaId); + } + } + + ASSERT_THAT(areasForUpdate1, UnorderedElementsAre(WHEEL_FRONT_LEFT, WHEEL_FRONT_RIGHT, + WHEEL_REAR_LEFT, WHEEL_REAR_RIGHT)); + ASSERT_THAT(areasForUpdate2, UnorderedElementsAre(WHEEL_FRONT_LEFT, WHEEL_FRONT_RIGHT, + WHEEL_REAR_LEFT, WHEEL_REAR_RIGHT)); +} + TEST_F(DefaultVhalImplTest, testSubscribeInvalidProp) { EXPECT_EQ(StatusCode::INVALID_ARG, mHal->subscribe(toInt(VehicleProperty::INFO_MAKE), 10)); } @@ -1318,7 +1355,6 @@ TEST_F(DefaultVhalImplTest, testDebugSetInt) { ASSERT_EQ((size_t)1, events.size()); ASSERT_EQ((size_t)1, events[0]->value.int32Values.size()); EXPECT_EQ(2022, events[0]->value.int32Values[0]); - EXPECT_EQ(1000, events[0]->timestamp); VehiclePropValue value; StatusCode status; @@ -1352,7 +1388,6 @@ TEST_F(DefaultVhalImplTest, testDebugSetBool) { ASSERT_EQ((size_t)1, events.size()); EXPECT_EQ(0, events[0]->value.int32Values[0]); EXPECT_EQ(DOOR_1_LEFT, events[0]->areaId); - EXPECT_EQ(1000, events[0]->timestamp); VehiclePropValue value; StatusCode status; @@ -1391,7 +1426,6 @@ TEST_F(DefaultVhalImplTest, testDebugSetFloat) { ASSERT_EQ((size_t)1, events.size()); ASSERT_EQ((size_t)1, events[0]->value.floatValues.size()); EXPECT_EQ(10.5, events[0]->value.floatValues[0]); - EXPECT_EQ(1000, events[0]->timestamp); VehiclePropValue value; StatusCode status; From e5456f1c5780f3f82a6de458ee76093f80b584fa Mon Sep 17 00:00:00 2001 From: Yu Shan Date: Wed, 29 Nov 2023 16:01:06 -0800 Subject: [PATCH 2/2] Refactor RecurrentTimer to use Looper. Use Looper to make the logic cleaner. Test: atest VehicleHalVehicleUtilsTest Bug: 305103906 Change-Id: I5a4669b104964ac1dddff4b70b19b2f4e94c2b90 --- .../utils/common/include/RecurrentTimer.h | 56 +++--- .../impl/utils/common/src/RecurrentTimer.cpp | 179 +++++++----------- .../utils/common/test/RecurrentTimerTest.cpp | 16 +- .../impl/vhal/test/DefaultVehicleHalTest.cpp | 1 + 4 files changed, 114 insertions(+), 138 deletions(-) diff --git a/automotive/vehicle/aidl/impl/utils/common/include/RecurrentTimer.h b/automotive/vehicle/aidl/impl/utils/common/include/RecurrentTimer.h index cd2b72713c..712359a885 100644 --- a/automotive/vehicle/aidl/impl/utils/common/include/RecurrentTimer.h +++ b/automotive/vehicle/aidl/impl/utils/common/include/RecurrentTimer.h @@ -19,6 +19,8 @@ #include +#include +#include #include #include #include @@ -31,6 +33,9 @@ namespace hardware { namespace automotive { namespace vehicle { +// Forward declaration +class RecurrentMessageHandler; + // A thread-safe recurrent timer. class RecurrentTimer final { public: @@ -43,49 +48,44 @@ class RecurrentTimer final { // Registers a recurrent callback for a given interval. // Registering the same callback twice will override the interval provided before. - void registerTimerCallback(int64_t intervalInNano, std::shared_ptr callback); + void registerTimerCallback(int64_t intervalInNanos, std::shared_ptr callback); // Unregisters a previously registered recurrent callback. void unregisterTimerCallback(std::shared_ptr callback); private: - // friend class for unit testing. + friend class RecurrentMessageHandler; + + // For unit test friend class RecurrentTimerTest; struct CallbackInfo { std::shared_ptr callback; - int64_t interval; - int64_t nextTime; - // A flag to indicate whether this CallbackInfo is already outdated and should be ignored. - // The reason we need this flag is because we cannot easily remove an element from a heap. - bool outdated = false; - - static bool cmp(const std::unique_ptr& lhs, - const std::unique_ptr& rhs); + int64_t intervalInNanos; + int64_t nextTimeInNanos; }; + android::sp mLooper; + android::sp mHandler; + + std::atomic mStopRequested = false; + std::atomic mCallbackId = 0; std::mutex mLock; std::thread mThread; - std::condition_variable mCond; - bool mStopRequested GUARDED_BY(mLock) = false; - // A map to map each callback to its current active CallbackInfo in the mCallbackQueue. - std::unordered_map, CallbackInfo*> mCallbacks GUARDED_BY(mLock); - // A min-heap sorted by nextTime. Note that because we cannot remove arbitrary element from the - // heap, a single Callback can have multiple entries in this queue, all but one should be valid. - // The rest should be mark as outdated. The valid one is one stored in mCallbacks. - std::vector> mCallbackQueue GUARDED_BY(mLock); + std::unordered_map, int> mIdByCallback GUARDED_BY(mLock); + std::unordered_map> mCallbackInfoById GUARDED_BY(mLock); - void loop(); + void handleMessage(const android::Message& message) EXCLUDES(mLock); + int getCallbackIdLocked(std::shared_ptr callback) REQUIRES(mLock); +}; - // Mark the callbackInfo as outdated and should be ignored when popped from the heap. - void markOutdatedLocked(CallbackInfo* callback) REQUIRES(mLock); - // Remove all outdated callbackInfos from the top of the heap. This function must be called - // each time we might introduce outdated elements to the top. We must make sure the heap is - // always valid from the top. - void removeInvalidCallbackLocked() REQUIRES(mLock); - // Gets the next calblack to run (must be valid) from the heap, update its nextTime and put - // it back to the heap. - std::shared_ptr getNextCallbackLocked(int64_t now) REQUIRES(mLock); +class RecurrentMessageHandler final : public android::MessageHandler { + public: + RecurrentMessageHandler(RecurrentTimer* timer) { mTimer = timer; } + void handleMessage(const android::Message& message) override; + + private: + RecurrentTimer* mTimer; }; } // namespace vehicle diff --git a/automotive/vehicle/aidl/impl/utils/common/src/RecurrentTimer.cpp b/automotive/vehicle/aidl/impl/utils/common/src/RecurrentTimer.cpp index c6d3687553..8dec695df1 100644 --- a/automotive/vehicle/aidl/impl/utils/common/src/RecurrentTimer.cpp +++ b/automotive/vehicle/aidl/impl/utils/common/src/RecurrentTimer.cpp @@ -17,6 +17,7 @@ #include "RecurrentTimer.h" #include +#include #include #include @@ -27,153 +28,119 @@ namespace hardware { namespace automotive { namespace vehicle { +namespace { + using ::android::base::ScopedLockAssertion; +constexpr int INVALID_ID = -1; + +} // namespace + RecurrentTimer::RecurrentTimer() { - mThread = std::thread(&RecurrentTimer::loop, this); + mHandler = sp::make(this); + mLooper = sp::make(/*allowNonCallbacks=*/false); + mThread = std::thread([this] { + Looper::setForThread(mLooper); + + while (!mStopRequested) { + mLooper->pollOnce(/*timeoutMillis=*/-1); + } + }); } RecurrentTimer::~RecurrentTimer() { - { - std::scoped_lock lockGuard(mLock); - mStopRequested = true; - } - mCond.notify_one(); + mStopRequested = true; + mLooper->removeMessages(mHandler); + mLooper->wake(); if (mThread.joinable()) { mThread.join(); } } -void RecurrentTimer::registerTimerCallback(int64_t intervalInNano, +int RecurrentTimer::getCallbackIdLocked(std::shared_ptr callback) { + const auto& it = mIdByCallback.find(callback); + if (it != mIdByCallback.end()) { + return it->second; + } + return INVALID_ID; +} + +void RecurrentTimer::registerTimerCallback(int64_t intervalInNanos, std::shared_ptr callback) { { std::scoped_lock lockGuard(mLock); + int callbackId = getCallbackIdLocked(callback); + + if (callbackId == INVALID_ID) { + callbackId = mCallbackId++; + mIdByCallback.insert({callback, callbackId}); + } else { + ALOGI("Replacing an existing timer callback with a new interval, current: %" PRId64 + " ns, new: %" PRId64 " ns", + mCallbackInfoById[callbackId]->intervalInNanos, intervalInNanos); + mLooper->removeMessages(mHandler, callbackId); + } + // Aligns the nextTime to multiply of interval. - int64_t nextTime = ceil(uptimeNanos() / intervalInNano) * intervalInNano; + int64_t nextTimeInNanos = ceil(uptimeNanos() / intervalInNanos) * intervalInNanos; std::unique_ptr info = std::make_unique(); info->callback = callback; - info->interval = intervalInNano; - info->nextTime = nextTime; + info->intervalInNanos = intervalInNanos; + info->nextTimeInNanos = nextTimeInNanos; + mCallbackInfoById.insert({callbackId, std::move(info)}); - auto it = mCallbacks.find(callback); - if (it != mCallbacks.end()) { - ALOGI("Replacing an existing timer callback with a new interval, current: %" PRId64 - " ns, new: %" PRId64 " ns", - it->second->interval, intervalInNano); - markOutdatedLocked(it->second); - } - mCallbacks[callback] = info.get(); - mCallbackQueue.push_back(std::move(info)); - // Insert the last element into the heap. - std::push_heap(mCallbackQueue.begin(), mCallbackQueue.end(), CallbackInfo::cmp); + mLooper->sendMessageAtTime(nextTimeInNanos, mHandler, Message(callbackId)); } - mCond.notify_one(); } void RecurrentTimer::unregisterTimerCallback(std::shared_ptr callback) { { std::scoped_lock lockGuard(mLock); - auto it = mCallbacks.find(callback); - if (it == mCallbacks.end()) { + int callbackId = getCallbackIdLocked(callback); + + if (callbackId == INVALID_ID) { ALOGE("No event found to unregister"); return; } - markOutdatedLocked(it->second); - mCallbacks.erase(it); - } - - mCond.notify_one(); -} - -void RecurrentTimer::markOutdatedLocked(RecurrentTimer::CallbackInfo* info) { - info->outdated = true; - info->callback = nullptr; - // Make sure the first element is always valid. - removeInvalidCallbackLocked(); -} - -void RecurrentTimer::removeInvalidCallbackLocked() { - while (mCallbackQueue.size() != 0 && mCallbackQueue[0]->outdated) { - std::pop_heap(mCallbackQueue.begin(), mCallbackQueue.end(), CallbackInfo::cmp); - mCallbackQueue.pop_back(); + mLooper->removeMessages(mHandler, callbackId); + mCallbackInfoById.erase(callbackId); + mIdByCallback.erase(callback); } } -std::shared_ptr RecurrentTimer::getNextCallbackLocked(int64_t now) { - std::pop_heap(mCallbackQueue.begin(), mCallbackQueue.end(), CallbackInfo::cmp); - auto& callbackInfo = mCallbackQueue[mCallbackQueue.size() - 1]; - auto nextCallback = callbackInfo->callback; - // intervalCount is the number of interval we have to advance until we pass now. - size_t intervalCount = (now - callbackInfo->nextTime) / callbackInfo->interval + 1; - callbackInfo->nextTime += intervalCount * callbackInfo->interval; - std::push_heap(mCallbackQueue.begin(), mCallbackQueue.end(), CallbackInfo::cmp); +void RecurrentTimer::handleMessage(const Message& message) { + std::shared_ptr callback; + { + std::scoped_lock lockGuard(mLock); - // Make sure the first element is always valid. - removeInvalidCallbackLocked(); + int callbackId = message.what; - return nextCallback; -} - -void RecurrentTimer::loop() { - std::vector> callbacksToRun; - while (true) { - { - std::unique_lock uniqueLock(mLock); - ScopedLockAssertion lockAssertion(mLock); - // Wait until the timer exits or we have at least one recurrent callback. - mCond.wait(uniqueLock, [this] { - ScopedLockAssertion lockAssertion(mLock); - return mStopRequested || mCallbackQueue.size() != 0; - }); - - int64_t interval; - if (mStopRequested) { - return; - } - // The first element is the nearest next event. - int64_t nextTime = mCallbackQueue[0]->nextTime; - int64_t now = uptimeNanos(); - - if (nextTime > now) { - interval = nextTime - now; - } else { - interval = 0; - } - - // Wait for the next event or the timer exits. - if (mCond.wait_for(uniqueLock, std::chrono::nanoseconds(interval), [this] { - ScopedLockAssertion lockAssertion(mLock); - return mStopRequested; - })) { - return; - } - - now = uptimeNanos(); - callbacksToRun.clear(); - while (mCallbackQueue.size() > 0) { - int64_t nextTime = mCallbackQueue[0]->nextTime; - if (nextTime > now) { - break; - } - - callbacksToRun.push_back(getNextCallbackLocked(now)); - } + auto it = mCallbackInfoById.find(callbackId); + if (it == mCallbackInfoById.end()) { + ALOGW("The event for callback ID: %d is outdated, ignore", callbackId); + return; } - // Do not execute the callback while holding the lock. - for (size_t i = 0; i < callbacksToRun.size(); i++) { - (*callbacksToRun[i])(); - } + CallbackInfo* callbackInfo = it->second.get(); + callback = callbackInfo->callback; + int64_t nowNanos = uptimeNanos(); + // intervalCount is the number of interval we have to advance until we pass now. + size_t intervalCount = + (nowNanos - callbackInfo->nextTimeInNanos) / callbackInfo->intervalInNanos + 1; + callbackInfo->nextTimeInNanos += intervalCount * callbackInfo->intervalInNanos; + + mLooper->sendMessageAtTime(callbackInfo->nextTimeInNanos, mHandler, Message(callbackId)); } + + (*callback)(); } -bool RecurrentTimer::CallbackInfo::cmp(const std::unique_ptr& lhs, - const std::unique_ptr& rhs) { - return lhs->nextTime > rhs->nextTime; +void RecurrentMessageHandler::handleMessage(const Message& message) { + mTimer->handleMessage(message); } } // namespace vehicle diff --git a/automotive/vehicle/aidl/impl/utils/common/test/RecurrentTimerTest.cpp b/automotive/vehicle/aidl/impl/utils/common/test/RecurrentTimerTest.cpp index 62046f38dc..e8ac2a5b38 100644 --- a/automotive/vehicle/aidl/impl/utils/common/test/RecurrentTimerTest.cpp +++ b/automotive/vehicle/aidl/impl/utils/common/test/RecurrentTimerTest.cpp @@ -60,9 +60,14 @@ class RecurrentTimerTest : public testing::Test { mCallbacks.clear(); } - size_t countTimerCallbackQueue(RecurrentTimer* timer) { + size_t countCallbackInfoById(RecurrentTimer* timer) { std::scoped_lock lockGuard(timer->mLock); - return timer->mCallbackQueue.size(); + return timer->mCallbackInfoById.size(); + } + + size_t countIdByCallback(RecurrentTimer* timer) { + std::scoped_lock lockGuard(timer->mLock); + return timer->mIdByCallback.size(); } private: @@ -109,6 +114,9 @@ TEST_F(RecurrentTimerTest, testRegisterUnregisterRegister) { << "Not enough callbacks called before timeout"; timer.unregisterTimerCallback(action); + + ASSERT_EQ(countCallbackInfoById(&timer), 0u); + ASSERT_EQ(countIdByCallback(&timer), 0u); } TEST_F(RecurrentTimerTest, testDestroyTimerWithCallback) { @@ -198,8 +206,8 @@ TEST_F(RecurrentTimerTest, testRegisterSameCallbackMultipleTimes) { timer.unregisterTimerCallback(action); - // Make sure there is no item in the callback queue. - ASSERT_EQ(countTimerCallbackQueue(&timer), static_cast(0)); + ASSERT_EQ(countCallbackInfoById(&timer), 0u); + ASSERT_EQ(countIdByCallback(&timer), 0u); } TEST_F(RecurrentTimerTest, testRegisterCallbackMultipleTimesNoDeadLock) { diff --git a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp index 63964efa54..a63cb846eb 100644 --- a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp +++ b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp @@ -59,6 +59,7 @@ using ::aidl::android::hardware::automotive::vehicle::SetValueResult; using ::aidl::android::hardware::automotive::vehicle::SetValueResults; using ::aidl::android::hardware::automotive::vehicle::StatusCode; using ::aidl::android::hardware::automotive::vehicle::SubscribeOptions; +using ::aidl::android::hardware::automotive::vehicle::VehicleAreaConfig; using ::aidl::android::hardware::automotive::vehicle::VehicleAreaWindow; using ::aidl::android::hardware::automotive::vehicle::VehiclePropConfig; using ::aidl::android::hardware::automotive::vehicle::VehiclePropConfigs;