From 90f321722ce37554fb70481f44f8b4a32389a9e6 Mon Sep 17 00:00:00 2001 From: mukesh agrawal Date: Wed, 25 Jan 2017 19:43:08 -0800 Subject: [PATCH] wifi(vts): simplify HIDL calls Presently, the core of the Wifi test logic is obscured by the boilerplate required to create a callback. This CL provides some utilities to simplify the creation of a HIDL result callback, and migrates existing Wifi code to use the new utilities. Along the way: add a .clang-format file, so that I don't misformat code with 2-space indents (the Google default). Bug: 34817351 Test: vts-tradefed run commandAndExit vts --module=HalWifiHidlTargetTest Change-Id: Id2c728f96c3369c74adc8dfce7228b0a15a0781e --- wifi/.clang-format | 2 + wifi/1.0/vts/functional/Android.bp | 1 + wifi/1.0/vts/functional/wifi_hidl_call_util.h | 123 ++++++++++++++++ .../wifi_hidl_call_util_selftest.cpp | 114 +++++++++++++++ .../vts/functional/wifi_hidl_test_utils.cpp | 135 +++++------------- 5 files changed, 272 insertions(+), 103 deletions(-) create mode 100644 wifi/.clang-format create mode 100644 wifi/1.0/vts/functional/wifi_hidl_call_util.h create mode 100644 wifi/1.0/vts/functional/wifi_hidl_call_util_selftest.cpp diff --git a/wifi/.clang-format b/wifi/.clang-format new file mode 100644 index 0000000000..25ed9323e4 --- /dev/null +++ b/wifi/.clang-format @@ -0,0 +1,2 @@ +BasedOnStyle: Google +IndentWidth: 4 \ No newline at end of file diff --git a/wifi/1.0/vts/functional/Android.bp b/wifi/1.0/vts/functional/Android.bp index e2cd7b1e40..8a5d7e0ae0 100644 --- a/wifi/1.0/vts/functional/Android.bp +++ b/wifi/1.0/vts/functional/Android.bp @@ -21,6 +21,7 @@ cc_test { "main.cpp", "wifi_ap_iface_hidl_test.cpp", "wifi_chip_hidl_test.cpp", + "wifi_hidl_call_util_selftest.cpp", "wifi_hidl_test.cpp", "wifi_hidl_test_utils.cpp", "wifi_nan_iface_hidl_test.cpp", diff --git a/wifi/1.0/vts/functional/wifi_hidl_call_util.h b/wifi/1.0/vts/functional/wifi_hidl_call_util.h new file mode 100644 index 0000000000..03200a06d8 --- /dev/null +++ b/wifi/1.0/vts/functional/wifi_hidl_call_util.h @@ -0,0 +1,123 @@ +/* + * Copyright (C) 2017 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. + */ + +#pragma once + +#include +#include +#include +#include + +#include + +namespace { +namespace detail { +template +struct functionArgSaver; + +// Provides a std::function that takes one argument, and a buffer +// wherein the function will store its argument. The buffer has +// the same type as the argument, but with const and reference +// modifiers removed. +template +struct functionArgSaver> final { + using StorageT = typename std::remove_const< + typename std::remove_reference::type>::type; + + std::function saveArgs = [this](ArgT arg) { + this->saved_values = arg; + }; + + StorageT saved_values; +}; + +// Provides a std::function that takes two arguments, and a buffer +// wherein the function will store its arguments. The buffer is a +// std::pair, whose elements have the same types as the arguments +// (but with const and reference modifiers removed). +template +struct functionArgSaver> final { + using StorageT = + std::pair::type>::type, + typename std::remove_const< + typename std::remove_reference::type>::type>; + + std::function saveArgs = [this](Arg1T arg1, + Arg2T arg2) { + this->saved_values = {arg1, arg2}; + }; + + StorageT saved_values; +}; + +// Provides a std::function that takes three or more arguments, and a +// buffer wherein the function will store its arguments. The buffer is a +// std::tuple whose elements have the same types as the arguments (but +// with const and reference modifiers removed). +template +struct functionArgSaver> final { + using StorageT = std::tuple::type>::type...>; + + std::function saveArgs = [this](ArgT... arg) { + this->saved_values = {arg...}; + }; + + StorageT saved_values; +}; + +// Invokes |method| on |object|, providing |method| a CallbackT as the +// final argument. Returns a copy of the parameters that |method| provided +// to CallbackT. (The parameters are returned by value.) +template +typename functionArgSaver::StorageT invokeMethod( + MethodT method, ObjectT object, ArgT&&... methodArg) { + functionArgSaver result_buffer; + const auto& res = ((*object).*method)(std::forward(methodArg)..., + result_buffer.saveArgs); + EXPECT_TRUE(res.isOk()); + return result_buffer.saved_values; +} +} // namespace detail +} // namespace + +// Invokes |method| on |strong_pointer|, passing provided arguments through to +// |method|. +// +// Returns either: +// - A copy of the result callback parameter (for callbacks with a single +// parameter), OR +// - A pair containing a copy of the result callback parameters (for callbacks +// with two parameters), OR +// - A tuple containing a copy of the result callback paramters (for callbacks +// with three or more parameters). +// +// Example usage: +// EXPECT_EQ(WifiStatusCode::SUCCESS, +// HIDL_INVOKE(strong_pointer, methodReturningWifiStatus).code); +// EXPECT_EQ(WifiStatusCode::SUCCESS, +// HIDL_INVOKE(strong_pointer, methodReturningWifiStatusAndOneMore) +// .first.code); +// EXPECT_EQ(WifiStatusCode::SUCCESS, std::get<0>( +// HIDL_INVOKE(strong_pointer, methodReturningWifiStatusAndTwoMore)) +// .code); +#define HIDL_INVOKE(strong_pointer, method, ...) \ + (detail::invokeMethod< \ + std::remove_reference::type::method##_cb>( \ + &std::remove_reference::type::method, \ + strong_pointer, ##__VA_ARGS__)) diff --git a/wifi/1.0/vts/functional/wifi_hidl_call_util_selftest.cpp b/wifi/1.0/vts/functional/wifi_hidl_call_util_selftest.cpp new file mode 100644 index 0000000000..129bdb2b7c --- /dev/null +++ b/wifi/1.0/vts/functional/wifi_hidl_call_util_selftest.cpp @@ -0,0 +1,114 @@ +/* + * Copyright (C) 2017 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. + */ + +#include +#include + +#include +#include +#include + +#include "wifi_hidl_call_util.h" + +namespace { +/* + * Example of a user-defined data-type. + * + * Used to verify that, within the internals of HIDL_INVOKE, + * reference parameters are stored by copy. + */ +class Dummy {}; + +/* + * Example of what a HIDL-generated proxy might look like. + */ +class IExample : public ::android::RefBase { + public: + // The callback type, for a method called startWithCallbackCopy, which + // has a callback that takes an |int|. Both the name, and the value, + // must match what would appear in HIDL-generated code. + using startWithCallbackCopy_cb = std::function; + + // The callback type, for a method called startWithCallbackReference, which + // has a callback that takes an |int|. Both the name, and the value, + // must match what would appear in HIDL-generated code. + using startWithCallbackReference_cb = std::function; + + // Constants which allow tests to verify that the proxy methods can + // correctly return a value. We use different values for by-copy and + // by-reference, to double-check that a call was dispatched properly. + static constexpr int kByCopyResult = 42; + static constexpr int kByReferenceResult = 420; + + // Example of what a no-arg method would look like, if the callback + // is passed by-value. + ::android::hardware::Return startWithCallbackCopy( + startWithCallbackCopy_cb _hidl_cb) { + _hidl_cb(kByCopyResult); + return ::android::hardware::Void(); + } + // Example of what a no-arg method would look like, if the callback + // is passed by const-reference. + ::android::hardware::Return startWithCallbackReference( + const startWithCallbackReference_cb& _hidl_cb) { + _hidl_cb(kByReferenceResult); + return ::android::hardware::Void(); + } +}; + +constexpr int IExample::kByCopyResult; +constexpr int IExample::kByReferenceResult; +} // namespace + +static_assert(std::is_same>::StorageT>::value, + "Single-arg result should be stored directly."); + +static_assert( + std::is_same, detail::functionArgSaver>::StorageT>::value, + "Two-arg result should be stored as a pair."); + +static_assert( + std::is_same, + detail::functionArgSaver< + std::function>::StorageT>::value, + "Three-arg result should be stored as a tuple."); + +static_assert(std::is_same>::StorageT>::value, + "Reference should be stored by copy."); + +/* + * Verifies that HIDL_INVOKE can be used with methods that take the result + * callback as a by-value parameter. (This reflects the current implementation + * of HIDL-generated code.) + */ +TEST(HidlInvokeTest, WorksWithMethodThatTakesResultCallbackByValue) { + ::android::sp sp = new IExample(); + EXPECT_EQ(IExample::kByCopyResult, HIDL_INVOKE(sp, startWithCallbackCopy)); +} + +/* + * Verifies that HIDL_INVOKE can be used with methods that take the result + * callback as a const-reference parameter. (This ensures that HIDL_INVOKE will + * continue to work, if the HIDL-generated code switches to const-ref.) + */ +TEST(HidlInvokeTest, WorksWithMethodThatTakesResultCallbackByConstReference) { + ::android::sp sp = new IExample(); + EXPECT_EQ(IExample::kByReferenceResult, + HIDL_INVOKE(sp, startWithCallbackReference)); +} diff --git a/wifi/1.0/vts/functional/wifi_hidl_test_utils.cpp b/wifi/1.0/vts/functional/wifi_hidl_test_utils.cpp index 050bba3fc7..8f34a882da 100644 --- a/wifi/1.0/vts/functional/wifi_hidl_test_utils.cpp +++ b/wifi/1.0/vts/functional/wifi_hidl_test_utils.cpp @@ -16,6 +16,7 @@ #include +#include "wifi_hidl_call_util.h" #include "wifi_hidl_test_utils.h" using ::android::hardware::wifi::V1_0::IWifi; @@ -52,41 +53,23 @@ sp getWifiChip() { return nullptr; } - bool operation_failed = false; - wifi->start([&](WifiStatus status) { - if (status.code != WifiStatusCode::SUCCESS) { - operation_failed = true; - } - }); - if (operation_failed) { + if (HIDL_INVOKE(wifi, start).code != WifiStatusCode::SUCCESS) { return nullptr; } - std::vector wifi_chip_ids; - wifi->getChipIds( - [&](const WifiStatus& status, const hidl_vec& chip_ids) { - if (status.code != WifiStatusCode::SUCCESS) { - operation_failed = true; - } - wifi_chip_ids = chip_ids; - }); - // We don't expect more than 1 chip currently. - if (operation_failed || wifi_chip_ids.size() != 1) { + const auto& status_and_chip_ids = HIDL_INVOKE(wifi, getChipIds); + const auto& chip_ids = status_and_chip_ids.second; + if (status_and_chip_ids.first.code != WifiStatusCode::SUCCESS || + chip_ids.size() != 1) { return nullptr; } - sp wifi_chip; - wifi->getChip(wifi_chip_ids[0], - [&](const WifiStatus& status, const sp& chip) { - if (status.code != WifiStatusCode::SUCCESS) { - operation_failed = true; - } - wifi_chip = chip; - }); - if (operation_failed) { + const auto& status_and_chip = HIDL_INVOKE(wifi, getChip, chip_ids[0]); + if (status_and_chip.first.code != WifiStatusCode::SUCCESS) { return nullptr; } - return wifi_chip; + + return status_and_chip.second; } // Since we currently only support one iface of each type. Just iterate thru the @@ -116,30 +99,18 @@ bool findModeToSupportIfaceType(IfaceType type, bool configureChipToSupportIfaceType(const sp& wifi_chip, IfaceType type) { - bool operation_failed = false; - std::vector chip_modes; - wifi_chip->getAvailableModes( - [&](WifiStatus status, const hidl_vec& modes) { - if (status.code != WifiStatusCode::SUCCESS) { - operation_failed = true; - } - chip_modes = modes; - }); - if (operation_failed) { + const auto& status_and_modes = HIDL_INVOKE(wifi_chip, getAvailableModes); + if (status_and_modes.first.code != WifiStatusCode::SUCCESS) { return false; } ChipModeId mode_id; - if (!findModeToSupportIfaceType(type, chip_modes, &mode_id)) { + if (!findModeToSupportIfaceType(type, status_and_modes.second, &mode_id)) { return false; } - wifi_chip->configureChip(mode_id, [&](WifiStatus status) { - if (status.code != WifiStatusCode::SUCCESS) { - operation_failed = true; - } - }); - if (operation_failed) { + if (HIDL_INVOKE(wifi_chip, configureChip, mode_id).code != + WifiStatusCode::SUCCESS) { return false; } return true; @@ -154,19 +125,11 @@ sp getWifiApIface() { return nullptr; } - bool operation_failed = false; - sp wifi_ap_iface; - wifi_chip->createApIface( - [&](const WifiStatus& status, const sp& iface) { - if (status.code != WifiStatusCode::SUCCESS) { - operation_failed = true; - } - wifi_ap_iface = iface; - }); - if (operation_failed) { + const auto& status_and_iface = HIDL_INVOKE(wifi_chip, createApIface); + if (status_and_iface.first.code != WifiStatusCode::SUCCESS) { return nullptr; } - return wifi_ap_iface; + return status_and_iface.second; } sp getWifiNanIface() { @@ -178,19 +141,11 @@ sp getWifiNanIface() { return nullptr; } - bool operation_failed = false; - sp wifi_nan_iface; - wifi_chip->createNanIface( - [&](const WifiStatus& status, const sp& iface) { - if (status.code != WifiStatusCode::SUCCESS) { - operation_failed = true; - } - wifi_nan_iface = iface; - }); - if (operation_failed) { + const auto& status_and_iface = HIDL_INVOKE(wifi_chip, createNanIface); + if (status_and_iface.first.code != WifiStatusCode::SUCCESS) { return nullptr; } - return wifi_nan_iface; + return status_and_iface.second; } sp getWifiP2pIface() { @@ -202,19 +157,11 @@ sp getWifiP2pIface() { return nullptr; } - bool operation_failed = false; - sp wifi_p2p_iface; - wifi_chip->createP2pIface( - [&](const WifiStatus& status, const sp& iface) { - if (status.code != WifiStatusCode::SUCCESS) { - operation_failed = true; - } - wifi_p2p_iface = iface; - }); - if (operation_failed) { + const auto& status_and_iface = HIDL_INVOKE(wifi_chip, createP2pIface); + if (status_and_iface.first.code != WifiStatusCode::SUCCESS) { return nullptr; } - return wifi_p2p_iface; + return status_and_iface.second; } sp getWifiStaIface() { @@ -226,19 +173,11 @@ sp getWifiStaIface() { return nullptr; } - bool operation_failed = false; - sp wifi_sta_iface; - wifi_chip->createStaIface( - [&](const WifiStatus& status, const sp& iface) { - if (status.code != WifiStatusCode::SUCCESS) { - operation_failed = true; - } - wifi_sta_iface = iface; - }); - if (operation_failed) { + const auto& status_and_iface = HIDL_INVOKE(wifi_chip, createStaIface); + if (status_and_iface.first.code != WifiStatusCode::SUCCESS) { return nullptr; } - return wifi_sta_iface; + return status_and_iface.second; } sp getWifiRttController() { @@ -251,26 +190,16 @@ sp getWifiRttController() { return nullptr; } - bool operation_failed = false; - sp wifi_rtt_controller; - wifi_chip->createRttController( - wifi_sta_iface, [&](const WifiStatus& status, - const sp& controller) { - if (status.code != WifiStatusCode::SUCCESS) { - operation_failed = true; - } - wifi_rtt_controller = controller; - }); - if (operation_failed) { + const auto& status_and_controller = + HIDL_INVOKE(wifi_chip, createRttController, wifi_sta_iface); + if (status_and_controller.first.code != WifiStatusCode::SUCCESS) { return nullptr; } - return wifi_rtt_controller; + return status_and_controller.second; } void stopWifi() { sp wifi = getWifi(); ASSERT_NE(wifi, nullptr); - wifi->stop([](const WifiStatus& status) { - ASSERT_EQ(status.code, WifiStatusCode::SUCCESS); - }); + ASSERT_EQ(HIDL_INVOKE(wifi, stop).code, WifiStatusCode::SUCCESS); }