From 0b32c8d8c1ad2e85d21d00fcd4c270e1ee7f8e25 Mon Sep 17 00:00:00 2001 From: "Jorge E. Moreira" Date: Mon, 8 Aug 2016 11:39:52 -0700 Subject: [PATCH] Refactor vendor library to use async manager class Homogenizes the use of lambdas/binds (uses c++ lambdas only) Uses STL's time library implementation (std::chrono) Change-Id: I6194b26c0a7fabffddf96acb5c2379ff52026a84 --- .../test_vendor_lib/include/dual_mode_controller.h | 6 +- .../test_vendor_lib/include/hci_transport.h | 71 ++++++++------------- .../include/test_channel_transport.h | 10 +-- .../test_vendor_lib/include/vendor_manager.h | 31 ++-------- .../test_vendor_lib/src/dual_mode_controller.cc | 9 ++- vendor_libs/test_vendor_lib/src/hci_transport.cc | 72 +++++++--------------- .../test_vendor_lib/src/test_channel_transport.cc | 2 - vendor_libs/test_vendor_lib/src/vendor_manager.cc | 71 ++++++++------------- .../test_vendor_lib/test/hci_transport_unittest.cc | 61 +++++++++--------- 9 files changed, 120 insertions(+), 213 deletions(-) diff --git a/vendor_libs/test_vendor_lib/include/dual_mode_controller.h b/vendor_libs/test_vendor_lib/include/dual_mode_controller.h index ae7c50191..b3884e6c2 100644 --- a/vendor_libs/test_vendor_lib/include/dual_mode_controller.h +++ b/vendor_libs/test_vendor_lib/include/dual_mode_controller.h @@ -188,8 +188,8 @@ class DualModeController { std::function)> send_event); void RegisterDelayedEventChannel( - std::function, base::TimeDelta)> - send_event); + std::function, + std::chrono::milliseconds)> send_event); // Controller commands. For error codes, see the Bluetooth Core Specification, // Version 4.2, Volume 2, Part D (page 370). @@ -480,7 +480,7 @@ class DualModeController { // Callback provided to send events from the controller back to the HCI. std::function)> send_event_; - std::function, base::TimeDelta)> + std::function, std::chrono::milliseconds)> send_delayed_event_; // Maintains the commands to be registered and used in the HciHandler object. diff --git a/vendor_libs/test_vendor_lib/include/hci_transport.h b/vendor_libs/test_vendor_lib/include/hci_transport.h index 3583e18d4..e90f5a0e0 100644 --- a/vendor_libs/test_vendor_lib/include/hci_transport.h +++ b/vendor_libs/test_vendor_lib/include/hci_transport.h @@ -24,9 +24,8 @@ extern "C" { #include } // extern "C" +#include "async_manager.h" #include "base/files/scoped_file.h" -#include "base/memory/weak_ptr.h" -#include "base/message_loop/message_loop.h" #include "base/time/time.h" #include "command_packet.h" #include "event_packet.h" @@ -37,7 +36,7 @@ namespace test_vendor_lib { // Manages communication channel between HCI and the controller by providing the // socketing mechanisms for reading/writing between the HCI and the controller. -class HciTransport : public base::MessageLoopForIO::Watcher { +class HciTransport { public: HciTransport(); @@ -60,59 +59,45 @@ class HciTransport : public base::MessageLoopForIO::Watcher { void RegisterCommandHandler( std::function)> callback); + // Sets the callback that is to schedule events. + void RegisterEventScheduler( + std::function + evtScheduler); + + // Sets the callback that is to schedule events. + void RegisterPeriodicEventScheduler( + std::function periodicEvtScheduler); + // Posts the event onto |outbound_events_| to be written sometime in the // future when the vendor file descriptor is ready for writing. - void PostEventResponse(std::unique_ptr event); + void PostEventResponse(const EventPacket& event); // Posts the event onto |outbound_events_| after |delay| ms. A call to // |PostEventResponse| with |delay| 0 is equivalent to a call to |PostEvent|. - void PostDelayedEventResponse(std::unique_ptr event, - base::TimeDelta delay); - - private: - // Wrapper class for sending events on a delay. The TimeStampedEvent object - // takes ownership of a given event packet. - class TimeStampedEvent { - public: - TimeStampedEvent(std::unique_ptr event, base::TimeDelta delay); - - // Using this constructor is equivalent to calling the 2-argument - // constructor with a |delay| of 0. It is used to generate event responses - // with no delay. - explicit TimeStampedEvent(std::unique_ptr event); - - const base::TimeTicks& GetTimeStamp() const; - - const EventPacket& GetEvent(); - - private: - std::shared_ptr event_; + void PostDelayedEventResponse(const EventPacket& event, + std::chrono::milliseconds delay); - // The time associated with the event, indicating the earliest time at which - // |event_| will be sent. - base::TimeTicks time_stamp_; - }; - - // base::MessageLoopForIO::Watcher overrides: - void OnFileCanReadWithoutBlocking(int fd) override; - - void OnFileCanWriteWithoutBlocking(int fd) override; + void OnFileCanReadWithoutBlocking(int fd); + private: // Reads in a command packet and calls the command ready callback, // |command_handler_|, passing ownership of the command packet to the handler. void ReceiveReadyCommand() const; - void AddEventToOutboundEvents(std::unique_ptr event); - - // Write queue for sending events to the HCI. Event packets are removed from - // the queue and written when write-readiness is signalled by the message - // loop. After being written, the event packets are destructed. - std::list> outbound_events_; - // Callback executed in ReceiveReadyCommand() to pass the incoming command // over to the handler for further processing. std::function)> command_handler_; + // Callbacks to schedule events. + std::function + schedule_event_; + std::function + schedule_periodic_event_; + // For performing packet-based IO. PacketStream packet_stream_; @@ -124,10 +109,6 @@ class HciTransport : public base::MessageLoopForIO::Watcher { std::unique_ptr hci_fd_; std::unique_ptr vendor_fd_; - // This should remain the last member so it'll be destroyed and invalidate - // its weak pointers before any other members are destroyed. - base::WeakPtrFactory weak_ptr_factory_; - HciTransport(const HciTransport& cmdPckt) = delete; HciTransport& operator=(const HciTransport& cmdPckt) = delete; }; diff --git a/vendor_libs/test_vendor_lib/include/test_channel_transport.h b/vendor_libs/test_vendor_lib/include/test_channel_transport.h index 408cf6cca..0460cbdb4 100644 --- a/vendor_libs/test_vendor_lib/include/test_channel_transport.h +++ b/vendor_libs/test_vendor_lib/include/test_channel_transport.h @@ -23,13 +23,12 @@ using std::vector; #include "base/files/scoped_file.h" -#include "base/message_loop/message_loop.h" namespace test_vendor_lib { // Manages communications between test channel and the controller. Mirrors the // HciTransport for the test channel. -class TestChannelTransport : public base::MessageLoopForIO::Watcher { +class TestChannelTransport { public: TestChannelTransport(bool enabled, int port); @@ -58,12 +57,9 @@ class TestChannelTransport : public base::MessageLoopForIO::Watcher { std::function&)> callback); - private: - // base::MessageLoopForIO::Watcher overrides: - void OnFileCanReadWithoutBlocking(int fd) override; - - void OnFileCanWriteWithoutBlocking(int fd) override; + void OnFileCanReadWithoutBlocking(int fd); + private: std::function&)> command_handler_; diff --git a/vendor_libs/test_vendor_lib/include/vendor_manager.h b/vendor_libs/test_vendor_lib/include/vendor_manager.h index adbb24fa5..74c85e6e8 100644 --- a/vendor_libs/test_vendor_lib/include/vendor_manager.h +++ b/vendor_libs/test_vendor_lib/include/vendor_manager.h @@ -14,9 +14,10 @@ // limitations under the License. // +#pragma once + +#include "async_manager.h" #include "base/macros.h" -#include "base/memory/weak_ptr.h" -#include "base/threading/thread.h" #include "base/time/time.h" #include "dual_mode_controller.h" #include "event_packet.h" @@ -68,13 +69,6 @@ class VendorManager { ~VendorManager() = default; - // Posts a callback to |thread_|'s task runner. Equivalent to calling - // |PostDelayedTask| with a delay of 0. - bool PostTask(const base::Closure& task); - - // Posts a callback to be run after |delay| ms (or longer) have passed. - bool PostDelayedTask(const base::Closure& task, base::TimeDelta delay); - // Starts watching for incoming data from the HCI and the test hook. void StartWatchingOnThread(); @@ -95,22 +89,9 @@ class VendorManager { // True if the underlying message loop (in |thread_|) is running. bool running_; - // Dedicated thread for managing the message loop to receive and send packets - // from the HCI and to receive additional parameters from the test hook file - // descriptor. - base::Thread thread_; - - // Used to handle further watching of the vendor's/test channel's file - // descriptor after WatchFileDescriptor() is called. - base::MessageLoopForIO::FileDescriptorWatcher hci_watcher_; - - // Used to handle further watching of the test channel's file descriptor after - // WatchFileDescriptor() is called. - base::MessageLoopForIO::FileDescriptorWatcher test_channel_watcher_; - - // This should remain the last member so it'll be destroyed and invalidate - // its weak pointers before any other members are destroyed. - base::WeakPtrFactory weak_ptr_factory_; + // The object that manages asynchronous tasks such as watching a file + // descriptor or doing something in the future + AsyncManager async_manager_; VendorManager(const VendorManager& cmdPckt) = delete; VendorManager& operator=(const VendorManager& cmdPckt) = delete; diff --git a/vendor_libs/test_vendor_lib/src/dual_mode_controller.cc b/vendor_libs/test_vendor_lib/src/dual_mode_controller.cc index 97b6cec82..d48cb485a 100644 --- a/vendor_libs/test_vendor_lib/src/dual_mode_controller.cc +++ b/vendor_libs/test_vendor_lib/src/dual_mode_controller.cc @@ -99,7 +99,7 @@ DualModeController::DualModeController() : state_(kStandby), properties_(kControllerPropertiesFile), test_channel_state_(kNone) { -#define SET_HANDLER(opcode, method) \ +#define SET_HANDLER(opcode, method) \ active_hci_commands_[opcode] = [this](const vector& param) { \ method(param); \ }; @@ -210,7 +210,7 @@ void DualModeController::RegisterEventChannel( } void DualModeController::RegisterDelayedEventChannel( - std::function, base::TimeDelta)> + std::function, std::chrono::milliseconds)> callback) { send_delayed_event_ = callback; SetEventDelay(0); @@ -220,8 +220,7 @@ void DualModeController::SetEventDelay(int64_t delay) { if (delay < 0) delay = 0; send_event_ = [this, delay](std::unique_ptr arg) { - send_delayed_event_(std::move(arg), - base::TimeDelta::FromMilliseconds(delay)); + send_delayed_event_(std::move(arg), std::chrono::milliseconds(delay)); }; } @@ -488,7 +487,7 @@ void DualModeController::HciInquiry(const vector& args) { } break; } send_delayed_event_(EventPacket::CreateInquiryCompleteEvent(kSuccessStatus), - base::TimeDelta::FromMilliseconds(args[4] * 1280)); + std::chrono::milliseconds(args[4] * 1280)); } void DualModeController::HciInquiryCancel(const vector& /* args */) { diff --git a/vendor_libs/test_vendor_lib/src/hci_transport.cc b/vendor_libs/test_vendor_lib/src/hci_transport.cc index 4aefb0884..d5015ea5d 100644 --- a/vendor_libs/test_vendor_lib/src/hci_transport.cc +++ b/vendor_libs/test_vendor_lib/src/hci_transport.cc @@ -20,9 +20,6 @@ #include "hci_transport.h" -#include "base/bind.h" -#include "base/threading/thread_task_runner_handle.h" - extern "C" { #include @@ -32,7 +29,7 @@ extern "C" { namespace test_vendor_lib { -HciTransport::HciTransport() : weak_ptr_factory_(this) {} +HciTransport::HciTransport() = default; void HciTransport::CloseHciFd() { hci_fd_.reset(nullptr); @@ -102,37 +99,27 @@ void HciTransport::RegisterCommandHandler( command_handler_ = callback; } -void HciTransport::OnFileCanWriteWithoutBlocking(int fd) { - CHECK(fd == GetVendorFd()); - if (!outbound_events_.empty()) { - base::TimeTicks current_time = base::TimeTicks::Now(); - // Check outbound events for events that can be sent, i.e. events with a - // timestamp before the current time. Stop sending events when - // |packet_stream_| fails writing. - for (auto it = outbound_events_.begin(); it != outbound_events_.end();) { - if ((*it)->GetTimeStamp() > current_time) { - ++it; - continue; - } - if (!packet_stream_.SendEvent((*it)->GetEvent(), fd)) - return; - it = outbound_events_.erase(it); - } - } +void HciTransport::RegisterEventScheduler( + std::function + evtScheduler) { + schedule_event_ = evtScheduler; } -void HciTransport::AddEventToOutboundEvents( - std::unique_ptr event) { - outbound_events_.push_back(std::move(event)); +void HciTransport::RegisterPeriodicEventScheduler( + std::function periodicEvtScheduler) { + schedule_periodic_event_ = periodicEvtScheduler; } -void HciTransport::PostEventResponse(std::unique_ptr event) { - AddEventToOutboundEvents( - std::make_unique(std::move(event))); +void HciTransport::PostEventResponse(const EventPacket& event) { + schedule_event_(std::chrono::milliseconds(0), [this, event]() { + packet_stream_.SendEvent(event, GetVendorFd()); + }); } -void HciTransport::PostDelayedEventResponse(std::unique_ptr event, - base::TimeDelta delay) { +void HciTransport::PostDelayedEventResponse(const EventPacket& event, + std::chrono::milliseconds delay) { // TODO(dennischeng): When it becomes available for MessageLoopForIO, use the // thread's task runner to post |PostEventResponse| as a delayed task, being // sure to CHECK the appropriate task runner attributes using @@ -145,31 +132,18 @@ void HciTransport::PostDelayedEventResponse(std::unique_ptr event, LOG_INFO(LOG_TAG, "System does not support high resolution timing. Sending event " "without delay."); - PostEventResponse(std::move(event)); + schedule_event_(std::chrono::milliseconds(0), [this, event]() { + packet_stream_.SendEvent(event, GetVendorFd()); + }); } LOG_INFO(LOG_TAG, "Posting event response with delay of %" PRId64 " ms.", - delay.InMilliseconds()); - - AddEventToOutboundEvents( - std::make_unique(std::move(event), delay)); -} - -HciTransport::TimeStampedEvent::TimeStampedEvent( - std::unique_ptr event, base::TimeDelta delay) - : event_(std::move(event)), time_stamp_(base::TimeTicks::Now() + delay) {} - -HciTransport::TimeStampedEvent::TimeStampedEvent( - std::unique_ptr event) - : event_(std::move(event)), time_stamp_(base::TimeTicks::UnixEpoch()) {} - -const base::TimeTicks& HciTransport::TimeStampedEvent::GetTimeStamp() const { - return time_stamp_; -} + static_cast(std::chrono::milliseconds(delay).count())); -const EventPacket& HciTransport::TimeStampedEvent::GetEvent() { - return *(event_.get()); + schedule_event_(delay, [this, event]() { + packet_stream_.SendEvent(event, GetVendorFd()); + }); } } // namespace test_vendor_lib diff --git a/vendor_libs/test_vendor_lib/src/test_channel_transport.cc b/vendor_libs/test_vendor_lib/src/test_channel_transport.cc index a1c969c07..2572c06a6 100644 --- a/vendor_libs/test_vendor_lib/src/test_channel_transport.cc +++ b/vendor_libs/test_vendor_lib/src/test_channel_transport.cc @@ -125,8 +125,6 @@ void TestChannelTransport::OnFileCanReadWithoutBlocking(int fd) { command_handler_(command_name, args); } -void TestChannelTransport::OnFileCanWriteWithoutBlocking(int fd UNUSED_ATTR) {} - void TestChannelTransport::RegisterCommandHandler( std::function&)> callback) { diff --git a/vendor_libs/test_vendor_lib/src/vendor_manager.cc b/vendor_libs/test_vendor_lib/src/vendor_manager.cc index 11ddc25ac..215baf750 100644 --- a/vendor_libs/test_vendor_lib/src/vendor_manager.cc +++ b/vendor_libs/test_vendor_lib/src/vendor_manager.cc @@ -18,7 +18,6 @@ #include "vendor_manager.h" -#include "base/bind.h" #include "base/logging.h" extern "C" { @@ -49,10 +48,7 @@ void VendorManager::Initialize() { } VendorManager::VendorManager() - : test_channel_transport_(true, 6111), - running_(false), - thread_("TestVendorLibrary"), - weak_ptr_factory_(this) {} + : test_channel_transport_(true, 6111), running_(false) {} bool VendorManager::Run() { CHECK(!running_); @@ -79,60 +75,47 @@ bool VendorManager::Run() { controller_.RegisterHandlersWithHciTransport(transport_); // TODO(dennischeng): Register PostDelayedEventResponse instead. - controller_.RegisterDelayedEventChannel( - [this](std::unique_ptr event, base::TimeDelta delay) { - transport_.PostDelayedEventResponse(std::move(event), delay); + controller_.RegisterDelayedEventChannel([this]( + std::unique_ptr event, std::chrono::milliseconds delay) { + transport_.PostDelayedEventResponse(*event, delay); + }); + + transport_.RegisterEventScheduler( + [this](std::chrono::milliseconds delay, const TaskCallback& task) { + async_manager_.ExecAsync(delay, task); }); - running_ = true; - if (!thread_.StartWithOptions( - base::Thread::Options(base::MessageLoop::TYPE_IO, 0))) { - LOG_ERROR(LOG_TAG, "Error starting TestVendorLibrary thread."); - running_ = false; - return false; - } + transport_.RegisterPeriodicEventScheduler( + [this](std::chrono::milliseconds delay, + std::chrono::milliseconds period, + const TaskCallback& task) { + async_manager_.ExecAsyncPeriodically(delay, period, task); + }); - if (!PostTask(base::Bind(&VendorManager::StartWatchingOnThread, - weak_ptr_factory_.GetWeakPtr()))) { - LOG_ERROR(LOG_TAG, "Error posting StartWatchingOnThread to task runner."); - running_ = false; - return false; - } + running_ = true; + StartWatchingOnThread(); return true; } void VendorManager::StartWatchingOnThread() { CHECK(running_); - CHECK(base::MessageLoopForIO::IsCurrent()); - - if (!base::MessageLoopForIO::current()->WatchFileDescriptor( - transport_.GetVendorFd(), - true, - base::MessageLoopForIO::WATCH_READ_WRITE, - &hci_watcher_, - &transport_)) { + + if (async_manager_.WatchFdForNonBlockingReads( + transport_.GetVendorFd(), [this](int fd) { + transport_.OnFileCanReadWithoutBlocking(fd); + }) != 0) { LOG_ERROR(LOG_TAG, "Error watching vendor fd."); return; } if (test_channel_transport_.IsEnabled()) - if (!base::MessageLoopForIO::current()->WatchFileDescriptor( - test_channel_transport_.GetFd(), - true, - base::MessageLoopForIO::WATCH_READ, - &test_channel_watcher_, - &test_channel_transport_)) + if (async_manager_.WatchFdForNonBlockingReads( + test_channel_transport_.GetFd(), [this](int fd) { + test_channel_transport_.OnFileCanReadWithoutBlocking(fd); + }) != 0) { LOG_ERROR(LOG_TAG, "Error watching test channel fd."); -} - -bool VendorManager::PostTask(const base::Closure& task) { - return PostDelayedTask(task, base::TimeDelta::FromMilliseconds(0)); -} - -bool VendorManager::PostDelayedTask(const base::Closure& task, - base::TimeDelta delay) { - return thread_.task_runner()->PostDelayedTask(FROM_HERE, task, delay); + } } void VendorManager::SetVendorCallbacks(const bt_vendor_callbacks_t& callbacks) { diff --git a/vendor_libs/test_vendor_lib/test/hci_transport_unittest.cc b/vendor_libs/test_vendor_lib/test/hci_transport_unittest.cc index 39c3a1a08..b9b7c63d0 100644 --- a/vendor_libs/test_vendor_lib/test/hci_transport_unittest.cc +++ b/vendor_libs/test_vendor_lib/test/hci_transport_unittest.cc @@ -17,13 +17,12 @@ #include "hci_transport.h" #include "command_packet.h" -#include "base/bind.h" -#include "base/message_loop/message_loop.h" -#include "base/threading/thread.h" +#include "async_manager.h" #include #include #include +#include extern "C" { #include "stack/include/hcidefs.h" @@ -47,13 +46,9 @@ namespace test_vendor_lib { class HciTransportTest : public ::testing::Test { public: - HciTransportTest() - : command_callback_count_(0), - thread_("HciTransportTest"), - weak_ptr_factory_(this) { + HciTransportTest() : command_callback_count_(0) { SetUpTransport(); StartThread(); - PostStartWatchingOnThread(); } ~HciTransportTest() { transport_.CloseHciFd(); } @@ -65,6 +60,7 @@ class HciTransportTest : public ::testing::Test { EXPECT_EQ(HCI_RESET, command->GetOpcode()); EXPECT_EQ(static_cast(1), command->GetPayloadSize()); transport_.CloseVendorFd(); + SignalCommandhandlerFinished(); } void MultiCommandCallback(std::unique_ptr command) { @@ -73,44 +69,43 @@ class HciTransportTest : public ::testing::Test { EXPECT_EQ(DATA_TYPE_COMMAND, command->GetType()); EXPECT_EQ(HCI_RESET, command->GetOpcode()); EXPECT_EQ(static_cast(1), command->GetPayloadSize()); - if (command_callback_count_ == kMultiIterations) + if (command_callback_count_ == kMultiIterations) { transport_.CloseVendorFd(); + SignalCommandhandlerFinished(); + } } protected: // Tracks the number of commands received. int command_callback_count_; - base::Thread thread_; + AsyncManager async_manager_; HciTransport transport_; - base::MessageLoopForIO::FileDescriptorWatcher watcher_; - base::WeakPtrFactory weak_ptr_factory_; + bool command_handler_finished_ = false; + std::mutex mutex_; + std::condition_variable cond_var_; + + void WaitCommandhandlerFinish() { + std::unique_lock lock(mutex_); + while (!command_handler_finished_) { + cond_var_.wait(lock); + } + } private: // Workaround because ASSERT cannot be used directly in a constructor void SetUpTransport() { ASSERT_TRUE(transport_.SetUp()); } void StartThread() { - ASSERT_TRUE(thread_.StartWithOptions( - base::Thread::Options(base::MessageLoop::TYPE_IO, 0))); - } - - void PostStartWatchingOnThread() { - thread_.task_runner()->PostTask( - FROM_HERE, - base::Bind(&HciTransportTest::StartWatchingOnThread, - weak_ptr_factory_.GetWeakPtr())); + ASSERT_TRUE(async_manager_.WatchFdForNonBlockingReads( + transport_.GetVendorFd(), [this](int fd) { + transport_.OnFileCanReadWithoutBlocking(fd); + }) == 0); } - void StartWatchingOnThread() { - base::MessageLoopForIO* loop = - static_cast(thread_.message_loop()); - ASSERT_TRUE(loop); - ASSERT_TRUE( - loop->WatchFileDescriptor(transport_.GetVendorFd(), - true, - base::MessageLoopForIO::WATCH_READ_WRITE, - &watcher_, - &transport_)); + void SignalCommandhandlerFinished() { + std::unique_lock lock(mutex_); + command_handler_finished_ = true; + cond_var_.notify_one(); } }; @@ -121,7 +116,7 @@ TEST_F(HciTransportTest, SingleCommandCallback) { }); EXPECT_EQ(0, command_callback_count_); WriteStubCommand(transport_.GetHciFd()); - thread_.Stop(); // Wait for the command handler to finish. + WaitCommandhandlerFinish(); EXPECT_EQ(1, command_callback_count_); } @@ -134,7 +129,7 @@ TEST_F(HciTransportTest, MultiCommandCallback) { WriteStubCommand(transport_.GetHciFd()); for (int i = 1; i < kMultiIterations; ++i) WriteStubCommand(transport_.GetHciFd()); - thread_.Stop(); // Wait for the command handler to finish. + WaitCommandhandlerFinish(); EXPECT_EQ(kMultiIterations, command_callback_count_); } -- 2.11.0