wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.08k stars 611 forks source link

Allow Sendables to be logged to DataLog #5513

Open Gold856 opened 1 year ago

Gold856 commented 1 year ago

Is your feature request related to a problem? Please describe. Sendable currently allows data from various components to be declaratively published over NetworkTables. However, bandwidth is always a concern, and NetworkTables can be slow, and not all data needs to be viewed live during robot operation (sometimes it's just good to record diagnostic data).

Describe the solution you'd like DataLog should have the ability to log Sendables, as DataLog is faster, and records data to local storage instead of using bandwidth to publish over the network.

Additional context This issue serves as a place to discuss and make decisions about how to get the infrastructure surrounding Sendable into a form where DataLog can be added as a 2nd backend. When all the infrastructure is in place, I will gladly modify DataLog so it can log Sendables.

Also, would it be possible to pull this off by kickoff?

Starlight220 commented 1 year ago

I think the answer would be to add an set of interfaces that the *Publisher and *LogEntry interfaces would inherit from, and then an abstract class/interface that would be subclassed per-backend and create the appropriate objects. I do want to find a solution for the explosion of typed overloads of add*Property etc; haven't had a good idea for that yet.

Gold856 commented 1 year ago

abstract class/interface that would be subclassed per-backend and create the appropriate objects.

You just described SendableBuilder and SendableBuilderImpl. This is why I think multiple builders is the way to go.

For the typed overloads, in C++, I think you can do something with templates and concepts to restrict the types a function will accept, which would allow you to accept something like std::function<T()> for all compatible types. Java's lack of reified generics make it harder, so maybe addProperty accepts Supplier<T> and uses instanceof to ensure the type is correct and to create the specific Publisher and LogEntry? Even if instanceof is slow, it only needs to be done once for each property, so the slowdown will happen at start time, and performance shouldn't be terrible at runtime.

void addProperty(String key, Supplier<T> getter, Consumer<T> setter) {
  T val = getter.get();
  if (val instanceof String s) {
    // String code
  } else if (val instanceof Long s) {
    // long code
  }
  //...
}
Starlight220 commented 1 year ago

You just described SendableBuilder and SendableBuilderImpl. This is why I think multiple builders is the way to go.

There is so much functionality in SendableBuilderImpl beyond this, and duplicating it per backend isn't a good idea. My suggestion is to extract the backend-specific elements (the publisher types used) to a separate class, and then SendableBuilderImpl can be folded into SendableBuilder as it won't be coupled to a specific backend.

What use cases do you expect to involve multiple SendableBuilder objects for the same Sendable?

As for the typed overloads, generics don't work with primitives -- your solution involves a lot of boxing, which has a performance cost.

Gold856 commented 1 year ago

I'd like to have the ability to log and publish a Sendable at the same time. I might have encoder data broadcast over NT and show up on a dashboard to analyze a mechanism live, and also have it logged for analysis at a later time and to catch data I might've missed. While all NT traffic can be logged, I'd like to control what gets logged and/or published so my logs aren't cluttered.