Send correct key state and ignore keys from inactive device (1/2)
Also make it clearer whether the key state is pushed or released.
Bug: 79178216
Test: Run host native test net_test_avrcp
Change-Id: I8595ac97718bcb2af8f33643df4b9c8059a1b30b
(cherry picked from commit c4546f592b943463337a260ccbbe92546832200d)
diff --git a/btif/avrcp/avrcp_service.cc b/btif/avrcp/avrcp_service.cc
index 2508354..4338722 100644
--- a/btif/avrcp/avrcp_service.cc
+++ b/btif/avrcp/avrcp_service.cc
@@ -115,9 +115,9 @@
public:
MediaInterfaceWrapper(MediaInterface* cb) : wrapped_(cb){};
- void SendKeyEvent(uint8_t key, uint8_t status) override {
+ void SendKeyEvent(uint8_t key, KeyState state) override {
do_in_jni_thread(base::Bind(&MediaInterface::SendKeyEvent,
- base::Unretained(wrapped_), key, status));
+ base::Unretained(wrapped_), key, state));
}
void GetSongInfo(SongInfoCallback info_cb) override {
diff --git a/include/hardware/avrcp/avrcp.h b/include/hardware/avrcp/avrcp.h
index dbd0ad9..8adba47 100644
--- a/include/hardware/avrcp/avrcp.h
+++ b/include/hardware/avrcp/avrcp.h
@@ -101,7 +101,7 @@
// behavior in case the threading model changes on either side.
class MediaInterface {
public:
- virtual void SendKeyEvent(uint8_t key, uint8_t status) = 0;
+ virtual void SendKeyEvent(uint8_t key, KeyState state) = 0;
using SongInfoCallback = base::Callback<void(SongInfo)>;
virtual void GetSongInfo(SongInfoCallback info_cb) = 0;
diff --git a/include/hardware/avrcp/avrcp_common.h b/include/hardware/avrcp/avrcp_common.h
index c805aa5..685eb1a 100644
--- a/include/hardware/avrcp/avrcp_common.h
+++ b/include/hardware/avrcp/avrcp_common.h
@@ -137,6 +137,11 @@
DOWN = 0x01,
};
+enum class KeyState : uint8_t {
+ PUSHED = 0x00,
+ RELEASED = 0x01,
+};
+
class AttributeEntry {
public:
AttributeEntry(const Attribute& attribute, const std::string& value)
diff --git a/include/hardware/avrcp/avrcp_logging_helper.h b/include/hardware/avrcp/avrcp_logging_helper.h
index ce4951b..ce95337 100644
--- a/include/hardware/avrcp/avrcp_logging_helper.h
+++ b/include/hardware/avrcp/avrcp_logging_helper.h
@@ -226,5 +226,18 @@
return os << DirectionText(dir);
}
+inline std::string KeyStateText(const KeyState& state) {
+ switch (state) {
+ CASE_RETURN_TEXT(KeyState::PUSHED);
+ CASE_RETURN_TEXT(KeyState::RELEASED);
+ default:
+ return "Unknown KeyState: " + loghex((uint8_t)state);
+ }
+}
+
+inline std::ostream& operator<<(std::ostream& os, const KeyState& dir) {
+ return os << KeyStateText(dir);
+}
+
} // namespace avrcp
} // namespace bluetooth
diff --git a/packet/avrcp/pass_through_packet.cc b/packet/avrcp/pass_through_packet.cc
index a384cc0..67b8d16 100644
--- a/packet/avrcp/pass_through_packet.cc
+++ b/packet/avrcp/pass_through_packet.cc
@@ -46,8 +46,9 @@
return true;
}
-bool PassThroughPacket::GetPushed() const {
- return (*(begin() + Packet::kMinSize()) & 0b10000000) == 0;
+KeyState PassThroughPacket::GetKeyState() const {
+ auto it = begin() + Packet::kMinSize();
+ return static_cast<KeyState>(((*it) & 0b10000000) >> 7);
}
uint8_t PassThroughPacket::GetOperationId() const {
@@ -63,7 +64,7 @@
ss << " └ Subunit Type = " << loghex(GetSubunitType()) << std::endl;
ss << " └ Subunit ID = " << loghex(GetSubunitId()) << std::endl;
ss << " └ OpCode = " << GetOpcode() << std::endl;
- ss << " └ Pushed = " << GetPushed() << std::endl;
+ ss << " └ Pushed = " << GetKeyState() << std::endl;
ss << " └ Opperation ID = " << loghex(GetOperationId()) << std::endl;
return ss.str();
diff --git a/packet/avrcp/pass_through_packet.h b/packet/avrcp/pass_through_packet.h
index f2cf1d7..903b3d8 100644
--- a/packet/avrcp/pass_through_packet.h
+++ b/packet/avrcp/pass_through_packet.h
@@ -62,7 +62,7 @@
static constexpr size_t kMinSize() { return Packet::kMinSize() + 2; }
// Getter Functions
- bool GetPushed() const;
+ KeyState GetKeyState() const;
uint8_t GetOperationId() const;
// Overloaded Functions
diff --git a/packet/tests/avrcp/pass_through_packet_test.cc b/packet/tests/avrcp/pass_through_packet_test.cc
index 0ef2879..5ea749f 100644
--- a/packet/tests/avrcp/pass_through_packet_test.cc
+++ b/packet/tests/avrcp/pass_through_packet_test.cc
@@ -42,11 +42,11 @@
TEST(PassThroughPacketTest, getterTest) {
auto test_packet =
TestPassThroughPacket::Make(pass_through_command_play_pushed);
- ASSERT_TRUE(test_packet->GetPushed());
+ ASSERT_EQ(test_packet->GetKeyState(), KeyState::PUSHED);
ASSERT_EQ(test_packet->GetOperationId(), 0x44);
test_packet = TestPassThroughPacket::Make(pass_through_command_play_released);
- ASSERT_FALSE(test_packet->GetPushed());
+ ASSERT_EQ(test_packet->GetKeyState(), KeyState::RELEASED);
ASSERT_EQ(test_packet->GetOperationId(), 0x44);
}
diff --git a/profile/avrcp/device.cc b/profile/avrcp/device.cc
index 6c442d4..aa5cc9c 100644
--- a/profile/avrcp/device.cc
+++ b/profile/avrcp/device.cc
@@ -491,14 +491,13 @@
case Opcode::PASS_THROUGH: {
auto pass_through_packet = Packet::Specialize<PassThroughPacket>(pkt);
auto response = PassThroughPacketBuilder::MakeBuilder(
- true, pass_through_packet->GetPushed(),
+ true, pass_through_packet->GetKeyState() == KeyState::PUSHED,
pass_through_packet->GetOperationId());
send_message(label, false, std::move(response));
- // TODO (apanicke): Use an enum for media key ID's also handle
- // other keys like forward and back for device switching.
+ // TODO (apanicke): Use an enum for media key ID's
if (pass_through_packet->GetOperationId() == 0x44 &&
- !pass_through_packet->GetPushed()) {
+ pass_through_packet->GetKeyState() == KeyState::PUSHED) {
// We need to get the play status since we need to know
// what the actual playstate is without being modified
// by whether the device is active.
@@ -516,14 +515,16 @@
}
}
- d->media_interface_->SendKeyEvent(0x44, 0);
+ d->media_interface_->SendKeyEvent(0x44, KeyState::PUSHED);
},
base::Unretained(this)));
return;
}
- media_interface_->SendKeyEvent(pass_through_packet->GetOperationId(),
- pass_through_packet->GetPushed() ? 0 : 1);
+ if (IsActive()) {
+ media_interface_->SendKeyEvent(pass_through_packet->GetOperationId(),
+ pass_through_packet->GetKeyState());
+ }
} break;
case Opcode::VENDOR: {
auto vendor_pkt = Packet::Specialize<VendorPacket>(pkt);
diff --git a/profile/avrcp/tests/avrcp_device_test.cc b/profile/avrcp/tests/avrcp_device_test.cc
index b005ec4..6e3bb17 100644
--- a/profile/avrcp/tests/avrcp_device_test.cc
+++ b/profile/avrcp/tests/avrcp_device_test.cc
@@ -762,7 +762,7 @@
test_device->RegisterInterfaces(&interface, &a2dp_interface, &vol_interface);
- // Pretend the device is active
+ // Pretend the device isn't active
EXPECT_CALL(a2dp_interface, active_peer())
.WillRepeatedly(Return(RawAddress::kEmpty));
@@ -814,5 +814,129 @@
EXPECT_CALL(response_cb, Call(_, _, _)).Times(0);
}
+TEST_F(AvrcpDeviceTest, playPushedActiveDeviceTest) {
+ MockMediaInterface interface;
+ NiceMock<MockA2dpInterface> a2dp_interface;
+ MockVolumeInterface vol_interface;
+
+ test_device->RegisterInterfaces(&interface, &a2dp_interface, &vol_interface);
+
+ // Pretend the device is active
+ EXPECT_CALL(a2dp_interface, active_peer())
+ .WillRepeatedly(Return(test_device->GetAddress()));
+
+ auto play_pushed = PassThroughPacketBuilder::MakeBuilder(false, true, 0x44);
+ auto play_pushed_response =
+ PassThroughPacketBuilder::MakeBuilder(true, true, 0x44);
+ EXPECT_CALL(response_cb,
+ Call(_, false, matchPacket(std::move(play_pushed_response))))
+ .Times(1);
+
+ PlayStatus status = {0x1234, 0x5678, PlayState::PLAYING};
+ EXPECT_CALL(interface, GetPlayStatus(_))
+ .Times(1)
+ .WillOnce(InvokeCb<0>(status));
+
+ EXPECT_CALL(interface, SendKeyEvent(0x44, KeyState::PUSHED)).Times(1);
+
+ auto play_pushed_pkt = TestAvrcpPacket::Make();
+ play_pushed->Serialize(play_pushed_pkt);
+
+ SendMessage(1, play_pushed_pkt);
+}
+
+TEST_F(AvrcpDeviceTest, playPushedInactiveDeviceTest) {
+ MockMediaInterface interface;
+ NiceMock<MockA2dpInterface> a2dp_interface;
+ MockVolumeInterface vol_interface;
+
+ test_device->RegisterInterfaces(&interface, &a2dp_interface, &vol_interface);
+
+ // Pretend the device is not active
+ EXPECT_CALL(a2dp_interface, active_peer())
+ .WillRepeatedly(Return(RawAddress::kEmpty));
+
+ auto play_pushed = PassThroughPacketBuilder::MakeBuilder(false, true, 0x44);
+ auto play_pushed_response =
+ PassThroughPacketBuilder::MakeBuilder(true, true, 0x44);
+ EXPECT_CALL(response_cb,
+ Call(_, false, matchPacket(std::move(play_pushed_response))))
+ .Times(1);
+
+ // Expect that the device will try to set itself as active
+ EXPECT_CALL(interface, SetActiveDevice(test_device->GetAddress())).Times(1);
+
+ // No play command should be sent since the music is already playing
+ PlayStatus status = {0x1234, 0x5678, PlayState::PLAYING};
+ EXPECT_CALL(interface, GetPlayStatus(_))
+ .Times(1)
+ .WillOnce(InvokeCb<0>(status));
+ EXPECT_CALL(interface, SendKeyEvent(0x44, KeyState::PUSHED)).Times(0);
+
+ auto play_pushed_pkt = TestAvrcpPacket::Make();
+ play_pushed->Serialize(play_pushed_pkt);
+
+ SendMessage(1, play_pushed_pkt);
+}
+
+TEST_F(AvrcpDeviceTest, mediaKeyActiveDeviceTest) {
+ MockMediaInterface interface;
+ NiceMock<MockA2dpInterface> a2dp_interface;
+ MockVolumeInterface vol_interface;
+
+ test_device->RegisterInterfaces(&interface, &a2dp_interface, &vol_interface);
+
+ // Pretend the device is active
+ EXPECT_CALL(a2dp_interface, active_peer())
+ .WillRepeatedly(Return(test_device->GetAddress()));
+
+ auto play_released =
+ PassThroughPacketBuilder::MakeBuilder(false, false, 0x44);
+ auto play_released_response =
+ PassThroughPacketBuilder::MakeBuilder(true, false, 0x44);
+ EXPECT_CALL(response_cb,
+ Call(_, false, matchPacket(std::move(play_released_response))))
+ .Times(1);
+
+ EXPECT_CALL(interface, GetPlayStatus(_)).Times(0);
+
+ EXPECT_CALL(interface, SendKeyEvent(0x44, KeyState::RELEASED)).Times(1);
+
+ auto play_released_pkt = TestAvrcpPacket::Make();
+ play_released->Serialize(play_released_pkt);
+
+ SendMessage(1, play_released_pkt);
+}
+
+TEST_F(AvrcpDeviceTest, mediaKeyInactiveDeviceTest) {
+ MockMediaInterface interface;
+ NiceMock<MockA2dpInterface> a2dp_interface;
+ MockVolumeInterface vol_interface;
+
+ test_device->RegisterInterfaces(&interface, &a2dp_interface, &vol_interface);
+
+ // Pretend the device is not active
+ EXPECT_CALL(a2dp_interface, active_peer())
+ .WillRepeatedly(Return(RawAddress::kEmpty));
+
+ auto play_released =
+ PassThroughPacketBuilder::MakeBuilder(false, false, 0x44);
+ auto play_released_response =
+ PassThroughPacketBuilder::MakeBuilder(true, false, 0x44);
+ EXPECT_CALL(response_cb,
+ Call(_, false, matchPacket(std::move(play_released_response))))
+ .Times(1);
+
+ EXPECT_CALL(interface, GetPlayStatus(_)).Times(0);
+
+ // Expect that the key event wont be sent to the media interface
+ EXPECT_CALL(interface, SendKeyEvent(0x44, KeyState::RELEASED)).Times(0);
+
+ auto play_released_pkt = TestAvrcpPacket::Make();
+ play_released->Serialize(play_released_pkt);
+
+ SendMessage(1, play_released_pkt);
+}
+
} // namespace avrcp
} // namespace bluetooth
\ No newline at end of file
diff --git a/profile/avrcp/tests/avrcp_test_helper.h b/profile/avrcp/tests/avrcp_test_helper.h
index 594a0ae..16fc31f 100644
--- a/profile/avrcp/tests/avrcp_test_helper.h
+++ b/profile/avrcp/tests/avrcp_test_helper.h
@@ -33,7 +33,7 @@
class MockMediaInterface : public MediaInterface {
public:
- MOCK_METHOD2(SendKeyEvent, void(uint8_t, uint8_t));
+ MOCK_METHOD2(SendKeyEvent, void(uint8_t, KeyState));
MOCK_METHOD1(GetSongInfo, void(MediaInterface::SongInfoCallback));
MOCK_METHOD1(GetPlayStatus, void(MediaInterface::PlayStatusCallback));
MOCK_METHOD1(GetNowPlayingList, void(MediaInterface::NowPlayingCallback));