Skip to content
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

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

Closed
wants to merge 60 commits into from

Conversation

PeterJohnson
Copy link
Member

@PeterJohnson PeterJohnson commented Mar 20, 2024

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.

Copy link
Member

@Starlight220 Starlight220 left a comment

Choose a reason for hiding this comment

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

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.

This looks like a very very good start!


void publishDouble(String name, DoubleSupplier supplier, PubSubOption... options);

DoubleConsumer publishDouble(String name, PubSubOption... options);
Copy link
Member

Choose a reason for hiding this comment

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

There are a few styles of overloads here; can you explain what the expected use case / syntax for each is?

Copy link
Member Author

@PeterJohnson PeterJohnson Mar 20, 2024

Choose a reason for hiding this comment

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

We might not want the two publishX ones to be overloads, particularly with the variable arguments, but I need to figure out what to name them. The first one works like today's Sendable property getters, in that the supplier is called periodically (e.g. every periodic loop). The second one works like a normal DoublePublisher (and actually will be one under the hood!), in that there is no polling--instead the class can decide when to publish a new value by calling the returned DoubleConsumer.

Copy link
Member

Choose a reason for hiding this comment

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

Called periodically by what?
How will the DoublePublisher be closed?
Is setDouble analogous to the current publishConst?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just like today's SendableBuilderImpl in wpilibj, there will be an implementation class that runs as part of the robot periodic loop that loops over all the SendableTables.

The publishers and subscribers are owned by the SendableTable implementation.

Yes, setDouble is the same as today's publishConst.

@PeterJohnson
Copy link
Member Author

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.

Copy link
Member

@Starlight220 Starlight220 left a comment

Choose a reason for hiding this comment

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

I'll probably say this a bunch of times, but this API looks incredible!

/**
* Gets the current value of a property (as a JSON string).
*
* @param name name
Copy link
Member

Choose a reason for hiding this comment

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

It's very early for this, but this parameter description should be improved and unambiguous that it's the name of the topic to apply the metadata to.

I'm not a fan of splitting metadata from the data, but I admit that unifying would cause an overload explosion. What's the expected behavior if the topic doesn't exist (or if it's a Sendable)? If there is an ordering needed between the data call and the metadata call, it should be documented.

Also, I'm inferring that this won't be doing table "metadata" like the old SmartDashboard dot-prefixed entries? Though an implementation for that would be akin to setString etc, so I guess specific functions aren't needed.

Copy link
Member Author

@PeterJohnson PeterJohnson Apr 1, 2024

Choose a reason for hiding this comment

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

Yes, we'll probably need to implement caching of it to handle it being set prior to the publish call. We'll have to do the same thing for publish/subscribe options. It's more complex on the backend implementation, but makes the API a lot simpler to separate the two, otherwise these functions get very unwieldy to use. The combinatorial explosion is already an issue (we haven't even added all the array variants yet...).

Old-style metadata can still be done just through normal setString et al.

Copy link
Member

@Starlight220 Starlight220 left a comment

Choose a reason for hiding this comment

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

(apparently the github android app has a bug that doesn't allow reviewing with an empty comment even if there are code reviews)

@Starlight220
Copy link
Member

Regarding 276e65d, I think booleanPublisher (without the add) would flow better and is less verbose.

@PeterJohnson
Copy link
Member Author

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

@Starlight220
Copy link
Member

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).

@PeterJohnson
Copy link
Member Author

PeterJohnson commented Apr 7, 2024

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?

@spacey-sooty
Copy link
Contributor

This resolves #5513 correct?

@PeterJohnson PeterJohnson changed the base branch from development to main June 17, 2024 14:37
@spacey-sooty
Copy link
Contributor

This also resolves #5413

@PeterJohnson
Copy link
Member Author

Superseded by #7773.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants