From f3da9b6c1b5ca2f4dfe1e49d3d122e065a9bceba Mon Sep 17 00:00:00 2001 From: Tomasz Wasilczyk Date: Fri, 14 Feb 2020 10:54:28 -0800 Subject: [PATCH 1/2] Simplify bus configuration. Previous bus configuration struct was meant for flexibility, but it turned out that the only dimension that flexibility would go was a serial number parameter. Let's rotate that configuration matrix by 90 degrees and just go a straightforward route of discriminating against interface type. Test: VTS Bug: 135918744 Change-Id: I08967d0f78c998b0582958eb51bd387f9dbe15fe --- automotive/can/1.0/ICanController.hal | 149 +++++++++--------- automotive/can/1.0/default/CanBus.cpp | 2 +- automotive/can/1.0/default/CanBusNative.cpp | 2 +- automotive/can/1.0/default/CanBusSlcan.cpp | 2 +- automotive/can/1.0/default/CanController.cpp | 35 ++-- automotive/can/1.0/default/CanController.h | 3 +- automotive/can/1.0/tools/canhalctrl.cpp | 26 ++- .../VtsHalCanBusVirtualV1_0TargetTest.cpp | 8 +- .../VtsHalCanControllerV1_0TargetTest.cpp | 140 ++++++++++------ .../include/can-vts-utils/can-hal-printers.h | 3 +- 10 files changed, 214 insertions(+), 156 deletions(-) diff --git a/automotive/can/1.0/ICanController.hal b/automotive/can/1.0/ICanController.hal index 0c6f53eff3..e32e593655 100644 --- a/automotive/can/1.0/ICanController.hal +++ b/automotive/can/1.0/ICanController.hal @@ -27,51 +27,22 @@ package android.hardware.automotive.can@1.0; */ interface ICanController { /** - * Type of an interface, a mean to express the domain of device address. + * Type of an interface, an equivalent to BusConfig::InterfaceId + * union discriminator. Defines a number of specific standard hardware + * families and a generic catch-all type of {@see INDEXED}. */ enum InterfaceType : uint8_t { - /** - * Virtual SocketCAN interface. - * - * The address is an interface name, such as vcan0. If the interface - * doesn't exist, HAL server must create it. - * - * Valid InterfaceIdentifier types: - * - address. - */ + /** Virtual SocketCAN interface. */ VIRTUAL, - /** - * Native SocketCAN interface. - * - * The address is an interface name, such as can0. - * - * Valid InterfaceIdentifier types: - * - address; - * - serialno. - */ + /** Native SocketCAN interface. */ SOCKETCAN, - /** - * Serial-based interface. - * - * The address is a patch to a device, such as /dev/ttyUSB0. - * - * Valid InterfaceIdentifier types: - * - address; - * - serialno. - */ + /** Serial line CAN interface. */ SLCAN, - /** - * Proprietary interface, specific to the hardware system Android - * is running on. Instead of using address field, the interface is - * addressed with 0-based index. - * - * Valid InterfaceIdentifier types: - * - index - */ - INDEXED + /** Proprietary, device-specific interface. */ + INDEXED, }; enum Result : uint8_t { @@ -92,10 +63,10 @@ interface ICanController { NOT_SUPPORTED, /** - * Provided address (interface name, device path) doesn't exist or there - * is no device with a given serial no. + * Provided interface ID (index, name, device path) doesn't exist or + * there is no device with a given serial number. */ - BAD_ADDRESS, + BAD_INTERFACE_ID, /** Provided bit rate is not supported by the hardware. */ BAD_BITRATE, @@ -106,49 +77,76 @@ interface ICanController { * * ISO TP and CAN FD are currently not supported. */ - struct BusConfiguration { + struct BusConfig { /** * Name under which ICanBus HIDL service should be published. * * It must consist of only alphanumeric characters and underscore * (a-z, A-Z, 0-9, '_'), at least 1 and at most 32 characters long. + * + * This field is *not* meant to distinguish between hardware interfaces + * nor preselect parameters like bitrate. The only intended side-effect + * of changing it should be a different ICanBus HIDL service name and + * the HIDL service should make no assumptions on its contents. */ string name; /** - * Type of the hardware (or virtual) CAN interface. + * Hardware interface configuration. + * + * This union's discriminator has an equivalent enum + * {@see InterfaceType} to express compatibility via + * getSupportedInterfaceTypes(). */ - InterfaceType iftype; + safe_union InterfaceId { + /** Virtual SocketCAN interface. */ + struct Virtual { + /** Interface name, such as vcan0. If the interface doesn't + * exist, HAL server must create it. + */ + string ifname; + } virtualif; - /** - * Identification of hardware interface to configure. - */ - safe_union InterfaceIdentifier { - /** - * Interface name or other mean of identification of the specific - * interface port. Syntax depends on {@see iftype}, for details - * {@see InterfaceType}. - */ - string address; + /** Native SocketCAN interface. */ + safe_union Socketcan { + /** Interface name, such as can0. */ + string ifname; + /** + * Alternatively to providing {@see ifname}, one may provide a + * list of interface serial number suffixes. If there happens to + * be a device (like USB2CAN) with a matching serial number + * suffix, the HAL service will have to select it. + * + * Client may utilize this in two ways: by matching against the + * entire serial number, or the last few characters (usually + * one). The former is better for small-scale test deployments + * (with just a handful of vehicles), the latter is good for + * larger scale (where a small suffix list may support large + * test fleet). + */ + vec serialno; + } socketcan; + + /** Serial line CAN interface. */ + safe_union Slcan { + /** Path to a device, such as /dev/ttyUSB0. */ + string ttyname; + /** + * List of interface serial number suffixes. + * {@see Socketcan::serialno} + */ + vec serialno; + } slcan; /** - * Numerical identifier of interface, used for InterfaceType#INDEXED. - */ - uint8_t index; - - /** - * Alternatively to providing {@see address}, one may provide a list - * of interface serial number suffixes. If there happens to be - * a device (like USB2CAN) with a matching serial number suffix, - * it gets selected. + * Proprietary, device-specific interface. * - * Client may utilize this in two ways: by matching against the - * entire serial number, or the last few characters (usually one). - * The former is better for small-scale test deployments (with just - * a handful of vehicles), the latter is good for larger scale - * (where a small suffix list may support large test fleet). + * Non-SocketCAN interfaces should use this variant. */ - vec serialno; + struct Indexed { + /** Interface number, 0-based. */ + uint8_t index; + } indexed; } interfaceId; /** @@ -156,7 +154,8 @@ interface ICanController { * * Typical bit rates are: 100000, 125000, 250000, 500000. * - * For virtual interfaces this value is ignored. + * For {@see interfaceId#virtual} and pre-configured + * {@see interfaceId#indexed} interfaces this value is ignored. */ uint32_t bitrate; }; @@ -164,17 +163,17 @@ interface ICanController { /** * Fetches the list of interface types supported by this HAL server. * - * @return iftypes The list of supported interface types + * @return iftypes The list of supported interface types. */ getSupportedInterfaceTypes() generates (vec iftypes); /** * Bring up the CAN interface and publish ICanBus server instance. * - * @param config Configuration of the CAN interface + * @param config Configuration of the CAN interface. * @return result OK if the operation succeeded; error code otherwise. */ - upInterface(BusConfiguration config) generates (Result result); + upInterface(BusConfig config) generates (Result result); /** * Unpublish ICanBus server instance and bring down the CAN interface. @@ -182,9 +181,9 @@ interface ICanController { * In case of failure, at least the ICanBus server instance must be * unpublished and resources freed on best-effort basis. * - * @param name Name of the interface (@see BusConfiguration#name} to - * bring down - * @return success true in case of success, false otherwise + * @param name Name of the interface (@see BusConfig#name} to + * bring down. + * @return success true in case of success, false otherwise. */ downInterface(string name) generates (bool success); }; diff --git a/automotive/can/1.0/default/CanBus.cpp b/automotive/can/1.0/default/CanBus.cpp index 8fb09eb10c..9f704c1ebd 100644 --- a/automotive/can/1.0/default/CanBus.cpp +++ b/automotive/can/1.0/default/CanBus.cpp @@ -124,7 +124,7 @@ ICanController::Result CanBus::up() { if (!isUp.has_value()) { // preUp() should prepare the interface (either create or make sure it's there) LOG(ERROR) << "Interface " << mIfname << " didn't get prepared"; - return ICanController::Result::BAD_ADDRESS; + return ICanController::Result::BAD_INTERFACE_ID; } if (!*isUp && !netdevice::up(mIfname)) { diff --git a/automotive/can/1.0/default/CanBusNative.cpp b/automotive/can/1.0/default/CanBusNative.cpp index ef04d01832..aafbeccdb8 100644 --- a/automotive/can/1.0/default/CanBusNative.cpp +++ b/automotive/can/1.0/default/CanBusNative.cpp @@ -28,7 +28,7 @@ CanBusNative::CanBusNative(const std::string& ifname, uint32_t bitrate) ICanController::Result CanBusNative::preUp() { if (!netdevice::exists(mIfname)) { LOG(ERROR) << "Interface " << mIfname << " doesn't exist"; - return ICanController::Result::BAD_ADDRESS; + return ICanController::Result::BAD_INTERFACE_ID; } if (mBitrate == 0) { diff --git a/automotive/can/1.0/default/CanBusSlcan.cpp b/automotive/can/1.0/default/CanBusSlcan.cpp index 0feee8f51a..d15905da53 100644 --- a/automotive/can/1.0/default/CanBusSlcan.cpp +++ b/automotive/can/1.0/default/CanBusSlcan.cpp @@ -81,7 +81,7 @@ ICanController::Result CanBusSlcan::preUp() { mFd = base::unique_fd(open(mUartName.c_str(), O_RDWR | O_NONBLOCK | O_NOCTTY)); if (!mFd.ok()) { LOG(ERROR) << "SLCAN Failed to open " << mUartName << ": " << strerror(errno); - return ICanController::Result::BAD_ADDRESS; + return ICanController::Result::BAD_INTERFACE_ID; } // If the device is already up, update the iface name in our CanBusSlcan object diff --git a/automotive/can/1.0/default/CanController.cpp b/automotive/can/1.0/default/CanController.cpp index fb648c1d80..dd8040233e 100644 --- a/automotive/can/1.0/default/CanController.cpp +++ b/automotive/can/1.0/default/CanController.cpp @@ -27,7 +27,8 @@ namespace android::hardware::automotive::can::V1_0::implementation { -using IfaceIdDisc = ICanController::BusConfiguration::InterfaceIdentifier::hidl_discriminator; +using IfId = ICanController::BusConfig::InterfaceId; +using IfIdDisc = ICanController::BusConfig::InterfaceId::hidl_discriminator; Return CanController::getSupportedInterfaceTypes(getSupportedInterfaceTypes_cb _hidl_cb) { _hidl_cb({ICanController::InterfaceType::VIRTUAL, ICanController::InterfaceType::SOCKETCAN, @@ -40,8 +41,7 @@ static bool isValidName(const std::string& name) { return std::regex_match(name, nameRE); } -Return CanController::upInterface( - const ICanController::BusConfiguration& config) { +Return CanController::upInterface(const ICanController::BusConfig& config) { LOG(VERBOSE) << "Attempting to bring interface up: " << toString(config); std::lock_guard lck(mCanBusesGuard); @@ -58,24 +58,23 @@ Return CanController::upInterface( sp busService; - if (config.iftype == ICanController::InterfaceType::SOCKETCAN) { - // TODO(b/135918744): support serialno - if (config.interfaceId.getDiscriminator() == IfaceIdDisc::address) { - busService = new CanBusNative(config.interfaceId.address(), config.bitrate); + if (config.interfaceId.getDiscriminator() == IfIdDisc::socketcan) { + // TODO(b/142654031): support serialno + auto& socketcan = config.interfaceId.socketcan(); + if (socketcan.getDiscriminator() == IfId::Socketcan::hidl_discriminator::ifname) { + busService = new CanBusNative(socketcan.ifname(), config.bitrate); } else { - return ICanController::Result::BAD_ADDRESS; + return ICanController::Result::BAD_INTERFACE_ID; } - } else if (config.iftype == ICanController::InterfaceType::VIRTUAL) { - if (config.interfaceId.getDiscriminator() == IfaceIdDisc::address) { - busService = new CanBusVirtual(config.interfaceId.address()); + } else if (config.interfaceId.getDiscriminator() == IfIdDisc::virtualif) { + busService = new CanBusVirtual(config.interfaceId.virtualif().ifname); + } else if (config.interfaceId.getDiscriminator() == IfIdDisc::slcan) { + // TODO(b/142654031): support serialno + auto& slcan = config.interfaceId.slcan(); + if (slcan.getDiscriminator() == IfId::Slcan::hidl_discriminator::ttyname) { + busService = new CanBusSlcan(slcan.ttyname(), config.bitrate); } else { - return ICanController::Result::BAD_ADDRESS; - } - } else if (config.iftype == ICanController::InterfaceType::SLCAN) { - if (config.interfaceId.getDiscriminator() == IfaceIdDisc::address) { - busService = new CanBusSlcan(config.interfaceId.address(), config.bitrate); - } else { - return ICanController::Result::BAD_ADDRESS; + return ICanController::Result::BAD_INTERFACE_ID; } } else { return ICanController::Result::NOT_SUPPORTED; diff --git a/automotive/can/1.0/default/CanController.h b/automotive/can/1.0/default/CanController.h index 99a551af77..27e82f3f3f 100644 --- a/automotive/can/1.0/default/CanController.h +++ b/automotive/can/1.0/default/CanController.h @@ -25,8 +25,7 @@ namespace android::hardware::automotive::can::V1_0::implementation { struct CanController : public ICanController { Return getSupportedInterfaceTypes(getSupportedInterfaceTypes_cb _hidl_cb) override; - Return upInterface( - const ICanController::BusConfiguration& config) override; + Return upInterface(const ICanController::BusConfig& config) override; Return downInterface(const hidl_string& name) override; private: diff --git a/automotive/can/1.0/tools/canhalctrl.cpp b/automotive/can/1.0/tools/canhalctrl.cpp index 5494ba31a3..33755bfffd 100644 --- a/automotive/can/1.0/tools/canhalctrl.cpp +++ b/automotive/can/1.0/tools/canhalctrl.cpp @@ -71,15 +71,31 @@ static int up(const std::string& busName, ICanController::InterfaceType type, if (!isSupported(ctrl, type)) continue; anySupported = true; - ICanController::BusConfiguration config = {}; + ICanController::BusConfig config = {}; config.name = busName; - config.iftype = type; config.bitrate = bitrate; - if (type == ICanController::InterfaceType::INDEXED) { - config.interfaceId.index(std::stol(interface)); + // TODO(b/146214370): move interfaceId constructors to a library + using IfCfg = ICanController::BusConfig::InterfaceId; + if (type == ICanController::InterfaceType::VIRTUAL) { + config.interfaceId.virtualif({interface}); + } else if (type == ICanController::InterfaceType::SOCKETCAN) { + IfCfg::Socketcan socketcan = {}; + socketcan.ifname(interface); + config.interfaceId.socketcan(socketcan); + } else if (type == ICanController::InterfaceType::SLCAN) { + IfCfg::Slcan slcan = {}; + slcan.ttyname(interface); + config.interfaceId.slcan(slcan); + } else if (type == ICanController::InterfaceType::INDEXED) { + auto idx = std::stol(interface); + if (idx < 0 || idx > UINT8_MAX) { + std::cerr << "Interface index out of range: " << idx; + return -1; + } + config.interfaceId.indexed({uint8_t(idx)}); } else { - config.interfaceId.address(interface); + CHECK(false) << "Unexpected interface type: " << toString(type); } const auto upresult = ctrl->upInterface(config); diff --git a/automotive/can/1.0/vts/functional/VtsHalCanBusVirtualV1_0TargetTest.cpp b/automotive/can/1.0/vts/functional/VtsHalCanBusVirtualV1_0TargetTest.cpp index efaad53a78..68d555dd20 100644 --- a/automotive/can/1.0/vts/functional/VtsHalCanBusVirtualV1_0TargetTest.cpp +++ b/automotive/can/1.0/vts/functional/VtsHalCanBusVirtualV1_0TargetTest.cpp @@ -81,7 +81,7 @@ struct CanMessageListener : public can::V1_0::ICanMessageListener { struct Bus { DISALLOW_COPY_AND_ASSIGN(Bus); - Bus(sp controller, const ICanController::BusConfiguration& config) + Bus(sp controller, const ICanController::BusConfig& config) : mIfname(config.name), mController(controller) { const auto result = controller->upInterface(config); EXPECT_EQ(ICanController::Result::OK, result); @@ -122,6 +122,7 @@ struct Bus { void send(const CanMessage& msg) { EXPECT_NE(mBus, nullptr); + if (!mBus) return; const auto result = mBus->send(msg); EXPECT_EQ(Result::OK, result); } @@ -196,10 +197,9 @@ Bus CanBusVirtualHalTest::makeBus() { const auto idx = mLastIface++; EXPECT_LT(idx, mBusNames.size()); - ICanController::BusConfiguration config = {}; + ICanController::BusConfig config = {}; config.name = mBusNames[idx]; - config.iftype = InterfaceType::VIRTUAL; - config.interfaceId.address("vcan50"); + config.interfaceId.virtualif({"vcan50"}); return Bus(mCanController, config); } diff --git a/automotive/can/1.0/vts/functional/VtsHalCanControllerV1_0TargetTest.cpp b/automotive/can/1.0/vts/functional/VtsHalCanControllerV1_0TargetTest.cpp index b2edd78391..0c5d976053 100644 --- a/automotive/can/1.0/vts/functional/VtsHalCanControllerV1_0TargetTest.cpp +++ b/automotive/can/1.0/vts/functional/VtsHalCanControllerV1_0TargetTest.cpp @@ -31,6 +31,7 @@ namespace android::hardware::automotive::can::V1_0::vts { using hardware::hidl_vec; using InterfaceType = ICanController::InterfaceType; +using IfId = ICanController::BusConfig::InterfaceId; static utils::SimpleHidlEnvironment* gEnv = nullptr; @@ -89,10 +90,23 @@ bool CanControllerHalTest::isSupported(InterfaceType iftype) { bool CanControllerHalTest::up(InterfaceType iftype, std::string srvname, std::string ifname, ICanController::Result expected) { - ICanController::BusConfiguration config = {}; + ICanController::BusConfig config = {}; config.name = srvname; - config.iftype = iftype; - config.interfaceId.address(ifname); + + // TODO(b/146214370): move interfaceId constructors to a library + if (iftype == InterfaceType::SOCKETCAN) { + IfId::Socketcan socketcan = {}; + socketcan.ifname(ifname); + config.interfaceId.socketcan(socketcan); + } else if (iftype == InterfaceType::SLCAN) { + IfId::Slcan slcan = {}; + slcan.ttyname(ifname); + config.interfaceId.slcan(slcan); + } else if (iftype == InterfaceType::VIRTUAL) { + config.interfaceId.virtualif({ifname}); + } else { + EXPECT_TRUE(false) << "Unexpected iftype: " << toString(iftype); + } const auto upresult = mCanController->upInterface(config); @@ -155,54 +169,64 @@ TEST_F(CanControllerHalTest, UpTwice) { assertRegistered(name, false); } -TEST_F(CanControllerHalTest, IdentifierCompatibility) { - using IdDisc = ICanController::BusConfiguration::InterfaceIdentifier::hidl_discriminator; - static const std::map> compatMatrix = { - {InterfaceType::VIRTUAL, {IdDisc::address}}, - {InterfaceType::SOCKETCAN, {IdDisc::address, IdDisc::serialno}}, - {InterfaceType::SLCAN, {IdDisc::address, IdDisc::serialno}}, - {InterfaceType::INDEXED, {IdDisc::index}}, +TEST_F(CanControllerHalTest, ConfigCompatibility) { + // using random-ish addresses, which may not be valid - we can't test the success case + // TODO(b/146214370): move interfaceId constructors to a library + IfId virtualCfg = {}; + virtualCfg.virtualif({"vcan70"}); + + IfId::Socketcan socketcanIfname = {}; + socketcanIfname.ifname("can0"); + IfId socketcanIfnameCfg = {}; + socketcanIfnameCfg.socketcan(socketcanIfname); + + IfId::Socketcan socketcanSerial = {}; + socketcanSerial.serialno({"1234", "2345"}); + IfId socketcanSerialCfg = {}; + socketcanSerialCfg.socketcan(socketcanSerial); + + IfId::Slcan slcanTtyname = {}; + slcanTtyname.ttyname("/dev/ttyUSB0"); + IfId slcanTtynameCfg = {}; + slcanTtynameCfg.slcan(slcanTtyname); + + IfId::Slcan slcanSerial = {}; + slcanSerial.serialno({"dead", "beef"}); + IfId slcanSerialCfg = {}; + slcanSerialCfg.slcan(slcanSerial); + + IfId indexedCfg = {}; + indexedCfg.indexed({0}); + + static const std::vector> compatMatrix = { + {InterfaceType::VIRTUAL, virtualCfg}, + {InterfaceType::SOCKETCAN, socketcanIfnameCfg}, + {InterfaceType::SOCKETCAN, socketcanSerialCfg}, + {InterfaceType::SLCAN, slcanTtynameCfg}, + {InterfaceType::SLCAN, slcanSerialCfg}, + {InterfaceType::INDEXED, indexedCfg}, }; - static const std::vector allDisc = {IdDisc::address, IdDisc::index, IdDisc::serialno}; - for (const auto [iftype, supported] : compatMatrix) { - for (const auto iddisc : allDisc) { - LOG(INFO) << "Compatibility testing: " << iftype << " / " << iddisc; + for (const auto [iftype, cfg] : compatMatrix) { + LOG(INFO) << "Compatibility testing: " << iftype << " / " << cfg; - ICanController::BusConfiguration config = {}; - config.name = "compattestsrv"; - config.iftype = iftype; - config.bitrate = 125000; + ICanController::BusConfig config = {}; + config.name = "compattestsrv"; + config.bitrate = 125000; + config.interfaceId = cfg; - // using random-ish addresses, which may not be valid - we can't test the success case - if (iddisc == IdDisc::address) { - config.interfaceId.address("can0"); - } else if (iddisc == IdDisc::index) { - config.interfaceId.index(0); - } else if (iddisc == IdDisc::serialno) { - config.interfaceId.serialno({"dummy", "dummier"}); - } + const auto upresult = mCanController->upInterface(config); - const auto upresult = mCanController->upInterface(config); + if (!isSupported(iftype)) { + ASSERT_EQ(ICanController::Result::NOT_SUPPORTED, upresult); + continue; + } + ASSERT_NE(ICanController::Result::NOT_SUPPORTED, upresult); - if (!isSupported(iftype)) { - ASSERT_EQ(ICanController::Result::NOT_SUPPORTED, upresult); - continue; - } - ASSERT_NE(ICanController::Result::NOT_SUPPORTED, upresult); - - bool isSupportedDisc = - std::find(supported.begin(), supported.end(), iddisc) != supported.end(); - if (!isSupportedDisc) { - ASSERT_EQ(ICanController::Result::BAD_ADDRESS, upresult); - continue; - } - - if (upresult == ICanController::Result::OK) { - const auto dnresult = mCanController->downInterface(config.name); - ASSERT_TRUE(dnresult); - continue; - } + if (upresult == ICanController::Result::OK) { + const auto dnresult = mCanController->downInterface(config.name); + ASSERT_TRUE(dnresult); + continue; } } } @@ -232,7 +256,9 @@ TEST_F(CanControllerHalTest, FailBadVirtualAddress) { const std::string name = mBusNames[0]; assertRegistered(name, false); - if (!up(InterfaceType::VIRTUAL, name, "", ICanController::Result::BAD_ADDRESS)) GTEST_SKIP(); + if (!up(InterfaceType::VIRTUAL, name, "", ICanController::Result::BAD_INTERFACE_ID)) { + GTEST_SKIP(); + } assertRegistered(name, false); } @@ -240,10 +266,30 @@ TEST_F(CanControllerHalTest, FailBadSocketcanAddress) { const std::string name = mBusNames[0]; assertRegistered(name, false); - if (!up(InterfaceType::SOCKETCAN, name, "can87", ICanController::Result::BAD_ADDRESS)) { + if (!up(InterfaceType::SOCKETCAN, name, "can87", ICanController::Result::BAD_INTERFACE_ID)) { GTEST_SKIP(); } assertRegistered(name, false); + + auto supported = + up(InterfaceType::SOCKETCAN, name, "", ICanController::Result::BAD_INTERFACE_ID); + ASSERT_TRUE(supported); + assertRegistered(name, false); +} + +TEST_F(CanControllerHalTest, FailBadSlcanAddress) { + const std::string name = mBusNames[0]; + + assertRegistered(name, false); + if (!up(InterfaceType::SLCAN, name, "/dev/shouldnotexist123", + ICanController::Result::BAD_INTERFACE_ID)) { + GTEST_SKIP(); + } + assertRegistered(name, false); + + auto supported = up(InterfaceType::SLCAN, name, "", ICanController::Result::BAD_INTERFACE_ID); + ASSERT_TRUE(supported); + assertRegistered(name, false); } } // namespace android::hardware::automotive::can::V1_0::vts diff --git a/automotive/can/1.0/vts/utils/include/can-vts-utils/can-hal-printers.h b/automotive/can/1.0/vts/utils/include/can-vts-utils/can-hal-printers.h index 3c30744802..383b54cae5 100644 --- a/automotive/can/1.0/vts/utils/include/can-vts-utils/can-hal-printers.h +++ b/automotive/can/1.0/vts/utils/include/can-vts-utils/can-hal-printers.h @@ -35,8 +35,7 @@ namespace android::hardware::automotive::can::V1_0 { DEFINE_CAN_HAL_PRINTER(CanMessage, toString) DEFINE_CAN_HAL_PRINTER(ErrorEvent, toString) -DEFINE_CAN_HAL_PRINTER_SIMPLE( - ICanController::BusConfiguration::InterfaceIdentifier::hidl_discriminator, int) +DEFINE_CAN_HAL_PRINTER(ICanController::BusConfig::InterfaceId, toString); DEFINE_CAN_HAL_PRINTER(ICanController::InterfaceType, toString) DEFINE_CAN_HAL_PRINTER(ICanController::Result, toString) DEFINE_CAN_HAL_PRINTER(Result, toString) From 793fab0b07fdec2acc5117367aa3e0a8f66f5dca Mon Sep 17 00:00:00 2001 From: Tomasz Wasilczyk Date: Tue, 18 Feb 2020 15:19:35 -0800 Subject: [PATCH 2/2] Add ICanController BAD_SERVICE_NAME error Bug: 137798319 Test: VTS Change-Id: I4722346239728f3ab359688658c23441e83671a8 --- automotive/can/1.0/ICanController.hal | 6 ++++++ automotive/can/1.0/default/CanController.cpp | 4 ++-- .../vts/functional/VtsHalCanControllerV1_0TargetTest.cpp | 4 ++-- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/automotive/can/1.0/ICanController.hal b/automotive/can/1.0/ICanController.hal index e32e593655..aaf85e94c9 100644 --- a/automotive/can/1.0/ICanController.hal +++ b/automotive/can/1.0/ICanController.hal @@ -70,6 +70,12 @@ interface ICanController { /** Provided bit rate is not supported by the hardware. */ BAD_BITRATE, + + /** + * Provided service name ({@see BusConfig#name}) either has invalid + * format or is not listed in device manifest file. + */ + BAD_SERVICE_NAME, }; /** diff --git a/automotive/can/1.0/default/CanController.cpp b/automotive/can/1.0/default/CanController.cpp index dd8040233e..0700c77513 100644 --- a/automotive/can/1.0/default/CanController.cpp +++ b/automotive/can/1.0/default/CanController.cpp @@ -48,7 +48,7 @@ Return CanController::upInterface(const ICanController:: if (!isValidName(config.name)) { LOG(ERROR) << "Bus name " << config.name << " is invalid"; - return ICanController::Result::UNKNOWN_ERROR; + return ICanController::Result::BAD_SERVICE_NAME; } if (mCanBuses.find(config.name) != mCanBuses.end()) { @@ -90,7 +90,7 @@ Return CanController::upInterface(const ICanController:: if (!busService->down()) { LOG(WARNING) << "Failed to bring down CAN bus that failed to register"; } - return ICanController::Result::UNKNOWN_ERROR; + return ICanController::Result::BAD_SERVICE_NAME; } mCanBuses[config.name] = busService; diff --git a/automotive/can/1.0/vts/functional/VtsHalCanControllerV1_0TargetTest.cpp b/automotive/can/1.0/vts/functional/VtsHalCanControllerV1_0TargetTest.cpp index 0c5d976053..83972153c2 100644 --- a/automotive/can/1.0/vts/functional/VtsHalCanControllerV1_0TargetTest.cpp +++ b/automotive/can/1.0/vts/functional/VtsHalCanControllerV1_0TargetTest.cpp @@ -235,7 +235,7 @@ TEST_F(CanControllerHalTest, FailEmptyName) { const std::string name = ""; assertRegistered(name, false); - if (!up(InterfaceType::VIRTUAL, name, "vcan57", ICanController::Result::UNKNOWN_ERROR)) { + if (!up(InterfaceType::VIRTUAL, name, "vcan57", ICanController::Result::BAD_SERVICE_NAME)) { GTEST_SKIP(); } assertRegistered(name, false); @@ -246,7 +246,7 @@ TEST_F(CanControllerHalTest, FailBadName) { const std::string name = "ab012345678901234567890123456789c"; assertRegistered(name, false); - if (!up(InterfaceType::VIRTUAL, name, "vcan57", ICanController::Result::UNKNOWN_ERROR)) { + if (!up(InterfaceType::VIRTUAL, name, "vcan57", ICanController::Result::BAD_SERVICE_NAME)) { GTEST_SKIP(); } assertRegistered(name, false);