Split 'IFoo' into 'IFoo' and 'IHwFoo'
The traditional binder "IFoo" interface is coupled
very tightly with Binder; it inherits from Binder
objects and implements several Binder-only methods.
We want to abstract away the RPC mechanism as much as we can; therefore,
we want to create a "clean" IFoo interface, which contains just the
methods defined in the HIDL interface. Separately, there's an IHwFoo
interface, which implements IFoo as well as the necessary Binder
methods.
Changes in hidl-gen to support this:
- Generates IHwFoo.h
- Moved (DECLARE|IMPLEMENT)_HW_BINDER macros from IFoo to IHwFoo
- Added REGISTER_AND_GET_SERVICE macros to IFoo
- Removed all hwbinder/ include paths from IFoo
- When passing an interface through a HIDL method, wrap a
BnInterface object around it
- BnFoo's implementation of IFoo calls through the register
interface implementation
- Updated test code
Tests: hidl_test, hidl_java_test, libhwbinder bench, NFC all work.
Bug: 30588200
Change-Id: Ie7ca4eef905f84aebd06bee971b5f6170e169797
diff --git a/AST.h b/AST.h
index f77a260..4de19dc 100644
--- a/AST.h
+++ b/AST.h
@@ -124,6 +124,7 @@
void enterLeaveNamespace(Formatter &out, bool enter) const;
status_t generateInterfaceHeader(const std::string &outputPath) const;
+ status_t generateHwBinderHeader(const std::string &outputPath) const;
status_t generateStubHeader(const std::string &outputPath) const;
status_t generateProxyHeader(const std::string &outputPath) const;
status_t generateAllSource(const std::string &outputPath) const;
diff --git a/Interface.cpp b/Interface.cpp
index f8647a5..fc7e48d 100644
--- a/Interface.cpp
+++ b/Interface.cpp
@@ -56,6 +56,11 @@
return *mAnnotationsByName;
}
+std::string Interface::getBaseName() const {
+ // cut off the leading 'I'.
+ return localName().substr(1);
+}
+
std::string Interface::getCppType(StorageMode mode, std::string *extra) const {
extra->clear();
const std::string base = "::android::sp<" + fullName() + ">";
@@ -103,7 +108,9 @@
out << name
<< " = "
- << fullName()
+ << fqName().cppNamespace()
+ << "::IHw"
+ << getBaseName()
<< "::asInterface("
<< binderName
<< ");\n";
@@ -111,14 +118,33 @@
out.unindent();
out << "}\n\n";
} else {
+
+ out << "if (" << name << "->isRemote()) {\n";
+ out.indent();
out << "_hidl_err = ";
out << parcelObjDeref
<< "writeStrongBinder("
- << fullName()
- << "::asBinder("
- << name
- << "));\n";
-
+ << fqName().cppNamespace()
+ << "::IHw"
+ << getBaseName()
+ << "::asBinder(static_cast<"
+ << fqName().cppNamespace()
+ << "::IHw"
+ << getBaseName()
+ << "*>("
+ << name << ".get()"
+ << ")));\n";
+ out.unindent();
+ out << "} else {\n";
+ out.indent();
+ out << "_hidl_err = ";
+ out << parcelObjDeref
+ << "writeStrongBinder("
+ << "new " << fqName().cppNamespace()
+ << "::Bn" << getBaseName() << " "
+ << "(" << name <<"));\n";
+ out.unindent();
+ out << "}\n";
handleError(out, mode);
}
}
diff --git a/Interface.h b/Interface.h
index 023742c..7d57218 100644
--- a/Interface.h
+++ b/Interface.h
@@ -46,6 +46,8 @@
const AnnotationVector &annotations() const;
+ std::string getBaseName() const;
+
std::string getCppType(StorageMode mode, std::string *extra) const override;
std::string getJavaType() const override;
diff --git a/generateCpp.cpp b/generateCpp.cpp
index e472245..57ed349 100644
--- a/generateCpp.cpp
+++ b/generateCpp.cpp
@@ -49,6 +49,10 @@
}
if (err == OK) {
+ err = generateHwBinderHeader(outputPath);
+ }
+
+ if (err == OK) {
err = generateProxyHeader(outputPath);
}
@@ -158,10 +162,9 @@
}
out << "#include <hidl/HidlSupport.h>\n";
+ out << "#include <hidl/IServiceManager.h>\n";
if (isInterface) {
- out << "#include <hwbinder/IBinder.h>\n";
- out << "#include <hwbinder/IInterface.h>\n";
out << "#include <hwbinder/Status.h>\n";
}
@@ -170,28 +173,27 @@
enterLeaveNamespace(out, true /* enter */);
out << "\n";
+ // cut off the leading 'I'.
+ const std::string baseName = ifaceName.substr(1);
+
if (isInterface) {
out << "struct "
- << ifaceName
- << " : public ";
+ << ifaceName;
const Interface *iface = mRootScope->getInterface();
const Interface *superType = iface->superType();
- if (superType != NULL) {
- out << superType->fullName();
+ if (superType == NULL) {
+ out << " : virtual public RefBase";
} else {
- out << "::android::hardware::IInterface";
+ out << " : public "
+ << superType->fullName();
}
out << " {\n";
out.indent();
- // cut off the leading 'I'.
- const std::string baseName = ifaceName.substr(1);
-
- out << "DECLARE_HWBINDER_META_INTERFACE(" << baseName << ");\n\n";
}
status_t err = emitTypeDeclarations(out);
@@ -204,33 +206,7 @@
const Interface *iface = mRootScope->getInterface();
const Interface *superType = iface->superType();
- out << "enum Call {\n";
- out.indent();
-
- bool first = true;
- for (const auto &method : iface->methods()) {
- out << upcase(method->name());
-
- if (first) {
- out << " = ";
- if (superType != NULL) {
- out << superType->fullName()
- << "::Call::CallCount";
- } else {
- out << "::android::hardware::IBinder::FIRST_CALL_TRANSACTION";
- }
-
- first = false;
- }
-
- out << ",\n";
- }
-
- out << "CallCount\n";
-
- out.unindent();
- out << "};\n\n";
-
+ out << "virtual bool isRemote() const { return false; } \n\n";
bool haveCallbacks = false;
for (const auto &method : iface->methods()) {
const bool returnsValue = !method->results().empty();
@@ -284,6 +260,8 @@
out << ") = 0;\n";
}
+
+ out << "DECLARE_REGISTER_AND_GET_SERVICE(" << baseName << ")\n";
}
if (isInterface) {
@@ -300,12 +278,143 @@
return OK;
}
+status_t AST::generateHwBinderHeader(const std::string &outputPath) const {
+ std::string ifaceName;
+ if(!AST::isInterface(&ifaceName)) {
+ // types.hal does not get an HwBinder header.
+ return OK;
+ }
+
+ // cut off the leading 'I'.
+ const std::string baseName = ifaceName.substr(1);
+
+ const std::string klassName = "IHw" + baseName;
+
+ std::string path = outputPath;
+ path.append(mCoordinator->convertPackageRootToPath(mPackage));
+ path.append(mCoordinator->getPackagePath(mPackage, true /* relative */));
+ path.append(klassName + ".h");
+
+ FILE* file = fopen(path.c_str(), "w");
+
+ if (file == NULL) {
+ return -errno;
+ }
+
+ Formatter out(file);
+
+ const std::string guard = makeHeaderGuard(klassName);
+
+ out << "#ifndef " << guard << "\n";
+ out << "#define " << guard << "\n\n";
+
+ std::vector<std::string> packageComponents;
+ getPackageAndVersionComponents(
+ &packageComponents, false /* cpp_compatible */);
+
+ out << "#include <";
+ for (const auto &component : packageComponents) {
+ out << component << "/";
+ }
+ out << ifaceName << ".h>\n\n";
+
+ for (const auto &item : mImportedNames) {
+ if (item.name() == "types") {
+ continue;
+ }
+
+ out << "#include <";
+
+ std::vector<std::string> components;
+ item.getPackageAndVersionComponents(
+ &components, false /* cpp_compatible */);
+
+ for (const auto &component : components) {
+ out << component << "/";
+ }
+
+ // cut off the leading I
+ const std::string itemBaseName = item.name().substr(1);
+
+ out << "Bn"
+ << itemBaseName
+ << ".h>\n";
+ }
+
+ out << "\n";
+
+ out << "#include <hidl/HidlSupport.h>\n";
+ out << "#include <hwbinder/IBinder.h>\n";
+ out << "#include <hwbinder/IInterface.h>\n";
+ out << "#include <hwbinder/Status.h>\n";
+
+ out << "\n";
+
+ enterLeaveNamespace(out, true /* enter */);
+ out << "\n";
+
+ out << "struct "
+ << klassName
+ << " : public "
+ << ifaceName;
+
+ const Interface *iface = mRootScope->getInterface();
+ const Interface *superType = iface->superType();
+
+ out << ", public ::android::hardware::IInterface";
+
+ out << " {\n";
+
+ out.indent();
+
+ out << "DECLARE_HWBINDER_META_INTERFACE(" << baseName << ");\n\n";
+
+ out << "enum Call {\n";
+ out.indent();
+
+ bool first = true;
+ for (const auto &method : iface->methods()) {
+ out << upcase(method->name());
+
+ if (first) {
+ out << " = ";
+ if (superType != NULL) {
+ out << superType->fqName().cppNamespace()
+ << "::IHw"
+ << superType->getBaseName()
+ << "::Call::CallCount";
+ } else {
+ out << "::android::hardware::IBinder::FIRST_CALL_TRANSACTION";
+ }
+
+ first = false;
+ }
+
+ out << ",\n";
+ }
+
+ out << "CallCount\n";
+
+ out.unindent();
+ out << "};\n\n";
+
+ out.unindent();
+
+ out << "};\n\n";
+
+ enterLeaveNamespace(out, false /* enter */);
+
+ out << "\n#endif // " << guard << "\n";
+
+ return OK;
+}
+
status_t AST::emitTypeDeclarations(Formatter &out) const {
return mRootScope->emitTypeDeclarations(out);
}
status_t AST::generateHeaderMethodSignatures(
- Formatter &out, bool abstract) const {
+ Formatter &out, bool stub) const {
const Interface *iface = mRootScope->getInterface();
std::vector<const Interface *> chain;
@@ -322,6 +431,9 @@
<< " follow.\n";
for (const auto &method : superInterface->methods()) {
+ if (stub) {
+ out << "inline ";
+ }
const bool returnsValue = !method->results().empty();
const TypedVar *elidedReturn = method->canElideCallback();
@@ -346,7 +458,33 @@
}
out << ") ";
- out << (abstract ? "= 0" : "override");
+ if (stub) {
+ out << " {\n";
+ out.indent();
+ out << "return mImpl->"
+ << method->name()
+ << "(";
+ bool first = true;
+ for (const auto &arg : method->args()) {
+ if (!first) {
+ out << ", ";
+ }
+ first = false;
+ out << arg->name();
+ }
+ if (returnsValue && elidedReturn == nullptr) {
+ if (!method->args().empty()) {
+ out << ", ";
+ }
+
+ out << "_hidl_cb";
+ }
+ out << ");\n";
+ out.unindent();
+ out << "}";
+ } else {
+ out << "override";
+ }
out << ";\n";
}
@@ -365,12 +503,12 @@
// cut off the leading 'I'.
const std::string baseName = ifaceName.substr(1);
+ const std::string klassName = "Bn" + baseName;
std::string path = outputPath;
path.append(mCoordinator->convertPackageRootToPath(mPackage));
path.append(mCoordinator->getPackagePath(mPackage, true /* relative */));
- path.append("Bn");
- path.append(baseName);
+ path.append(klassName);
path.append(".h");
CHECK(Coordinator::MakeParentHierarchy(path));
@@ -382,7 +520,7 @@
Formatter out(file);
- const std::string guard = makeHeaderGuard("Bn" + baseName);
+ const std::string guard = makeHeaderGuard(klassName);
out << "#ifndef " << guard << "\n";
out << "#define " << guard << "\n\n";
@@ -395,7 +533,7 @@
for (const auto &component : packageComponents) {
out << component << "/";
}
- out << ifaceName << ".h>\n\n";
+ out << "IHw" << baseName << ".h>\n\n";
enterLeaveNamespace(out, true /* enter */);
out << "\n";
@@ -403,12 +541,15 @@
out << "struct "
<< "Bn"
<< baseName
- << " : public ::android::hardware::BnInterface<"
- << ifaceName
+ << " : public ::android::hardware::BnInterface<I"
+ << baseName << ", IHw" << baseName
<< "> {\n";
out.indent();
-
+ out << "explicit Bn"
+ << baseName
+ << "(const ::android::sp<" << ifaceName << "> &_hidl_impl);"
+ << "\n\n";
out << "::android::status_t onTransact(\n";
out.indent();
out.indent();
@@ -420,8 +561,7 @@
out.unindent();
out.unindent();
- generateHeaderMethodSignatures(out, true); // = 0
-
+ generateHeaderMethodSignatures(out, true); // stub
out.unindent();
out << "};\n\n";
@@ -472,7 +612,7 @@
for (const auto &component : packageComponents) {
out << component << "/";
}
- out << ifaceName << ".h>\n\n";
+ out << "IHw" << baseName << ".h>\n\n";
enterLeaveNamespace(out, true /* enter */);
out << "\n";
@@ -480,8 +620,8 @@
out << "struct "
<< "Bp"
<< baseName
- << " : public ::android::hardware::BpInterface<"
- << ifaceName
+ << " : public ::android::hardware::BpInterface<IHw"
+ << baseName
<< "> {\n";
out.indent();
@@ -491,7 +631,9 @@
<< "(const ::android::sp<::android::hardware::IBinder> &_hidl_impl);"
<< "\n\n";
- generateHeaderMethodSignatures(out, false); // override
+ out << "virtual bool isRemote() const { return true; } \n\n";
+
+ generateHeaderMethodSignatures(out, false); // proxy
out.unindent();
@@ -570,6 +712,10 @@
err = generateStubSource(out, baseName);
}
+ if (err == OK && isInterface) {
+ out << "IMPLEMENT_REGISTER_AND_GET_SERVICE(" << baseName << ")\n";
+ }
+
enterLeaveNamespace(out, false /* enter */);
return err;
@@ -631,7 +777,7 @@
out.indent();
out << ": BpInterface"
- << "<I"
+ << "<IHw"
<< baseName
<< ">(_hidl_impl) {\n";
@@ -684,12 +830,13 @@
out << "::android::hardware::Parcel _hidl_reply;\n";
out << "::android::status_t _hidl_err;\n";
out << "::android::hardware::Status _hidl_status;\n\n";
-
declareCppReaderLocals(out, method->results());
out << "_hidl_err = _hidl_data.writeInterfaceToken("
- << superInterface->fullName()
- << "::getInterfaceDescriptor());\n";
+ << superInterface->fqName().cppNamespace()
+ << "::IHw"
+ << superInterface->getBaseName()
+ << "::descriptor);\n";
out << "if (_hidl_err != ::android::OK) { goto _hidl_error; }\n\n";
@@ -703,9 +850,11 @@
Type::ErrorMode_Goto);
}
- out << "_hidl_err = remote()->transact(I"
- << baseName
- << "::"
+ out << "_hidl_err = remote()->transact("
+ << superInterface->fqName().cppNamespace()
+ << "::IHw"
+ << superInterface->getBaseName()
+ << "::Call::"
<< upcase(method->name())
<< ", _hidl_data, &_hidl_reply";
if (method->isOneway()) {
@@ -798,6 +947,25 @@
const std::string klassName = "Bn" + baseName;
+ out << klassName
+ << "::"
+ << klassName
+ << "(const ::android::sp<I" << baseName <<"> &_hidl_impl)\n";
+
+ out.indent();
+ out.indent();
+
+ out << ": BnInterface"
+ << "<I"
+ << baseName
+ << ", IHw"
+ << baseName
+ << ">(_hidl_impl) {\n";
+
+ out.unindent();
+ out.unindent();
+ out << "}\n\n";
+
out << "::android::status_t " << klassName << "::onTransact(\n";
out.indent();
@@ -812,7 +980,6 @@
out.unindent();
out << "::android::status_t _hidl_err = ::android::OK;\n\n";
-
out << "switch (_hidl_code) {\n";
out.indent();
@@ -829,7 +996,9 @@
for (const auto &method : superInterface->methods()) {
out << "case "
- << superInterface->fullName()
+ << superInterface->fqName().cppNamespace()
+ << "::IHw"
+ << superInterface->getBaseName()
<< "::Call::"
<< upcase(method->name())
<< ":\n{\n";
@@ -852,7 +1021,7 @@
out.indent();
out << "return ::android::hardware::BnInterface<I"
- << baseName
+ << baseName << ", IHw" << baseName
<< ">::onTransact(\n";
out.indent();
@@ -898,8 +1067,10 @@
status_t AST::generateStubSourceForMethod(
Formatter &out, const Interface *iface, const Method *method) const {
out << "if (!_hidl_data.enforceInterface("
- << iface->fullName()
- << "::getInterfaceDescriptor())) {\n";
+ << iface->fqName().cppNamespace()
+ << "::IHw"
+ << iface->getBaseName()
+ << "::descriptor)) {\n";
out.indent();
out << "_hidl_err = ::android::BAD_TYPE;\n";
diff --git a/test/java_test/hidl_test_java_native.cpp b/test/java_test/hidl_test_java_native.cpp
index d831435..6ec7263 100644
--- a/test/java_test/hidl_test_java_native.cpp
+++ b/test/java_test/hidl_test_java_native.cpp
@@ -3,9 +3,7 @@
#include <android-base/logging.h>
-#include <android/hardware/tests/baz/1.0/BnBaz.h>
-#include <android/hardware/tests/baz/1.0/BnBazCallback.h>
-#include <android/hardware/tests/baz/1.0/BpBaz.h>
+#include <android/hardware/tests/baz/1.0/IBaz.h>
#include <gtest/gtest.h>
#include <hidl/IServiceManager.h>
@@ -18,16 +16,13 @@
using ::android::hardware::tests::baz::V1_0::IBase;
using ::android::hardware::tests::baz::V1_0::IBaz;
using ::android::hardware::tests::baz::V1_0::IBazCallback;
-using ::android::hardware::tests::baz::V1_0::BnBaz;
-using ::android::hardware::tests::baz::V1_0::BnBazCallback;
-using ::android::hardware::tests::baz::V1_0::BpBaz;
using ::android::hardware::hidl_vec;
using ::android::hardware::hidl_string;
using ::android::hardware::Return;
using ::android::hardware::Status;
-struct BazCallback : public BnBazCallback {
+struct BazCallback : public IBazCallback {
Status heyItsMe(const sp<IBazCallback> &cb) override;
};
@@ -38,7 +33,7 @@
return Status::ok();
}
-struct Baz : public BnBaz {
+struct Baz : public IBaz {
Status someBaseMethod() override;
Status someOtherBaseMethod(
@@ -349,13 +344,8 @@
const hidl_version kVersion = make_hidl_version(1, 0);
- sp<IBinder> service =
- defaultServiceManager()->getService(
- ::android::String16("baz"), kVersion);
+ baz = IBaz::getService(::android::String16("baz"), kVersion);
- CHECK(service != NULL);
-
- baz = IBaz::asInterface(service);
CHECK(baz != NULL);
}
@@ -599,7 +589,7 @@
const hidl_version kVersion = make_hidl_version(1, 0);
- defaultServiceManager()->addService(String16("baz"), baz, kVersion);
+ baz->registerAsService(String16("baz"), kVersion);
ProcessState::self()->startThreadPool();
ProcessState::self()->setThreadPoolMaxThreadCount(0);
diff --git a/test/main.cpp b/test/main.cpp
index 72ec8cd..cfe9701 100644
--- a/test/main.cpp
+++ b/test/main.cpp
@@ -30,6 +30,7 @@
using ::android::hardware::tests::foo::V1_0::IFoo;
using ::android::hardware::tests::foo::V1_0::IFooCallback;
using ::android::hardware::tests::bar::V1_0::IBar;
+using ::android::hardware::tests::bar::V1_0::IHwBar;
using ::android::hardware::tests::foo::V1_0::Abc;
using ::android::hardware::Return;
using ::android::hardware::Status;
@@ -39,7 +40,7 @@
using ::android::Mutex;
using ::android::Condition;
-struct FooCallback : public BnFooCallback {
+struct FooCallback : public IFooCallback {
FooCallback() : invokeInfo{}, mLock{}, mCond{} {}
Status heyItsYou(const sp<IFooCallback> &cb) override;
Return<bool> heyItsYouIsntIt(const sp<IFooCallback> &cb) override;
@@ -122,7 +123,7 @@
return Status::ok();
}
-struct Bar : public BnBar {
+struct Bar : public IBar {
Status doThis(float param) override;
Return<int32_t> doThatAndReturnSomething(int64_t param) override;
@@ -388,7 +389,7 @@
using namespace android::hardware;
using android::String16;
ALOGI("SERVER(%s) registering", tag);
- defaultServiceManager()->addService(String16(serviceName), server, kVersion);
+ server->registerAsService(String16(serviceName), kVersion);
ALOGI("SERVER(%s) starting", tag);
ProcessState::self()->setThreadPoolMaxThreadCount(0);
ProcessState::self()->startThreadPool();
@@ -410,22 +411,13 @@
using android::String16;
const hidl_version kVersion = make_hidl_version(1, 0);
- service =
- defaultServiceManager()->getService(String16("foo"), kVersion);
- ALOGI("CLIENT Found service foo.");
-
- CHECK(service != NULL);
-
- foo = IFoo::asInterface(service);
+ foo = IFoo::getService(String16("foo"), kVersion);
CHECK(foo != NULL);
- bar = IBar::asInterface(service);
+ bar = IBar::getService(String16("foo"), kVersion);
CHECK(bar != NULL);
- cbService = defaultServiceManager()->getService(String16("foo callback"), kVersion);
- CHECK(cbService != NULL);
-
- fooCb = IFooCallback::asInterface(cbService);
+ fooCb = IFooCallback::getService(String16("foo callback"), kVersion);
CHECK(fooCb != NULL);
ALOGI("Test setup complete");