From 67c465f7fe3c68019157290dff5235d43ef39c27 Mon Sep 17 00:00:00 2001 From: shrikar Date: Sun, 14 Jan 2024 06:11:07 +0000 Subject: [PATCH 1/2] Added resolution to SubscriptionManager in reference VHAL Bug: 276124296 Test: atest DefaultVehicleHalTest Change-Id: I972b7f0c04953b390f642317c491f3f99e045045 --- .../impl/utils/common/include/VehicleUtils.h | 17 +++ .../impl/vhal/include/SubscriptionManager.h | 19 +++- .../impl/vhal/src/SubscriptionManager.cpp | 79 ++++++++++--- .../vhal/test/SubscriptionManagerTest.cpp | 104 ++++++++++++++++++ 4 files changed, 198 insertions(+), 21 deletions(-) diff --git a/automotive/vehicle/aidl/impl/utils/common/include/VehicleUtils.h b/automotive/vehicle/aidl/impl/utils/common/include/VehicleUtils.h index 523cac527e..aca725df56 100644 --- a/automotive/vehicle/aidl/impl/utils/common/include/VehicleUtils.h +++ b/automotive/vehicle/aidl/impl/utils/common/include/VehicleUtils.h @@ -333,6 +333,23 @@ inline std::string propIdToString(int32_t propId) { static_cast(propId)); } +template +void roundToNearestResolution(std::vector& arrayToSanitize, float resolution) { + if (resolution == 0) { + return; + } + for (size_t i = 0; i < arrayToSanitize.size(); i++) { + arrayToSanitize[i] = (T)((std::round(arrayToSanitize[i] / resolution)) * resolution); + } +} + +inline void sanitizeByResolution(aidl::android::hardware::automotive::vehicle::RawPropValues* value, + float resolution) { + roundToNearestResolution(value->int32Values, resolution); + roundToNearestResolution(value->floatValues, resolution); + roundToNearestResolution(value->int64Values, resolution); +} + } // namespace vehicle } // namespace automotive } // namespace hardware diff --git a/automotive/vehicle/aidl/impl/vhal/include/SubscriptionManager.h b/automotive/vehicle/aidl/impl/vhal/include/SubscriptionManager.h index 5053c96efe..2f16fca93c 100644 --- a/automotive/vehicle/aidl/impl/vhal/include/SubscriptionManager.h +++ b/automotive/vehicle/aidl/impl/vhal/include/SubscriptionManager.h @@ -25,6 +25,8 @@ #include #include +#include +#include #include #include #include @@ -39,6 +41,7 @@ namespace vehicle { // A structure to represent subscription config for one subscription client. struct SubConfig { float sampleRateHz; + float resolution; bool enableVur; }; @@ -47,14 +50,19 @@ class ContSubConfigs final { public: using ClientIdType = const AIBinder*; - void addClient(const ClientIdType& clientId, float sampleRateHz, bool enableVur); + void addClient(const ClientIdType& clientId, const SubConfig& subConfig); void removeClient(const ClientIdType& clientId); float getMaxSampleRateHz() const; + float getMinRequiredResolution() const; bool isVurEnabled() const; - bool isVurEnabledForClient(const ClientIdType& clientId); + bool isVurEnabledForClient(const ClientIdType& clientId) const; + float getResolutionForClient(const ClientIdType& clientId) const; private: float mMaxSampleRateHz = 0.; + // Baseline for resolution is maximum possible float. We want to sanitize to the highest + // requested resolution, which is the smallest float value for resolution. + float mMinRequiredResolution = std::numeric_limits::max(); bool mEnableVur; std::unordered_map mConfigByClient; @@ -117,6 +125,9 @@ class SubscriptionManager final { // Checks whether the sample rate is valid. static bool checkSampleRateHz(float sampleRateHz); + // Checks whether the resolution is valid. + static bool checkResolution(float resolution); + private: // Friend class for testing. friend class DefaultVehicleHalTest; @@ -153,8 +164,8 @@ class SubscriptionManager final { VhalResult addContinuousSubscriberLocked(const ClientIdType& clientId, const PropIdAreaId& propIdAreaId, - float sampleRateHz, bool enableVur) - REQUIRES(mLock); + float sampleRateHz, float resolution, + bool enableVur) REQUIRES(mLock); VhalResult addOnChangeSubscriberLocked(const PropIdAreaId& propIdAreaId) REQUIRES(mLock); // Removes the subscription client for the continuous [propId, areaId]. VhalResult removeContinuousSubscriberLocked(const ClientIdType& clientId, diff --git a/automotive/vehicle/aidl/impl/vhal/src/SubscriptionManager.cpp b/automotive/vehicle/aidl/impl/vhal/src/SubscriptionManager.cpp index 29d81a75e7..f1106eea54 100644 --- a/automotive/vehicle/aidl/impl/vhal/src/SubscriptionManager.cpp +++ b/automotive/vehicle/aidl/impl/vhal/src/SubscriptionManager.cpp @@ -43,11 +43,12 @@ using ::ndk::ScopedAStatus; constexpr float ONE_SECOND_IN_NANOS = 1'000'000'000.; SubscribeOptions newSubscribeOptions(int32_t propId, int32_t areaId, float sampleRateHz, - bool enableVur) { + float resolution, bool enableVur) { SubscribeOptions subscribedOptions; subscribedOptions.propId = propId; subscribedOptions.areaIds = {areaId}; subscribedOptions.sampleRate = sampleRateHz; + subscribedOptions.resolution = resolution; subscribedOptions.enableVariableUpdateRate = enableVur; return subscribedOptions; @@ -81,8 +82,18 @@ Result SubscriptionManager::getIntervalNanos(float sampleRateHz) { return intervalNanos; } +bool SubscriptionManager::checkResolution(float resolution) { + if (resolution == 0) { + return true; + } + + float log = std::log10(resolution); + return log == (int)log; +} + void ContSubConfigs::refreshCombinedConfig() { float maxSampleRateHz = 0.; + float minRequiredResolution = std::numeric_limits::max(); bool enableVur = true; // This is not called frequently so a brute-focre is okay. More efficient way exists but this // is simpler. @@ -90,6 +101,9 @@ void ContSubConfigs::refreshCombinedConfig() { if (subConfig.sampleRateHz > maxSampleRateHz) { maxSampleRateHz = subConfig.sampleRateHz; } + if (subConfig.resolution < minRequiredResolution) { + minRequiredResolution = subConfig.resolution; + } if (!subConfig.enableVur) { // If one client does not enable variable update rate, we cannot enable variable update // rate in IVehicleHardware. @@ -97,14 +111,12 @@ void ContSubConfigs::refreshCombinedConfig() { } } mMaxSampleRateHz = maxSampleRateHz; + mMinRequiredResolution = minRequiredResolution; mEnableVur = enableVur; } -void ContSubConfigs::addClient(const ClientIdType& clientId, float sampleRateHz, bool enableVur) { - mConfigByClient[clientId] = { - .sampleRateHz = sampleRateHz, - .enableVur = enableVur, - }; +void ContSubConfigs::addClient(const ClientIdType& clientId, const SubConfig& subConfig) { + mConfigByClient[clientId] = subConfig; refreshCombinedConfig(); } @@ -117,12 +129,26 @@ float ContSubConfigs::getMaxSampleRateHz() const { return mMaxSampleRateHz; } +float ContSubConfigs::getMinRequiredResolution() const { + return mMinRequiredResolution; +} + bool ContSubConfigs::isVurEnabled() const { return mEnableVur; } -bool ContSubConfigs::isVurEnabledForClient(const ClientIdType& clientId) { - return mConfigByClient[clientId].enableVur; +bool ContSubConfigs::isVurEnabledForClient(const ClientIdType& clientId) const { + if (mConfigByClient.find(clientId) == mConfigByClient.end()) { + return false; + } + return mConfigByClient.at(clientId).enableVur; +} + +float ContSubConfigs::getResolutionForClient(const ClientIdType& clientId) const { + if (mConfigByClient.find(clientId) == mConfigByClient.end()) { + return 0.0f; + } + return mConfigByClient.at(clientId).resolution; } VhalResult SubscriptionManager::addOnChangeSubscriberLocked( @@ -135,7 +161,8 @@ VhalResult SubscriptionManager::addOnChangeSubscriberLocked( int32_t propId = propIdAreaId.propId; int32_t areaId = propIdAreaId.areaId; if (auto status = mVehicleHardware->subscribe( - newSubscribeOptions(propId, areaId, /*updateRateHz=*/0, /*enableVur*/ false)); + newSubscribeOptions(propId, areaId, /*updateRateHz=*/0, /*resolution*/ 0.0f, + /*enableVur*/ false)); status != StatusCode::OK) { return StatusError(status) << StringPrintf("failed subscribe for prop: %s, areaId: %" PRId32, @@ -146,10 +173,15 @@ VhalResult SubscriptionManager::addOnChangeSubscriberLocked( VhalResult SubscriptionManager::addContinuousSubscriberLocked( const ClientIdType& clientId, const PropIdAreaId& propIdAreaId, float sampleRateHz, - bool enableVur) { + float resolution, bool enableVur) { // Make a copy so that we don't modify 'mContSubConfigsByPropIdArea' on failure cases. ContSubConfigs newConfig = mContSubConfigsByPropIdArea[propIdAreaId]; - newConfig.addClient(clientId, sampleRateHz, enableVur); + SubConfig subConfig = { + .sampleRateHz = sampleRateHz, + .resolution = resolution, + .enableVur = enableVur, + }; + newConfig.addClient(clientId, subConfig); return updateContSubConfigsLocked(propIdAreaId, newConfig); } @@ -183,7 +215,10 @@ VhalResult SubscriptionManager::updateContSubConfigsLocked(const PropIdAre const auto& oldConfig = mContSubConfigsByPropIdArea[propIdAreaId]; float newRateHz = newConfig.getMaxSampleRateHz(); float oldRateHz = oldConfig.getMaxSampleRateHz(); - if (newRateHz == oldRateHz && newConfig.isVurEnabled() == oldConfig.isVurEnabled()) { + float newResolution = newConfig.getMinRequiredResolution(); + float oldResolution = oldConfig.getMinRequiredResolution(); + if (newRateHz == oldRateHz && newResolution == oldResolution && + newConfig.isVurEnabled() == oldConfig.isVurEnabled()) { mContSubConfigsByPropIdArea[propIdAreaId] = newConfig; return {}; } @@ -199,8 +234,8 @@ VhalResult SubscriptionManager::updateContSubConfigsLocked(const PropIdAre } } if (newRateHz != 0) { - if (auto status = mVehicleHardware->subscribe( - newSubscribeOptions(propId, areaId, newRateHz, newConfig.isVurEnabled())); + if (auto status = mVehicleHardware->subscribe(newSubscribeOptions( + propId, areaId, newRateHz, newResolution, newConfig.isVurEnabled())); status != StatusCode::OK) { return StatusError(status) << StringPrintf( "failed subscribe for prop: %s, areaId" @@ -231,6 +266,11 @@ VhalResult SubscriptionManager::subscribe(const std::shared_ptr SubscriptionManager::subscribe(const std::shared_ptr result; if (isContinuousProperty) { result = addContinuousSubscriberLocked(clientId, propIdAreaId, option.sampleRate, + option.resolution, option.enableVariableUpdateRate); } else { result = addOnChangeSubscriberLocked(propIdAreaId); @@ -393,15 +434,19 @@ SubscriptionManager::getSubscribedClients(std::vector&& update for (const auto& [client, callback] : mClientsByPropIdAreaId[propIdAreaId]) { auto& subConfigs = mContSubConfigsByPropIdArea[propIdAreaId]; + // Clients must be sent different VehiclePropValues with different levels of granularity + // as requested by the client using resolution. + VehiclePropValue newValue = value; + sanitizeByResolution(&(newValue.value), subConfigs.getResolutionForClient(client)); // If client wants VUR (and VUR is supported as checked in DefaultVehicleHal), it is // possible that VUR is not enabled in IVehicleHardware because another client does not // enable VUR. We will implement VUR filtering here for the client that enables it. if (subConfigs.isVurEnabledForClient(client) && !subConfigs.isVurEnabled()) { - if (isValueUpdatedLocked(callback, value)) { - clients[callback].push_back(value); + if (isValueUpdatedLocked(callback, newValue)) { + clients[callback].push_back(newValue); } } else { - clients[callback].push_back(value); + clients[callback].push_back(newValue); } } } diff --git a/automotive/vehicle/aidl/impl/vhal/test/SubscriptionManagerTest.cpp b/automotive/vehicle/aidl/impl/vhal/test/SubscriptionManagerTest.cpp index aa5f003dec..f3772022e8 100644 --- a/automotive/vehicle/aidl/impl/vhal/test/SubscriptionManagerTest.cpp +++ b/automotive/vehicle/aidl/impl/vhal/test/SubscriptionManagerTest.cpp @@ -520,6 +520,14 @@ TEST_F(SubscriptionManagerTest, testCheckSampleRateHzInvalidZero) { ASSERT_FALSE(SubscriptionManager::checkSampleRateHz(0)); } +TEST_F(SubscriptionManagerTest, testCheckResolutionValid) { + ASSERT_TRUE(SubscriptionManager::checkResolution(1.0)); +} + +TEST_F(SubscriptionManagerTest, testCheckResolutionInvalid) { + ASSERT_FALSE(SubscriptionManager::checkResolution(2.0)); +} + TEST_F(SubscriptionManagerTest, testSubscribe_enableVur) { std::vector options = {{ .propId = 0, @@ -641,6 +649,102 @@ TEST_F(SubscriptionManagerTest, testSubscribe_enableVur_filterUnchangedEvents) { << "Must filter out property events if VUR is enabled"; } +TEST_F(SubscriptionManagerTest, testSubscribe_enableVur_filterUnchangedEvents_withResolution) { + SpAIBinder binder1 = ndk::SharedRefBase::make()->asBinder(); + std::shared_ptr client1 = IVehicleCallback::fromBinder(binder1); + SpAIBinder binder2 = ndk::SharedRefBase::make()->asBinder(); + std::shared_ptr client2 = IVehicleCallback::fromBinder(binder2); + SubscribeOptions client1Option = { + .propId = 0, + .areaIds = {0}, + .sampleRate = 10.0, + .resolution = 0.01, + .enableVariableUpdateRate = false, + }; + auto result = getManager()->subscribe(client1, {client1Option}, true); + ASSERT_TRUE(result.ok()) << "failed to subscribe: " << result.error().message(); + + ASSERT_THAT(getHardware()->getSubscribeOptions(), UnorderedElementsAre(client1Option)); + + getHardware()->clearSubscribeOptions(); + SubscribeOptions client2Option = { + .propId = 0, + .areaIds = {0, 1}, + .sampleRate = 20.0, + .resolution = 0.1, + .enableVariableUpdateRate = true, + }; + + result = getManager()->subscribe(client2, {client2Option}, true); + ASSERT_TRUE(result.ok()) << "failed to subscribe: " << result.error().message(); + + ASSERT_THAT(getHardware()->getSubscribeOptions(), + UnorderedElementsAre( + SubscribeOptions{ + .propId = 0, + .areaIds = {0}, + .sampleRate = 20.0, + .resolution = 0.01, + // This is enabled for client2, but disabled for client1. + .enableVariableUpdateRate = false, + }, + SubscribeOptions{ + .propId = 0, + .areaIds = {1}, + .sampleRate = 20.0, + .resolution = 0.1, + .enableVariableUpdateRate = true, + })); + + std::vector propertyEvents = {{ + .prop = 0, + .areaId = 0, + .value = {.floatValues = {1.0}}, + .timestamp = 1, + }, + { + .prop = 0, + .areaId = 1, + .value = {.floatValues = {1.0}}, + .timestamp = 1, + }}; + auto clients = + getManager()->getSubscribedClients(std::vector(propertyEvents)); + + ASSERT_THAT(clients[client1], UnorderedElementsAre(propertyEvents[0])); + ASSERT_THAT(clients[client2], UnorderedElementsAre(propertyEvents[0], propertyEvents[1])); + + clients = getManager()->getSubscribedClients({{ + .prop = 0, + .areaId = 0, + .value = {.floatValues = {1.01}}, + .timestamp = 2, + }}); + + ASSERT_FALSE(clients.find(client1) == clients.end()) + << "Must not filter out property events if VUR is not enabled"; + ASSERT_TRUE(clients.find(client2) == clients.end()) + << "Must filter out property events if VUR is enabled and change is too small"; + ASSERT_TRUE(abs(clients[client1][0].value.floatValues[0] - 1.01) < 0.0000001) + << "Expected property value == 1.01, instead got " + << clients[client1][0].value.floatValues[0]; + + clients = getManager()->getSubscribedClients({{ + .prop = 0, + .areaId = 1, + .value = {.floatValues = {1.06}}, + .timestamp = 3, + }}); + + ASSERT_TRUE(clients.find(client1) == clients.end()) + << "Must not get property events for an areaId that the client hasn't subscribed to"; + ASSERT_FALSE(clients.find(client2) == clients.end()) + << "Must get property events significant changes"; + ASSERT_TRUE(abs(clients[client2][0].value.floatValues[0] - 1.1) < 0.0000001) + << "Expected property value == 1.1, instead got " + << clients[client2][0].value.floatValues[0]; +} + TEST_F(SubscriptionManagerTest, testSubscribe_enableVur_mustNotFilterStatusChange) { SpAIBinder binder1 = ndk::SharedRefBase::make()->asBinder(); std::shared_ptr client1 = IVehicleCallback::fromBinder(binder1); From cb59b0522b610dc3bb72ee9d2d0d8169a7860742 Mon Sep 17 00:00:00 2001 From: shrikar Date: Thu, 8 Feb 2024 21:11:53 +0000 Subject: [PATCH 2/2] Added resolution check in DefaultVehicleHal Bug: 276124296 Test: atest DefaultVehicleHalTest Change-Id: If53c6395a8ea9656e6b59a43872a0d9f6871dac5 --- .../vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp | 4 ++++ .../vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp index cc5edcc49c..a29861f475 100644 --- a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp +++ b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp @@ -722,6 +722,10 @@ VhalResult DefaultVehicleHal::checkSubscribeOptions( return StatusError(StatusCode::INVALID_ARG) << "invalid sample rate: " << sampleRateHz << " HZ"; } + if (!SubscriptionManager::checkResolution(option.resolution)) { + return StatusError(StatusCode::INVALID_ARG) + << "invalid resolution: " << option.resolution; + } } } return {}; diff --git a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp index bb82108c1a..11a8fc7ac1 100644 --- a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp +++ b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp @@ -233,6 +233,14 @@ std::vector getSubscribeInvalidOptionsTestCases .sampleRate = 0.0, }, }, + { + .name = "invalid_resolution", + .option = + { + .propId = GLOBAL_CONTINUOUS_PROP, + .resolution = 2.0, + }, + }, { .name = "static_property", .option =