-
Notifications
You must be signed in to change notification settings - Fork 41
Dynamically deserialize networktables structs #225
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for taking so long to get to this!
I'm very impressed that you were able to get this implemented! In the past I've avoided adding struct decoding since it felt out of scope with Elastic, but with more recent (and planned) struct integration throughout WPILib it would make sense to have the ability to display (but not edit) individual struct fields.
That being said though, there's still some things I would like to have changed/fixed. Firstly, I'm a bit confused between the difference between NT4StructMeta
and DynStructSchema
. I'm also not sure if it entirely makes sense to have the widgets store the entire schema in its json, since it means that any time a struct field widget is created, it will assume that the schema has never changed. For example, if a widget is created with a struct schema for some type of Swerve State, but then the dashboard connects to a robot with a different struct that has the same name, the widget will always assume that it's the same which can cause invalid values to be displayed. It might make sense to instead store the name of the struct and its field, and have it looked up whenever a new value comes in.
Lastly, I would want to have unit tests for the schema parsing, decoding, and displaying. Right now that might not be easy to add since there's a lot of stuff still being changed around, but I would definitely want lots of these new features in unit tests before being merged. They can be a pain to write, but they've saved me from breaking a lot of important features.
lib/pages/dashboard_page.dart
Outdated
} catch (e) { | ||
_showJsonLoadingError(e.toString()); | ||
return; | ||
} | ||
|
||
await preferences.setString(PrefKeys.layout, jsonEncode(jsonData)); | ||
await preferences.setString(PrefKeys.layoutPath, file.path); | ||
|
||
setState(() => _loadLayoutFromJsonData(jsonString)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The layout is stored in Shared Preferences to avoid an issue where deleting the file would delete the layout too. It could make sense to have the last path as an "initial directory" for the user, but when importing a layout I don't think it would be expected for that to turn into the layout location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I made this change cuz I was kind of annoyed when I clicked save and elastic didn't save to the file. I can make this save to user prefs as well and load it when the json is deleted.
lib/services/nt_connection.dart
Outdated
if (topic.type == "structschema") { | ||
subscribe(topic.name).listen((data, _) { | ||
if (data == null) return; | ||
String schema = String.fromCharCodes(data as List<int>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer having some sort of type checking, since otherwise what could happen is the app suddenly breaks without any notification to the user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type checking as in? That's kind of the only way (that I know how, at least) to check whether an NT field is a struct schema... Struct values are prefixed with struct:
.
Probably misunderstood you, not really sure what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also wouldn't the app break if we suddenly switched some published NT field from a string to a double between launches?
Also, I'm like 80% sure the code as it stands will bail out by displaying the raw bytes if the schema suddenly changes like that
lib/services/nt_widget_builder.dart
Outdated
}); | ||
|
||
typedef NTModelProvider = NTWidgetModel Function({ | ||
String dataType, | ||
required NTConnection ntConnection, | ||
double period, | ||
required SharedPreferences preferences, | ||
required NT4StructMeta? ntStructMeta, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a nullable field should be required, since for models that don't depend on a struct subscription (any multi-topic widget), it doesn't entirely make sense for it to have to be given a struct meta
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to do this without the required field, the register() function would need an overhaul, probably with a seperate type for nt multitopic model providers.
I'll begin work on this
|
||
bool lastElement = currentTopic == topic; | ||
// bool lastElement = currentTopic == topic; | ||
|
||
if (current != null) { | ||
if (current.hasRow(row)) { | ||
current = current.getRow(row); | ||
} else { | ||
current = current.createNewRow( | ||
topic: currentTopic, | ||
name: row, | ||
ntTopic: (lastElement) ? nt4Topic : null); | ||
current = | ||
current.createNewRow(topic: currentTopic, name: row, entry: null); | ||
} | ||
} else { | ||
if (shuffleboardTreeRoot.hasRow(row)) { | ||
current = shuffleboardTreeRoot.getRow(row); | ||
} else { | ||
current = shuffleboardTreeRoot.createNewRow( | ||
topic: currentTopic, | ||
name: row, | ||
ntTopic: (lastElement) ? nt4Topic : null); | ||
topic: currentTopic, name: row, entry: null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't any issue here but I do plan on removing the Shuffleboard API support for 2026, so these changes won't be needed
|
||
import 'package:dot_cast/dot_cast.dart'; | ||
|
||
class DynStructField { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer keeping class names less truncated, maybe rename this to DynamicStructField
?
sealed class DynStructValue { | ||
final Object? anyValue; | ||
|
||
DynStructValue({required this.anyValue}); | ||
|
||
bool get boolValue => (this as DynStructBoolean).value; | ||
int get intValue => (this as DynStructInt).value; | ||
int get longValue => (this as DynStructLong).value; | ||
double get floatValue => (this as DynStructFloat).value; | ||
double get doubleValue => (this as DynStructDouble).value; | ||
String get stringValue => (this as DynStructString).value; | ||
DynStructValue? get nullableValue => (this as DynStructNullable).value; | ||
List<DynStructValue> get arrayValue => (this as DynStructArray).value; | ||
DynStruct get structValue => (this as DynStructStruct).value; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be possible to have this be a generic class (like DynamicStructValue<T>
) instead of having to cast, and the value
field can be of type T?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I come from rust and was trying to use an enum lol
There's also the problem that NT fields can only be of a certain few types. Giving T the possibility to be any type, at least in my opinion, is probably not the best, unless it can be constrained in some way I don't know about
Thanks for reviewing it and sorry for the late reply. Comps keep me busy! DynStructSchema is a data class that contains the schema of the struct (i.e. fields and field names) NT4StructMeta contains a NT path to the root value (containing the entire struct and all of its data), a path to the subvalue (e.g. I'm 80% sure the code as it stands will bail out by showing raw bytes if the schemas don't line up for some reason. |
I went ahead and removed ntStructMeta from the multitopic widgets |
I just rebased this branch since the merges were getting very messy. I think I preserved the proper changes but I'm not entirely sure. Let me know when you would like me to re-review this, since I'd like to add this into the 2026 beta. |
i was in the middle of adding typechecking to nt types so I committed those changes too. im going to mark this as a draft until i finish fixing tests |
Also there's a problem where widgets in list layouts are too small. Dunno why that's happening; afaik I didn't change anything relating to that |
Very simply, we get struct information by listening for all announced topics and if it is has type
structschema
, we store it and parse as-needed into aDynStructSchema
(lib/services/struct_schemas/dyn_struct.dart
). When we subscribe to or get data from a topic that uses a schema, we provideDynStructSchema
along with theUint8List
toDynStruct
which recursively parses struct fields and values.I also made saving actually save to a file