search

wpilibsuite / allwpilib

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

Split SendableBuilder's addProperty methods into publish and subscribe methods #5413

Open Gold856 opened 1 year ago

Gold856 commented 1 year ago

Pulled from #5177. If no one else is doing this, I'll make a PR for this. If I do, I have a few questions about implementation:

  1. What are we doing with the addProperty methods? Are we going for full removal (breaking), deprecating for now (full removal later), or just leaving it?
  2. If full removal of addProperty will eventually happen, should I try to keep publishers and subscribers in the same Property object using a Map to find matching keys or something? I tested that, and it wasn't very type-safe.
  3. Along the lines of full removal, should Property be split into Publisher and Subscriber classes? It would be double the allocations, but it would already be double the allocations if 2 didn't work out.
Starlight220 commented 1 year ago

We need to know how exclusive publishing and subscribing are. If both are used, it results in issues like the command scheduling one. On the other hand, bidirectional use does make sense sometimes -- and in that case addProperty makes more sense than separate publish and subscribe. Perhaps splitting the interface would be a good idea? It would possibly avoid the weird situation of subscribing to a log (I think the publishing part of Sendable can easily be overloaded for logs) but would complicate usage. I'm not sure.

Gold856 commented 1 year ago

There are tons of cases where publishing is done without subscribing; look at all of the sensors. It doesn't make sense for them to do anything for subscribe, so splitting addProperty into publish and subscribe would have an impact for those classes.

There are also plenty of cases where both publishing and subscribing is done; anything that moves like motors, or controller classes like PIDController. Splitting addProperty wouldn't have much effect here.

Actually, doing a quick and dirty VS Code search, there are 76 instances of builder (builder.add.*Property.*.*\)), 53 of which have a null setter (builder.add.*Property.*null.*\)), including examples and only Java. Assuming this extrapolates to C++ as well, publishing and subscribing seem to be quite exclusive.

Splitting Sendable into publish and subscribe variants would be interesting, but I don't know if it would do much besides shuffle things around. And subscribing to a log can be dealt with by using no-ops.