Merge pull request #32 from ZaparooProject/fix/ndef-read-timeout-recovery

fix: resolve NDEF read timeout errors on marginal RF connections

Author: wizzomafizzo

communication_test.go

@@ -566,6 +566,102 @@ func TestDevice_SendRawCommand(t *testing.T) {
 	}
 }
 
+func TestDevice_SendDataExchangeWithRetry(t *testing.T) {
+	t.Parallel()
+
+	t.Run("Success_FirstAttempt", func(t *testing.T) {
+		t.Parallel()
+
+		device, mock := createMockDeviceWithTransport(t)
+
+		// Configure successful response
+		mock.SetResponse(testutil.CmdInDataExchange, []byte{0x41, 0x00, 0xDE, 0xAD, 0xBE, 0xEF})
+
+		result, err := device.SendDataExchangeWithRetry(context.Background(), []byte{0x30, 0x04})
+
+		require.NoError(t, err)
+		assert.Equal(t, []byte{0xDE, 0xAD, 0xBE, 0xEF}, result)
+		assert.Equal(t, 1, mock.GetCallCount(testutil.CmdInDataExchange))
+	})
+
+	t.Run("Success_AfterTimeoutRetry", func(t *testing.T) {
+		t.Parallel()
+
+		device, mock := createMockDeviceWithTransport(t)
+
+		// Queue: first call returns timeout (0x01), second call succeeds
+		// Timeout error is encoded as protocol error in response byte 1
+		mock.QueueResponses(testutil.CmdInDataExchange,
+			[]byte{0x41, 0x01},                         // First call: timeout error
+			[]byte{0x41, 0x00, 0xDE, 0xAD, 0xBE, 0xEF}, // Second call: success
+		)
+
+		result, err := device.SendDataExchangeWithRetry(context.Background(), []byte{0x30, 0x04})
+
+		require.NoError(t, err)
+		assert.Equal(t, []byte{0xDE, 0xAD, 0xBE, 0xEF}, result)
+		assert.Equal(t, 2, mock.GetCallCount(testutil.CmdInDataExchange), "Should have retried once")
+	})
+
+	t.Run("Success_AfterTwoTimeoutRetries", func(t *testing.T) {
+		t.Parallel()
+
+		device, mock := createMockDeviceWithTransport(t)
+
+		// Queue: first two calls timeout, third succeeds
+		mock.QueueResponses(testutil.CmdInDataExchange,
+			[]byte{0x41, 0x01},                         // First call: timeout error
+			[]byte{0x41, 0x01},                         // Second call: timeout error
+			[]byte{0x41, 0x00, 0xDE, 0xAD, 0xBE, 0xEF}, // Third call: success
+		)
+
+		result, err := device.SendDataExchangeWithRetry(context.Background(), []byte{0x30, 0x04})
+
+		require.NoError(t, err)
+		assert.Equal(t, []byte{0xDE, 0xAD, 0xBE, 0xEF}, result)
+		assert.Equal(t, 3, mock.GetCallCount(testutil.CmdInDataExchange), "Should have retried twice")
+	})
+
+	t.Run("Failure_AllThreeTimeoutsExhausted", func(t *testing.T) {
+		t.Parallel()
+
+		device, mock := createMockDeviceWithTransport(t)
+
+		// Queue: all three calls timeout
+		mock.QueueResponses(testutil.CmdInDataExchange,
+			[]byte{0x41, 0x01}, // First call: timeout error
+			[]byte{0x41, 0x01}, // Second call: timeout error
+			[]byte{0x41, 0x01}, // Third call: timeout error
+		)
+
+		result, err := device.SendDataExchangeWithRetry(context.Background(), []byte{0x30, 0x04})
+
+		require.Error(t, err)
+		assert.Nil(t, result)
+		assert.Equal(t, 3, mock.GetCallCount(testutil.CmdInDataExchange), "Should have tried 3 times")
+
+		// Verify it's a timeout error
+		var pn532Err *PN532Error
+		require.ErrorAs(t, err, &pn532Err)
+		assert.True(t, pn532Err.IsTimeoutError())
+	})
+
+	t.Run("NoRetry_NonTimeoutError", func(t *testing.T) {
+		t.Parallel()
+
+		device, mock := createMockDeviceWithTransport(t)
+
+		// Configure non-timeout error (0x14 = authentication error)
+		mock.SetResponse(testutil.CmdInDataExchange, []byte{0x41, 0x14})
+
+		result, err := device.SendDataExchangeWithRetry(context.Background(), []byte{0x30, 0x04})
+
+		require.Error(t, err)
+		assert.Nil(t, result)
+		assert.Equal(t, 1, mock.GetCallCount(testutil.CmdInDataExchange), "Should NOT retry non-timeout errors")
+	})
+}
+
 func TestDevice_PowerDown(t *testing.T) {
 	t.Parallel()
 

device_context.go

@@ -555,6 +555,54 @@ func (d *Device) SendDataExchange(ctx context.Context, data []byte) ([]byte, err
 	return res[2:], nil
 }
 
+// isRetryableTimeoutError checks if an error is a PN532 timeout (0x01) that should be retried.
+func isRetryableTimeoutError(err error) bool {
+	var pn532Err *PN532Error
+	return errors.As(err, &pn532Err) && pn532Err.IsTimeoutError()
+}
+
+// SendDataExchangeWithRetry sends a data exchange command with automatic retry on timeout.
+// Error 0x01 (timeout) indicates a transient RF communication failure where the packet was
+// lost. Immediate retry typically succeeds as the tag is still in the field.
+//
+// Configuration:
+//   - 3 attempts total (1 initial + 2 retries)
+//   - No delay between retries (immediate retry)
+//   - Only retries on timeout errors (0x01)
+func (d *Device) SendDataExchangeWithRetry(ctx context.Context, data []byte) ([]byte, error) {
+	const maxAttempts = 3
+
+	var lastErr error
+	for attempt := range maxAttempts {
+		if ctxErr := ctx.Err(); ctxErr != nil {
+			if lastErr != nil {
+				return nil, lastErr
+			}
+			return nil, ctxErr
+		}
+
+		result, err := d.SendDataExchange(ctx, data)
+		if err == nil {
+			if attempt > 0 {
+				Debugf("SendDataExchangeWithRetry: succeeded on attempt %d", attempt+1)
+			}
+			return result, nil
+		}
+
+		if !isRetryableTimeoutError(err) {
+			return nil, err
+		}
+
+		// Timeout error - save it and retry immediately
+		lastErr = err
+		if attempt < maxAttempts-1 {
+			Debugf("SendDataExchangeWithRetry: timeout on attempt %d, retrying immediately", attempt+1)
+		}
+	}
+
+	return nil, lastErr
+}
+
 // SendRawCommand sends a raw command with context support
 func (d *Device) SendRawCommand(ctx context.Context, data []byte) ([]byte, error) {
 	const targetNum byte = 1

mifare.go

@@ -307,8 +307,8 @@ func (t *MIFARETag) ReadBlock(ctx context.Context, block uint8) ([]byte, error)
 		return nil, fmt.Errorf("not authenticated to sector %d (block %d)", sector, block)
 	}
 
-	// Send read command
-	data, err := t.device.SendDataExchange(ctx, []byte{mifareCmdRead, block})
+	// Send read command with retry on timeout
+	data, err := t.device.SendDataExchangeWithRetry(ctx, []byte{mifareCmdRead, block})
 	if err != nil {
 		return nil, fmt.Errorf("%w (block %d): %w", ErrTagReadFailed, block, err)
 	}
@@ -329,10 +329,10 @@ func (t *MIFARETag) ReadBlockDirect(ctx context.Context, block uint8) ([]byte, e
 		return nil, err
 	}
 
-	// Send read command directly
-	data, err := t.device.SendDataExchange(ctx, []byte{mifareCmdRead, block})
+	// Send read command with retry on timeout
+	data, err := t.device.SendDataExchangeWithRetry(ctx, []byte{mifareCmdRead, block})
 	if err != nil {
-		// If we get a timeout error, try InCommunicateThru as fallback
+		// If we still get a timeout error after retries, try InCommunicateThru as fallback
 		if IsPN532TimeoutError(err) {
 			return t.readBlockCommunicateThru(ctx, block)
 		}

ntag.go

@@ -167,7 +167,7 @@ func (t *NTAGTag) ReadBlock(ctx context.Context, block uint8) ([]byte, error) {
 		return nil, err
 	}
 
-	data, err := t.device.SendDataExchange(ctx, []byte{ntagCmdRead, block})
+	data, err := t.device.SendDataExchangeWithRetry(ctx, []byte{ntagCmdRead, block})
 	if err != nil {
 		// If we get authentication error 14, try InCommunicateThru as fallback for clone devices
 		if IsPN532AuthenticationError(err) {
@@ -240,10 +240,9 @@ func (t *NTAGTag) ReadNDEF(ctx context.Context) (*NDEFMessage, error) {
 		return nil, err
 	}
 
-	// Ensure target is selected before reading. This is critical after DetectType()
-	// which uses GetVersion() via InCommunicateThru - that command doesn't maintain
-	// target selection state. Without re-selection, InDataExchange fails with timeout
-	// error 0x01. See PN532 User Manual §7.3.9.
+	// Ensure target is selected before reading. This is defensive against any prior
+	// operations that may have used InCommunicateThru (0x42), which doesn't maintain
+	// the PN532's target selection state. See PN532 User Manual §7.3.9.
 	if err := t.device.InSelect(ctx); err != nil {
 		Debugln("NTAG ReadNDEF: InSelect failed, continuing anyway:", err)
 	}
@@ -375,7 +374,7 @@ func (t *NTAGTag) readRawPages(ctx context.Context, startPage uint8, pageCount i
 	}
 
 	// NTAG READ command returns 16 bytes (4 pages) starting from the specified page
-	data, err := t.device.SendDataExchange(ctx, []byte{ntagCmdRead, startPage})
+	data, err := t.device.SendDataExchangeWithRetry(ctx, []byte{ntagCmdRead, startPage})
 	if err != nil {
 		return nil, err
 	}
@@ -1065,7 +1064,15 @@ func (t *NTAGTag) GetTotalPages() uint8 {
 	}
 }
 
-// DetectType attempts to detect the NTAG variant using GET_VERSION command with fallback
+// DetectType detects the NTAG variant using the Capability Container (CC).
+//
+// This function uses CC-based detection exclusively, avoiding the GET_VERSION command
+// which uses InCommunicateThru (0x42). InCommunicateThru doesn't maintain the PN532's
+// target selection state, causing subsequent InDataExchange calls to fail with timeout
+// error 0x01 on readers with marginal RF connections.
+//
+// CC-based detection accurately identifies NTAG213/215/216 from the size field in the
+// capability container, which is sufficient for NDEF operations.
 func (t *NTAGTag) DetectType(ctx context.Context) error {
 	if err := ctx.Err(); err != nil {
 		return err
@@ -1090,53 +1097,10 @@ func (t *NTAGTag) DetectType(ctx context.Context) error {
 		return errors.New("not an NTAG tag: invalid capability container")
 	}
 
-	// Check if this is likely a genuine NXP tag (UID starts with 0x04)
-	// Clone tags use different UID prefixes and typically don't support GET_VERSION.
-	// Sending GET_VERSION (0x60) to a clone tag that doesn't understand it causes
-	// the tag to enter IDLE state per ISO14443-3A, breaking subsequent writes.
-	// Skip GET_VERSION for non-NXP UIDs to keep clone tags in ACTIVE state.
-	if len(t.uid) > 0 && t.uid[0] != 0x04 {
-		Debugf("NTAG DetectType: non-NXP UID prefix 0x%02X, skipping GET_VERSION to avoid IDLE state", t.uid[0])
-		t.tagType = t.detectTypeFromCapabilityContainer(ccData)
-		return nil
-	}
-
-	// Now try to get the actual version information using GET_VERSION
-	version, err := t.GetVersion()
-
-	// Re-select target after GetVersion to restore PN532 internal state.
-	// GetVersion uses InCommunicateThru (0x42) which is a raw pass-through command
-	// that doesn't maintain the PN532's target selection state. Without re-selection,
-	// subsequent InDataExchange calls may fail with timeout error 01.
-	// See PN532 User Manual §7.3.9: "The host controller has to take care of the
-	// selection of the target it wants to reach (whereas when using the
-	// InDataExchange command, it is done automatically)."
-	if selectErr := t.device.InSelect(ctx); selectErr != nil {
-		Debugln("NTAG DetectType: InSelect after GetVersion failed:", selectErr)
-	}
-
-	if err != nil {
-		// If GET_VERSION fails, we still know it's an NTAG from the CC check
-		// Use fallback detection method based on capability container
-		Debugf("NTAG GET_VERSION failed, using CC-based detection fallback: %v", err)
-		t.tagType = t.detectTypeFromCapabilityContainer(ccData)
-		return nil // Don't return error if CC-based detection succeeded
-	}
-
-	// Use the version information to determine the exact variant
-	t.tagType = version.GetNTAGType()
-
-	if t.tagType == NTAGTypeUnknown {
-		// Even if storage size is unknown from GET_VERSION, try CC-based detection
-		Debugf("NTAG GET_VERSION returned unknown type, trying CC-based detection fallback")
-		fallbackType := t.detectTypeFromCapabilityContainer(ccData)
-		if fallbackType != NTAGTypeUnknown {
-			t.tagType = fallbackType
-		} else {
-			// Final fallback to conservative choice
-			t.tagType = NTAGType213 // Use smallest variant for safety
-		}
-	}
+	// Use CC-based detection for all tags (genuine NXP and clones alike)
+	// This avoids GET_VERSION which uses InCommunicateThru and can cause
+	// target selection issues on marginal RF connections
+	t.tagType = t.detectTypeFromCapabilityContainer(ccData)
 
 	return nil
 }
@@ -1321,21 +1285,20 @@ func (t *NTAGTag) getTagTypeName() string {
 }
 
 func (t *NTAGTag) readBlockWithRetry(ctx context.Context, block uint8) ([]byte, error) {
-	maxRetries := 3
+	const maxRetries = 3
 	for i := range maxRetries {
 		if err := ctx.Err(); err != nil {
 			return nil, err
 		}
 
-		data, err := t.device.SendDataExchange(ctx, []byte{ntagCmdRead, block})
+		// SendDataExchangeWithRetry handles timeout (0x01) retries internally
+		data, err := t.device.SendDataExchangeWithRetry(ctx, []byte{ntagCmdRead, block})
 		if err != nil {
 			// If we get authentication error 14, try InCommunicateThru as fallback for clone devices
 			if IsPN532AuthenticationError(err) {
 				return t.readBlockCommunicateThru(ctx, block)
 			}
-			if i < maxRetries-1 {
-				continue
-			}
+			// For other errors, fail immediately since SendDataExchangeWithRetry already retried timeouts
 			return nil, err
 		}
 
@@ -1344,6 +1307,7 @@ func (t *NTAGTag) readBlockWithRetry(ctx context.Context, block uint8) ([]byte,
 			return data[:ntagBlockSize], nil
 		}
 
+		// Short response - retry
 		if i < maxRetries-1 {
 			continue
 		}
@@ -1649,8 +1613,8 @@ func (t *NTAGTag) readBlockDirectFallback(ctx context.Context, block uint8) ([]b
 
 	Debugf("NTAG attempting direct InDataExchange fallback for block %d", block)
 
-	// Try direct InDataExchange, ignoring the error 14 that originally triggered the fallback chain
-	data, err := t.device.SendDataExchange(ctx, []byte{ntagCmdRead, block})
+	// Try direct InDataExchange with retry, ignoring the error 14 that originally triggered the fallback chain
+	data, err := t.device.SendDataExchangeWithRetry(ctx, []byte{ntagCmdRead, block})
 	if err != nil {
 		// If this also fails, the clone device simply doesn't support NTAG properly
 		Debugf("NTAG direct InDataExchange fallback also failed: %v", err)

ntag_test.go

@@ -1632,6 +1632,45 @@ func TestNTAGDetectType_SkipsGetVersionForCloneTags(t *testing.T) {
 	assert.NotEqual(t, NTAGTypeUnknown, tag.tagType)
 }
 
+// TestNTAGDetectType_UsesCCOnlyForNXPTags verifies that even genuine NXP tags
+// (UID starting with 0x04) use CC-based detection and never call GET_VERSION.
+// This is a regression test for the fix that removed GET_VERSION to avoid
+// timeout errors on marginal RF connections. See issue: NDEF read timeout 0x01.
+func TestNTAGDetectType_UsesCCOnlyForNXPTags(t *testing.T) {
+	t.Parallel()
+
+	device, mockTransport := createMockDeviceWithTransport(t)
+
+	// Use a genuine NXP tag UID (starts with 0x04)
+	nxpUID := []byte{0x04, 0x12, 0x34, 0x56, 0x78, 0x9A, 0xBC}
+	tag := NewNTAGTag(device, nxpUID, 0x00)
+
+	// Mock valid CC read for NTAG215 (size 0x3E)
+	// CC format: [Magic 0xE1] [Version] [Size] [Access]
+	ccData := make([]byte, 16) // Read returns 4 pages = 16 bytes
+	ccData[0] = 0xE1           // NDEF magic
+	ccData[1] = 0x10           // Version 1.0
+	ccData[2] = 0x3E           // Size field for NTAG215 (504 bytes)
+	ccData[3] = 0x00           // Access conditions
+
+	// Only set up response for InDataExchange (0x40), NOT for InCommunicateThru (0x42)
+	// If GetVersion was being called, it would use InCommunicateThru and fail
+	mockTransport.SetResponse(0x40, append([]byte{0x41, 0x00}, ccData...))
+
+	err := tag.DetectType(context.Background())
+
+	require.NoError(t, err, "DetectType should succeed with CC-based detection only")
+	assert.Equal(t, NTAGType215, tag.tagType, "Should detect NTAG215 from CC size field")
+
+	// Verify InCommunicateThru was NOT called (GetVersion uses InCommunicateThru)
+	assert.Equal(t, 0, mockTransport.GetCallCount(0x42),
+		"InCommunicateThru should NOT be called - GetVersion is no longer used")
+
+	// Verify InDataExchange WAS called (for CC read)
+	assert.GreaterOrEqual(t, mockTransport.GetCallCount(0x40), 1,
+		"InDataExchange should be called for CC read")
+}
+
 // TestNTAGTag_ReadNDEF_FudanClone tests that Fudan clone tags (UID prefix 0x1D)
 // use block-by-block reading instead of FAST_READ, since Fudan FM11NT021 clones
 // don't support FAST_READ (0x3A) and may return garbage or corrupt tag state.