From 5647665827980a6dd2189214e02009f4c8f78dfc Mon Sep 17 00:00:00 2001 From: Roshan Pius Date: Wed, 26 Oct 2016 14:43:05 -0700 Subject: [PATCH] wifi: Helper functions for invoking hidl cont callbacks The helper functions are used to invoke an internal method which implements the functionality and then invoke the HIDL callback with the return values. HIDL's auto-generated code uses on-stack callbacks to return non-primitive/multiple values from HIDL methods. This is unwieldy and the implementation of the method's functionality gets mixed up with the semantics of handling these callbacks. This tries to hide the semantics of HIDL auto-generated code from the functionality. Converted all IWifi methods to use these new helper functions. Bug: 32337072 Test: Compiles Change-Id: I57cbafcc2ecb52ec5055f4bd80bc064bd438b850 --- wifi/1.0/default/hidl_return_util.h | 109 ++++++++++++++++++++++++++ wifi/1.0/default/wifi.cpp | 109 +++++++++++++++++--------- wifi/1.0/default/wifi.h | 13 ++- wifi/1.0/default/wifi_status_util.cpp | 5 +- 4 files changed, 192 insertions(+), 44 deletions(-) create mode 100644 wifi/1.0/default/hidl_return_util.h diff --git a/wifi/1.0/default/hidl_return_util.h b/wifi/1.0/default/hidl_return_util.h new file mode 100644 index 0000000000..2986165507 --- /dev/null +++ b/wifi/1.0/default/hidl_return_util.h @@ -0,0 +1,109 @@ +/* + * Copyright (C) 2016 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. + */ + +#ifndef HIDL_RETURN_UTIL_H_ +#define HIDL_RETURN_UTIL_H_ + +#include "wifi_status_util.h" + +namespace android { +namespace hardware { +namespace wifi { +namespace V1_0 { +namespace implementation { +namespace hidl_return_util { + +/** + * These utility functions are used to invoke a method on the provided + * HIDL interface object. + * These functions checks if the provided HIDL interface object is valid. + * a) if valid, Invokes the corresponding internal implementation function of + * the HIDL method. It then invokes the HIDL continuation callback with + * the status and any returned values. + * b) if invalid, invokes the HIDL continuation callback with the + * provided error status and default values. + */ +// Use for HIDL methods which return only an instance of WifiStatus. +template +Return validateAndCall( + ObjT* obj, + WifiStatusCode status_code_if_invalid, + WorkFuncT&& work, + const std::function& hidl_cb, + Args&&... args) { + if (obj->isValid()) { + hidl_cb((obj->*work)(std::forward(args)...)); + } else { + hidl_cb(createWifiStatus(status_code_if_invalid)); + } + return Void(); +} + +// Use for HIDL methods which return instance of WifiStatus and a single return +// value. +template +Return validateAndCall( + ObjT* obj, + WifiStatusCode status_code_if_invalid, + WorkFuncT&& work, + const std::function& hidl_cb, + Args&&... args) { + if (obj->isValid()) { + const auto& ret_pair = (obj->*work)(std::forward(args)...); + const WifiStatus& status = std::get<0>(ret_pair); + const auto& ret_value = std::get<1>(ret_pair); + hidl_cb(status, ret_value); + } else { + hidl_cb(createWifiStatus(status_code_if_invalid), + typename std::remove_reference::type()); + } + return Void(); +} + +// Use for HIDL methods which return instance of WifiStatus and 2 return +// values. +template +Return validateAndCall( + ObjT* obj, + WifiStatusCode status_code_if_invalid, + WorkFuncT&& work, + const std::function& hidl_cb, + Args&&... args) { + if (obj->isValid()) { + const auto& ret_tuple = (obj->*work)(std::forward(args)...); + const WifiStatus& status = std::get<0>(ret_tuple); + const auto& ret_value1 = std::get<1>(ret_tuple); + const auto& ret_value2 = std::get<2>(ret_tuple); + hidl_cb(status, ret_value1, ret_value2); + } else { + hidl_cb(createWifiStatus(status_code_if_invalid), + typename std::remove_reference::type(), + typename std::remove_reference::type()); + } + return Void(); +} + +} // namespace hidl_util +} // namespace implementation +} // namespace V1_0 +} // namespace wifi +} // namespace hardware +} // namespace android +#endif // HIDL_RETURN_UTIL_H_ diff --git a/wifi/1.0/default/wifi.cpp b/wifi/1.0/default/wifi.cpp index e10826bfbb..73b921ada7 100644 --- a/wifi/1.0/default/wifi.cpp +++ b/wifi/1.0/default/wifi.cpp @@ -14,11 +14,10 @@ * limitations under the License. */ -#include "wifi.h" - #include -#include "wifi_chip.h" +#include "hidl_return_util.h" +#include "wifi.h" #include "wifi_status_util.h" namespace { @@ -31,15 +30,24 @@ namespace hardware { namespace wifi { namespace V1_0 { namespace implementation { +using hidl_return_util::validateAndCall; Wifi::Wifi() : legacy_hal_(new WifiLegacyHal()), run_state_(RunState::STOPPED) {} +bool Wifi::isValid() { + // This object is always valid. + return true; +} + Return Wifi::registerEventCallback( - const sp& event_callback) { - // TODO(b/31632518): remove the callback when the client is destroyed - event_callbacks_.emplace_back(event_callback); - return Void(); + const sp& event_callback, + registerEventCallback_cb hidl_status_cb) { + return validateAndCall(this, + WifiStatusCode::ERROR_UNKNOWN, + &Wifi::registerEventCallbackInternal, + hidl_status_cb, + event_callback); } Return Wifi::isStarted() { @@ -47,22 +55,53 @@ Return Wifi::isStarted() { } Return Wifi::start(start_cb hidl_status_cb) { + return validateAndCall(this, + WifiStatusCode::ERROR_UNKNOWN, + &Wifi::startInternal, + hidl_status_cb); +} + +Return Wifi::stop(stop_cb hidl_status_cb) { + return validateAndCall( + this, WifiStatusCode::ERROR_UNKNOWN, &Wifi::stopInternal, hidl_status_cb); +} + +Return Wifi::getChipIds(getChipIds_cb hidl_status_cb) { + return validateAndCall(this, + WifiStatusCode::ERROR_UNKNOWN, + &Wifi::getChipIdsInternal, + hidl_status_cb); +} + +Return Wifi::getChip(ChipId chip_id, getChip_cb hidl_status_cb) { + return validateAndCall(this, + WifiStatusCode::ERROR_UNKNOWN, + &Wifi::getChipInternal, + hidl_status_cb, + chip_id); +} + +WifiStatus Wifi::registerEventCallbackInternal( + const sp& event_callback) { + // TODO(b/31632518): remove the callback when the client is destroyed + event_callbacks_.emplace_back(event_callback); + return createWifiStatus(WifiStatusCode::SUCCESS); +} + +WifiStatus Wifi::startInternal() { if (run_state_ == RunState::STARTED) { - hidl_status_cb(createWifiStatus(WifiStatusCode::SUCCESS)); - return Void(); + return createWifiStatus(WifiStatusCode::SUCCESS); } else if (run_state_ == RunState::STOPPING) { - hidl_status_cb(createWifiStatus(WifiStatusCode::ERROR_NOT_AVAILABLE, - "HAL is stopping")); - return Void(); + return createWifiStatus(WifiStatusCode::ERROR_NOT_AVAILABLE, + "HAL is stopping"); } LOG(INFO) << "Starting HAL"; wifi_error legacy_status = legacy_hal_->start(); if (legacy_status != WIFI_SUCCESS) { LOG(ERROR) << "Failed to start Wifi HAL"; - hidl_status_cb( - createWifiStatusFromLegacyError(legacy_status, "Failed to start HAL")); - return Void(); + return createWifiStatusFromLegacyError(legacy_status, + "Failed to start HAL"); } // Create the chip instance once the HAL is started. @@ -73,18 +112,15 @@ Return Wifi::start(start_cb hidl_status_cb) { LOG(ERROR) << "Failed to invoke onStart callback"; }; } - hidl_status_cb(createWifiStatus(WifiStatusCode::SUCCESS)); - return Void(); + return createWifiStatus(WifiStatusCode::SUCCESS); } -Return Wifi::stop(stop_cb hidl_status_cb) { +WifiStatus Wifi::stopInternal() { if (run_state_ == RunState::STOPPED) { - hidl_status_cb(createWifiStatus(WifiStatusCode::SUCCESS)); - return Void(); + return createWifiStatus(WifiStatusCode::SUCCESS); } else if (run_state_ == RunState::STOPPING) { - hidl_status_cb(createWifiStatus(WifiStatusCode::ERROR_NOT_AVAILABLE, - "HAL is stopping")); - return Void(); + return createWifiStatus(WifiStatusCode::ERROR_NOT_AVAILABLE, + "HAL is stopping"); } LOG(INFO) << "Stopping HAL"; @@ -109,33 +145,28 @@ Return Wifi::stop(stop_cb hidl_status_cb) { for (const auto& callback : event_callbacks_) { callback->onFailure(wifi_status); } - hidl_status_cb(wifi_status); - return Void(); + return wifi_status; } - hidl_status_cb(createWifiStatus(WifiStatusCode::SUCCESS)); - return Void(); + return createWifiStatus(WifiStatusCode::SUCCESS); } -Return Wifi::getChipIds(getChipIds_cb hidl_status_cb) { +std::pair> Wifi::getChipIdsInternal() { std::vector chip_ids; if (chip_.get()) { chip_ids.emplace_back(kChipId); } - hidl_vec hidl_data; - hidl_data.setToExternal(chip_ids.data(), chip_ids.size()); - hidl_status_cb(hidl_data); - return Void(); + return {createWifiStatus(WifiStatusCode::SUCCESS), std::move(chip_ids)}; } -Return Wifi::getChip(ChipId chip_id, getChip_cb hidl_status_cb) { - if (chip_.get() && chip_id == kChipId) { - hidl_status_cb(chip_); - } else { - hidl_status_cb(nullptr); +std::pair> Wifi::getChipInternal(ChipId chip_id) { + if (!chip_.get()) { + return {createWifiStatus(WifiStatusCode::ERROR_NOT_STARTED), nullptr}; } - return Void(); + if (chip_id != kChipId) { + return {createWifiStatus(WifiStatusCode::ERROR_INVALID_ARGS), nullptr}; + } + return {createWifiStatus(WifiStatusCode::SUCCESS), chip_}; } - } // namespace implementation } // namespace V1_0 } // namespace wifi diff --git a/wifi/1.0/default/wifi.h b/wifi/1.0/default/wifi.h index 989b63019b..c6821164fd 100644 --- a/wifi/1.0/default/wifi.h +++ b/wifi/1.0/default/wifi.h @@ -39,9 +39,12 @@ class Wifi : public IWifi { public: Wifi(); + bool isValid(); + // HIDL methods exposed. Return registerEventCallback( - const sp& event_callback) override; + const sp& event_callback, + registerEventCallback_cb hidl_status_cb) override; Return isStarted() override; Return start(start_cb hidl_status_cb) override; Return stop(stop_cb hidl_status_cb) override; @@ -51,6 +54,14 @@ class Wifi : public IWifi { private: enum class RunState { STOPPED, STARTED, STOPPING }; + // Corresponding worker functions for the HIDL methods. + WifiStatus registerEventCallbackInternal( + const sp& event_callback); + WifiStatus startInternal(); + WifiStatus stopInternal(); + std::pair> getChipIdsInternal(); + std::pair> getChipInternal(ChipId chip_id); + // Instance is created in this root level |IWifi| HIDL interface object // and shared with all the child HIDL interface objects. std::shared_ptr legacy_hal_; diff --git a/wifi/1.0/default/wifi_status_util.cpp b/wifi/1.0/default/wifi_status_util.cpp index 21b4eebe7b..34a1c1d23e 100644 --- a/wifi/1.0/default/wifi_status_util.cpp +++ b/wifi/1.0/default/wifi_status_util.cpp @@ -50,10 +50,7 @@ std::string legacyErrorToString(wifi_error error) { WifiStatus createWifiStatus(WifiStatusCode code, const std::string& description) { - WifiStatus result; - result.code = code; - result.description = description.data(); - return result; + return {code, description}; } WifiStatus createWifiStatus(WifiStatusCode code) {