Add permission check and heartbeat event to VHAL.

Test: atest DefaultVehicleHalTest
Bug: 200737967
Change-Id: I5ee4209a59dd63173060fb52a69a80bfbb3522c9
diff --git a/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h b/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h
index 6becff8..b9975bc 100644
--- a/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h
+++ b/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h
@@ -126,6 +126,8 @@
     // TODO(b/214605968): define TIMEOUT_IN_NANO in IVehicle and allow getValues/setValues/subscribe
     // to specify custom timeouts.
     static constexpr int64_t TIMEOUT_IN_NANO = 30'000'000'000;
+    // heart beat event interval: 3s
+    static constexpr int64_t HEART_BEAT_INTERVAL_IN_NANO = 3'000'000'000;
     const std::shared_ptr<IVehicleHardware> mVehicleHardware;
 
     // mConfigsByPropId and mConfigFile are only modified during initialization, so no need to
@@ -146,6 +148,8 @@
             GUARDED_BY(mLock);
     // SubscriptionClients is thread-safe.
     std::shared_ptr<SubscriptionClients> mSubscriptionClients;
+    // RecurrentTimer is thread-safe.
+    RecurrentTimer mRecurrentTimer;
 
     ::android::base::Result<void> checkProperty(
             const ::aidl::android::hardware::automotive::vehicle::VehiclePropValue& propValue);
@@ -162,6 +166,16 @@
             const std::vector<::aidl::android::hardware::automotive::vehicle::SubscribeOptions>&
                     options);
 
+    ::android::base::Result<void> checkReadPermission(
+            const ::aidl::android::hardware::automotive::vehicle::VehiclePropValue& value) const;
+
+    ::android::base::Result<void> checkWritePermission(
+            const ::aidl::android::hardware::automotive::vehicle::VehiclePropValue& value) const;
+
+    ::android::base::Result<
+            const ::aidl::android::hardware::automotive::vehicle::VehiclePropConfig*>
+    getConfig(int32_t propId) const;
+
     template <class T>
     static std::shared_ptr<T> getOrCreateClient(
             std::unordered_map<CallbackType, std::shared_ptr<T>>* clients,
@@ -178,6 +192,9 @@
             const std::vector<::aidl::android::hardware::automotive::vehicle::VehiclePropValue>&
                     updatedValues);
 
+    static void checkHealth(std::weak_ptr<IVehicleHardware> hardware,
+                            std::weak_ptr<SubscriptionManager> subscriptionManager);
+
     // Test-only
     // Set the default timeout for pending requests.
     void setTimeout(int64_t timeoutInNano);
diff --git a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp
index 7549635..c4cbc68 100644
--- a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp
+++ b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp
@@ -25,6 +25,7 @@
 #include <android-base/result.h>
 #include <android-base/stringprintf.h>
 #include <utils/Log.h>
+#include <utils/SystemClock.h>
 
 #include <inttypes.h>
 #include <set>
@@ -51,7 +52,10 @@
 using ::aidl::android::hardware::automotive::vehicle::VehicleAreaConfig;
 using ::aidl::android::hardware::automotive::vehicle::VehiclePropConfig;
 using ::aidl::android::hardware::automotive::vehicle::VehiclePropConfigs;
+using ::aidl::android::hardware::automotive::vehicle::VehicleProperty;
+using ::aidl::android::hardware::automotive::vehicle::VehiclePropertyAccess;
 using ::aidl::android::hardware::automotive::vehicle::VehiclePropertyChangeMode;
+using ::aidl::android::hardware::automotive::vehicle::VehiclePropertyStatus;
 using ::aidl::android::hardware::automotive::vehicle::VehiclePropValue;
 using ::android::automotive::car_binder_lib::LargeParcelableBase;
 using ::android::base::Error;
@@ -128,6 +132,13 @@
                     [subscriptionManagerCopy](std::vector<VehiclePropValue> updatedValues) {
                         onPropertyChangeEvent(subscriptionManagerCopy, updatedValues);
                     }));
+
+    // Register heartbeat event.
+    mRecurrentTimer.registerTimerCallback(
+            HEART_BEAT_INTERVAL_IN_NANO,
+            std::make_shared<std::function<void()>>([hardwareCopy, subscriptionManagerCopy]() {
+                checkHealth(hardwareCopy, subscriptionManagerCopy);
+            }));
 }
 
 void DefaultVehicleHal::onPropertyChangeEvent(
@@ -222,27 +233,35 @@
     return ScopedAStatus::ok();
 }
 
-Result<void> DefaultVehicleHal::checkProperty(const VehiclePropValue& propValue) {
-    int32_t propId = propValue.prop;
+Result<const VehiclePropConfig*> DefaultVehicleHal::getConfig(int32_t propId) const {
     auto it = mConfigsByPropId.find(propId);
     if (it == mConfigsByPropId.end()) {
         return Error() << "no config for property, ID: " << propId;
     }
-    const VehiclePropConfig& config = it->second;
-    const VehicleAreaConfig* areaConfig = getAreaConfig(propValue, config);
+    return &(it->second);
+}
+
+Result<void> DefaultVehicleHal::checkProperty(const VehiclePropValue& propValue) {
+    int32_t propId = propValue.prop;
+    auto result = getConfig(propId);
+    if (!result.ok()) {
+        return result.error();
+    }
+    const VehiclePropConfig* config = result.value();
+    const VehicleAreaConfig* areaConfig = getAreaConfig(propValue, *config);
     if (!isGlobalProp(propId) && areaConfig == nullptr) {
         // Ignore areaId for global property. For non global property, check whether areaId is
         // allowed. areaId must appear in areaConfig.
         return Error() << "invalid area ID: " << propValue.areaId << " for prop ID: " << propId
                        << ", not listed in config";
     }
-    if (auto result = checkPropValue(propValue, &config); !result.ok()) {
+    if (auto result = checkPropValue(propValue, config); !result.ok()) {
         return Error() << "invalid property value: " << propValue.toString()
-                       << ", error: " << result.error().message();
+                       << ", error: " << getErrorMsg(result);
     }
     if (auto result = checkValueRange(propValue, areaConfig); !result.ok()) {
         return Error() << "property value out of range: " << propValue.toString()
-                       << ", error: " << result.error().message();
+                       << ", error: " << getErrorMsg(result);
     }
     return {};
 }
@@ -263,9 +282,30 @@
         ALOGE("getValues: duplicate request ID");
         return toScopedAStatus(maybeRequestIds, StatusCode::INVALID_ARG);
     }
+
+    // A list of failed result we already know before sending to hardware.
+    std::vector<GetValueResult> failedResults;
+    // The list of requests that we would send to hardware.
+    std::vector<GetValueRequest> hardwareRequests;
+
+    for (const auto& request : getValueRequests) {
+        if (auto result = checkReadPermission(request.prop); !result.ok()) {
+            ALOGW("property does not support reading: %s", getErrorMsg(result).c_str());
+            failedResults.push_back(GetValueResult{
+                    .requestId = request.requestId,
+                    .status = getErrorCode(result),
+                    .prop = {},
+            });
+        } else {
+            hardwareRequests.push_back(request);
+        }
+    }
+
     // The set of request Ids that we would send to hardware.
-    std::unordered_set<int64_t> hardwareRequestIds(maybeRequestIds.value().begin(),
-                                                   maybeRequestIds.value().end());
+    std::unordered_set<int64_t> hardwareRequestIds;
+    for (const auto& request : hardwareRequests) {
+        hardwareRequestIds.insert(request.requestId);
+    }
 
     std::shared_ptr<GetValuesClient> client;
     {
@@ -275,12 +315,21 @@
     // Register the pending hardware requests and also check for duplicate request Ids.
     if (auto addRequestResult = client->addRequests(hardwareRequestIds); !addRequestResult.ok()) {
         ALOGE("getValues[%s]: failed to add pending requests, error: %s",
-              toString(hardwareRequestIds).c_str(), addRequestResult.error().message().c_str());
+              toString(hardwareRequestIds).c_str(), getErrorMsg(addRequestResult).c_str());
         return toScopedAStatus(addRequestResult);
     }
 
+    if (!failedResults.empty()) {
+        // First send the failed results we already know back to the client.
+        client->sendResults(failedResults);
+    }
+
+    if (hardwareRequests.empty()) {
+        return ScopedAStatus::ok();
+    }
+
     if (StatusCode status =
-                mVehicleHardware->getValues(client->getResultCallback(), getValueRequests);
+                mVehicleHardware->getValues(client->getResultCallback(), hardwareRequests);
         status != StatusCode::OK) {
         // If the hardware returns error, finish all the pending requests for this request because
         // we never expect hardware to call callback for these requests.
@@ -317,9 +366,17 @@
 
     for (auto& request : setValueRequests) {
         int64_t requestId = request.requestId;
+        if (auto result = checkWritePermission(request.value); !result.ok()) {
+            ALOGW("property does not support writing: %s", getErrorMsg(result).c_str());
+            failedResults.push_back(SetValueResult{
+                    .requestId = requestId,
+                    .status = getErrorCode(result),
+            });
+            continue;
+        }
         if (auto result = checkProperty(request.value); !result.ok()) {
-            ALOGW("setValues[%" PRId64 "]: property not valid: %s", requestId,
-                  result.error().message().c_str());
+            ALOGW("setValues[%" PRId64 "]: property is not valid: %s", requestId,
+                  getErrorMsg(result).c_str());
             failedResults.push_back(SetValueResult{
                     .requestId = requestId,
                     .status = StatusCode::INVALID_ARG,
@@ -345,7 +402,7 @@
     // Register the pending hardware requests and also check for duplicate request Ids.
     if (auto addRequestResult = client->addRequests(hardwareRequestIds); !addRequestResult.ok()) {
         ALOGE("setValues[%s], failed to add pending requests, error: %s",
-              toString(hardwareRequestIds).c_str(), addRequestResult.error().message().c_str());
+              toString(hardwareRequestIds).c_str(), getErrorMsg(addRequestResult).c_str());
         return toScopedAStatus(addRequestResult, StatusCode::INVALID_ARG);
     }
 
@@ -354,6 +411,10 @@
         client->sendResults(failedResults);
     }
 
+    if (hardwareRequests.empty()) {
+        return ScopedAStatus::ok();
+    }
+
     if (StatusCode status =
                 mVehicleHardware->setValues(client->getResultCallback(), hardwareRequests);
         status != StatusCode::OK) {
@@ -413,13 +474,21 @@
     for (const auto& option : options) {
         int32_t propId = option.propId;
         if (mConfigsByPropId.find(propId) == mConfigsByPropId.end()) {
-            return Error() << StringPrintf("no config for property, ID: %" PRId32, propId);
+            return Error(toInt(StatusCode::INVALID_ARG))
+                   << StringPrintf("no config for property, ID: %" PRId32, propId);
         }
         const VehiclePropConfig& config = mConfigsByPropId[propId];
 
         if (config.changeMode != VehiclePropertyChangeMode::ON_CHANGE &&
             config.changeMode != VehiclePropertyChangeMode::CONTINUOUS) {
-            return Error() << "only support subscribing to ON_CHANGE or CONTINUOUS property";
+            return Error(toInt(StatusCode::INVALID_ARG))
+                   << "only support subscribing to ON_CHANGE or CONTINUOUS property";
+        }
+
+        if (config.access != VehiclePropertyAccess::READ &&
+            config.access != VehiclePropertyAccess::READ_WRITE) {
+            return Error(toInt(StatusCode::ACCESS_DENIED))
+                   << StringPrintf("Property %" PRId32 " has no read access", propId);
         }
 
         if (config.changeMode == VehiclePropertyChangeMode::CONTINUOUS) {
@@ -427,12 +496,13 @@
             float minSampleRate = config.minSampleRate;
             float maxSampleRate = config.maxSampleRate;
             if (sampleRate < minSampleRate || sampleRate > maxSampleRate) {
-                return Error() << StringPrintf(
-                               "sample rate: %f out of range, must be within %f and %f", sampleRate,
-                               minSampleRate, maxSampleRate);
+                return Error(toInt(StatusCode::INVALID_ARG))
+                       << StringPrintf("sample rate: %f out of range, must be within %f and %f",
+                                       sampleRate, minSampleRate, maxSampleRate);
             }
             if (!SubscriptionManager::checkSampleRate(sampleRate)) {
-                return Error() << "invalid sample rate: " << sampleRate;
+                return Error(toInt(StatusCode::INVALID_ARG))
+                       << "invalid sample rate: " << sampleRate;
             }
         }
 
@@ -443,9 +513,10 @@
         // Non-global property.
         for (int32_t areaId : option.areaIds) {
             if (auto areaConfig = getAreaConfig(propId, areaId, config); areaConfig == nullptr) {
-                return Error() << StringPrintf("invalid area ID: %" PRId32 " for prop ID: %" PRId32
-                                               ", not listed in config",
-                                               areaId, propId);
+                return Error(toInt(StatusCode::INVALID_ARG))
+                       << StringPrintf("invalid area ID: %" PRId32 " for prop ID: %" PRId32
+                                       ", not listed in config",
+                                       areaId, propId);
             }
         }
     }
@@ -457,8 +528,8 @@
                                            [[maybe_unused]] int32_t maxSharedMemoryFileCount) {
     // TODO(b/205189110): Use shared memory file count.
     if (auto result = checkSubscribeOptions(options); !result.ok()) {
-        ALOGE("subscribe: invalid subscribe options: %s", result.error().message().c_str());
-        return toScopedAStatus(result, StatusCode::INVALID_ARG);
+        ALOGE("subscribe: invalid subscribe options: %s", getErrorMsg(result).c_str());
+        return toScopedAStatus(result);
     }
 
     std::vector<SubscribeOptions> onChangeSubscriptions;
@@ -513,6 +584,61 @@
     return mVehicleHardware.get();
 }
 
+Result<void> DefaultVehicleHal::checkWritePermission(const VehiclePropValue& value) const {
+    int32_t propId = value.prop;
+    auto result = getConfig(propId);
+    if (!result.ok()) {
+        return Error(toInt(StatusCode::INVALID_ARG)) << getErrorMsg(result);
+    }
+    const VehiclePropConfig* config = result.value();
+
+    if (config->access != VehiclePropertyAccess::WRITE &&
+        config->access != VehiclePropertyAccess::READ_WRITE) {
+        return Error(toInt(StatusCode::ACCESS_DENIED))
+               << StringPrintf("Property %" PRId32 " has no write access", propId);
+    }
+    return {};
+}
+
+Result<void> DefaultVehicleHal::checkReadPermission(const VehiclePropValue& value) const {
+    int32_t propId = value.prop;
+    auto result = getConfig(propId);
+    if (!result.ok()) {
+        return Error(toInt(StatusCode::INVALID_ARG)) << getErrorMsg(result);
+    }
+    const VehiclePropConfig* config = result.value();
+
+    if (config->access != VehiclePropertyAccess::READ &&
+        config->access != VehiclePropertyAccess::READ_WRITE) {
+        return Error(toInt(StatusCode::ACCESS_DENIED))
+               << StringPrintf("Property %" PRId32 " has no read access", propId);
+    }
+    return {};
+}
+
+void DefaultVehicleHal::checkHealth(std::weak_ptr<IVehicleHardware> hardware,
+                                    std::weak_ptr<SubscriptionManager> subscriptionManager) {
+    auto hardwarePtr = hardware.lock();
+    if (hardwarePtr == nullptr) {
+        ALOGW("the VehicleHardware is destroyed, DefaultVehicleHal is ending");
+        return;
+    }
+
+    StatusCode status = hardwarePtr->checkHealth();
+    if (status != StatusCode::OK) {
+        ALOGE("VHAL check health returns non-okay status");
+        return;
+    }
+    std::vector<VehiclePropValue> values = {{
+            .prop = toInt(VehicleProperty::VHAL_HEARTBEAT),
+            .areaId = 0,
+            .status = VehiclePropertyStatus::AVAILABLE,
+            .value.int64Values = {uptimeMillis()},
+    }};
+    onPropertyChangeEvent(subscriptionManager, values);
+    return;
+}
+
 }  // namespace vehicle
 }  // namespace automotive
 }  // namespace hardware
diff --git a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp
index d8c9fa2..54fc17d 100644
--- a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp
+++ b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp
@@ -28,6 +28,7 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 #include <utils/Log.h>
+#include <utils/SystemClock.h>
 
 #include <chrono>
 #include <list>
@@ -61,6 +62,8 @@
 using ::aidl::android::hardware::automotive::vehicle::VehiclePropConfig;
 using ::aidl::android::hardware::automotive::vehicle::VehiclePropConfigs;
 using ::aidl::android::hardware::automotive::vehicle::VehiclePropErrors;
+using ::aidl::android::hardware::automotive::vehicle::VehicleProperty;
+using ::aidl::android::hardware::automotive::vehicle::VehiclePropertyAccess;
 using ::aidl::android::hardware::automotive::vehicle::VehiclePropertyChangeMode;
 using ::aidl::android::hardware::automotive::vehicle::VehiclePropValue;
 using ::aidl::android::hardware::automotive::vehicle::VehiclePropValues;
@@ -87,6 +90,10 @@
 constexpr int32_t AREA_ON_CHANGE_PROP = 10004 + 0x10000000 + 0x03000000 + 0x00400000;
 // VehiclePropertyGroup:SYSTEM,VehicleArea:WINDOW,VehiclePropertyType:INT32
 constexpr int32_t AREA_CONTINUOUS_PROP = 10005 + 0x10000000 + 0x03000000 + 0x00400000;
+// VehiclePropertyGroup:SYSTEM,VehicleArea:GLOBAL,VehiclePropertyType:INT32
+constexpr int32_t READ_ONLY_PROP = 10006 + 0x10000000 + 0x01000000 + 0x00400000;
+// VehiclePropertyGroup:SYSTEM,VehicleArea:GLOBAL,VehiclePropertyType:INT32
+constexpr int32_t WRITE_ONLY_PROP = 10007 + 0x10000000 + 0x01000000 + 0x00400000;
 
 int32_t testInt32VecProp(size_t i) {
     // VehiclePropertyGroup:SYSTEM,VehicleArea:GLOBAL,VehiclePropertyType:INT32_VEC
@@ -145,6 +152,15 @@
                                     .areaId = toInt(VehicleAreaWindow::ROW_1_RIGHT),
                             },
                     .expectedStatus = StatusCode::INVALID_ARG,
+            },
+            {
+                    .name = "no_write_permission",
+                    .request =
+                            {
+                                    .prop = READ_ONLY_PROP,
+                                    .value.int32Values = {0},
+                            },
+                    .expectedStatus = StatusCode::ACCESS_DENIED,
             }};
 }
 
@@ -205,6 +221,7 @@
         for (size_t i = 0; i < 10000; i++) {
             testConfigs.push_back(VehiclePropConfig{
                     .prop = testInt32VecProp(i),
+                    .access = VehiclePropertyAccess::READ_WRITE,
                     .areaConfigs =
                             {
                                     {
@@ -218,6 +235,7 @@
         // A property with area config.
         testConfigs.push_back(
                 VehiclePropConfig{.prop = INT32_WINDOW_PROP,
+                                  .access = VehiclePropertyAccess::READ_WRITE,
                                   .areaConfigs = {{
                                           .areaId = toInt(VehicleAreaWindow::ROW_1_LEFT),
                                           .minInt32Value = 0,
@@ -226,11 +244,13 @@
         // A global on-change property.
         testConfigs.push_back(VehiclePropConfig{
                 .prop = GLOBAL_ON_CHANGE_PROP,
+                .access = VehiclePropertyAccess::READ_WRITE,
                 .changeMode = VehiclePropertyChangeMode::ON_CHANGE,
         });
         // A global continuous property.
         testConfigs.push_back(VehiclePropConfig{
                 .prop = GLOBAL_CONTINUOUS_PROP,
+                .access = VehiclePropertyAccess::READ_WRITE,
                 .changeMode = VehiclePropertyChangeMode::CONTINUOUS,
                 .minSampleRate = 0.0,
                 .maxSampleRate = 100.0,
@@ -238,6 +258,7 @@
         // A per-area on-change property.
         testConfigs.push_back(VehiclePropConfig{
                 .prop = AREA_ON_CHANGE_PROP,
+                .access = VehiclePropertyAccess::READ_WRITE,
                 .changeMode = VehiclePropertyChangeMode::ON_CHANGE,
                 .areaConfigs =
                         {
@@ -257,6 +278,7 @@
         // A per-area continuous property.
         testConfigs.push_back(VehiclePropConfig{
                 .prop = AREA_CONTINUOUS_PROP,
+                .access = VehiclePropertyAccess::READ_WRITE,
                 .changeMode = VehiclePropertyChangeMode::CONTINUOUS,
                 .minSampleRate = 0.0,
                 .maxSampleRate = 1000.0,
@@ -275,6 +297,28 @@
                                 },
                         },
         });
+        // A read-only property.
+        testConfigs.push_back(VehiclePropConfig{
+                .prop = READ_ONLY_PROP,
+                .access = VehiclePropertyAccess::READ,
+                .changeMode = VehiclePropertyChangeMode::CONTINUOUS,
+                .minSampleRate = 0.0,
+                .maxSampleRate = 1000.0,
+        });
+        // A write-only property.
+        testConfigs.push_back(VehiclePropConfig{
+                .prop = WRITE_ONLY_PROP,
+                .access = VehiclePropertyAccess::WRITE,
+                .changeMode = VehiclePropertyChangeMode::CONTINUOUS,
+                .minSampleRate = 0.0,
+                .maxSampleRate = 1000.0,
+        });
+        // Register the heartbeat event property.
+        testConfigs.push_back(VehiclePropConfig{
+                .prop = toInt(VehicleProperty::VHAL_HEARTBEAT),
+                .access = VehiclePropertyAccess::READ,
+                .changeMode = VehiclePropertyChangeMode::ON_CHANGE,
+        });
         hardware->setPropertyConfigs(testConfigs);
         mHardwarePtr = hardware.get();
         mVhal = ndk::SharedRefBase::make<DefaultVehicleHal>(std::move(hardware));
@@ -509,6 +553,39 @@
     ASSERT_EQ(status.getServiceSpecificError(), toInt(StatusCode::INVALID_ARG));
 }
 
+TEST_F(DefaultVehicleHalTest, testGetValuesNoReadPermission) {
+    GetValueRequests requests = {
+            .sharedMemoryFd = {},
+            .payloads =
+                    {
+                            {
+                                    .requestId = 0,
+                                    .prop =
+                                            {
+                                                    .prop = WRITE_ONLY_PROP,
+                                            },
+                            },
+                    },
+    };
+
+    auto status = getClient()->getValues(getCallbackClient(), requests);
+
+    ASSERT_TRUE(status.isOk()) << "getValue with no read permission should return okay with error "
+                                  "returned from callback"
+                               << ", error: " << status.getMessage();
+    EXPECT_TRUE(getHardware()->nextGetValueRequests().empty()) << "expect no request to hardware";
+
+    auto maybeResult = getCallback()->nextGetValueResults();
+    ASSERT_TRUE(maybeResult.has_value()) << "no results in callback";
+    EXPECT_EQ(maybeResult.value().payloads, std::vector<GetValueResult>({
+                                                    {
+                                                            .requestId = 0,
+                                                            .status = StatusCode::ACCESS_DENIED,
+                                                    },
+                                            }))
+            << "expect to get ACCESS_DENIED status if no read permission";
+}
+
 TEST_F(DefaultVehicleHalTest, testGetValuesFinishBeforeTimeout) {
     // timeout: 0.1s
     int64_t timeout = 100000000;
@@ -1318,7 +1395,7 @@
             return info.param.name;
         });
 
-TEST_P(SubscribeInvalidOptionsTest, testSubscribeInvalidRequest) {
+TEST_P(SubscribeInvalidOptionsTest, testSubscribeInvalidOptions) {
     std::vector<SubscribeOptions> options = {GetParam().option};
 
     auto status = getClient()->subscribe(getCallbackClient(), options, 0);
@@ -1327,6 +1404,17 @@
     ASSERT_EQ(status.getServiceSpecificError(), toInt(StatusCode::INVALID_ARG));
 }
 
+TEST_F(DefaultVehicleHalTest, testSubscribeNoReadPermission) {
+    std::vector<SubscribeOptions> options = {{
+            .propId = WRITE_ONLY_PROP,
+    }};
+
+    auto status = getClient()->subscribe(getCallbackClient(), options, 0);
+
+    ASSERT_FALSE(status.isOk()) << "subscribe to a write-only property must fail";
+    ASSERT_EQ(status.getServiceSpecificError(), toInt(StatusCode::ACCESS_DENIED));
+}
+
 TEST_F(DefaultVehicleHalTest, testUnsubscribeFailure) {
     auto status = getClient()->unsubscribe(getCallbackClient(),
                                            std::vector<int32_t>({GLOBAL_ON_CHANGE_PROP}));
@@ -1335,6 +1423,28 @@
     ASSERT_EQ(status.getServiceSpecificError(), toInt(StatusCode::INVALID_ARG));
 }
 
+TEST_F(DefaultVehicleHalTest, testHeartbeatEvent) {
+    std::vector<SubscribeOptions> options = {{
+            .propId = toInt(VehicleProperty::VHAL_HEARTBEAT),
+    }};
+    int64_t currentTime = uptimeMillis();
+    auto status = getClient()->subscribe(getCallbackClient(), options, 0);
+
+    ASSERT_TRUE(status.isOk()) << "unable to subscribe to heartbeat event: " << status.getMessage();
+
+    // We send out a heartbeat event every 3s, so sleep for 3s.
+    std::this_thread::sleep_for(std::chrono::seconds(3));
+
+    auto maybeResults = getCallback()->nextOnPropertyEventResults();
+    ASSERT_TRUE(maybeResults.has_value()) << "no results in callback";
+    ASSERT_EQ(maybeResults.value().payloads.size(), static_cast<size_t>(1));
+    VehiclePropValue gotValue = maybeResults.value().payloads[0];
+    ASSERT_EQ(gotValue.prop, toInt(VehicleProperty::VHAL_HEARTBEAT));
+    ASSERT_EQ(gotValue.value.int64Values.size(), static_cast<size_t>(1));
+    ASSERT_GE(gotValue.value.int64Values[0], currentTime)
+            << "expect to get the latest timestamp with the heartbeat event";
+}
+
 }  // namespace vehicle
 }  // namespace automotive
 }  // namespace hardware
diff --git a/automotive/vehicle/aidl/impl/vhal/test/SubscriptionManagerTest.cpp b/automotive/vehicle/aidl/impl/vhal/test/SubscriptionManagerTest.cpp
index fa08d6c..c14f4fd 100644
--- a/automotive/vehicle/aidl/impl/vhal/test/SubscriptionManagerTest.cpp
+++ b/automotive/vehicle/aidl/impl/vhal/test/SubscriptionManagerTest.cpp
@@ -185,7 +185,7 @@
 
     // Theoretically trigger 10 times, but check for at least 9 times to be stable.
     EXPECT_GE(getEvents().size(), static_cast<size_t>(9));
-    EXPECT_LE(getEvents().size(), static_cast<size_t>(11));
+    EXPECT_LE(getEvents().size(), static_cast<size_t>(15));
 }
 
 TEST_F(SubscriptionManagerTest, testSubscribeMultipleAreasContinuous) {