Repository navigation
Conversation
|
@codex review Comment generated by claude-opus-5-5 on behalf of Ole |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Repo Coverage
PR Coverage291/304 changed lines covered (Lines: 95.72%, Branches: 78.95%) Darp.Ble.WinRT🔴 home/runner/work/Darp.Ble/Darp.Ble/src/Darp.Ble.WinRT/WinRTBackend.cs — 0/4 changed lines covered (Lines: 0%, Branches: n/a)...
8 ⬛ {
9 🟥 public string Name => BleBackends.WinRT;
10 ⬛
...
15 ⬛ {
16 🟥 cancellationToken.ThrowIfCancellationRequested();
17 ⬛ #if WINDOWS
...
20 ⬛ #endif
21 🟥 context.LoggerFactory.CreateLogger<WinRTBackend>().LogNotOnWindows();
22 🟥 return Task.FromResult<IReadOnlyList<IBleAdapterSource>>([]);
23 ⬛ }
...🔴 home/runner/work/Darp.Ble/Darp.Ble/src/Darp.Ble.WinRT/WinRTBleManagerBuilderExtensions.cs — 0/2 changed lines covered (Lines: 0%, Branches: n/a)...
10 ⬛ {
11 🟥 ArgumentNullException.ThrowIfNull(builder);
12 🟥 return builder.AddBackend(new WinRTBackend());
13 ⬛ }
...🔴 home/runner/work/Darp.Ble/Darp.Ble/src/Darp.Ble.WinRT/Connections/CentralSessions.cs — 28/29 changed lines covered (Lines: 96.55%, Branches: 91.67%)...
60 🟩 Connection? connection = _byDevice.Values.FirstOrDefault(c => ReferenceEquals(c.Central, central));
61 🟨 if (connection is null)
62 🟥 return false;
63 🟩 _byDevice.Remove(connection.DeviceId);
...🟠 home/runner/work/Darp.Ble/Darp.Ble/src/Darp.Ble.WinRT/Gatt/SubscriptionSnapshots.cs — 16/16 changed lines covered (Lines: 100%, Branches: 80%)...
33 ⬛ lock (_lock)
34 🟨 return _subscribed.GetValueOrDefault(characteristic)?.Contains(client) is true;
35 🟩 }
...✓ 🟢 home/runner/work/Darp.Ble/Darp.Ble/src/Darp.Ble.WinRT/Gap/PublisherSections.cs — 41/41 changed lines covered Darp.Ble🔴 home/runner/work/Darp.Ble/Darp.Ble/src/Darp.Ble/Data/BleAddress.cs — 37/39 changed lines covered (Lines: 94.87%, Branches: 90%)...
96 🟩 BleAddressType type = BleAddressType.NotAvailable;
97 🟨 if (s.Length == 19 && s[1] == ':')
98 ⬛ {
...
162 ⬛ public string ToString(string? format, IFormatProvider? formatProvider) =>
163 🟨 format switch
164 🟩 {
165 🟨 null or "" => ToString(),
166 🟩 "T" => Type switch
...
169 🟩 BleAddressType.Random => $"R:{this}",
170 🟥 _ => ToString(),
171 🟩 },
172 🟥 _ => throw new FormatException($"The format '{format}' is not supported; use \"T\" or none"),
173 🟩 };
...🔴 home/runner/work/Darp.Ble/Darp.Ble/src/Darp.Ble/Gap/AdvertisingSet.cs — 9/11 changed lines covered (Lines: 81.82%, Branches: 50%)...
98 ⬛ public Task StopAsync(CancellationToken cancellationToken = default) =>
99 🟥 RunAsync(() => StopCoreAsync(cancellationToken), cancellationToken);
100 ⬛
...
263 ⬛ {
264 🟨 if (!_isAdvertising)
265 🟥 return;
266 🟩 _isAdvertising = false;
...
367 ⬛ {
368 🟨 _pendingRestart?.Cancel();
369 🟩 _pendingRestart = null;
...🟠 home/runner/work/Darp.Ble/Darp.Ble/src/Darp.Ble/Gap/GapAdvertisement.cs — 50/50 changed lines covered (Lines: 100%, Branches: 50%)...
116 ⬛ private bool Equals(GapAdvertisement other) =>
117 🟨 Timestamp.Equals(other.Timestamp)
118 🟩 && EventType == other.EventType
...🟠 home/runner/work/Darp.Ble/Darp.Ble/src/Darp.Ble/Connections/ConnectionEvents.cs — 4/4 changed lines covered (Lines: 100%, Branches: 50%)...
38 🟩 connection = _connection;
39 🟨 connection?.OnMtuChanged(mtu);
40 🟩 }
...🟠 home/runner/work/Darp.Ble/Darp.Ble/src/Darp.Ble/Gap/AdvertisementStreamExtensions.cs — 5/5 changed lines covered (Lines: 100%, Branches: 75%)...
20 ⬛ // An address without its type, e.g. parsed from text, matches its value of any type
21 🟨 return address.Type is BleAddressType.NotAvailable
22 🟩 ? source.Where(adv => adv.Address.Value == address.Value)
...🟠 home/runner/work/Darp.Ble/Darp.Ble/src/Darp.Ble/Gap/AdvertisingSetEvents.cs — 1/1 changed lines covered (Lines: 100%, Branches: 50%)...
14 ⬛
15 🟨 public void OnFailed(BleException error) => Volatile.Read(ref _set)?.OnFailed(error);
16 ⬛ }
17 ⬛ Darp.Ble.HciHost🔴 home/runner/work/Darp.Ble/Darp.Ble/src/Darp.Ble.HciHost/Gap/HciHostObserverDriver.cs — 20/22 changed lines covered (Lines: 90.91%, Branches: 71.43%)...
108 ⬛ // Anonymous advertisements have no address to connect to
109 🟨 if (type is BleAddressType.NotAvailable)
110 🟥 return;
111 🟨 if (_addressTypes.Count >= MaxKnownAddresses && !_addressTypes.ContainsKey(address))
112 🟥 _addressTypes.Clear();
113 🟩 _addressTypes[address] = type;
...🟠 home/runner/work/Darp.Ble/Darp.Ble/src/Darp.Ble.HciHost/Connections/ConnectAttempt.cs — 5/5 changed lines covered (Lines: 100%, Branches: 50%)...
52 ⬛ {
53 🟨 if (_result.Task.IsCompleted)
54 ⬛ return;
...
73 ⬛ // A link a central made to our peripheral role, or one to another peer, is not this attempt's
74 🟨 if (
75 🟩 hciEvent.Role is not HciLeConnectionRole.Central
...🟠 home/runner/work/Darp.Ble/Darp.Ble/src/Darp.Ble.HciHost/Gap/AdvertisingReportAssembler.cs — 25/25 changed lines covered (Lines: 100%, Branches: 95.45%)...
35 🟩 bool continues = _pending.Remove(sender, out Pending earlier);
36 🟨 txPower = continues && report.TxPower == TxPowerNotAvailable ? earlier.TxPower : report.TxPower;
37 🟩 data = default;
...✓ 🟢 home/runner/work/Darp.Ble/Darp.Ble/src/Darp.Ble.HciHost/HciHostAdapterDriver.cs — 1/1 changed lines covered Darp.Ble.Mock🟠 home/runner/work/Darp.Ble/Darp.Ble/src/Darp.Ble.Mock/Gap/MockObserverDriver.cs — 1/1 changed lines covered (Lines: 100%, Branches: 62.5%)...
62 ⬛ bool isLegacy = eventType.HasFlag(BleEventType.Legacy);
63 🟨 return GapAdvertisement.Create(
64 ⬛ timestamp,
...Legend: 🟥 uncovered · 🟨 partial branch · 🟩 covered · ⬛ non-executable/blank |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e76e5d60db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _data = data; | ||
| if (_publisher is null) | ||
| return; | ||
| await StopAsync(cancellationToken).ConfigureAwait(false); | ||
| await StartAsync(cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
When updating a running non-connectable advertisement, cancellation or a publisher error after StopAsync succeeds leaves the publisher stopped, but AdvertisingSet.SetAdvertisingDataAsync never clears the core's _isAdvertising flag. Consequently, AdvertisingSet.StartAsync becomes a no-op even though nothing is being advertised, and its Data property still reports the old payload while this driver retains the new one. Make this restart failure-safe so the driver and core retain consistent advertising and payload state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1fb0de5. A data update now starts a publisher with the new data before it stops the running one. When starting fails or is cancelled, the set keeps advertising its old data and the driver keeps the old data too, so it matches the core's Data and IsAdvertising.
Comment generated by claude-opus-5-5 on behalf of Ole
| request.HandOffToken.ThrowIfCancellationRequested(); | ||
| DeviceAccessStatus status = await _device.RequestAccessAsync().AsTask().ConfigureAwait(false); |
There was a problem hiding this comment.
If Windows' device-access request remains pending, cancelling service discovery only cancels the caller's wait: this await ignores request.HandOffToken. Since discovery has not yet called OnTransactionSent, it continues holding both the request slot and send order in RequestQueue, with no ATT timeout running, so subsequent operations remain blocked until the access request completes. Make the access wait cancellable with the hand-off token; unlike a sent GATT transaction, this is still a pre-send wait.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1fb0de5. The wait for Windows' device-access answer now ends with request.HandOffToken; nothing has been sent at that point, so cancelling it frees the request queue.
Comment generated by claude-opus-5-5 on behalf of Ole
e76e5d6 to
6c6920d
Compare
|
@codex review Comment generated by claude-opus-5-5 on behalf of Ole |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c6920debc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _publisher = null; | ||
| await StopPublisherAsync(publisher, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
If cancellation occurs after publisher.Stop() but before the Stopped event, StopPublisherAsync throws even though Windows is stopping the advertisement. The publisher reference has already been cleared, while AdvertisingSet.StopCoreAsync retains its _isAdvertising flag because the driver call failed. A subsequent StartAsync therefore does nothing despite the advertisement having stopped. Once the native stop has been issued, reconcile its completion with the core's state rather than abandoning the wait and losing the publisher.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 46ca3e1. Stopping checks the token before it asks Windows to stop; once Stop() is issued, the wait for Stopped is no longer cancellable, so a stop either does not happen or completes, and the core's IsAdvertising stays true to the publisher.
Comment generated by claude-opus-5-5 on behalf of Ole
| _device.ConnectionStatusChanged += OnConnectionStatusChanged; | ||
| _session.SessionStatusChanged += OnSessionStatusChanged; |
There was a problem hiding this comment.
If the peripheral disconnects immediately after its session becomes active, the disconnect can occur before these handlers are attached. WinRTCentralDriver.ConnectAsync only waits for the first Active status, and this constructor never checks whether the device disconnected or the session closed in the meantime. Consequently, no OnDisconnected callback reaches the core, and its local-only MTU exchange can succeed and return an already-dead connection. Recheck the native statuses after subscribing and report the disconnect through the existing idempotent LinkDown path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 46ca3e1. After attaching its handlers, the connection checks the device and session status and reports a link that already went down through LinkDown. Checked on Windows 11 against the dongle: the connect tests still pass repeatedly.
Comment generated by claude-opus-5-5 on behalf of Ole
6c6920d to
573c6fb
Compare
|
@codex review Comment generated by claude-opus-5-5 on behalf of Ole |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
573c6fb to
289cc63
Compare
289cc63 to
40c853d
Compare
8c5a492 to
faffcad
Compare
683b3d2 to
31a007c
Compare
31a007c to
bf9a7bb
Compare
bf9a7bb to
c921b19
Compare
a3dd484 to
325ad61
Compare
325ad61 to
89b7659
Compare
89b7659 to
3026d52
Compare
e8f568b to
d4d365b
Compare
e7e7bbd to
16c6dff
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16c6dffc6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Task stopped = WaitForAsync(publisher, BluetoothLEAdvertisementPublisherStatus.Stopped, CancellationToken.None); | ||
| publisher.Stop(); | ||
| await stopped.ConfigureAwait(false); |
There was a problem hiding this comment.
If Windows aborts the publisher while StopAsync is awaiting its terminal status, WaitForAsync throws even though advertising has ended. The driver has already cleared _publisher and detached OnRunningStatusChanged, so neither successful completion nor OnFailed clears the core's _isAdvertising flag; subsequent StartAsync calls become no-ops, including after the radio is restored. The cancellation fix does not cover this path: WaitForAsync still explicitly faults on Aborted. Treat that terminal status as a completed stop, or otherwise reconcile the core's state before propagating the error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right: the stop ended in an exception although nothing advertised any more, so the core kept the set running and ignored the next start. Fixed in 50274d8: a publisher Windows aborts while the set waits for it to stop now counts as stopped. The start path still fails with the same transient BleException on an abort.
Ran the radio tests and stop/start cycles on Windows, plus attempts to turn the radio off while StopAsync runs. Every stop completed and the set advertised again after the radio came back on. None of the attempts caught the abort exactly during the stop, so that path is covered by reading the code only.
Comment generated by claude-opus-5-5 on behalf of Ole
v1's WinRT backend sat on base classes that v2 removed. It now implements the drivers for all four roles. Where Windows cannot meet the driver contract, the driver says so: a request counts as sent once its Windows call started, a write without response is handed off at once, a connection ends for this app while Windows may keep the link, a central becomes known through its first request, and advertising does not stop when a central connects.
The tests need Windows' radio and an nRF52840 dongle running HciHost on the same machine, so CI only builds them.
A set the radio stopped still counted as advertising, so StartAsync did nothing and the app could not get it advertising again.
When the Bluetooth radio turned off, scans hung without reports and advertising sets stopped without anyone noticing. Scans and connectable sets started while the radio is off waited silently, or forever.
An advertisement was stored as one HCI report, whose one-byte length cut extended data over 255 bytes, and HciHost delivered each part of an advertisement the controller split as an advertisement of its own. Every backend now reports the whole advertisement as values.
The driver interfaces are written for this library's backends and will gain members, e.g. backend features; a backend written elsewhere against 2.0 would break with them. Using them now reports DARPBLE0001.
…t it An address saved as text and parsed back had lost its type, so it matched no scanned device and HciHost could not connect to it. The "T" format keeps the type, Parse reads it and rejects malformed text, and an address without a type matches and connects by its value: HciHost uses the type it last heard the address with.
Of a failed LE Connection Complete event only the status is valid. A failure whose role field read peripheral was ignored, so the attempt waited until its caller cancelled, holding the controller's only attempt and keeping scans paused.
A central that connected while a set with autoRestart was being disposed could still admit a restart, which disposal then waited for until the central disconnected.
StopAsync cancelled a pending restart before it marked the set as stopping, so a central connecting in between still admitted a restart, and the set advertised again once that central disconnected.
… again Windows may return the service objects a connection already wraps; releasing the duplicates closed them, so earlier characteristics threw ObjectDisposedException.
Windows exchanges the MTU itself, usually after the connection is handed out, so writes stayed limited to 20 bytes. Backends now report a larger MTU through IConnectionSink.OnMtuChanged.
Windows exchanges the MTU itself about 0.2 s after the session is active, so a connection handed out at once only allowed 20-byte writes. Windows reports only a larger MTU, so the wait is capped at 500 ms; an MTU that grows later still reaches the sink.
…t while stopping The stop failed although nothing advertised any more, so the core kept the set running and ignored the next start.
50274d8 to
1a81b64
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a81b649b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Task stopped = WaitForAsync(publisher, BluetoothLEAdvertisementPublisherStatus.Stopped, CancellationToken.None); | ||
| publisher.Stop(); | ||
| await stopped.ConfigureAwait(false); |
There was a problem hiding this comment.
If the radio turns off after the publisher.Status check but before WaitForAsync attaches its handler, the publisher's Aborted event is lost. Calling Stop() on that already-aborted publisher need not produce another status change, so this non-cancellable wait can remain pending indefinitely, blocking the set's subsequent operations and disposal. Although the earlier abort-during-stop fix now accepts an observed Aborted event, this check-before-subscribe window still misses it entirely. Attach the handler before checking the current terminal status, and complete the wait if the publisher has already stopped or aborted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right: the abort could come between the status check and the subscription, and nothing would complete the wait. Fixed in b247d96: StopPublisherAsync subscribes first, then reads the status, and completes on Stopped or Aborted.
Comment generated by claude-opus-5-5 on behalf of Ole
…ping An abort between reading the status and subscribing was lost, and Windows need not report another change for an aborted publisher, so the stop could hang.
Summary
v1's WinRT backend sat on base classes that v2 removed, so the v2 solution had no Windows backend. This PR ports it to the driver layer for all four roles on Windows' default adapter.
The core keeps the queues, timeouts and app-facing types; the drivers wrap Windows' APIs, and where Windows cannot meet a driver contract, the driver's docs say so. The bookkeeping that needs no Windows (central sessions, subscription lists, advertising sections) builds for both target frameworks and is unit-tested on Linux.
The PR also changes the core and the other backends:
BleAddresskeeps its type in text with the"T"format, and an address without a type matches and connects by its value.DARPBLE0001, as it will gain members.Changes
Review
WinRTPeripheralDriver.cswithWinRTHostedService.cs,WinRTConnectionDriver.cs,CentralSessions.cs,WinRTAdvertisingSetDriver.cswithWinRTRadio.cs, and HciHost'sAdvertisingReportAssembler.cs.ConnectAsyncwaits up to 500 ms for it. A later change reaches the core through the newIConnectionSink.OnMtuChanged(int), soMaxWritePayloadLengthgrows.BleException. Windows restarts connectable sets itself.IAdvertisingSetSink.OnFailed(BleException), soStartAsyncstarts it again.autoRestartthat is stopped or disposed no longer restarts for a central that connected meanwhile, HciHost ends a connect attempt on any failed connection event, and characteristics found earlier keep working after services are discovered again.Testing
Impact
WinBleFactoryandAddWinRT(Action<WinBleFactory>)are replaced byAddWinRT().net10.0-windows10.0.19041.0or later. On other targets it offers no adapter.IGapAdvertisement.AsReadOnlyMemory()is removed: read the properties, e.g.Data.GapAdvertisement.FromExtendedAdvertisingReport(...)is renamed toGapAdvertisement.Create(...), and its overload that took report bytes is removed.AddBackendorGapAdvertisement.CreatesuppressesDARPBLE0001.BleAddress.Parserejects text with anything after the address.ConnectAsynctakes up to 500 ms longer, while Windows exchanges the MTU. A peripheral that keeps MTU 23 waits the whole 500 ms, as Windows reports no exchange that leaves the MTU unchanged.Checklist
PR generated by claude-opus-5-5 on behalf of Ole