Merge changes from topic "canhal"

* changes:
  Add ICanController BAD_SERVICE_NAME error
  Simplify bus configuration.
This commit is contained in:
Tomasz Wasilczyk 2020-02-20 17:10:29 +00:00 committed by Android (Google) Code Review
commit 4763857c6e
10 changed files with 224 additions and 160 deletions

View file

@ -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,13 +63,19 @@ 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,
/**
* Provided service name ({@see BusConfig#name}) either has invalid
* format or is not listed in device manifest file.
*/
BAD_SERVICE_NAME,
};
/**
@ -106,49 +83,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<string> 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<string> 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<string> serialno;
struct Indexed {
/** Interface number, 0-based. */
uint8_t index;
} indexed;
} interfaceId;
/**
@ -156,7 +160,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 +169,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<InterfaceType> 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 +187,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);
};

View file

@ -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)) {

View file

@ -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) {

View file

@ -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

View file

@ -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<void> CanController::getSupportedInterfaceTypes(getSupportedInterfaceTypes_cb _hidl_cb) {
_hidl_cb({ICanController::InterfaceType::VIRTUAL, ICanController::InterfaceType::SOCKETCAN,
@ -40,15 +41,14 @@ static bool isValidName(const std::string& name) {
return std::regex_match(name, nameRE);
}
Return<ICanController::Result> CanController::upInterface(
const ICanController::BusConfiguration& config) {
Return<ICanController::Result> CanController::upInterface(const ICanController::BusConfig& config) {
LOG(VERBOSE) << "Attempting to bring interface up: " << toString(config);
std::lock_guard<std::mutex> lck(mCanBusesGuard);
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()) {
@ -58,24 +58,23 @@ Return<ICanController::Result> CanController::upInterface(
sp<CanBus> 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;
@ -91,7 +90,7 @@ Return<ICanController::Result> CanController::upInterface(
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;

View file

@ -25,8 +25,7 @@ namespace android::hardware::automotive::can::V1_0::implementation {
struct CanController : public ICanController {
Return<void> getSupportedInterfaceTypes(getSupportedInterfaceTypes_cb _hidl_cb) override;
Return<ICanController::Result> upInterface(
const ICanController::BusConfiguration& config) override;
Return<ICanController::Result> upInterface(const ICanController::BusConfig& config) override;
Return<bool> downInterface(const hidl_string& name) override;
private:

View file

@ -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);

View file

@ -81,7 +81,7 @@ struct CanMessageListener : public can::V1_0::ICanMessageListener {
struct Bus {
DISALLOW_COPY_AND_ASSIGN(Bus);
Bus(sp<ICanController> controller, const ICanController::BusConfiguration& config)
Bus(sp<ICanController> 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);
}

View file

@ -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<ICanController>* 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<InterfaceType, std::vector<IdDisc>> 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<std::pair<InterfaceType, IfId>> compatMatrix = {
{InterfaceType::VIRTUAL, virtualCfg},
{InterfaceType::SOCKETCAN, socketcanIfnameCfg},
{InterfaceType::SOCKETCAN, socketcanSerialCfg},
{InterfaceType::SLCAN, slcanTtynameCfg},
{InterfaceType::SLCAN, slcanSerialCfg},
{InterfaceType::INDEXED, indexedCfg},
};
static const std::vector<IdDisc> 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;
}
}
}
@ -211,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);
@ -222,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);
@ -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

View file

@ -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)