search

wpilibsuite / allwpilib

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

SmartDashboard Sendable NT entry ".name" Is never closed #5912

Open m10653 opened 8 months ago

m10653 commented 8 months ago

Describe the bug When removing a sendable from the SendableRegistry the NT entry of .name is failed to be closed.

There appears to be no way via the Smartdashboard class to remove a Sendable that has been published, Removing the sendable via the SendableRegistry causes only portions of the Sendable to be properly closed. The .name entry still remains after removing the sendable from the registry.

To Reproduce Steps to reproduce the behavior:

  1. Add a Sendable to SmartDashboard SmartDashboard.putdata("path",someobj)
  2. Remove the Sendable from the SendableRegistry SendableRegistry.remove(someobj)

  3. Entry of .name remains in networktables under /path/.name

    • Link to code:

Expected behavior To fully close all resources associated with the sendable, including all network table entries.

Screenshots

Desktop (please complete the following information):

Additional context Is there a particular reason that it is relatively unintuitive to remove sendable entries from SmartDashboard, I found it surprising that there was no methods within the SmartDashboard Class to remove them.

Starlight220 commented 8 months ago

There isn't much API for closing anything, especially Sendables. It's a common problem, especially for testing.

m10653 commented 8 months ago

I also stumbled across another issue with the '.controllable' entry as well. If you add a sendable to smartdashboard ether via the Smartdashboard class or directly via the SendableRegistry. Normally when you remove it or close the sendablebuilder it will correctly remove the '.controllable' entry. However if you switch to Test mode it will fail to remove the key when removed from the SendableRegistry.

Is there a design reason that the Smart Dashboard class is not set up to remove entries or sendables or would that added feature be welcomed as a PR? Fixing the issue with the '.name' entry would be a bit weird because the .name entry is created in the static SmartDashboard class and not within the send able itself, so that may need to be moved or manually deleted afterword.

ThadHouse commented 8 months ago

Is there a design reason that the Smart Dashboard class is not set up to remove entries or sendables

Because it was never designed for how complicated sendable objects are. It was the original data sending class, and was built just for sending single values back and forth.

Ideally at some point we actually build a better API. PR's could be accepted, but theres some design weirdness that needs to be worked out.