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
diff --git a/automotive/can/1.0/Android.bp b/automotive/can/1.0/Android.bp
index fe2a2be..2221e66 100644
--- a/automotive/can/1.0/Android.bp
+++ b/automotive/can/1.0/Android.bp
@@ -10,6 +10,7 @@
"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 6ed89f3..e68f16c 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 @@
*/
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);
};
diff --git a/automotive/can/1.0/ICanErrorListener.hal b/automotive/can/1.0/ICanErrorListener.hal
new file mode 100644
index 0000000..8a6ba05
--- /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 992d1c7..28161fa 100644
--- a/automotive/can/1.0/ICanMessageListener.hal
+++ b/automotive/can/1.0/ICanMessageListener.hal
@@ -30,11 +30,4 @@
* @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 65da291..38a9974 100644
--- a/automotive/can/1.0/default/CanBus.cpp
+++ b/automotive/can/1.0/default/CanBus.cpp
@@ -68,14 +68,14 @@
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 @@
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 @@
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 @@
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 @@
}
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 @@
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
diff --git a/automotive/can/1.0/default/CanBus.h b/automotive/can/1.0/default/CanBus.h
index 81a83c8..30a2924 100644
--- a/automotive/can/1.0/default/CanBus.h
+++ b/automotive/can/1.0/default/CanBus.h
@@ -34,12 +34,16 @@
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 @@
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 @@
*/
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 20adbe1..3b63fe4 100644
--- a/automotive/can/1.0/default/CanController.cpp
+++ b/automotive/can/1.0/default/CanController.cpp
@@ -81,6 +81,8 @@
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::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 @@
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 4d86de6..ecf4044 100644
--- a/automotive/can/1.0/default/CanSocket.cpp
+++ b/automotive/can/1.0/default/CanSocket.cpp
@@ -61,7 +61,14 @@
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 @@
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 @@
}
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 cd7162d..284e1ea 100644
--- a/automotive/can/1.0/default/CanSocket.h
+++ b/automotive/can/1.0/default/CanSocket.h
@@ -36,7 +36,7 @@
*/
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 @@
const base::unique_fd mSocket;
std::thread mReaderThread;
std::atomic<bool> mStopReaderThread = false;
+ std::atomic<bool> 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 13693d2..aba2c49 100644
--- a/automotive/can/1.0/default/CloseHandle.cpp
+++ b/automotive/can/1.0/default/CloseHandle.cpp
@@ -33,7 +33,7 @@
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 94972e0..5191739 100644
--- a/automotive/can/1.0/default/CloseHandle.h
+++ b/automotive/can/1.0/default/CloseHandle.h
@@ -39,13 +39,13 @@
*
* \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);
diff --git a/automotive/can/1.0/hidl-utils/Android.bp b/automotive/can/1.0/hidl-utils/Android.bp
index bc8ff13..63b07c9 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 37877c6..6f690f7 100644
--- a/automotive/can/1.0/types.hal
+++ b/automotive/can/1.0/types.hal
@@ -95,6 +95,9 @@
/** 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 215328e..1a05716 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 @@
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 @@
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 @@
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 @@
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 @@
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 699c138..ba29c29 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 @@
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) {