From 32fc12ea4f38224a18b124434da49a56c6d83f86 Mon Sep 17 00:00:00 2001 From: Roshan Pius Date: Wed, 25 Jan 2017 17:44:42 -0800 Subject: [PATCH] wifi: Fixing Nits Changes in the CL to fix bugs found while integrating with framework: 1. Added IWifiApIface.setCountryCode() Bug: 34682168 2. Fixed documentation nits in .hal files. Bug: 34468218 3. Changed roaming state enum values. Bug: 34715231 4. Added log for EBUSY in legacyErrorToString() Bug: 34715469 5. Remove "-Wno-unused parameter" from Android.mk Bug: 34611289 6. Remove the erroneous |onFailure| callback invocation in wifi.cpp. 7. Add null terminations to strings passed to legacy HAL API's 8. Remove unused |IWifiStaIface.stopDebugPacketFateMonitoring|. Test: Compiles Change-Id: If3a3e2f360a378c59dc56b8bbe3a0c6d400b3bd8 --- wifi/1.0/Android.mk | 4 +++ wifi/1.0/IWifiApIface.hal | 12 +++++++- wifi/1.0/IWifiStaIface.hal | 17 ++---------- wifi/1.0/default/Android.mk | 2 +- wifi/1.0/default/wifi.cpp | 5 ---- wifi/1.0/default/wifi_ap_iface.cpp | 16 +++++++++++ wifi/1.0/default/wifi_ap_iface.h | 3 ++ wifi/1.0/default/wifi_legacy_hal.cpp | 40 +++++++++++++++++---------- wifi/1.0/default/wifi_legacy_hal.h | 2 ++ wifi/1.0/default/wifi_sta_iface.cpp | 13 --------- wifi/1.0/default/wifi_sta_iface.h | 3 -- wifi/1.0/default/wifi_status_util.cpp | 4 +-- wifi/1.0/types.hal | 12 ++++---- 13 files changed, 73 insertions(+), 60 deletions(-) diff --git a/wifi/1.0/Android.mk b/wifi/1.0/Android.mk index 1058cc79d8..708c14c707 100644 --- a/wifi/1.0/Android.mk +++ b/wifi/1.0/Android.mk @@ -1589,6 +1589,8 @@ $(GEN): PRIVATE_HIDL := $(HIDL) $(GEN): PRIVATE_DEPS := $(LOCAL_PATH)/IWifiApIface.hal $(GEN): PRIVATE_DEPS += $(LOCAL_PATH)/IWifiIface.hal $(GEN): $(LOCAL_PATH)/IWifiIface.hal +$(GEN): PRIVATE_DEPS += $(LOCAL_PATH)/types.hal +$(GEN): $(LOCAL_PATH)/types.hal $(GEN): PRIVATE_OUTPUT_DIR := $(intermediates) $(GEN): PRIVATE_CUSTOM_TOOL = \ $(PRIVATE_HIDL) -o $(PRIVATE_OUTPUT_DIR) \ @@ -3447,6 +3449,8 @@ $(GEN): PRIVATE_HIDL := $(HIDL) $(GEN): PRIVATE_DEPS := $(LOCAL_PATH)/IWifiApIface.hal $(GEN): PRIVATE_DEPS += $(LOCAL_PATH)/IWifiIface.hal $(GEN): $(LOCAL_PATH)/IWifiIface.hal +$(GEN): PRIVATE_DEPS += $(LOCAL_PATH)/types.hal +$(GEN): $(LOCAL_PATH)/types.hal $(GEN): PRIVATE_OUTPUT_DIR := $(intermediates) $(GEN): PRIVATE_CUSTOM_TOOL = \ $(PRIVATE_HIDL) -o $(PRIVATE_OUTPUT_DIR) \ diff --git a/wifi/1.0/IWifiApIface.hal b/wifi/1.0/IWifiApIface.hal index 6bc3580137..aeca2cd44b 100644 --- a/wifi/1.0/IWifiApIface.hal +++ b/wifi/1.0/IWifiApIface.hal @@ -22,5 +22,15 @@ import IWifiIface; * Interface used to represent a single AP iface. */ interface IWifiApIface extends IWifiIface { - /** TODO(rpius): Add methods to the interface. */ + /** + * Set country code for this iface. + * + * @param code 2 byte country code (as defined in ISO 3166) to set. + * @return status Status of the operation. + * Possible status codes: + * |WifiStatusCode.SUCCESS|, + * |WifiStatusCode.FAILURE_UNKNOWN|, + * |WifiStatusCode.FAILURE_IFACE_INVALID| + */ + setCountryCode(int8_t[2] code) generates (WifiStatus status); }; diff --git a/wifi/1.0/IWifiStaIface.hal b/wifi/1.0/IWifiStaIface.hal index 6a738a9092..2c72eadfcb 100644 --- a/wifi/1.0/IWifiStaIface.hal +++ b/wifi/1.0/IWifiStaIface.hal @@ -439,7 +439,8 @@ interface IWifiStaIface extends IWifiIface { /** * API to start packet fate monitoring. - * - Once stared, monitoring must remain active until HAL is unloaded. + * - Once started, monitoring must remain active until HAL is stopped or the + * chip is reconfigured. * - When HAL is unloaded, all packet fate buffers must be cleared. * - The packet fates are used to monitor the state of packets transmitted/ * received during association. @@ -454,20 +455,6 @@ interface IWifiStaIface extends IWifiIface { */ startDebugPacketFateMonitoring() generates (WifiStatus status); - /** - * API to stop packet fate monitoring. - * - * @return status WifiStatus of the operation. - * Possible status codes: - * |WifiStatusCode.SUCCESS|, - * |WifiStatusCode.ERROR_WIFI_IFACE_INVALID|, - * |WifiStatusCode.ERROR_NOT_SUPPORTED|, - * |WifiStatusCode.ERROR_NOT_STARTED|, - * |WifiStatusCode.ERROR_NOT_AVAILABLE|, - * |WifiStatusCode.ERROR_UNKNOWN| - */ - stopDebugPacketFateMonitoring() generates (WifiStatus status); - /** * API to retrieve fates of outbound packets. * - HAL implementation must return the fates of diff --git a/wifi/1.0/default/Android.mk b/wifi/1.0/default/Android.mk index f0c78ea6af..144c067b92 100644 --- a/wifi/1.0/default/Android.mk +++ b/wifi/1.0/default/Android.mk @@ -16,7 +16,7 @@ LOCAL_PATH := $(call my-dir) include $(CLEAR_VARS) LOCAL_MODULE := android.hardware.wifi@1.0-service LOCAL_MODULE_RELATIVE_PATH := hw -LOCAL_CPPFLAGS := -Wall -Wno-unused-parameter -Werror -Wextra +LOCAL_CPPFLAGS := -Wall -Werror -Wextra LOCAL_SRC_FILES := \ hidl_struct_util.cpp \ service.cpp \ diff --git a/wifi/1.0/default/wifi.cpp b/wifi/1.0/default/wifi.cpp index c06abe8fef..8feb8364ba 100644 --- a/wifi/1.0/default/wifi.cpp +++ b/wifi/1.0/default/wifi.cpp @@ -107,11 +107,6 @@ WifiStatus Wifi::startInternal() { LOG(ERROR) << "Failed to invoke onStart callback"; }; } - for (const auto& callback : event_callbacks_) { - if (!callback->onFailure(wifi_status).isOk()) { - LOG(ERROR) << "Failed to invoke onFailure callback"; - } - } } else { for (const auto& callback : event_callbacks_) { if (!callback->onFailure(wifi_status).isOk()) { diff --git a/wifi/1.0/default/wifi_ap_iface.cpp b/wifi/1.0/default/wifi_ap_iface.cpp index b8b7a3a41b..1a8b31db0a 100644 --- a/wifi/1.0/default/wifi_ap_iface.cpp +++ b/wifi/1.0/default/wifi_ap_iface.cpp @@ -55,6 +55,15 @@ Return WifiApIface::getType(getType_cb hidl_status_cb) { hidl_status_cb); } +Return WifiApIface::setCountryCode(const hidl_array& code, + setCountryCode_cb hidl_status_cb) { + return validateAndCall(this, + WifiStatusCode::ERROR_WIFI_IFACE_INVALID, + &WifiApIface::setCountryCodeInternal, + hidl_status_cb, + code); +} + std::pair WifiApIface::getNameInternal() { return {createWifiStatus(WifiStatusCode::SUCCESS), ifname_}; } @@ -63,6 +72,13 @@ std::pair WifiApIface::getTypeInternal() { return {createWifiStatus(WifiStatusCode::SUCCESS), IfaceType::AP}; } +WifiStatus WifiApIface::setCountryCodeInternal( + const std::array& code) { + legacy_hal::wifi_error legacy_status = + legacy_hal_.lock()->setCountryCode(code); + return createWifiStatusFromLegacyError(legacy_status); +} + } // namespace implementation } // namespace V1_0 } // namespace wifi diff --git a/wifi/1.0/default/wifi_ap_iface.h b/wifi/1.0/default/wifi_ap_iface.h index ee5dc565d9..23d6435ea8 100644 --- a/wifi/1.0/default/wifi_ap_iface.h +++ b/wifi/1.0/default/wifi_ap_iface.h @@ -42,11 +42,14 @@ class WifiApIface : public IWifiApIface { // HIDL methods exposed. Return getName(getName_cb hidl_status_cb) override; Return getType(getType_cb hidl_status_cb) override; + Return setCountryCode(const hidl_array& code, + setCountryCode_cb hidl_status_cb) override; private: // Corresponding worker functions for the HIDL methods. std::pair getNameInternal(); std::pair getTypeInternal(); + WifiStatus setCountryCodeInternal(const std::array& code); std::string ifname_; std::weak_ptr legacy_hal_; diff --git a/wifi/1.0/default/wifi_legacy_hal.cpp b/wifi/1.0/default/wifi_legacy_hal.cpp index 3bfd2bb327..e4eddcd161 100644 --- a/wifi/1.0/default/wifi_legacy_hal.cpp +++ b/wifi/1.0/default/wifi_legacy_hal.cpp @@ -22,12 +22,7 @@ #include "wifi_legacy_hal.h" #include "wifi_legacy_hal_stubs.h" -namespace android { -namespace hardware { -namespace wifi { -namespace V1_0 { -namespace implementation { -namespace legacy_hal { +namespace { // Constants ported over from the legacy HAL calling code // (com_android_server_wifi_WifiNative.cpp). This will all be thrown // away when this shim layer is replaced by the real vendor @@ -39,6 +34,21 @@ static constexpr uint32_t kLinkLayerStatsDataMpduSizeThreshold = 128; static constexpr uint32_t kMaxWakeReasonStatsArraySize = 32; static constexpr uint32_t kMaxRingBuffers = 10; +// Helper function to create a non-const char* for legacy Hal API's. +std::vector makeCharVec(const std::string& str) { + std::vector vec(str.size() + 1); + vec.assign(str.begin(), str.end()); + vec.push_back('\0'); + return vec; +} +} // namespace + +namespace android { +namespace hardware { +namespace wifi { +namespace V1_0 { +namespace implementation { +namespace legacy_hal { // Legacy HAL functions accept "C" style function pointers, so use global // functions to pass to the legacy HAL function and store the corresponding // std::function methods to be invoked. @@ -804,19 +814,17 @@ wifi_error WifiLegacyHal::startRingBufferLogging(const std::string& ring_name, uint32_t verbose_level, uint32_t max_interval_sec, uint32_t min_data_size) { - std::vector ring_name_internal(ring_name.begin(), ring_name.end()); return global_func_table_.wifi_start_logging(wlan_interface_handle_, verbose_level, 0, max_interval_sec, min_data_size, - ring_name_internal.data()); + makeCharVec(ring_name).data()); } wifi_error WifiLegacyHal::getRingBufferData(const std::string& ring_name) { - std::vector ring_name_internal(ring_name.begin(), ring_name.end()); return global_func_table_.wifi_get_ring_data(wlan_interface_handle_, - ring_name_internal.data()); + makeCharVec(ring_name).data()); } wifi_error WifiLegacyHal::registerErrorAlertCallbackHandler( @@ -1091,16 +1099,14 @@ wifi_error WifiLegacyHal::nanGetCapabilities(transaction_id id) { wifi_error WifiLegacyHal::nanDataInterfaceCreate( transaction_id id, const std::string& iface_name) { - std::vector iface_name_internal(iface_name.begin(), iface_name.end()); return global_func_table_.wifi_nan_data_interface_create( - id, wlan_interface_handle_, iface_name_internal.data()); + id, wlan_interface_handle_, makeCharVec(iface_name).data()); } wifi_error WifiLegacyHal::nanDataInterfaceDelete( transaction_id id, const std::string& iface_name) { - std::vector iface_name_internal(iface_name.begin(), iface_name.end()); return global_func_table_.wifi_nan_data_interface_delete( - id, wlan_interface_handle_, iface_name_internal.data()); + id, wlan_interface_handle_, makeCharVec(iface_name).data()); } wifi_error WifiLegacyHal::nanDataRequestInitiator( @@ -1124,6 +1130,12 @@ wifi_error WifiLegacyHal::nanDataEnd(transaction_id id, id, wlan_interface_handle_, &msg_internal); } +wifi_error WifiLegacyHal::setCountryCode(std::array code) { + std::string code_str(code.data(), code.data() + code.size()); + return global_func_table_.wifi_set_country_code(wlan_interface_handle_, + code_str.c_str()); +} + wifi_error WifiLegacyHal::retrieveWlanInterfaceHandle() { const std::string& ifname_to_find = getStaIfaceName(); wifi_interface_handle* iface_handles = nullptr; diff --git a/wifi/1.0/default/wifi_legacy_hal.h b/wifi/1.0/default/wifi_legacy_hal.h index a3ac07533e..1ab74b7a15 100644 --- a/wifi/1.0/default/wifi_legacy_hal.h +++ b/wifi/1.0/default/wifi_legacy_hal.h @@ -256,6 +256,8 @@ class WifiLegacyHal { wifi_error nanDataIndicationResponse( transaction_id id, const NanDataPathIndicationResponse& msg); wifi_error nanDataEnd(transaction_id id, const NanDataPathEndRequest& msg); + // AP functions. + wifi_error setCountryCode(std::array code); private: // Retrieve the interface handle to be used for the "wlan" interface. diff --git a/wifi/1.0/default/wifi_sta_iface.cpp b/wifi/1.0/default/wifi_sta_iface.cpp index a00c5bcb89..be2fe37f1a 100644 --- a/wifi/1.0/default/wifi_sta_iface.cpp +++ b/wifi/1.0/default/wifi_sta_iface.cpp @@ -258,14 +258,6 @@ Return WifiStaIface::startDebugPacketFateMonitoring( hidl_status_cb); } -Return WifiStaIface::stopDebugPacketFateMonitoring( - stopDebugPacketFateMonitoring_cb hidl_status_cb) { - return validateAndCall(this, - WifiStatusCode::ERROR_WIFI_IFACE_INVALID, - &WifiStaIface::stopDebugPacketFateMonitoringInternal, - hidl_status_cb); -} - Return WifiStaIface::getDebugTxPacketFates( getDebugTxPacketFates_cb hidl_status_cb) { return validateAndCall(this, @@ -567,11 +559,6 @@ WifiStatus WifiStaIface::startDebugPacketFateMonitoringInternal() { return createWifiStatusFromLegacyError(legacy_status); } -WifiStatus WifiStaIface::stopDebugPacketFateMonitoringInternal() { - // There is no stop in legacy HAL implementation. - return createWifiStatus(WifiStatusCode::ERROR_NOT_SUPPORTED); -} - std::pair> WifiStaIface::getDebugTxPacketFatesInternal() { legacy_hal::wifi_error legacy_status; diff --git a/wifi/1.0/default/wifi_sta_iface.h b/wifi/1.0/default/wifi_sta_iface.h index 311c991a30..ca79c5bdaf 100644 --- a/wifi/1.0/default/wifi_sta_iface.h +++ b/wifi/1.0/default/wifi_sta_iface.h @@ -97,8 +97,6 @@ class WifiStaIface : public IWifiStaIface { uint32_t cmd_id, stopSendingKeepAlivePackets_cb hidl_status_cb) override; Return startDebugPacketFateMonitoring( startDebugPacketFateMonitoring_cb hidl_status_cb) override; - Return stopDebugPacketFateMonitoring( - stopDebugPacketFateMonitoring_cb hidl_status_cb) override; Return getDebugTxPacketFates( getDebugTxPacketFates_cb hidl_status_cb) override; Return getDebugRxPacketFates( @@ -143,7 +141,6 @@ class WifiStaIface : public IWifiStaIface { uint32_t period_in_ms); WifiStatus stopSendingKeepAlivePacketsInternal(uint32_t cmd_id); WifiStatus startDebugPacketFateMonitoringInternal(); - WifiStatus stopDebugPacketFateMonitoringInternal(); std::pair> getDebugTxPacketFatesInternal(); std::pair> diff --git a/wifi/1.0/default/wifi_status_util.cpp b/wifi/1.0/default/wifi_status_util.cpp index 518032f343..c2d0758cbb 100644 --- a/wifi/1.0/default/wifi_status_util.cpp +++ b/wifi/1.0/default/wifi_status_util.cpp @@ -42,8 +42,9 @@ std::string legacyErrorToString(legacy_hal::wifi_error error) { return "TOO_MANY_REQUESTS"; case legacy_hal::WIFI_ERROR_OUT_OF_MEMORY: return "OUT_OF_MEMORY"; + case legacy_hal::WIFI_ERROR_BUSY: + return "BUSY"; case legacy_hal::WIFI_ERROR_UNKNOWN: - default: return "UNKNOWN"; } } @@ -90,7 +91,6 @@ WifiStatus createWifiStatusFromLegacyError(legacy_hal::wifi_error error, return createWifiStatus(WifiStatusCode::SUCCESS, desc); case legacy_hal::WIFI_ERROR_UNKNOWN: - default: return createWifiStatus(WifiStatusCode::ERROR_UNKNOWN, "unknown"); } } diff --git a/wifi/1.0/types.hal b/wifi/1.0/types.hal index edf306d4c4..bb00114e9b 100644 --- a/wifi/1.0/types.hal +++ b/wifi/1.0/types.hal @@ -548,15 +548,15 @@ struct StaRoamingConfig { * control to. */ enum StaRoamingState : uint8_t { + /** + * Driver/Firmware must not perform any roaming. + */ + DISABLED = 0, /** * Driver/Firmware is allowed to perform roaming respecting * the |StaRoamingConfig| parameters set using |configureRoaming|. */ - ENABLED = 0, - /** - * Driver/Firmware must not perform any roaming. - */ - DISABLED = 1 + ENABLED = 1 }; /** @@ -2142,7 +2142,7 @@ struct WifiDebugPacketFateFrameInfo { }; /** - * Struct describing packet fate report for each Rx frame. + * Struct describing packet fate report for each Tx frame. */ struct WifiDebugTxPacketFateReport { WifiDebugTxPacketFate fate;