DIRCON server lifetime: give the endpoint process lifetime (steps 1-2) - #2
Closed
nickolas122 wants to merge 10 commits into
Closed
DIRCON server lifetime: give the endpoint process lifetime (steps 1-2)#2nickolas122 wants to merge 10 commits into
nickolas122 wants to merge 10 commits into
Conversation
Handoff for continuing this work in another session, on another machine. Covers the four discovery faults already fixed on kind-of-stable and which commit closed each, so nobody re-debugs a solved one; the remaining problem, which is that the DIRCON listener and its mDNS advertisement are created when a bike connects and destroyed when it leaves; and a staged plan for giving that endpoint the lifetime of the process instead. Includes the attach/detachDirconManager precedent already in the tree, the macro tables that generate the advertised service set and the question of what to advertise before a device type is known, the mDNS probe technique with the two traps that cost time the first time round, and the environment constraints - fork-only CI, Smart App Control, the trimmed matrix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Re-read the document against the code it points at. One claim was wrong, two line references were off, and the plan was missing the detail that decides whether it works. Corrections: - "a bike that disconnects mid-session" was listed alongside hard kills and crashes as leaving a record over a dead port. It does not. Either the device object survives - the normal path, since every connect(..., SLOT(restart())) in bluetooth.cpp is commented out and the only live restart() call is the gym-mode handler at bluetooth.cpp:3786 - and the listener stays up with the record valid; or it is destroyed and ~ProviderPrivate() sends the goodbye first. The window that matters is the startup one, which is what the cited Rouvy log actually shows. - virtualbike.cpp:47 pointed at the else-if; the construction is :48. - The macro tables run to dirconmanager.cpp:86, not :70 - :12-70 cuts off the Zwift Play characteristics at :84-86. Additions: - Scope section: bikes only, desktop only. Treadmill and iOS paths are context, not work. - Step 2 named only the notifiers as holding the device pointer. It is copied into three places: DirconManager::bt, the notifiers, and the write processors at dirconmanager.cpp:212-215. The write processors are the control path, so one left dangling is a use-after-free on a client write, not a stale reading. - Step 3 preferred "rebuild the service set if a treadmill attaches". The listening port is derived from the machine type (server_base_port + DM_MACHINE_##DESC, dirconmanager.cpp:143), so that rebuild moves the listener 36866 -> 36869 and invalidates every cached record - the exact failure the refactor removes. Fixed the profile at bike instead, which the scope now permits. - Step 4 is a no-op under that scope; kept the reasoning for why a rebuild would have to send its goodbye before probing, since the service name comes from dircon_id and old and new records collide. - attachDirconManager() re-parents (virtualbike.cpp:570), which is what must not happen once the manager outlives the device. - Risks: three checks that are silent when wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y4agZ4NHtMVSyAYuN7fVbz
Step 1 of the DIRCON server lifetime refactor. Pure refactor: nothing calls setDevice() yet, ownership is unchanged, and the endpoint still appears when a bike connects. This reorders the plan. The handoff suggested moving ownership first with the device still required, but that is not safe on its own: a process-lifetime manager keeps serving whichever device it was built for, and bluetooth::restart() deletes and rebuilds the device objects (reachable from the gym-mode handler at bluetooth.cpp:3786), leaving the manager dereferencing a freed pointer. The rebinding has to exist before ownership moves. The device pointer was copied into three places, all of which now rebind together in DirconManager::setDevice(): - DirconManager::bt - the ten CharacteristicNotifier objects. Each subclass declared its own bluetoothdevice *Bike; that member is hoisted into the base class, which gains setDevice(). Subclass constructor signatures are unchanged, so every call site in virtualbike and virtualtreadmill is untouched, and subclass bodies keep referring to `Bike` - it just resolves to the inherited member now. - the write processors 2AD9, E005 and 0003, via a setDevice() on their shared base. These are the control path: a stale pointer here turns a client's power or resistance write into a use-after-free rather than a stale reading. setDevice() drives the notifier chain through DM_CHAR_NOTIF_OP, the same macro list that builds the notifiers, so the two cannot drift apart. Verified the expansion covers all ten. Neither the TCP listener nor the mDNS advertisement is touched by setDevice(), which is the property the rest of the refactor depends on. Not built: this container has no Qt, so the change is verified by inspection and macro expansion only. It needs a CI build before it is trusted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y4agZ4NHtMVSyAYuN7fVbz
Step 2 of the refactor. The TCP listener and the mDNS advertisement now survive a bike disconnecting: a reconnect rebinds the device through setDevice() without a re-probe or a re-announcement, so a client holding a cached discovery record keeps finding someone listening on 36866. DirconManager::shared() owns the endpoint, parented to the application object. It is created on first use and rebound on every later call rather than built a second time, which would collide on the listening port. virtualbike now borrows it: - attachDirconManager() no longer re-parents. Adopting the manager would put it back under bluetoothdevice::setVirtualDevice(), which deletes the outgoing virtual device outright and would take the endpoint down with it on the next bridge switch - the failure this refactor exists to remove. - ~virtualbike() releases the device binding, but only when nothing detached the manager first and it is still bound to that virtual device's own bike. The bridge switches detach before building the successor, so they hand the endpoint over untouched; a device being destroyed with no successor drops the binding before it dangles. The manager can now tick with no device attached, so the paths that dereference it are guarded: bikeProvider() returns early, and writeProcess() on 2AD9, E005 and 0003 refuses the write instead of dereferencing a device that is not there. This is the part of step 3 that step 2 cannot do without. Treadmills stay out of scope and keep per-virtual-device ownership, but a shared endpoint left from a bike earlier in the session would still hold the same base port (both map to WAHOO_KICKR), so virtualtreadmill calls releaseShared() first. Without it, a treadmill connected after a bike in one run would fail to listen. Deleting the manager sends the mDNS goodbye through ~ProviderPrivate(), so clients are told before the socket goes. Still open: with no bike ever connected in a run, shared() has never been called and nothing is listening. That startup window is step 3. Not built: this container has no Qt. Verified by inspection only; needs a CI build before it is trusted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y4agZ4NHtMVSyAYuN7fVbz
Owner
Author
CI status
Every later step — Install Qt, Build, the lot — is reported as The job is gated on I have not touched it: creating a The jobs that actually compile the change are still running:
Generated by Claude Code |
Owner
Author
Build result: it compilesRun 31596576237 finished. 2 failed jobs out of 19, and both are the Everything that actually builds the change passed:
In This closes the open question on these two commits. The hoisting of What a green build still does not tell us: nothing here exercises the runtime behaviour. The PR will keep showing a red X until the Generated by Claude Code |
Step 3, and the point of the refactor. The listener and the advertisement now exist from process start, so a client that caches discovery results and does not retry a failed connect - Rouvy does both - can no longer end up holding a record for a port nobody is listening on. The documented workaround of connecting the bike in QZ before touching the trainer in Rouvy should no longer be needed. DirconManager::startIdleEndpoint() is called from main.cpp once the app is up, guarded by dircon_yes and idempotent. The constructor no longer requires a device: it took the machine type from Bike->deviceType(), which is now machineTypeFor(), returning the bike profile when there is nothing attached. Machine type is recorded on the manager and checked in shared(). A device of the other kind cannot reuse an endpoint - the listening port is derived from the machine type, server_base_port + DM_MACHINE_##DESC - so the old one is withdrawn, sending its mDNS goodbye, and the right one built. That covers an elliptical arriving through virtualbike, which would otherwise have been served a bike profile. The tick timer is stopped while no device is attached and restarted by setDevice(), rather than firing every 50ms into a handler that returns immediately. Deviating from the plan on one point: it called for serving zeroed values while idle. What this does instead is serve discovery and reads while idle and send no notifications until a device attaches. Discovery, characteristic discovery and reads already work with no device, since the read values in the DM_CHAR_OP table are static byte arrays rather than device readings, and under rouvy_compatibility the initial 0x2AD2 frame on connect is a hardcoded 29-byte zero payload. Fabricating zeroed notifications would instead mean null-handling inside all ten CharacteristicNotifier::notify() implementations, each of which dereferences the device throughout - a large change to the part of the code that currently works - and a stream of 0 W is not obviously better than silence, since a client could latch on to it as real trainer data. Whether Rouvy stays interested in an endpoint that answers discovery but sends nothing until a bike appears cannot be determined from this repository. If it needs traffic, the fallback is the notifier null-handling described above. Not built or run: verified by inspection here. Steps 1-2 built green on CI (run 31596576237); this commit has not been through it yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y4agZ4NHtMVSyAYuN7fVbz
stateChanged() is connected to every service object, so it fires once per service. The guard at the top returns early until all of them report ServiceDiscovered, but after the last one arrives the remaining firings all fall through and repeat the entire subscription pass. On a YPBM001264 that wrote the FTMS control point CCCD four times per session. A fresh connection tolerates the repeats. A reconnection does not: with the bike bonded, CCCD values persist across connections per the spec, and the device then rejects the rewrites. In a failing session all fourteen indication subscriptions were attempted and none were written, against four written in the session that worked, with descriptor write errors up from 25 to 41. The control point is indicate-only, so it was never subscribed, REQUEST_CONTROL was never acknowledged, and the bike streamed nothing but battery - which is notify-only and survived. Symptom is that QZ works once and then reports only battery until the bike is power cycled. servicesSubscribed gates the pass and is cleared in serviceScanDone(), where the service objects are rebuilt and any previous subscriptions stop referring to anything live. Not verified on hardware yet: it needs a CI build and a restart test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The trial timer starts unconditionally on Windows and macOS, then polls robertoviola.cloud:4010 with the configured email and closes the app after ten minutes if the reply is not OK. A self-compiled build has no way to satisfy that, so it cannot be run at all. Android already compiles the same code only when LICENSE is defined, and STEAM_STORE disables it everywhere. This extends the existing LICENSE flag to the desktop platforms rather than introducing a mechanism: official builds define it and behave exactly as before, self-compiled builds do not and run. QZ is GPLv3, which grants the right to modify and run your own build. All four guard sites move together - two in homeform.cpp and two in homeform.h. They have to stay identical: the header declares members and slots whose definitions live under the same condition, and moc emits calls to the slots, so any mismatch is a link error rather than a warning. This deliberately diverges from upstream behaviour and must not be sent there; it is for this fork only. It is not a way to avoid paying for QZ - support the author at https://www.patreon.com/bePatron?u=45290147 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
Step 3 compilesRun 31624887752 on
All three steps of the refactor are now implemented and compiling:
The endpoint listens on 36866 from process start and stays up for the life of the process. A bike binds to it, a bike going away unbinds, and neither touches the listener or the advertisement. What is still unverified, and it is the important part. Nothing here has been run. One open question that only a real test can answer: the idle endpoint answers discovery and reads but sends no notifications until a bike attaches. If Rouvy needs traffic to stay interested, the fallback is null-handling in the ten Unrelated to the DIRCON work, Generated by Claude Code |
tools/qz-rouvy-rtss/ already does this, but as a Python bridge that reads QZ over the WebSocket template and writes the RTSS shared memory. That existed because QZ ran on a tablet and Rouvy on the PC. Now that QZ runs on the same Windows box the network hop, the WebSocket template, the extra process and python are all avoidable: QZ can write the shared memory directly. RTSS draws inside the training app own frame through its D3D hook, so this is the only thing that survives exclusive fullscreen. An app claims an OSD slot by writing its name into the slot owner field, writes its text, and bumps dwOSDFrame to ask for a redraw. Layout is read from the header - dwOSDArrOffset, dwOSDEntrySize, dwOSDArrSize - rather than hardcoded, and whether the 4096 byte szOSDEx exists is decided from the entry size rather than from a version number, so RTSS versions that grew the entry still work. Verified against the SDK behaviour as implemented by RTSSSharedMemoryNET. Everything is best effort and silent. With RTSS absent OpenFileMapping simply fails, publish() does nothing and the attach is retried every five seconds rather than every tick. The signature is rechecked on every publish because RTSS restarting leaves us mapped onto a block it no longer owns. The slot is cleared on quit, otherwise RTSS keeps drawing the last gear over the game forever. A previously claimed QZ slot is reused, which is what recovers from a run that was killed rather than closed. Windows only; the methods compile to no-ops elsewhere so homeform needs no platform checks. No new setting, so nothing to reconcile in allsettingscount or settings-catalog.json - it activates only when RTSS is there to talk to. Not verified against a running RTSS yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8f198c5 skipped the whole subscription pass once it had run, which stopped the bike working at all. serviceScanDone() calls discoverDetails() on each service as it creates it, and on Windows that resolves before the loop reaches the next one, so the first stateChanged arrives with exactly one service in gattCommunicationChannelService. "All services discovered" is trivially true of a list of one. The pass therefore subscribed to 0x1800, latched, and every service built afterwards was skipped - including 0x1826. The log says it outright: two characteristics enumerated all session, 2a00 and 2a01, both Generic Access. No control point found, no CCCD written, and 71 consecutive "write of opcode 0 could not be queued" from the handshake ticking against a null gattFTMSService. The countdown on the console during that run came from another QZ instance on a tablet, not from this one. So the repeated passes the previous commit removed were not redundant, they were how services two through six got subscribed as the list grew. What is redundant is re-subscribing a service that already has been, which is what rewrote the control point's CCCD four times per session and what a reconnection rejects. A QSet of the service objects tells the two apart: each service is set up once, on whichever firing first sees it discovered, and later firings skip only what they have already done. Cleared in serviceScanDone() alongside the objects it refers to, so a recycled pointer cannot read as already subscribed. Untested on the bike. The check is 0x1826 in the log with its characteristics enumerated, "FTMS service and Control Point found", indications subscribed on 2ad9, descriptorWritten 02 00, and the handshake reporting control granted - then the same again after quitting and relaunching without touching the bike. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Opened to get CI to build this — the changes have never been compiled. The dev container has no Qt, so everything below was verified by inspection only. Please do not merge until the Windows job is green.
What is here
Steps 1 and 2 of the refactor described in
DIRCON-SERVER-REFACTOR.md, plus corrections to that document.Step 1 —
DirconManager::setDevice()rebinds the bound device. The pointer was copied into three places, all of which now rebind together:DirconManager::bt, the tenCharacteristicNotifierobjects, and the write processors 2AD9/E005/0003. Each notifier subclass declared its ownbluetoothdevice *Bike; that member is hoisted into the base class. Subclass constructor signatures are unchanged, so every call site invirtualbikeandvirtualtreadmillis untouched and subclass bodies keep referring toBike, now resolving to the inherited member. The rebinding walksDM_CHAR_NOTIF_OP, the same macro list that builds the notifiers, so the two cannot drift apart.Step 2 — process lifetime.
DirconManager::shared()owns the endpoint, parented to the application object, created on first use and rebound on later calls rather than built a second time (a second one would collide on the listening port).virtualbikeborrows it:attachDirconManager()no longer re-parents, and~virtualbike()releases the device binding only when nothing detached the manager first and it is still bound to that virtual device's own bike.Result: the TCP listener and the mDNS advertisement now survive a bike disconnecting. A reconnect rebinds the device with no re-probe and no re-announcement, so a client holding a cached discovery record keeps finding someone listening on 36866.
Why the plan was reordered
The handoff suggested moving ownership first with the device still required. That is not safe on its own: a process-lifetime manager keeps serving whichever device it was built for, and
bluetooth::restart()deletes and rebuilds the device objects, leaving the manager dereferencing a freed pointer. The rebinding has to exist first.Null-device guards pulled forward
The manager can now tick with nothing attached, so
bikeProvider()returns early andwriteProcess()on 2AD9/E005/0003 refuses the write rather than dereferencing a device that is not there. This is the part of step 3 that step 2 cannot do without.Treadmills
Out of scope and unchanged, with one exception: a shared endpoint left over from a bike earlier in the session would still hold the same base port (both map to
WAHOO_KICKR), sovirtualtreadmillcallsDirconManager::releaseShared()before building its own. Without it, a treadmill connected after a bike in one run would fail to listen.Document corrections
connect(..., SLOT(restart()))inbluetooth.cppis commented out — and the listener stays up with the record valid; or it is destroyed and~ProviderPrivate()sends the goodbye first.virtualbike.cpp:47→:48; the macro tables run todirconmanager.cpp:86, not:70.server_base_port + DM_MACHINE_##DESC). That invalidates every cached record — the exact failure the refactor removes. The profile is now fixed at bike.Still open
With no bike ever connected in a run,
shared()has never been called and nothing is listening. That startup window — the case in the 21:19:55 Rouvy log — is step 3.Note on CI
Scheduled runs on
masterare currently failing (12 Aug and 11 Aug). If the Windows job fails here, worth comparing againstmasterbefore assuming these commits are the cause.🤖 Generated with Claude Code
https://claude.ai/code/session_01Y4agZ4NHtMVSyAYuN7fVbz
Generated by Claude Code