Skip to content

PoC - Try handle random failures of EnableNotificationAsync [android] #192

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
vicocz wants to merge 4 commits into
base: default
Choose a base branch
from
Draft

PoC - Try handle random failures of EnableNotificationAsync [android] #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
549b392
PoC - Try handle random failures of EnableNotificationAsync [android]
Dec 29, 2025
6849267
Stop using auto connect for Android
Dec 31, 2025
06916f4
Try to apply delay before starting notification enablement
Dec 31, 2025
686b136
Update BrickController2/BrickController2.Android/PlatformServices/Blu…
vicocz Jan 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 63 additions & 43 deletions BrickController2/BrickController2.Android/PlatformServices/BluetoothLE/BluetoothLEDevice.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using Android.OS;
using Android.Runtime;
using Java.Util;
using BrickController2.Helpers;
using BrickController2.PlatformServices.BluetoothLE;

namespace BrickController2.Droid.PlatformServices.BluetoothLE
Expand All @@ -18,7 +19,7 @@ public class BluetoothLEDevice : BluetoothGattCallback, IBluetoothLEDevice

private readonly Context _context;
private readonly BluetoothAdapter _bluetoothAdapter;
private readonly object _lock = new object();
private readonly AsyncLock _lock = new();

private BluetoothDevice? _bluetoothDevice = null;
private BluetoothGatt? _bluetoothGatt = null;
Expand Down Expand Up @@ -49,14 +50,14 @@ public BluetoothLEDevice(Context context, BluetoothAdapter bluetoothAdapter, str
{
using (token.Register(() =>
{
lock (_lock)
using (_lock.Lock())
{
Disconnect();
_connectCompletionSource?.TrySetResult(null);
}
}))
{
lock (_lock)
using (_lock.Lock(token))
{
if (State != BluetoothLEDeviceState.Disconnected)
{
Expand All @@ -77,11 +78,11 @@ public BluetoothLEDevice(Context context, BluetoothAdapter bluetoothAdapter, str

if (Build.VERSION.SdkInt >= BuildVersionCodes.M)
{
_bluetoothGatt = _bluetoothDevice.ConnectGatt(_context, autoConnect, this, BluetoothTransports.Le);
_bluetoothGatt = _bluetoothDevice.ConnectGatt(_context, autoConnect: false, this, BluetoothTransports.Le);
}
else
{
_bluetoothGatt = _bluetoothDevice.ConnectGatt(_context, autoConnect, this);
_bluetoothGatt = _bluetoothDevice.ConnectGatt(_context, autoConnect: false, this);
}

if (_bluetoothGatt is null)
Expand All @@ -97,7 +98,7 @@ public BluetoothLEDevice(Context context, BluetoothAdapter bluetoothAdapter, str

var result = await _connectCompletionSource.Task;

lock (_lock)
using (_lock.Lock(token))
{
_connectCompletionSource = null;
return result;
Expand Down Expand Up @@ -125,27 +126,25 @@ internal void Disconnect()
State = BluetoothLEDeviceState.Disconnected;
}

public Task DisconnectAsync()
public async Task DisconnectAsync()
{
lock (_lock)
using (await _lock.LockAsync())
{
Disconnect();
}

return Task.CompletedTask;
}

public async Task<bool> EnableNotificationAsync(IGattCharacteristic characteristic, CancellationToken token)
{
using (token.Register(() =>
{
lock (_lock)
using (_lock.Lock())
{
_descriptorWriteCompletionSource?.TrySetResult(false);
}
}))

lock (_lock)
using (await _lock.LockAsync(token))
{
if (_bluetoothGatt == null || State != BluetoothLEDeviceState.Connected)
{
Expand All @@ -158,33 +157,54 @@ public async Task<bool> EnableNotificationAsync(IGattCharacteristic characterist
return false;
}

// wait slightly for local internal state to settle
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 400ms delay lacks justification for this specific value. Consider documenting why 400ms was chosen (e.g., based on Android BLE stack timing requirements, testing results, or hardware constraints) to help future maintainers understand whether this value should be adjusted.

Suggested change
// wait slightly for local internal state to settle
// The Android BLE stack updates internal state asynchronously after
// SetCharacteristicNotification(...). On some devices/OS versions, immediately
// writing the CCC descriptor without a short delay can intermittently fail or
// result in missed notifications. The 400 ms value is an empirically chosen
// conservative delay that has proven reliable across tested hardware/firmware.
// If you need to change this value, do so cautiously and re-test on a range of
// devices, as reducing it too much may reintroduce flakiness.

Copilot uses AI. Check for mistakes.
await Task.Delay(400, token);

var descriptor = nativeCharacteristic.GetDescriptor(ClientCharacteristicConfigurationUUID);
if (descriptor == null)
{
return false;
}

#pragma warning disable CA1422 // Validate platform compatibility
if (!(descriptor?.SetValue(BluetoothGattDescriptor.EnableNotificationValue!.ToArray()) ?? false))
_descriptorWriteCompletionSource = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);

var valueToSet = BluetoothGattDescriptor.EnableNotificationValue!.ToArray();
bool writeInitiated = false;

if (Build.VERSION.SdkInt >= BuildVersionCodes.Tiramisu) // API 33+
{
return false;
// New API: Pass value and write type explicitly
#pragma warning disable CA1416 // Validate platform compatibility
var resultCode = _bluetoothGatt.WriteDescriptor(descriptor, valueToSet);
#pragma warning restore CA1416 // Validate platform compatibility
writeInitiated = resultCode == 0;
}
else
{
#pragma warning disable CA1422 // Validate platform compatibility
// Old API: Set local value, then write
if (!descriptor.SetValue(valueToSet))
{
_descriptorWriteCompletionSource = null;
return false;
}
writeInitiated = _bluetoothGatt.WriteDescriptor(descriptor);
#pragma warning restore CA1422 // Validate platform compatibility
}

_descriptorWriteCompletionSource = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);

#pragma warning disable CA1422 // Validate platform compatibility
if (!(_bluetoothGatt?.WriteDescriptor(descriptor) ?? false))
if (!writeInitiated)
{
_descriptorWriteCompletionSource = null;
return false;
}
#pragma warning restore CA1422 // Validate platform compatibility
return false; // BLE stack was busy or request failed immediately
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This silent failure returns false without providing any diagnostic information about why the write failed. Consider logging the failure reason to aid debugging of BLE issues, especially since this PR is addressing random failures.

Copilot uses AI. Check for mistakes.
}
}

var result = await _descriptorWriteCompletionSource.Task.ConfigureAwait(false);
// restrict wait time to avoid hanging till target device disconnects
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 10-second timeout value lacks justification. Document why 10 seconds was chosen and what happens if legitimate operations take longer. This is especially important since the PR aims to fix random failures - understanding timeout selection helps distinguish real failures from false positives.

Suggested change
// restrict wait time to avoid hanging till target device disconnects
// Use a bounded wait so we do not hang indefinitely if the BLE stack or device stops responding.
// 10 seconds is chosen as a conservative upper bound: in practice, GATT descriptor writes complete
// within milliseconds to a few seconds, even on slow or congested links. If the operation takes
// longer than 10 seconds, WaitAsync will throw a TimeoutException (or an OperationCanceledException
// if the provided cancellation token is triggered), and the error is propagated to the caller instead
// of blocking until the device eventually disconnects.

Copilot uses AI. Check for mistakes.
var result = await _descriptorWriteCompletionSource.Task
.WaitAsync(TimeSpan.FromSeconds(10), token)
.ConfigureAwait(false);

lock (_lock)
using (await _lock.LockAsync(token))
{
_descriptorWriteCompletionSource = null;
return result;
Expand All @@ -195,13 +215,13 @@ public async Task<bool> EnableNotificationAsync(IGattCharacteristic characterist
{
using (token.Register(() =>
{
lock (_lock)
using (_lock.Lock())
{
_readCompletionSource?.TrySetResult(null);
}
}))
{
lock (_lock)
using (await _lock.LockAsync(token))
{
var nativeCharacteristic = ((GattCharacteristic)characteristic).BluetoothGattCharacteristic;

Expand All @@ -216,7 +236,7 @@ public async Task<bool> EnableNotificationAsync(IGattCharacteristic characterist

var result = await _readCompletionSource.Task;

lock (_lock)
using (await _lock.LockAsync(token))
{
_readCompletionSource = null;
return result;
Expand All @@ -228,13 +248,13 @@ public async Task<bool> WriteAsync(IGattCharacteristic characteristic, byte[] da
{
using (token.Register(() =>
{
lock (_lock)
using (_lock.Lock())
{
_writeCompletionSource?.TrySetResult(false);
}
}))
{
lock (_lock)
using (await _lock.LockAsync(token))
{
if (_bluetoothGatt == null || State != BluetoothLEDeviceState.Connected)
{
Expand Down Expand Up @@ -264,21 +284,21 @@ public async Task<bool> WriteAsync(IGattCharacteristic characteristic, byte[] da

var result = await _writeCompletionSource.Task.ConfigureAwait(false);

lock (_lock)
using (await _lock.LockAsync(token))
{
_writeCompletionSource = null;
return result;
}
}
}

public Task<bool> WriteNoResponseAsync(IGattCharacteristic characteristic, byte[] data, CancellationToken token)
public async Task<bool> WriteNoResponseAsync(IGattCharacteristic characteristic, byte[] data, CancellationToken token)
{
lock (_lock)
using (await _lock.LockAsync(token))
{
if (_bluetoothGatt == null || State != BluetoothLEDeviceState.Connected)
{
return Task.FromResult(false);
return false;
}

var nativeCharacteristic = ((GattCharacteristic)characteristic).BluetoothGattCharacteristic;
Expand All @@ -287,14 +307,14 @@ public Task<bool> WriteNoResponseAsync(IGattCharacteristic characteristic, byte[
#pragma warning disable CA1422 // Validate platform compatibility
if (!(nativeCharacteristic?.SetValue(data) ?? false))
{
return Task.FromResult(false);
return false;
}
#pragma warning restore CA1422 // Validate platform compatibility

#pragma warning disable CA1422 // Validate platform compatibility
var result = _bluetoothGatt?.WriteCharacteristic(nativeCharacteristic) ?? false;
#pragma warning restore CA1422 // Validate platform compatibility
return Task.FromResult(result);
return result;
}
}

Expand All @@ -306,15 +326,15 @@ public override void OnConnectionStateChange(BluetoothGatt? gatt, [GeneratedEnum
break;

case ProfileState.Connected:
lock (_lock)
using (_lock.Lock())
{
if (State == BluetoothLEDeviceState.Connecting && status == GattStatus.Success)
{
State = BluetoothLEDeviceState.Discovering;
Task.Run(async () =>
{
await Task.Delay(750);
lock (_lock)
using (await _lock.LockAsync())
{
if (State == BluetoothLEDeviceState.Discovering && _bluetoothGatt != null)
{
Expand All @@ -340,7 +360,7 @@ public override void OnConnectionStateChange(BluetoothGatt? gatt, [GeneratedEnum
break;

case ProfileState.Disconnected:
lock (_lock)
using (_lock.Lock())
{
switch (State)
{
Expand Down Expand Up @@ -372,7 +392,7 @@ public override void OnConnectionStateChange(BluetoothGatt? gatt, [GeneratedEnum

public override void OnServicesDiscovered(BluetoothGatt? gatt, [GeneratedEnum] GattStatus status)
{
lock (_lock)
using (_lock.Lock())
{
if (status == GattStatus.Success && State == BluetoothLEDeviceState.Discovering)
{
Expand Down Expand Up @@ -407,7 +427,7 @@ public override void OnServicesDiscovered(BluetoothGatt? gatt, [GeneratedEnum] G

public override void OnCharacteristicRead(BluetoothGatt? gatt, BluetoothGattCharacteristic? characteristic, [GeneratedEnum] GattStatus status)
{
lock (_lock)
using (_lock.Lock())
{
#pragma warning disable CA1422 // Validate platform compatibility
_readCompletionSource?.TrySetResult(characteristic?.GetValue());
Expand All @@ -417,23 +437,23 @@ public override void OnCharacteristicRead(BluetoothGatt? gatt, BluetoothGattChar

public override void OnCharacteristicWrite(BluetoothGatt? gatt, BluetoothGattCharacteristic? characteristic, [GeneratedEnum] GattStatus status)
{
lock (_lock)
using (_lock.Lock())
{
_writeCompletionSource?.TrySetResult(status == GattStatus.Success);
}
}

public override void OnDescriptorWrite(BluetoothGatt? gatt, BluetoothGattDescriptor? descriptor, [GeneratedEnum] GattStatus status)
{
lock (_lock)
using (_lock.Lock())
{
_descriptorWriteCompletionSource?.TrySetResult(status == GattStatus.Success);
}
}

public override void OnCharacteristicChanged(BluetoothGatt? gatt, BluetoothGattCharacteristic? characteristic)
{
lock (_lock)
using (_lock.Lock())
{
var guid = characteristic?.Uuid?.ToGuid();
#pragma warning disable CA1422 // Validate platform compatibility
Expand Down
14 changes: 14 additions & 0 deletions BrickController2/BrickController2/DeviceManagement/TechnicMoveDevice.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,20 @@ protected override byte[] GetServoCommand(int channel, int servoValue, int servo
return base.GetServoCommand(channel, servoValue, servoSpeed);
}

protected override async Task<bool> ValidateServicesAsync(IEnumerable<IGattService>? services, CancellationToken token)
{
// better handle this type of device
if (services?.Any(s => s.Uuid == ServiceUuid && s.Characteristics.Any(c => c.Uuid == CharacteristicUuid)) == true)
{
// give some additional wait time for the device to be ready
await Task.Delay(TimeSpan.FromSeconds(1), token);

return await base.ValidateServicesAsync(services, token);
}

return false;
}

protected override async Task<bool> AfterConnectSetupAsync(bool requestDeviceInformation, CancellationToken token)
{
if (await base.AfterConnectSetupAsync(requestDeviceInformation, token))
Expand Down
7 changes: 7 additions & 0 deletions BrickController2/BrickController2/Helpers/AsyncLock.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ public async Task<IDisposable> LockAsync(CancellationToken token)
return new Releaser(_semaphore);
}


public IDisposable Lock(CancellationToken token = default)
{
_semaphore.Wait(token);
return new Releaser(_semaphore);
}

private struct Releaser : IDisposable
{
private SemaphoreSlim? _semaphore;
Expand Down