v0.4.0: community fixes, WH-1000XM6 support, and a new look

.gitignore

@@ -89,6 +89,7 @@ coverage.xml
 .hypothesis/
 .pytest_cache/
 pytestdebug.log
+*.actual
 
 # Translations
 *.mo
@@ -530,3 +531,5 @@ Client/imgui.ini
 
 # linux build
 Client/linux/sonyheadphonesclient.build/
+
+.superpowers/

Client/BluetoothWrapper.cpp

@@ -1,5 +1,10 @@
 #include "BluetoothWrapper.h"
 
+// How many frames one transaction will read before giving up. The device interleaves ACKs and
+// unsolicited notifications with the reply we're after, so a handful of unrelated frames is normal;
+// this only bounds a device that never sends what we asked for.
+static constexpr int MAX_FRAMES_PER_TRANSACTION = 16;
+
 BluetoothWrapper::BluetoothWrapper(std::unique_ptr<IBluetoothConnector> connector)
 {
 	this->_connector.swap(connector);
@@ -24,10 +29,10 @@ BluetoothWrapper& BluetoothWrapper::operator=(BluetoothWrapper&& other) noexcept
 	return *this;
 }
 
-int BluetoothWrapper::sendCommand(const std::vector<char>& bytes)
+int BluetoothWrapper::sendCommand(const std::vector<char>& bytes, DATA_TYPE dataType)
 {
 	std::lock_guard guard(this->_connectorMtx);
-	auto data = CommandSerializer::packageDataForBt(bytes, DATA_TYPE::DATA_MDR, this->_seqNumber++);
+	auto data = CommandSerializer::packageDataForBt(bytes, dataType, this->_seqNumber++);
 	auto bytesSent = this->_connector->send(data.data(), data.size());
 
 	this->_waitForAck();
@@ -48,10 +53,21 @@ void BluetoothWrapper::connect(const std::string& addr)
 
 void BluetoothWrapper::disconnect() noexcept
 {
+#if defined(_WIN32)
+	// Close the link BEFORE taking the lock. A worker thread can hold _connectorMtx while blocked in
+	// recv(), and closing the socket is exactly what wakes it up - taking the lock first deadlocks the
+	// UI thread until the recv timeout expires, and hangs the app on exit. The Windows connector's
+	// disconnect() is safe to call concurrently with an in-flight recv().
+	this->_connector->disconnect();
+	std::lock_guard guard(this->_connectorMtx);
+	this->_seqNumber = 0;
+	this->_leftoverBytes.clear();
+#else
 	std::lock_guard guard(this->_connectorMtx);
 	this->_seqNumber = 0;
 	this->_leftoverBytes.clear();
 	this->_connector->disconnect();
+#endif
 }
 
 
@@ -65,22 +81,23 @@ SonyProtocolVersion BluetoothWrapper::getProtocolVersion() noexcept
 	return this->_connector->getProtocolVersion();
 }
 
-Buffer BluetoothWrapper::sendCommandAndReadResponse(const std::vector<char>& bytes, unsigned char retCommandId, int retSubType)
+Buffer BluetoothWrapper::sendCommandAndReadResponse(const std::vector<char>& bytes, unsigned char retCommandId, int retSubType,
+	DATA_TYPE dataType)
 {
 	std::lock_guard guard(this->_connectorMtx);
-	auto data = CommandSerializer::packageDataForBt(bytes, DATA_TYPE::DATA_MDR, this->_seqNumber++);
+	auto data = CommandSerializer::packageDataForBt(bytes, dataType, this->_seqNumber++);
 	this->_connector->send(data.data(), data.size());
 
 	// The device replies with an ACK and then the RET frame; unrelated notifications may interleave.
-	for (int i = 0; i < 16; i++)
+	for (int i = 0; i < MAX_FRAMES_PER_TRANSACTION; i++)
 	{
 		auto msg = this->_readMessage();
 		if (msg.dataType == DATA_TYPE::ACK)
 		{
 			this->_seqNumber = msg.seqNumber;
 			continue;
 		}
-		if (msg.dataType == DATA_TYPE::DATA_MDR && !msg.payload.empty()
+		if (msg.dataType == dataType && !msg.payload.empty()
 			&& (unsigned char)msg.payload[0] == retCommandId
 			&& (retSubType < 0 || (msg.payload.size() >= 2 && (unsigned char)msg.payload[1] == retSubType)))
 		{
@@ -92,8 +109,18 @@ Buffer BluetoothWrapper::sendCommandAndReadResponse(const std::vector<char>& byt
 
 void BluetoothWrapper::_waitForAck()
 {
-	auto msg = this->_readMessage();
-	this->_seqNumber = msg.seqNumber;
+	// Notifications can arrive between a SET and its ACK. _readMessage() acknowledges DATA_MDR frames;
+	// keep consuming until the device's actual ACK arrives so it cannot poison the next command.
+	for (int i = 0; i < MAX_FRAMES_PER_TRANSACTION; i++)
+	{
+		auto msg = this->_readMessage();
+		if (msg.dataType == DATA_TYPE::ACK)
+		{
+			this->_seqNumber = msg.seqNumber;
+			return;
+		}
+	}
+	throw RecoverableException("No acknowledgment received from device", true);
 }
 
 CommandSerializer::Message BluetoothWrapper::_readMessage()
@@ -118,19 +145,29 @@ CommandSerializer::Message BluetoothWrapper::_readMessage()
 		{
 			numRecvd = this->_connector->recv(buf, sizeof(buf));
 		}
+		if (numRecvd <= 0)
+		{
+			throw RecoverableException("Bluetooth connection closed while reading a frame", true);
+		}
 
-		size_t messageStart = 0;
-		size_t messageEnd = numRecvd;
-
-		for (size_t i = 0; i < numRecvd; i++)
+		// Every 60/61/62 inside a frame is escaped, so a START_MARKER can only ever begin one. That makes
+		// resyncing unambiguous, which is what matters after a brief link glitch garbles or truncates a
+		// chunk: bytes before the first START_MARKER are the tail of a frame we can no longer parse, and
+		// a second START_MARKER means the frame we were collecting never finished. Both cases resync on
+		// the new marker here. Previously the first silently concatenated garbage into the message and
+		// the second threw, in either case leaving the parser stranded mid-stream for the rest of the
+		// session - every command after that failed and the UI kept snapping back to the old value.
+		size_t messageStart = ongoingMessage ? 0 : static_cast<size_t>(numRecvd);
+		size_t messageEnd = static_cast<size_t>(numRecvd);
+
+		for (size_t i = 0; i < static_cast<size_t>(numRecvd); i++)
 		{
 			if (buf[i] == START_MARKER)
 			{
-				if (ongoingMessage)
-				{
-					throw RecoverableException("Invalid: Multiple start markers without an end marker", true);
-				}
+				// Whatever we had collected was a truncated frame, not the start of this one.
+				msgBytes.clear();
 				messageStart = i + 1;
+				messageEnd = static_cast<size_t>(numRecvd);
 				ongoingMessage = true;
 			}
 			else if (ongoingMessage && buf[i] == END_MARKER)
@@ -139,25 +176,35 @@ CommandSerializer::Message BluetoothWrapper::_readMessage()
 				ongoingMessage = false;
 				messageFinished = true;
 				// A single recv() can return more than one framed message back-to-back; keep whatever's
-				// past this message's END_MARKER for the next _waitForAck() call instead of dropping it.
+				// past this message's END_MARKER for the next read instead of dropping it.
 				this->_leftoverBytes.assign(buf + i + 1, buf + numRecvd);
 				break;
 			}
 		}
 
-		msgBytes.insert(msgBytes.end(), buf + messageStart, buf + messageEnd);
+		if (messageStart < messageEnd)
+		{
+			msgBytes.insert(msgBytes.end(), buf + messageStart, buf + messageEnd);
+		}
+
+		// Bail out on a stream that keeps delivering bytes but never an END_MARKER, so the next read can
+		// resync on the next START_MARKER instead of growing this buffer forever.
+		if (msgBytes.size() > MAX_BLUETOOTH_MESSAGE_SIZE)
+		{
+			throw RecoverableException("Invalid: message exceeded the maximum size without an end marker", true);
+		}
 	} while (!messageFinished);
 
 	auto msg = CommandSerializer::unpackBtMessage(msgBytes);
 
-	// The device retransmits any DATA_MDR frame it sends until the host ACKs it. The ack carries the
-	// toggled 1-bit sequence number (1 - deviceSeq), matching the device's own ack-to-us behaviour.
-	if (msg.dataType == DATA_TYPE::DATA_MDR)
+	// The device retransmits any DATA_MDR / DATA_MDR_NO2 frame it sends until the host ACKs it. The ack
+	// carries the toggled 1-bit sequence number (1 - deviceSeq), matching the device's own ack-to-us behaviour.
+	// Both tables share the one sequence counter.
+	if (msg.dataType == DATA_TYPE::DATA_MDR || msg.dataType == DATA_TYPE::DATA_MDR_NO2)
 	{
 		auto ack = CommandSerializer::packageDataForBt({}, DATA_TYPE::ACK, (unsigned char)(1 - msg.seqNumber));
 		this->_connector->send(ack.data(), ack.size());
 	}
 
 	return msg;
 }
-

Client/BluetoothWrapper.h

@@ -21,12 +21,15 @@ class BluetoothWrapper
 	BluetoothWrapper(BluetoothWrapper&& other) noexcept;
 	BluetoothWrapper& operator=(BluetoothWrapper&& other) noexcept;
 
-	int sendCommand(const std::vector<char>& bytes);
-
-	// Sends an inquiry command and returns the payload of the first DATA_MDR response whose command id
-	// (payload[0]) matches retCommandId, skipping the ACK and any unrelated notifications. When retSubType
-	// is >= 0, payload[1] must also match it (for multiplexed command families that share one opcode).
-	Buffer sendCommandAndReadResponse(const std::vector<char>& bytes, unsigned char retCommandId, int retSubType = -1);
+	// dataType selects the command table: DATA_MDR (0x0c, table 1) or DATA_MDR_NO2 (0x0e, table 2).
+	// The two tables are separate command spaces - the same opcode means different things in each.
+	int sendCommand(const std::vector<char>& bytes, DATA_TYPE dataType = DATA_TYPE::DATA_MDR);
+
+	// Sends an inquiry command and returns the payload of the first response of the same dataType whose
+	// command id (payload[0]) matches retCommandId, skipping the ACK and any unrelated notifications. When
+	// retSubType is >= 0, payload[1] must also match it (for multiplexed command families that share one opcode).
+	Buffer sendCommandAndReadResponse(const std::vector<char>& bytes, unsigned char retCommandId, int retSubType = -1,
+		DATA_TYPE dataType = DATA_TYPE::DATA_MDR);
 
 	bool isConnected() noexcept;
 	//Try to connect to the headphones

Client/ByteMagic.cpp

@@ -48,7 +48,7 @@ std::string MACBytesToString(unsigned char addr[])
 {
 	// Create hex string from the bytes.
 	std::stringstream mac_addr_str;
-	for (int i = 0; i < MAC_ADDR_BYTES_LEN; i++)
+	for (size_t i = 0; i < MAC_ADDR_BYTES_LEN; i++)
 	{
 		mac_addr_str << std::hex << (((addr[MAC_ADDR_BYTES_LEN - 1 - i] & 0xF0) >> 4) & 0xF);
 		mac_addr_str << std::hex << (addr[MAC_ADDR_BYTES_LEN - 1 - i] & 0x0F);

Client/CMakeLists.txt

@@ -84,7 +84,10 @@ if (LINUX)
 endif ()
 
 if (WIN32)
-    ADD_DEFINITIONS(-DUNICODE)
+    target_compile_definitions(SonyHeadphonesClient PRIVATE
+        UNICODE _UNICODE
+        NOMINMAX
+        WIN32_LEAN_AND_MEAN)
 
     target_sources(SonyHeadphonesClient
         PRIVATE windows/WindowsGUI.cpp
@@ -94,8 +97,11 @@ if (WIN32)
         imgui/backends/imgui_impl_win32.cpp
     )
 
+    # Non-MSVC toolchains have no #pragma comment(lib, ...), so name every import library here.
     target_link_libraries(SonyHeadphonesClient
-        PRIVATE d3d11
+        PRIVATE d3d11 dxgi d3dcompiler
+        ws2_32 rpcrt4 bthprops
+        dwmapi          # imgui_impl_win32.cpp
     )
 endif ()
 

Client/CommandSerializer.cpp

@@ -4,7 +4,7 @@ constexpr unsigned char ESCAPED_BYTE_SENTRY = 61;
 constexpr unsigned char ESCAPED_60 = 44;
 constexpr unsigned char ESCAPED_61 = 45;
 constexpr unsigned char ESCAPED_62 = 46;
-constexpr int MAX_STEPS_WH_1000_XM3 = 19;
+constexpr int MAX_AMBIENT_SOUND_LEVEL_V1 = 20;
 
 namespace CommandSerializer
 {
@@ -53,7 +53,7 @@ namespace CommandSerializer
 			{
 				if (i == src.size() - 1)
 				{
-					throw std::runtime_error("No data left for escaped byte data");
+					throw RecoverableException("No data left for escaped byte data", true);
 				}
 				i = i + 1;
 				switch (src[i])
@@ -71,7 +71,7 @@ namespace CommandSerializer
 					break;
 
 				default:
-					throw std::runtime_error("Unexpected escaped byte");
+					throw RecoverableException("Unexpected escaped byte", true);
 					break;
 				}
 			}
@@ -141,7 +141,7 @@ namespace CommandSerializer
 		//1 (type) + 1 (seq) + 4 (size) + 1 (checksum) = 7 bytes minimum, before any payload.
 		if (unescaped.size() < 7)
 		{
-			throw std::runtime_error("Invalid message: Smaller than the minimum message size");
+			throw RecoverableException("Invalid message: Smaller than the minimum message size", true);
 		}
 
 		unsigned int dataSize =
@@ -150,10 +150,14 @@ namespace CommandSerializer
 			((unsigned int)(unsigned char)unescaped[4] << 8) |
 			((unsigned int)(unsigned char)unescaped[5]);
 
-		if (unescaped.size() < (size_t)6 + dataSize + 1)
+		if (dataSize > unescaped.size() - 7)
 		{
 			throw RecoverableException("Invalid message: declared size exceeds received data", true);
 		}
+		if (dataSize != unescaped.size() - 7)
+		{
+			throw RecoverableException("Invalid message: trailing data after checksum", true);
+		}
 
 		//Checksum covers everything from DATA_TYPE up to (not including) the checksum byte, over the unescaped bytes.
 		if ((unsigned char)unescaped[6 + dataSize] != _sumChecksum(unescaped.data(), 6 + dataSize))
@@ -168,32 +172,18 @@ namespace CommandSerializer
 		return ret;
 	}
 
-	NC_DUAL_SINGLE_VALUE getDualSingleForAsmLevel(char asmLevel)
+	Buffer serializeNcAndAsmSetting(NC_ASM_EFFECT ncAsmEffect, NC_ASM_SETTING_TYPE ncAsmSettingType, NC_DUAL_SINGLE_VALUE ncValue, ASM_SETTING_TYPE asmSettingType, ASM_ID asmId, char asmLevel)
 	{
-		NC_DUAL_SINGLE_VALUE val = NC_DUAL_SINGLE_VALUE::OFF;
-		if (asmLevel > MAX_STEPS_WH_1000_XM3)
-		{
-			throw std::runtime_error("Exceeded max steps");
-		}
-		else if (asmLevel == 1)
-		{
-			val = NC_DUAL_SINGLE_VALUE::SINGLE;
-		}
-		else if (asmLevel == 0)
+		if (asmLevel < 0 || asmLevel > MAX_AMBIENT_SOUND_LEVEL_V1)
 		{
-			val = NC_DUAL_SINGLE_VALUE::DUAL;
+			throw std::runtime_error("Ambient sound level must be between 0 and 20");
 		}
-		return val;
-	}
-
-	Buffer serializeNcAndAsmSetting(NC_ASM_EFFECT ncAsmEffect, NC_ASM_SETTING_TYPE ncAsmSettingType, ASM_SETTING_TYPE asmSettingType, ASM_ID asmId, char asmLevel)
-	{
 		Buffer ret;
 		ret.push_back(static_cast<unsigned char>(COMMAND_TYPE::NCASM_SET_PARAM));
 		ret.push_back(static_cast<unsigned char>(NC_ASM_INQUIRED_TYPE::NOISE_CANCELLING_AND_AMBIENT_SOUND_MODE));
 		ret.push_back(static_cast<unsigned char>(ncAsmEffect));
 		ret.push_back(static_cast<unsigned char>(ncAsmSettingType));
-		ret.push_back(static_cast<unsigned char>(getDualSingleForAsmLevel(asmLevel)));
+		ret.push_back(static_cast<unsigned char>(ncValue));
 		ret.push_back(static_cast<unsigned char>(asmSettingType));
 		ret.push_back(static_cast<unsigned char>(asmId));
 		ret.push_back(asmLevel);
@@ -226,4 +216,3 @@ namespace CommandSerializer
 	}
 
 }
-

Client/CommandSerializer.h

@@ -7,7 +7,6 @@
 #include "Exceptions.h"
 
 constexpr int MINIMUM_VOICE_FOCUS_STEP = 2;
-constexpr unsigned int ASM_LEVEL_DISABLED = -1;
 
 namespace CommandSerializer
 {
@@ -37,10 +36,8 @@ namespace CommandSerializer
 
 	Message unpackBtMessage(const Buffer& src);
 
-	NC_DUAL_SINGLE_VALUE getDualSingleForAsmLevel(char asmLevel);
-	Buffer serializeNcAndAsmSetting(NC_ASM_EFFECT ncAsmEffect, NC_ASM_SETTING_TYPE ncAsmSettingType, ASM_SETTING_TYPE asmSettingType, ASM_ID asmId, char asmLevel);
+	Buffer serializeNcAndAsmSetting(NC_ASM_EFFECT ncAsmEffect, NC_ASM_SETTING_TYPE ncAsmSettingType, NC_DUAL_SINGLE_VALUE ncValue, ASM_SETTING_TYPE asmSettingType, ASM_ID asmId, char asmLevel);
 	//v2 protocol variant (WH-CH720N, XM5, newer XM4, etc) - different NCASM_SET_PARAM byte layout than v1.
 	Buffer serializeNcAndAsmSettingV2(NC_ASM_EFFECT ncAsmEffect, NC_ASM_SETTING_TYPE_V2 ncAsmSettingType, ASM_ID voicePassthrough, char asmLevel);
 	Buffer serializeVPTSetting(VPT_INQUIRED_TYPE type, unsigned char preset);
 }
-