From a90619662b6d0b1874180d9729719061efe4a570 Mon Sep 17 00:00:00 2001 From: Tomasz Wasilczyk Date: Mon, 4 Nov 2019 12:53:09 -0800 Subject: [PATCH] Improve error handling with separate ICanErrorListener Error handling highlights: - moved onError from ICanMessageListener to ICanErrorListener - added isFatal callback argument to request client disconnect - don't down interface that's already down Also: - don't crash if it's not possible to unregister ICanBus - don't crash while trying to down interface that failed - make hidl-utils available to vendor libraries Bug: 143779011 Test: implemented a VHAL service prototype that communicates with this HAL Change-Id: I98d054da9da0ead5ef952aebc086e052ac996212 --- automotive/can/1.0/Android.bp | 1 + automotive/can/1.0/ICanBus.hal | 12 ++ automotive/can/1.0/ICanErrorListener.hal | 32 ++++++ automotive/can/1.0/ICanMessageListener.hal | 7 -- automotive/can/1.0/default/CanBus.cpp | 103 +++++++++++++----- automotive/can/1.0/default/CanBus.h | 21 +++- automotive/can/1.0/default/CanController.cpp | 13 ++- automotive/can/1.0/default/CanSocket.cpp | 20 +++- automotive/can/1.0/default/CanSocket.h | 3 +- automotive/can/1.0/default/CloseHandle.cpp | 2 +- automotive/can/1.0/default/CloseHandle.h | 4 +- automotive/can/1.0/hidl-utils/Android.bp | 1 + automotive/can/1.0/types.hal | 3 + .../functional/VtsHalCanBusV1_0TargetTest.cpp | 39 ++++++- .../VtsHalCanBusVirtualV1_0TargetTest.cpp | 7 +- 15 files changed, 209 insertions(+), 59 deletions(-) create mode 100644 automotive/can/1.0/ICanErrorListener.hal diff --git a/automotive/can/1.0/Android.bp b/automotive/can/1.0/Android.bp index fe2a2bed21..2221e6623e 100644 --- a/automotive/can/1.0/Android.bp +++ b/automotive/can/1.0/Android.bp @@ -10,6 +10,7 @@ hidl_interface { "types.hal", "ICanBus.hal", "ICanController.hal", + "ICanErrorListener.hal", "ICanMessageListener.hal", "ICloseHandle.hal", ], diff --git a/automotive/can/1.0/ICanBus.hal b/automotive/can/1.0/ICanBus.hal index 6ed89f3b22..e68f16c6fc 100644 --- a/automotive/can/1.0/ICanBus.hal +++ b/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 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); }; diff --git a/automotive/can/1.0/ICanErrorListener.hal b/automotive/can/1.0/ICanErrorListener.hal new file mode 100644 index 0000000000..8a6ba054bc --- /dev/null +++ b/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); +}; diff --git a/automotive/can/1.0/ICanMessageListener.hal b/automotive/can/1.0/ICanMessageListener.hal index 992d1c77ff..28161fa747 100644 --- a/automotive/can/1.0/ICanMessageListener.hal +++ b/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); }; diff --git a/automotive/can/1.0/default/CanBus.cpp b/automotive/can/1.0/default/CanBus.cpp index 65da291b0b..38a9974771 100644 --- a/automotive/can/1.0/default/CanBus.cpp +++ b/automotive/can/1.0/default/CanBus.cpp @@ -68,14 +68,14 @@ Return CanBus::listen(const hidl_vec& filter, return {}; } - std::lock_guard lckListeners(mListenersGuard); + std::lock_guard lckListeners(mMsgListenersGuard); sp closeHandle = new CloseHandle([this, listenerCb]() { - std::lock_guard lck(mListenersGuard); - std::erase_if(mListeners, [&](const auto& e) { return e.callback == listenerCb; }); + std::lock_guard 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 lck(mIsUpGuard); CHECK(!mIsUp) << "Interface is still up while being destroyed"; - std::lock_guard lckListeners(mListenersGuard); - CHECK(mListeners.empty()) << "Listeners list is not empty while interface is being destroyed"; + std::lock_guard 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> listenersToClose; { - std::lock_guard lck(mListenersGuard); - std::transform(mListeners.begin(), mListeners.end(), std::back_inserter(listenersToClose), + std::lock_guard 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 lck(mListenersGuard); - CHECK(mListeners.empty()) << "Listeners list wasn't emptied"; + std::lock_guard lck(mMsgListenersGuard); + CHECK(mMsgListeners.empty()) << "Listeners list wasn't emptied"; +} + +void CanBus::clearErrListeners() { + std::lock_guard lck(mErrListenersGuard); + mErrListeners.clear(); +} + +Return> CanBus::listenForErrors(const sp& listener) { + if (listener == nullptr) { + return new CloseHandle(); + } + + std::lock_guard upLck(mIsUpGuard); + if (!mIsUp) { + listener->onError(ErrorEvent::INTERFACE_DOWN, true); + return new CloseHandle(); + } + + std::lock_guard errLck(mErrListenersGuard); + mErrListeners.emplace_back(listener); + + return new CloseHandle([this, listener]() { + std::lock_guard 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 lck(mListenersGuard); - for (auto& listener : mListeners) { + std::lock_guard 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 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 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 diff --git a/automotive/can/1.0/default/CanBus.h b/automotive/can/1.0/default/CanBus.h index 81a83c82b8..30a2924942 100644 --- a/automotive/can/1.0/default/CanBus.h +++ b/automotive/can/1.0/default/CanBus.h @@ -34,12 +34,16 @@ namespace V1_0 { namespace implementation { struct CanBus : public ICanBus { + using ErrorCallback = std::function; + virtual ~CanBus(); Return send(const CanMessage& message) override; Return listen(const hidl_vec& filter, const sp& listener, listen_cb _hidl_cb) override; + Return> listenForErrors(const sp& listener) override; + void setErrorCallback(ErrorCallback errcb); ICanController::Result up(); bool down(); @@ -68,17 +72,22 @@ struct CanBus : public ICanBus { sp callback; hidl_vec filter; wp 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 mListeners GUARDED_BY(mListenersGuard); + std::mutex mMsgListenersGuard; + std::vector mMsgListeners GUARDED_BY(mMsgListenersGuard); + + std::mutex mErrListenersGuard; + std::vector> mErrListeners GUARDED_BY(mErrListenersGuard); std::unique_ptr 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 diff --git a/automotive/can/1.0/default/CanController.cpp b/automotive/can/1.0/default/CanController.cpp index 20adbe120e..3b63fe404b 100644 --- a/automotive/can/1.0/default/CanController.cpp +++ b/automotive/can/1.0/default/CanController.cpp @@ -81,6 +81,8 @@ Return 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 CanController::upInterface( return ICanController::Result::OK; } +static bool unregisterCanBusService(const hidl_string& name, sp 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 CanController::downInterface(const hidl_string& name) { LOG(VERBOSE) << "Attempting to bring interface down: " << name; @@ -110,8 +120,7 @@ Return 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; diff --git a/automotive/can/1.0/default/CanSocket.cpp b/automotive/can/1.0/default/CanSocket.cpp index 4d86de6a28..ecf4044db8 100644 --- a/automotive/can/1.0/default/CanSocket.cpp +++ b/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"; } diff --git a/automotive/can/1.0/default/CanSocket.h b/automotive/can/1.0/default/CanSocket.h index cd7162db75..284e1ea25e 100644 --- a/automotive/can/1.0/default/CanSocket.h +++ b/automotive/can/1.0/default/CanSocket.h @@ -36,7 +36,7 @@ namespace implementation { */ struct CanSocket { using ReadCallback = std::function; - using ErrorCallback = std::function; + using ErrorCallback = std::function; /** * Open and bind SocketCAN socket. @@ -68,6 +68,7 @@ struct CanSocket { const base::unique_fd mSocket; std::thread mReaderThread; std::atomic mStopReaderThread = false; + std::atomic mReaderThreadFinished = false; DISALLOW_COPY_AND_ASSIGN(CanSocket); }; diff --git a/automotive/can/1.0/default/CloseHandle.cpp b/automotive/can/1.0/default/CloseHandle.cpp index 13693d29e6..aba2c49a48 100644 --- a/automotive/can/1.0/default/CloseHandle.cpp +++ b/automotive/can/1.0/default/CloseHandle.cpp @@ -33,7 +33,7 @@ Return CloseHandle::close() { const auto wasClosed = mIsClosed.exchange(true); if (wasClosed) return {}; - mCallback(); + if (mCallback != nullptr) mCallback(); return {}; } diff --git a/automotive/can/1.0/default/CloseHandle.h b/automotive/can/1.0/default/CloseHandle.h index 94972e03b5..5191739e07 100644 --- a/automotive/can/1.0/default/CloseHandle.h +++ b/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 close() override; private: - Callback mCallback; + const Callback mCallback; std::atomic mIsClosed = false; DISALLOW_COPY_AND_ASSIGN(CloseHandle); diff --git a/automotive/can/1.0/hidl-utils/Android.bp b/automotive/can/1.0/hidl-utils/Android.bp index bc8ff136e5..63b07c9391 100644 --- a/automotive/can/1.0/hidl-utils/Android.bp +++ b/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, } diff --git a/automotive/can/1.0/types.hal b/automotive/can/1.0/types.hal index 37877c6023..6f690f7851 100644 --- a/automotive/can/1.0/types.hal +++ b/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, diff --git a/automotive/can/1.0/vts/functional/VtsHalCanBusV1_0TargetTest.cpp b/automotive/can/1.0/vts/functional/VtsHalCanBusV1_0TargetTest.cpp index 215328e878..1a05716fd0 100644 --- a/automotive/can/1.0/vts/functional/VtsHalCanBusV1_0TargetTest.cpp +++ b/automotive/can/1.0/vts/functional/VtsHalCanBusV1_0TargetTest.cpp @@ -36,8 +36,11 @@ using hardware::hidl_vec; static utils::SimpleHidlEnvironment* gEnv = nullptr; struct CanMessageListener : public can::V1_0::ICanMessageListener { - virtual Return onReceive(const can::V1_0::CanMessage&) { return {}; } - virtual Return onError(can::V1_0::ErrorEvent) { return {}; } + virtual Return onReceive(const can::V1_0::CanMessage&) override { return {}; } +}; + +struct CanErrorListener : public can::V1_0::ICanErrorListener { + virtual Return onError(ErrorEvent, bool) override { return {}; } }; class CanBusHalTest : public ::testing::VtsHalHidlTargetTestBase { @@ -47,6 +50,7 @@ class CanBusHalTest : public ::testing::VtsHalHidlTargetTestBase { std::tuple> listen(const hidl_vec& filter, const sp& listener); + sp listenForErrors(const sp& listener); sp mCanBus; }; @@ -70,6 +74,12 @@ std::tuple> CanBusHalTest::listen( return {halResult, closeHandle}; } +sp CanBusHalTest::listenForErrors(const sp& 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 diff --git a/automotive/can/1.0/vts/functional/VtsHalCanBusVirtualV1_0TargetTest.cpp b/automotive/can/1.0/vts/functional/VtsHalCanBusVirtualV1_0TargetTest.cpp index 699c1387a3..ba29c29e9c 100644 --- a/automotive/can/1.0/vts/functional/VtsHalCanBusVirtualV1_0TargetTest.cpp +++ b/automotive/can/1.0/vts/functional/VtsHalCanBusVirtualV1_0TargetTest.cpp @@ -50,18 +50,13 @@ struct CanMessageListener : public can::V1_0::ICanMessageListener { CanMessageListener() {} - virtual Return onReceive(const can::V1_0::CanMessage& msg) { + virtual Return onReceive(const can::V1_0::CanMessage& msg) override { std::unique_lock lk(mMessagesGuard); mMessages.push_back(msg); mMessagesUpdated.notify_one(); return {}; } - virtual Return onError(can::V1_0::ErrorEvent event) { - EXPECT_TRUE(false) << "Got error: " << event; - return {}; - } - virtual ~CanMessageListener() { mCloseHandle->close(); } void assignCloseHandle(sp closeHandle) {