Skip to content

Commit

Permalink
Fixes for NT-backed widgets when the server restarts
Browse files Browse the repository at this point in the history 
Widgets were getting destroyed sources added, but weren't replacing the backing NT source. These widgets would be permanently disabled and would need to be deleted and recreated, or a dashboard application restart, to be usable again

Topics written to by the dashboard would not receive "unpublish" events from the server, so they'd always be active and available. The `allUris` observable list was incorrectly adding/removing entries so URIs for those topics could still be removed from the list (disabling connected widgets with a destroyed source) if those URIs happened to be at the same index in the `allUris` list as a removed item in the plugin's URI list

Topics written to by the dashboard would not receive a "publish" event from the server on reconnect, so no data sources would be created for them. We now manually trigger the data source creation logic for every topic that still exists in the NT client upon a server reconnect to ensure those sources are recreated
  • Loading branch information
@SamCarlberg
SamCarlberg committed Jul 2, 2024
1 parent aa0af6b commit 5923cd4
Show file tree
Hide file tree
Showing 3 changed files with 127 additions and 51 deletions.
24 changes: 18 additions & 6 deletions api/src/main/java/edu/wpi/first/shuffleboard/api/sources/SourceTypes.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,13 @@
import edu.wpi.first.shuffleboard.api.data.DataType;
import edu.wpi.first.shuffleboard.api.data.DataTypes;
import edu.wpi.first.shuffleboard.api.sources.recording.TimestampedData;
import edu.wpi.first.shuffleboard.api.util.PropertyUtils;
import edu.wpi.first.shuffleboard.api.util.Registry;

import org.fxmisc.easybind.EasyBind;
import javafx.collections.ListChangeListener;

import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;

import javafx.beans.InvalidationListener;
import javafx.collections.FXCollections;
Expand Down Expand Up @@ -49,11 +47,25 @@ public SourceTypes() {
register(Static);

typeNames.addListener((InvalidationListener) __ -> {
Optional<ObservableList<String>> names = typeNames.stream()
typeNames
.stream()
.map(this::forName)
.map(SourceType::getAvailableSourceUris)
.reduce(PropertyUtils::combineLists);
names.ifPresent(l -> EasyBind.listBind(allUris, l));
.forEach(observableUriList -> {
observableUriList.addListener((ListChangeListener<? super String>) c -> {
while (c.next()) {
if (c.wasAdded()) {
for (String uri : c.getAddedSubList()) {
if (!allUris.contains(uri)) {
allUris.add(uri);
}
}
} else if (c.wasRemoved()) {
allUris.removeAll(c.getRemoved());
}
}
});
});
});
}

Expand Down
26 changes: 26 additions & 0 deletions api/src/main/java/edu/wpi/first/shuffleboard/api/widget/SingleSourceWidget.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import javafx.beans.property.ObjectProperty;
import javafx.beans.property.Property;
import javafx.beans.property.SimpleObjectProperty;
import javafx.collections.ListChangeListener;

/**
* A partial implementation of {@code Widget} that only has a single source.
Expand All @@ -14,6 +15,31 @@ public abstract class SingleSourceWidget extends AbstractWidget {

protected final ObjectProperty<DataSource> source = new SimpleObjectProperty<>(this, "source", DataSource.none());

public SingleSourceWidget() {
// Bidirectional binding to make the sources list act like a single-element wrapper around
// the source property
source.addListener((__, oldSource, newSource) -> sources.setAll(newSource));

sources.addListener(new ListChangeListener<DataSource>() {
@Override
public void onChanged(Change<? extends DataSource> c) {
while (c.next()) {
if (c.wasAdded()) {
var added = c.getAddedSubList();
if (!added.isEmpty()) {
var s = added.get(0);
if (s != source.get()) {
source.set(s);
}
}
} else if (c.wasRemoved()) {
source.set(DataSource.none());
}
}
}
});
}

@Override
public final void addSource(DataSource source) throws IncompatibleSourceException {
if (getDataTypes().contains(source.getDataType())) {
Expand Down
128 changes: 83 additions & 45 deletions .../java/edu/wpi/first/shuffleboard/plugin/networktables/sources/NetworkTableSourceType.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package edu.wpi.first.shuffleboard.plugin.networktables.sources;

import edu.wpi.first.networktables.Topic;
import edu.wpi.first.networktables.TopicInfo;
import edu.wpi.first.shuffleboard.api.data.DataType;
import edu.wpi.first.shuffleboard.api.data.DataTypes;
Expand All @@ -19,6 +20,8 @@
import edu.wpi.first.networktables.NetworkTableInstance;
import edu.wpi.first.networktables.PubSubOption;

import com.google.gson.Gson;

import java.util.EnumSet;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -50,57 +53,47 @@ public NetworkTableSourceType(NetworkTablesPlugin plugin) {
plugin.serverIdProperty().addListener((__, old, serverId) -> setConnectionStatus(serverId, false));
inst.addConnectionListener(true,
event -> setConnectionStatus(plugin.getServerId(), event.is(NetworkTableEvent.Kind.kConnected)));

inst.addConnectionListener(true, event -> {
AsyncUtils.runAsync(() -> {
if (event.is(NetworkTableEvent.Kind.kDisconnected)) {
// Explicitly clear everything on disconnect.
// Topics that are written to by the dashboard are cached locally and do not receive an
// "unpublish" event from the server, so the topic listener would never fire to remove
// their associated data sources and widgets would still be controllable when the server
// shuts down.
availableSourceIds.clear();
availableSources.clear();
availableDataTypes.clear();
} else if (event.is(NetworkTableEvent.Kind.kConnected)) {
// For every topic that's still retained by the client, create a fake event to regenerate
// the sources. We only subscribe to the available topics, not their values, so because
// the server will not send a "publish" event for a topic that is already cached in
// shuffleboard, the topic-only subscribe is never triggered for these topics and we would
// incorrectly assume they've gone away.
for (Topic topic : inst.getTopics()) {
handleEvent(
new NetworkTableEvent(
inst,
0,
0x0080 | 0x0008, // local, publish
null,
topic.getInfo(),
null,
null,
null));
}
}
});
});

subscriber = new MultiSubscriber(inst, new String[] {""}, PubSubOption.topicsOnly(true));
topicListener = inst.addListener(
subscriber,
EnumSet.of(
NetworkTableEvent.Kind.kImmediate,
NetworkTableEvent.Kind.kTopic),
event -> {
AsyncUtils.runAsync(() -> {
final boolean delete = event.is(NetworkTableEvent.Kind.kUnpublish);
final TopicInfo topicInfo = event.topicInfo;
if (topicInfo.name.endsWith("/.type") && !delete) {
// Got type metadata for composite data
var compositeSourceId = toUri(topicInfo.name.substring(0, topicInfo.name.length() - 6));
var topic = topicInfo.getTopic();
var typeName = topic.getProperty("SmartDashboard");
if ("null".equals(typeName)) {
// Metadata property hasn't been set, fall back to use the generic map data type
availableDataTypes.put(compositeSourceId, DataTypes.Map);
} else {
var dataType = DataTypes.getDefault().forName(typeName).orElse(DataTypes.Map);
availableDataTypes.put(compositeSourceId, dataType);
}
}

final String name = NetworkTableUtils.topicNameForEvent(event);
List<String> hierarchy = NetworkTable.getHierarchy(name);
for (int i = 0; i < hierarchy.size(); i++) {
String uri = toUri(hierarchy.get(i));
if (i == hierarchy.size() - 1) {
// The full key
if (delete) {
availableSources.remove(uri);
availableDataTypes.remove(uri);
Sources sources = Sources.getDefault();
sources.get(uri).ifPresent(sources::unregister);
NetworkTableSource.removeCachedSource(uri);
} else {
var dataType = NetworkTableUtils.dataTypeForTypeString(topicInfo.typeStr);
availableSources.put(uri, dataType.getName());
availableDataTypes.put(uri, dataType);
}
}
if (delete) {
availableSourceIds.remove(uri);
availableDataTypes.remove(uri);
} else if (!availableSourceIds.contains(uri)) {
availableSourceIds.add(uri);
}
}
});
});
event -> AsyncUtils.runAsync(() -> handleEvent(event)));
}

@Override
Expand Down Expand Up @@ -198,4 +191,49 @@ public String toUri(String sourceName) {
return super.toUri(NetworkTable.normalizeKey(sourceName));
}

private void handleEvent(NetworkTableEvent event) {
final boolean delete = event.is(NetworkTableEvent.Kind.kUnpublish);
final TopicInfo topicInfo = event.topicInfo;
if (topicInfo.name.endsWith("/.type") && !delete) {
// Got type metadata for composite data
// Remove trailing "/.type"
var compositeSourceId = toUri(topicInfo.name.substring(0, topicInfo.name.length() - 6));
var topic = topicInfo.getTopic();
var typeNameJson = topic.getProperty("SmartDashboard");
if ("null".equals(typeNameJson)) {
// Metadata property hasn't been set, fall back to use the generic map data type
availableDataTypes.put(compositeSourceId, DataTypes.Map);
} else {
var typeName = new Gson().fromJson(typeNameJson, String.class);
var dataType = DataTypes.getDefault().forName(typeName).orElse(DataTypes.Map);
availableDataTypes.put(compositeSourceId, dataType);
}
}

final String name = NetworkTableUtils.topicNameForEvent(event);
List<String> hierarchy = NetworkTable.getHierarchy(name);
for (int i = 0; i < hierarchy.size(); i++) {
String uri = toUri(hierarchy.get(i));
if (i == hierarchy.size() - 1) {
// The full key
if (delete) {
availableSources.remove(uri);
availableDataTypes.remove(uri);
Sources sources = Sources.getDefault();
sources.get(uri).ifPresent(sources::unregister);
NetworkTableSource.removeCachedSource(uri);
} else {
var dataType = NetworkTableUtils.dataTypeForTypeString(topicInfo.typeStr);
availableSources.put(uri, dataType.getName());
availableDataTypes.put(uri, dataType);
}
}
if (delete) {
availableSourceIds.remove(uri);
availableDataTypes.remove(uri);
} else if (!availableSourceIds.contains(uri)) {
availableSourceIds.add(uri);
}
}
}
}

0 comments on commit 5923cd4

Please sign in to comment.