Skip to content

More robust radio bridge connection #16

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.

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

Merged
merged 1 commit into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 23 additions & 4 deletions lib/usb-radio-bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ export class MicrobitRadioBridgeConnection
private logging: Logging;
private serialSession: RadioBridgeSerialSession | undefined;
private remoteDeviceId: number | undefined;
private disconnectPromise: Promise<void> | undefined;
private serialSessionOpen = false;

private delegateStatusListner = (e: ConnectionStatusEvent) => {
const currentStatus = this.status;
Expand All @@ -53,7 +55,10 @@ export class MicrobitRadioBridgeConnection
this.serialSession?.dispose();
} else {
this.status = ConnectionStatus.NOT_CONNECTED;
if (currentStatus === ConnectionStatus.NOT_CONNECTED) {
if (
currentStatus === ConnectionStatus.NOT_CONNECTED &&
this.serialSessionOpen
) {
this.serialSession?.connect();
}
}
Expand Down Expand Up @@ -96,6 +101,9 @@ export class MicrobitRadioBridgeConnection
}

async connect(): Promise<ConnectionStatus> {
if (this.disconnectPromise) {
await this.disconnectPromise;
}
Comment on lines +104 to +106
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed I'll replace this with a generic async state change queue.

// TODO: previously this skipped overlapping connect attempts but that seems awkward
// can we... just not do that? or wait?

Expand All @@ -108,7 +116,10 @@ export class MicrobitRadioBridgeConnection
message: "Serial connect start",
});

await this.delegate.connect();
if (this.delegate.status !== ConnectionStatus.CONNECTED) {
await this.delegate.connect();
}

try {
this.serialSession = new RadioBridgeSerialSession(
this.logging,
Expand Down Expand Up @@ -137,6 +148,7 @@ export class MicrobitRadioBridgeConnection
);

await this.serialSession.connect();
this.serialSessionOpen = true;

this.logging.event({
type: "Connect",
Expand All @@ -154,7 +166,14 @@ export class MicrobitRadioBridgeConnection
}

async disconnect(): Promise<void> {
await this.serialSession?.dispose();
if (this.disconnectPromise) {
return this.disconnectPromise;
}
this.disconnectPromise = (async () => {
this.serialSessionOpen = false;
await this.serialSession?.dispose();
this.disconnectPromise = undefined;
})();
}

private setStatus(status: ConnectionStatus) {
Expand Down Expand Up @@ -260,7 +279,6 @@ class RadioBridgeSerialSession {
this.delegate.addEventListener("serialerror", this.serialErrorListener);

try {
await this.delegate.softwareReset();
await this.handshake();
this.onStatusChanged(ConnectionStatus.CONNECTED);

Expand Down Expand Up @@ -318,6 +336,7 @@ class RadioBridgeSerialSession {
this.responseMap.clear();
this.delegate.removeEventListener("serialdata", this.serialDataListener);
this.delegate.removeEventListener("serialerror", this.serialErrorListener);
await this.delegate.softwareReset();

this.onStatusChanged(ConnectionStatus.NOT_CONNECTED);
}
Expand Down
3 changes: 2 additions & 1 deletion lib/usb.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,8 @@ export class MicrobitWebUSBConnection
};
this.removeEventListener = (type, ...args) => {
this._removeEventListener(type, ...args);
if (--this.addedListeners[type] === 0) {
if (--this.addedListeners[type] <= 0) {
this.addedListeners[type] = 0;
Comment on lines +152 to +153
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could better manage our own callers as well. Continually clicking disconnect got us into negative numbers.

this.stopNotifications(type);
}
};
Expand Down