Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit a9061966 authored by Tomasz Wasilczyk's avatar Tomasz Wasilczyk
Browse files

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
parent 1cebd62c

automotive/can/1.0/Android.bp

+1 −0
Original line number Diff line number Diff line
@@ -10,6 +10,7 @@ hidl_interface { 
        "types.hal",

        "ICanBus.hal",

        "ICanController.hal",

        "ICanErrorListener.hal",

        "ICanMessageListener.hal",

        "ICloseHandle.hal",

    ],

automotive/can/1.0/ICanBus.hal

+12 −0
Original line number Diff line number Diff line
@@ -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 → 100644
+32 −0
Original line number Diff line number Diff line 
/*

 * 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

+0 −7
Original line number Diff line number Diff line
@@ -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

+75 −28
Original line number Diff line number Diff line
@@ -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,24 +257,37 @@ 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()) {

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

}  // namespace V1_0

}  // namespace can