Merge "Improve error handling with separate ICanErrorListener"

automotive/can/1.0/Android.bp

@@ -10,6 +10,7 @@ hidl_interface {
         "types.hal",
         "ICanBus.hal",
         "ICanController.hal",
+        "ICanErrorListener.hal",
         "ICanMessageListener.hal",
         "ICloseHandle.hal",
     ],

automotive/can/1.0/ICanBus.hal

@@ -15,6 +15,7 @@
  */
 package android.hardware.automotive.can@1.0;
 
+import ICanErrorListener;
 import ICanMessageListener;
 import ICloseHandle;
 
@@ -57,4 +58,15 @@ interface ICanBus {
      */
     listen(vec<CanMessageFilter> filter, ICanMessageListener listener)
             generates (Result result, ICloseHandle close);
+
+    /**
+     * Adds a new listener for CAN bus or interface errors.
+     *
+     * If the error is fatal, the client is supposed to drop any references to
+     * this specific ICanBus instance (see ICanErrorListener).
+     *
+     * @param listener The interface to receive the error events on
+     * @return close A handle to call in order to remove the listener
+     */
+    listenForErrors(ICanErrorListener listener) generates (ICloseHandle close);
 };

automotive/can/1.0/ICanErrorListener.hal

@@ -0,0 +1,32 @@
+/*
+ * Copyright (C) 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package android.hardware.automotive.can@1.0;
+
+/**
+ * CAN error listener.
+ */
+interface ICanErrorListener {
+    /**
+     * Called on error event.
+     *
+     * If the error is fatal, the client is supposed to drop any references to
+     * this specific ICanBus instance.
+     *
+     * @param error Error type
+     * @param isFatal Whether an error would result with ICanBus instance being unusable.
+     */
+    onError(ErrorEvent error, bool isFatal);
+};

automotive/can/1.0/ICanMessageListener.hal

@@ -30,11 +30,4 @@ interface ICanMessageListener {
      * @param message Received CAN message
      */
     onReceive(CanMessage message);
-
-    /**
-     * Called on error event.
-     *
-     * @param error Error type
-     */
-    onError(ErrorEvent error);
 };

automotive/can/1.0/default/CanBus.cpp

@@ -68,14 +68,14 @@ Return<void> CanBus::listen(const hidl_vec<CanMessageFilter>& filter,
         return {};
     }
 
-    std::lock_guard<std::mutex> lckListeners(mListenersGuard);
+    std::lock_guard<std::mutex> lckListeners(mMsgListenersGuard);
 
     sp<CloseHandle> closeHandle = new CloseHandle([this, listenerCb]() {
-        std::lock_guard<std::mutex> lck(mListenersGuard);
-        std::erase_if(mListeners, [&](const auto& e) { return e.callback == listenerCb; });
+        std::lock_guard<std::mutex> lck(mMsgListenersGuard);
+        std::erase_if(mMsgListeners, [&](const auto& e) { return e.callback == listenerCb; });
     });
-    mListeners.emplace_back(CanMessageListener{listenerCb, filter, closeHandle});
-    auto& listener = mListeners.back();
+    mMsgListeners.emplace_back(CanMessageListener{listenerCb, filter, closeHandle});
+    auto& listener = mMsgListeners.back();
 
     // fix message IDs to have all zeros on bits not covered by mask
     std::for_each(listener.filter.begin(), listener.filter.end(),
@@ -91,8 +91,15 @@ CanBus::~CanBus() {
     std::lock_guard<std::mutex> lck(mIsUpGuard);
     CHECK(!mIsUp) << "Interface is still up while being destroyed";
 
-    std::lock_guard<std::mutex> lckListeners(mListenersGuard);
-    CHECK(mListeners.empty()) << "Listeners list is not empty while interface is being destroyed";
+    std::lock_guard<std::mutex> lckListeners(mMsgListenersGuard);
+    CHECK(mMsgListeners.empty()) << "Listener list is not empty while interface is being destroyed";
+}
+
+void CanBus::setErrorCallback(ErrorCallback errcb) {
+    CHECK(!mIsUp) << "Can't set error callback while interface is up";
+    CHECK(mErrCb == nullptr) << "Error callback is already set";
+    mErrCb = errcb;
+    CHECK(!mIsUp) << "Can't set error callback while interface is up";
 }
 
 ICanController::Result CanBus::preUp() {
@@ -120,19 +127,19 @@ ICanController::Result CanBus::up() {
         LOG(ERROR) << "Interface " << mIfname << " didn't get prepared";
         return ICanController::Result::BAD_ADDRESS;
     }
-    mWasUpInitially = *isUp;
 
-    if (!mWasUpInitially && !netdevice::up(mIfname)) {
+    if (!*isUp && !netdevice::up(mIfname)) {
         LOG(ERROR) << "Can't bring " << mIfname << " up";
         return ICanController::Result::UNKNOWN_ERROR;
     }
+    mDownAfterUse = !*isUp;
 
     using namespace std::placeholders;
     CanSocket::ReadCallback rdcb = std::bind(&CanBus::onRead, this, _1, _2);
-    CanSocket::ErrorCallback errcb = std::bind(&CanBus::onError, this);
+    CanSocket::ErrorCallback errcb = std::bind(&CanBus::onError, this, _1);
     mSocket = CanSocket::open(mIfname, rdcb, errcb);
     if (!mSocket) {
-        if (!mWasUpInitially) netdevice::down(mIfname);
+        if (mDownAfterUse) netdevice::down(mIfname);
         return ICanController::Result::UNKNOWN_ERROR;
     }
 
@@ -140,24 +147,50 @@ ICanController::Result CanBus::up() {
     return ICanController::Result::OK;
 }
 
-void CanBus::clearListeners() {
+void CanBus::clearMsgListeners() {
     std::vector<wp<ICloseHandle>> listenersToClose;
     {
-        std::lock_guard<std::mutex> lck(mListenersGuard);
-        std::transform(mListeners.begin(), mListeners.end(), std::back_inserter(listenersToClose),
+        std::lock_guard<std::mutex> lck(mMsgListenersGuard);
+        std::transform(mMsgListeners.begin(), mMsgListeners.end(),
+                       std::back_inserter(listenersToClose),
                        [](const auto& e) { return e.closeHandle; });
     }
 
     for (auto& weakListener : listenersToClose) {
         /* Between populating listenersToClose and calling close method here, some listeners might
-         * have been already removed from the original mListeners list (resulting in a dangling weak
-         * pointer here). It's fine - we just want to clean them up. */
+         * have been already removed from the original mMsgListeners list (resulting in a dangling
+         * weak pointer here). It's fine - we just want to clean them up. */
         auto listener = weakListener.promote();
         if (listener != nullptr) listener->close();
     }
 
-    std::lock_guard<std::mutex> lck(mListenersGuard);
-    CHECK(mListeners.empty()) << "Listeners list wasn't emptied";
+    std::lock_guard<std::mutex> lck(mMsgListenersGuard);
+    CHECK(mMsgListeners.empty()) << "Listeners list wasn't emptied";
+}
+
+void CanBus::clearErrListeners() {
+    std::lock_guard<std::mutex> lck(mErrListenersGuard);
+    mErrListeners.clear();
+}
+
+Return<sp<ICloseHandle>> CanBus::listenForErrors(const sp<ICanErrorListener>& listener) {
+    if (listener == nullptr) {
+        return new CloseHandle();
+    }
+
+    std::lock_guard<std::mutex> upLck(mIsUpGuard);
+    if (!mIsUp) {
+        listener->onError(ErrorEvent::INTERFACE_DOWN, true);
+        return new CloseHandle();
+    }
+
+    std::lock_guard<std::mutex> errLck(mErrListenersGuard);
+    mErrListeners.emplace_back(listener);
+
+    return new CloseHandle([this, listener]() {
+        std::lock_guard<std::mutex> lck(mErrListenersGuard);
+        std::erase(mErrListeners, listener);
+    });
 }
 
 bool CanBus::down() {
@@ -169,12 +202,13 @@ bool CanBus::down() {
     }
     mIsUp = false;
 
-    clearListeners();
+    clearMsgListeners();
+    clearErrListeners();
     mSocket.reset();
 
     bool success = true;
 
-    if (!mWasUpInitially && !netdevice::down(mIfname)) {
+    if (mDownAfterUse && !netdevice::down(mIfname)) {
         LOG(ERROR) << "Can't bring " << mIfname << " down";
         // don't return yet, let's try to do best-effort cleanup
         success = false;
@@ -223,22 +257,35 @@ void CanBus::onRead(const struct canfd_frame& frame, std::chrono::nanoseconds ti
         LOG(VERBOSE) << "Got message " << toString(message);
     }
 
-    std::lock_guard<std::mutex> lck(mListenersGuard);
-    for (auto& listener : mListeners) {
+    std::lock_guard<std::mutex> lck(mMsgListenersGuard);
+    for (auto& listener : mMsgListeners) {
         if (!match(listener.filter, message.id)) continue;
-        if (!listener.callback->onReceive(message).isOk()) {
+        if (!listener.callback->onReceive(message).isOk() && !listener.failedOnce) {
+            listener.failedOnce = true;
             LOG(WARNING) << "Failed to notify listener about message";
         }
     }
 }
 
-void CanBus::onError() {
-    std::lock_guard<std::mutex> lck(mListenersGuard);
-    for (auto& listener : mListeners) {
-        if (!listener.callback->onError(ErrorEvent::HARDWARE_ERROR).isOk()) {
-            LOG(WARNING) << "Failed to notify listener about error";
+void CanBus::onError(int errnoVal) {
+    auto eventType = ErrorEvent::HARDWARE_ERROR;
+
+    if (errnoVal == ENODEV || errnoVal == ENETDOWN) {
+        mDownAfterUse = false;
+        eventType = ErrorEvent::INTERFACE_DOWN;
+    }
+
+    {
+        std::lock_guard<std::mutex> lck(mErrListenersGuard);
+        for (auto& listener : mErrListeners) {
+            if (!listener->onError(eventType, true).isOk()) {
+                LOG(WARNING) << "Failed to notify listener about error";
+            }
         }
     }
+
+    const auto errcb = mErrCb;
+    if (errcb != nullptr) errcb();
 }
 
 }  // namespace implementation

automotive/can/1.0/default/CanBus.h

@@ -34,12 +34,16 @@ namespace V1_0 {
 namespace implementation {
 
 struct CanBus : public ICanBus {
+    using ErrorCallback = std::function<void()>;
+
     virtual ~CanBus();
 
     Return<Result> send(const CanMessage& message) override;
     Return<void> listen(const hidl_vec<CanMessageFilter>& filter,
                         const sp<ICanMessageListener>& listener, listen_cb _hidl_cb) override;
+    Return<sp<ICloseHandle>> listenForErrors(const sp<ICanErrorListener>& listener) override;
 
+    void setErrorCallback(ErrorCallback errcb);
     ICanController::Result up();
     bool down();
 
@@ -68,17 +72,22 @@ struct CanBus : public ICanBus {
         sp<ICanMessageListener> callback;
         hidl_vec<CanMessageFilter> filter;
         wp<ICloseHandle> closeHandle;
+        bool failedOnce = false;
     };
-    void clearListeners();
+    void clearMsgListeners();
+    void clearErrListeners();
 
     void onRead(const struct canfd_frame& frame, std::chrono::nanoseconds timestamp);
-    void onError();
+    void onError(int errnoVal);
 
-    std::mutex mListenersGuard;
-    std::vector<CanMessageListener> mListeners GUARDED_BY(mListenersGuard);
+    std::mutex mMsgListenersGuard;
+    std::vector<CanMessageListener> mMsgListeners GUARDED_BY(mMsgListenersGuard);
+
+    std::mutex mErrListenersGuard;
+    std::vector<sp<ICanErrorListener>> mErrListeners GUARDED_BY(mErrListenersGuard);
 
     std::unique_ptr<CanSocket> mSocket;
-    bool mWasUpInitially;
+    bool mDownAfterUse;
 
     /**
      * Guard for up flag is required to be held for entire time when the interface is being used
@@ -87,6 +96,8 @@ struct CanBus : public ICanBus {
      */
     std::mutex mIsUpGuard;
     bool mIsUp GUARDED_BY(mIsUpGuard) = false;
+
+    ErrorCallback mErrCb;
 };
 
 }  // namespace implementation

automotive/can/1.0/default/CanController.cpp

@@ -81,6 +81,8 @@ Return<ICanController::Result> CanController::upInterface(
         return ICanController::Result::NOT_SUPPORTED;
     }
 
+    busService->setErrorCallback([this, name = config.name]() { downInterface(name); });
+
     const auto result = busService->up();
     if (result != ICanController::Result::OK) return result;
 
@@ -97,6 +99,14 @@ Return<ICanController::Result> CanController::upInterface(
     return ICanController::Result::OK;
 }
 
+static bool unregisterCanBusService(const hidl_string& name, sp<CanBus> busService) {
+    auto manager = hidl::manager::V1_2::IServiceManager::getService();
+    if (!manager) return false;
+    const auto res = manager->tryUnregister(ICanBus::descriptor, name, busService);
+    if (!res.isOk()) return false;
+    return res;
+}
+
 Return<bool> CanController::downInterface(const hidl_string& name) {
     LOG(VERBOSE) << "Attempting to bring interface down: " << name;
 
@@ -110,8 +120,7 @@ Return<bool> CanController::downInterface(const hidl_string& name) {
 
     auto success = true;
 
-    auto manager = hidl::manager::V1_2::IServiceManager::getService();
-    if (!manager || !manager->tryUnregister(ICanBus::descriptor, name, busEntry.mapped())) {
+    if (!unregisterCanBusService(name, busEntry.mapped())) {
         LOG(ERROR) << "Couldn't unregister " << name;
         // don't return yet, let's try to do best-effort cleanup
         success = false;

automotive/can/1.0/default/CanSocket.cpp

@@ -61,7 +61,14 @@ CanSocket::CanSocket(base::unique_fd socket, ReadCallback rdcb, ErrorCallback er
 
 CanSocket::~CanSocket() {
     mStopReaderThread = true;
-    mReaderThread.join();
+
+    /* CanSocket can be brought down as a result of read failure, from the same thread,
+     * so let's just detach and let it finish on its own. */
+    if (mReaderThreadFinished) {
+        mReaderThread.detach();
+    } else {
+        mReaderThread.join();
+    }
 }
 
 bool CanSocket::send(const struct canfd_frame& frame) {
@@ -94,6 +101,7 @@ static int selectRead(const base::unique_fd& fd, std::chrono::microseconds timeo
 
 void CanSocket::readerThread() {
     LOG(VERBOSE) << "Reader thread started";
+    int errnoCopy = 0;
 
     while (!mStopReaderThread) {
         /* The ideal would be to have a blocking read(3) call and interrupt it with shutdown(3).
@@ -130,14 +138,20 @@ void CanSocket::readerThread() {
             }
             if (errno == EAGAIN) continue;
 
-            LOG(ERROR) << "Failed to read CAN packet: " << errno;
+            errnoCopy = errno;
+            LOG(ERROR) << "Failed to read CAN packet: " << strerror(errno) << " (" << errno << ")";
             break;
         }
 
         mReadCallback(frame, ts);
     }
 
-    if (!mStopReaderThread) mErrorCallback();
+    bool failed = !mStopReaderThread;
+    auto errCb = mErrorCallback;
+    mReaderThreadFinished = true;
+
+    // Don't access any fields from here, see CanSocket::~CanSocket comment about detached thread
+    if (failed) errCb(errnoCopy);
 
     LOG(VERBOSE) << "Reader thread stopped";
 }

automotive/can/1.0/default/CanSocket.h

@@ -36,7 +36,7 @@ namespace implementation {
  */
 struct CanSocket {
     using ReadCallback = std::function<void(const struct canfd_frame&, std::chrono::nanoseconds)>;
-    using ErrorCallback = std::function<void()>;
+    using ErrorCallback = std::function<void(int errnoVal)>;
 
     /**
      * Open and bind SocketCAN socket.
@@ -68,6 +68,7 @@ struct CanSocket {
     const base::unique_fd mSocket;
     std::thread mReaderThread;
     std::atomic<bool> mStopReaderThread = false;
+    std::atomic<bool> mReaderThreadFinished = false;
 
     DISALLOW_COPY_AND_ASSIGN(CanSocket);
 };

automotive/can/1.0/default/CloseHandle.cpp

@@ -33,7 +33,7 @@ Return<void> CloseHandle::close() {
     const auto wasClosed = mIsClosed.exchange(true);
     if (wasClosed) return {};
 
-    mCallback();
+    if (mCallback != nullptr) mCallback();
     return {};
 }
 

automotive/can/1.0/default/CloseHandle.h

@@ -39,13 +39,13 @@ struct CloseHandle : public ICloseHandle {
      *
      * \param callback Called on the first close() call, or on destruction of the handle
      */
-    CloseHandle(Callback callback);
+    CloseHandle(Callback callback = nullptr);
     virtual ~CloseHandle();
 
     Return<void> close() override;
 
   private:
-    Callback mCallback;
+    const Callback mCallback;
     std::atomic<bool> mIsClosed = false;
 
     DISALLOW_COPY_AND_ASSIGN(CloseHandle);

automotive/can/1.0/hidl-utils/Android.bp

@@ -17,4 +17,5 @@
 cc_library_headers {
     name: "android.hardware.automotive.can@hidl-utils-lib",
     export_include_dirs: ["include"],
+    vendor_available: true,
 }

automotive/can/1.0/types.hal

@@ -95,6 +95,9 @@ enum ErrorEvent : uint8_t {
     /** A problem with CAN interface HW. */
     HARDWARE_ERROR,
 
+    /** CAN interface is down. */
+    INTERFACE_DOWN,
+
     /** TX buffer overflow: client is sending too many packets. */
     TX_OVERFLOW,
 

automotive/can/1.0/vts/functional/VtsHalCanBusV1_0TargetTest.cpp

@@ -36,8 +36,11 @@ using hardware::hidl_vec;
 static utils::SimpleHidlEnvironment<ICanBus>* gEnv = nullptr;
 
 struct CanMessageListener : public can::V1_0::ICanMessageListener {
-    virtual Return<void> onReceive(const can::V1_0::CanMessage&) { return {}; }
-    virtual Return<void> onError(can::V1_0::ErrorEvent) { return {}; }
+    virtual Return<void> onReceive(const can::V1_0::CanMessage&) override { return {}; }
+};
+
+struct CanErrorListener : public can::V1_0::ICanErrorListener {
+    virtual Return<void> onError(ErrorEvent, bool) override { return {}; }
 };
 
 class CanBusHalTest : public ::testing::VtsHalHidlTargetTestBase {
@@ -47,6 +50,7 @@ class CanBusHalTest : public ::testing::VtsHalHidlTargetTestBase {
 
     std::tuple<Result, sp<ICloseHandle>> listen(const hidl_vec<CanMessageFilter>& filter,
                                                 const sp<ICanMessageListener>& listener);
+    sp<ICloseHandle> listenForErrors(const sp<ICanErrorListener>& listener);
 
     sp<ICanBus> mCanBus;
 };
@@ -70,6 +74,12 @@ std::tuple<Result, sp<ICloseHandle>> CanBusHalTest::listen(
     return {halResult, closeHandle};
 }
 
+sp<ICloseHandle> CanBusHalTest::listenForErrors(const sp<ICanErrorListener>& listener) {
+    const auto res = mCanBus->listenForErrors(listener);
+    res.assertOk();
+    return res;
+}
+
 TEST_F(CanBusHalTest, SendNoPayload) {
     CanMessage msg = {};
     msg.id = 0x123;
@@ -129,7 +139,7 @@ TEST_F(CanBusHalTest, ListenNull) {
     ASSERT_EQ(Result::INVALID_ARGUMENTS, result);
 }
 
-TEST_F(CanBusHalTest, DoubleCloseListen) {
+TEST_F(CanBusHalTest, DoubleCloseListener) {
     const auto [result, closeHandle] = listen({}, new CanMessageListener());
     ASSERT_EQ(Result::OK, result);
 
@@ -137,11 +147,32 @@ TEST_F(CanBusHalTest, DoubleCloseListen) {
     closeHandle->close().assertOk();
 }
 
-TEST_F(CanBusHalTest, DontCloseListen) {
+TEST_F(CanBusHalTest, DontCloseListener) {
     const auto [result, closeHandle] = listen({}, new CanMessageListener());
     ASSERT_EQ(Result::OK, result);
 }
 
+TEST_F(CanBusHalTest, DoubleCloseErrorListener) {
+    auto closeHandle = listenForErrors(new CanErrorListener());
+    ASSERT_NE(nullptr, closeHandle.get());
+
+    closeHandle->close().assertOk();
+    closeHandle->close().assertOk();
+}
+
+TEST_F(CanBusHalTest, DoubleCloseNullErrorListener) {
+    auto closeHandle = listenForErrors(nullptr);
+    ASSERT_NE(nullptr, closeHandle.get());
+
+    closeHandle->close().assertOk();
+    closeHandle->close().assertOk();
+}
+
+TEST_F(CanBusHalTest, DontCloseErrorListener) {
+    auto closeHandle = listenForErrors(new CanErrorListener());
+    ASSERT_NE(nullptr, closeHandle.get());
+}
+
 }  // namespace vts
 }  // namespace V1_0
 }  // namespace can

automotive/can/1.0/vts/functional/VtsHalCanBusVirtualV1_0TargetTest.cpp

@@ -50,18 +50,13 @@ struct CanMessageListener : public can::V1_0::ICanMessageListener {
 
     CanMessageListener() {}
 
-    virtual Return<void> onReceive(const can::V1_0::CanMessage& msg) {
+    virtual Return<void> onReceive(const can::V1_0::CanMessage& msg) override {
         std::unique_lock<std::mutex> lk(mMessagesGuard);
         mMessages.push_back(msg);
         mMessagesUpdated.notify_one();
         return {};
     }
 
-    virtual Return<void> onError(can::V1_0::ErrorEvent event) {
-        EXPECT_TRUE(false) << "Got error: " << event;
-        return {};
-    }
-
     virtual ~CanMessageListener() { mCloseHandle->close(); }
 
     void assignCloseHandle(sp<ICloseHandle> closeHandle) {