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

@@ -0,0 +1,2 @@
+BasedOnStyle: Google
+IndentWidth: 4

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",

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 <functional>
+#include <tuple>
+#include <type_traits>
+#include <utility>
+
+#include <gtest/gtest.h>
+
+namespace {
+namespace detail {
+template <typename>
+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 <typename ArgT>
+struct functionArgSaver<std::function<void(ArgT)>> final {
+    using StorageT = typename std::remove_const<
+        typename std::remove_reference<ArgT>::type>::type;
+
+    std::function<void(ArgT)> 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 <typename Arg1T, typename Arg2T>
+struct functionArgSaver<std::function<void(Arg1T, Arg2T)>> final {
+    using StorageT =
+        std::pair<typename std::remove_const<
+                      typename std::remove_reference<Arg1T>::type>::type,
+                  typename std::remove_const<
+                      typename std::remove_reference<Arg2T>::type>::type>;
+
+    std::function<void(Arg1T, Arg2T)> 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 <typename... ArgT>
+struct functionArgSaver<std::function<void(ArgT...)>> final {
+    using StorageT = std::tuple<typename std::remove_const<
+        typename std::remove_reference<ArgT>::type>::type...>;
+
+    std::function<void(ArgT...)> 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 CallbackT, typename MethodT, typename ObjectT,
+          typename... ArgT>
+typename functionArgSaver<CallbackT>::StorageT invokeMethod(
+    MethodT method, ObjectT object, ArgT&&... methodArg) {
+    functionArgSaver<CallbackT> result_buffer;
+    const auto& res = ((*object).*method)(std::forward<ArgT>(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<decltype(*strong_pointer)>::type::method##_cb>( \
+        &std::remove_reference<decltype(*strong_pointer)>::type::method,      \
+        strong_pointer, ##__VA_ARGS__))

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 <functional>
+#include <type_traits>
+
+#include <hidl/Status.h>
+#include <utils/RefBase.h>
+#include <utils/StrongPointer.h>
+
+#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<void(int)>;
+
+    // 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<void(int)>;
+
+    // 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<void> 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<void> 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<int, detail::functionArgSaver<
+                                    std::function<void(int)>>::StorageT>::value,
+              "Single-arg result should be stored directly.");
+
+static_assert(
+    std::is_same<std::pair<int, long>, detail::functionArgSaver<std::function<
+                                           void(int, long)>>::StorageT>::value,
+    "Two-arg result should be stored as a pair.");
+
+static_assert(
+    std::is_same<std::tuple<char, int, long>,
+                 detail::functionArgSaver<
+                     std::function<void(char, int, long)>>::StorageT>::value,
+    "Three-arg result should be stored as a tuple.");
+
+static_assert(std::is_same<Dummy, detail::functionArgSaver<std::function<
+                                      void(const Dummy&)>>::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<IExample> 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<IExample> sp = new IExample();
+    EXPECT_EQ(IExample::kByReferenceResult,
+              HIDL_INVOKE(sp, startWithCallbackReference));
+}

wifi/1.0/vts/functional/wifi_hidl_test_utils.cpp

@@ -16,6 +16,7 @@
 
 #include <gtest/gtest.h>
 
+#include "wifi_hidl_call_util.h"
 #include "wifi_hidl_test_utils.h"
 
 using ::android::hardware::wifi::V1_0::IWifi;
@@ -52,41 +53,23 @@ sp<IWifiChip> 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<ChipId> wifi_chip_ids;
-    wifi->getChipIds(
-        [&](const WifiStatus& status, const hidl_vec<ChipId>& 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<IWifiChip> wifi_chip;
-    wifi->getChip(wifi_chip_ids[0],
-                  [&](const WifiStatus& status, const sp<IWifiChip>& 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<IWifiChip>& wifi_chip,
                                      IfaceType type) {
-    bool operation_failed = false;
-    std::vector<IWifiChip::ChipMode> chip_modes;
-    wifi_chip->getAvailableModes(
-        [&](WifiStatus status, const hidl_vec<IWifiChip::ChipMode>& 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<IWifiApIface> getWifiApIface() {
         return nullptr;
     }
 
-    bool operation_failed = false;
-    sp<IWifiApIface> wifi_ap_iface;
-    wifi_chip->createApIface(
-        [&](const WifiStatus& status, const sp<IWifiApIface>& 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<IWifiNanIface> getWifiNanIface() {
@@ -178,19 +141,11 @@ sp<IWifiNanIface> getWifiNanIface() {
         return nullptr;
     }
 
-    bool operation_failed = false;
-    sp<IWifiNanIface> wifi_nan_iface;
-    wifi_chip->createNanIface(
-        [&](const WifiStatus& status, const sp<IWifiNanIface>& 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<IWifiP2pIface> getWifiP2pIface() {
@@ -202,19 +157,11 @@ sp<IWifiP2pIface> getWifiP2pIface() {
         return nullptr;
     }
 
-    bool operation_failed = false;
-    sp<IWifiP2pIface> wifi_p2p_iface;
-    wifi_chip->createP2pIface(
-        [&](const WifiStatus& status, const sp<IWifiP2pIface>& 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<IWifiStaIface> getWifiStaIface() {
@@ -226,19 +173,11 @@ sp<IWifiStaIface> getWifiStaIface() {
         return nullptr;
     }
 
-    bool operation_failed = false;
-    sp<IWifiStaIface> wifi_sta_iface;
-    wifi_chip->createStaIface(
-        [&](const WifiStatus& status, const sp<IWifiStaIface>& 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<IWifiRttController> getWifiRttController() {
@@ -251,26 +190,16 @@ sp<IWifiRttController> getWifiRttController() {
         return nullptr;
     }
 
-    bool operation_failed = false;
-    sp<IWifiRttController> wifi_rtt_controller;
-    wifi_chip->createRttController(
-        wifi_sta_iface, [&](const WifiStatus& status,
-                            const sp<IWifiRttController>& 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<IWifi> 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);
 }