allwpilib icon indicating copy to clipboard operation
allwpilib copied to clipboard

[wpiutil] Rewrite Sendable in the style of Struct and Protobuf

Open PeterJohnson opened this issue 5 months ago • 7 comments

Early draft / work in progress.

Early work is being done in separate namespace to ease review and enable gradual transition during implementation. Final version will be full replacement (breaking change).

One key change for C++ is to make the SendableTable class have concrete (not virtual) functions, so that future function additions do not break ABI. The backend class will have virtual functions. This does mean that any templated functions (e.g. for struct and protobuf) will need to resolve to a concrete raw function at this class level (WIP).

This uses a "flat" style of function calls for various data types. It's a bit ugly, but it avoids intermediate calls/objects such as an equivalent to the NT Topic class.

Dealing with close(): the idea is to use SendableSet to close the SendableTable when the object is closed. At some level (where?) a closed SendableTable should be deleted, and certainly its functions should no longer call the object functions. C++ destructors should work similarly to notify the SendableTables the object no longer exists (WIP), but in that case it's even more important to disable later calls.

Things to discuss:

  • The current approach doesn't support getting or setting timestamps. While this might be nice to have, adding it will add at least 2 functions per data type.
  • Are we removing LiveWindow?
  • The SendableTable class has a very similar interface to what a unified Telemetry class (successor to a SmartDashboard class) would have. I think it may be possible to unify this (or have SendableTable be the base class for Telemetry).
  • How do we handle the same object being published to two places? SendableTable handles it to first order, but do the getters get called for each one? What if a dashboard sets the value to one place, how does that get propagated to the other?

Fixes #5513. Fixes #5413.

PeterJohnson avatar Mar 20 '24 07:03 PeterJohnson

You've started to do this with PubSubOption, but I suggest having an easy way to add full metadata -- this will lay the groundwork for changing the way Shuffleboard etc metadata is done.

Actually having metadata be a PubSubOption could be an approach? Assuming metadata is static, that is? Or if we're fine with metadata being dynamic, we could just add functions to access it. We were probably on a path where PubSubOption isn't a direct copy/move of the NT one anyway, so we just need to figure out a new name for it. We might also split publish and subscribe options.

PeterJohnson avatar Mar 20 '24 17:03 PeterJohnson

Regarding https://github.com/wpilibsuite/allwpilib/commit/276e65d62e627e8a0838ecd544dbec65a8649205, I think booleanPublisher (without the add) would flow better and is less verbose.

Starlight220 avatar Apr 06 '24 17:04 Starlight220

Won't it be confusing to have both publishBoolean() and booleanPublisher()?

PeterJohnson avatar Apr 06 '24 17:04 PeterJohnson

How is it better with addBooleanPublisher?

An API idea I've been thinking of is breaking out the overloads: ie, there'd be one booleanPublisher(String) function that returns an object that can be used for either of the three forms. The problem is that it requires an intermediate type, per type (for Java).

Starlight220 avatar Apr 06 '24 18:04 Starlight220

If we did that we would just end up recreating all of the NT type-specific classes ala Topic, and it creates a potential lifetime management issue too, plus it's just more verbose from the user point of view.

Or we could just return to my original concept of overloading the two versions using the same name?

PeterJohnson avatar Apr 07 '24 06:04 PeterJohnson