Add fluent builders for more flexibly adding data to Shuffleboard #1022

SamCarlberg · 2018-05-04T04:59:09Z

Overview

Motivation

A big question people have had about Shuffleboard (and, to an extent, SmartDashboard) is how to configure the dashboard from their robot code. Specifically, how to use something like SmartDashboard.getX without having to also remember to add a call to SmartDashboard.putX at the beginning of the program.

Unfortunately, simply using SmartDashboard.getX doesn't work without doing some legwork ahead of time without introducing a custom compile task to scan for those method calls - this would not be a worthwhile solution. A solution that could be done in the current version of WPILib, or with just minor tweaks, would to be to use NetworkTableEntry instances for these variables; for example:

// Current approach with WPILib
void useX() {
  double x = SmartDashboard.getNumber("X");
}

// With NetworkTableEntry approach
public static final NetworkTableEntry xEntry = SmartDashboard.getEntry("X");

void useX() {
  double x = xEntry.getDouble(defaultValue);
}

An advantage to this method is that typos are eliminated; there's only one instance of the key string, and a typo made to the variable name would cause a compiler error. However, this still limits how data gets displayed in Shuffleboard; everything is still in the "Smartdashboard" tab.

Using the simple entry approach would fail for defining a Shuffleboard configuration. Users should not have to keep track of the names of the tab or layouts that contain the data they really want to read or update. It also doesn't address the issue of how the configuration should be sent to the dashboard.

Setup of the builders

The builders do two things: makes it easy to define where and how a piece of data is displayed or controlled, and gives a handle to a NetworkTableEntry for that data, when applicable (Sendables do not get entry handles, since they update and read their own tables). Widgets and layouts also have their properties configurable via the builders. For ease of use, whitespace and capitalization are ignored so users don't have to know the actual property names, which are not what gets displayed in the property editor dialogs in the dashboard.

Data, tab, and layout names are prohibited from containing forward slashes /; typically, these are used for defining subtables under the base SmartDashboard table. This is unnecessary in Shuffleboard, since these values can now be placed in specific tabs or layouts.

Internally, the Shuffleboard class will handle specifying the configuration in NetworkTables for the NetworkTables plugin to read on the client side.

Adding an entry

NetworkTableEntry myEntry = Shuffleboard.getTab(tabName)
  .getLayout(layoutType, layoutTitle) // optional
  .withProperties(layoutProperties)   // optional (requires layout to be set)
  .add(entryName, initialValue)       // REQUIRED
  .toWidget(widgetType)               // optional
  .withProperties(widgetProperties)   // optional
  .getEntry();

Adding a sendable

Shuffleboard.getTab(tabName)
  .getLayout(layoutType, layoutTitle) // optional
  .withProperties(layoutProperties)   // optional (requires layout to be set)
  .add(title, sendable)               // REQUIRED (name can be omitted if the sendable's name has been set)
  .toWidget(widgetType)               // optional
  .withProperties(widgetProperties)   // optional

The two builders look identical, except that the builder for entries has a terminal getEntry() function. This can be called at any point during the chain, but must be called in order to get a handle to the NetworkTableEntry being added. A call to Shuffleboard.update after completing a builder is required to set up the configuration metadata (tab name, layout info, widget info, etc), as well as update sendables.

Keeping variables around

ShuffleboardTab myTab = Shuffleboard.getTab(tabName);
ShuffleboardLayout myLayout = myTab.getLayout(layoutType, layoutTitle);
myLayout.add(name, value);
myTab.add(name, value);

Example use

Adding a slider to a tab named "Drivers Tab" to control the maximum speed of the drivetrain

RobotDrive driveBase = ...
NetworkTableEntry maxSpeed = Shuffleboard.getTab("Drivers")
  .add("Max Speed", 1)
  .withWidget("Number Slider")
  .withProperties(Map.of("min", 0.0, "max", 1.0)) // Note: Map.of introduced in Java 9
  .getEntry();
...

driveBase.setMaxOutput(maxSpeed.getDouble(1.0));

Still to do

C++ version
Add support in Shuffleboard (this is important)

Waiting on

WPILib migration to Java 9+; specifically, for the addition of Map.of for easily setting properties

Possible feature improvements

~~Allow default values to be specified for entries~~ Added
~~Allow for nested layouts~~ Added
~~Allow for adding NetworkTableEntry instances directly i.e. myTab.add(myEntry)~~ Not feasible
~~More documentation to base classes and interfaces.~~
~~Specifying exact layout properties (tab's grid size, tile's position and size in the grid)~~ Added

ThadHouse · 2018-05-04T05:06:13Z

wpilibjExamples/src/main/java/edu/wpi/first/wpilibj/examples/shuffleboard/Robot.java

+  @Override
+  public void teleopPeriodic() {
+    Shuffleboard.update();
+  }


We need to add this example to the examples.xml (actually by the time this merges I hope to have replaced that with the json equivalent). Also, these updates should be in robotPeriodic, not the individual periodic methods. robotPeriodic was specificially added for purposes like this.

I'll wait on the VSCode JSON stuff before merging.

I see the point of using robotPeriodic for consistency, but I do like specifying both autonomous and teleop periodic functions because those are what users will be using in their real programs.

Yeah, but I would worry about teams forgetting about it in test mode and wondering why its not running. robotPeriodic is specifically for things that should always be updated every loop. I've been trying to suggest it more to teams when helping for dashboard things. I'm actually debating adding it to the default project as well.

Oh, cool. In that case, I'll definitely move it over

ThadHouse · 2018-05-04T05:09:23Z

wpilibj/src/main/java/edu/wpi/first/wpilibj/shuffleboard/Shuffleboard.java

+    // Update sendables
+    // Basic entries do not need to be updated by us; they are set by users via the
+    // NetworkTableEntry instances they get when completing an add() operation
+    m_sendables.forEach(data -> {


Does this allocate a lambda per call? Why not just a standard loop? Since this is called once per robot loop, we should avoid allocations as much as possible.

It's a non-capturing lambda, so there's only one object allocated.

ThadHouse · 2018-05-04T05:11:23Z

wpilibj/src/main/java/edu/wpi/first/wpilibj/shuffleboard/Shuffleboard.java

+    NetworkTable baseTable = getBaseTable();
+    // Update tabs
+    NetworkTable tabsTable = baseTable.getSubTable(".tabs");
+    m_builders.stream()


Same question about the allocations here, but this one has a ton more. We should try and store these instead of having to reupdate every loop. And should definitely try and avoid some of the spurious allocations if possible.

b -> b.getTab().getName() is non-capturing, so only one object is allocated.

~~The lambda in the forEach is capturing, but could easily be rewritten to be noncapturing. And doing this stream in a conventional loop would be a bit nasty.~~ It is now noncapturing

Can specify a layout to place the data and its properties Can specify the widget and its properties 100% unit test coverage

Uses resource ID 63

AustinShalit · 2018-06-15T17:18:01Z

wpilibj/src/test/java/edu/wpi/first/wpilibj/shuffleboard/ShuffleboardTest.java

+        () -> assertThrows(NullPointerException.class, () -> add("foo", (NetworkTableType) null)),
+        () -> assertThrows(IllegalArgumentException.class, () -> add("", kDouble)),
+        () -> assertThrows(IllegalArgumentException.class, () -> add("a/b", kDouble)));
+  }


One test per test case please. If you want them grouped use nested test cases.

Ooh, that's new. Would you recommend grouping entries vs sendables, or have something more finely grained?

I would make this a parameterized test called invalidInputsToAddEntryTest. Parameters would be the expected exception, the key for the add method, and the object for the add method.

Add comments to shuffleboard example robot

AustinShalit · 2018-06-26T16:25:10Z

I'd like to move away from the singleton model for classes like this (and SmartDashboard) because it makes code hard to test. Instead of having static methods in Shuffleboard.java, can we make this an object instead with 2 constructors? One constructor with no arguments would use the default nt table, the second one would take a nt table.

SamCarlberg · 2018-06-26T16:28:31Z

It's a tradeoff for API simplicity, and to follow the style of SmartDashboard and LiveWindow to make it easier to transition to.

AustinShalit · 2018-06-26T16:32:31Z

I don't know if I agree with the API simplicity argument because the rest of our API uses objects rather than statics.

SamCarlberg · 2018-06-26T16:37:21Z

SmartDashboard and LiveWindow both use statics, so that's what I'm going with here

frcjenkins · 2018-06-26T16:49:47Z

Those statics, I believe, just point to the default dynamically created instance of NetworkTables. And aren’t there non-static versions of the NetworkTables calls?

…

On Jun 26, 2018, 12:37 PM -0400, Sam Carlberg ***@***.***>, wrote: SmartDashboard and LiveWindow both use statics, so that's what I'm going with here — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

SamCarlberg · 2018-06-26T16:54:38Z

SmartDashboard only references the default NetworkTables instance. Shuffleboard can (and does) use new instances in tests

AustinShalit · 2018-07-29T15:58:45Z

wpilibj/src/main/java/edu/wpi/first/wpilibj/shuffleboard/ContainerHelper.java

+  private final ShuffleboardContainer m_container;
+  private final Set<String> m_usedTitles = new HashSet<>();
+  private final List<ShuffleboardComponent<?>> m_components = new ArrayList<>();
+  private final Map<String, ShuffleboardLayout> m_layouts = new LinkedHashMap<>(); // NOPMD


Please remove NOPMD when #1243 is merged

AustinShalit · 2018-07-29T15:59:01Z

wpilibj/src/main/java/edu/wpi/first/wpilibj/shuffleboard/ShuffleboardInstance.java

+import edu.wpi.first.networktables.NetworkTableInstance;
+
+final class ShuffleboardInstance implements ShuffleboardRoot {
+  private final Map<String, ShuffleboardTab> m_tabs = new LinkedHashMap<>(); // NOPMD


Please remove NOPMD when #1243 is merged

AustinShalit · 2018-07-30T20:57:06Z

wpilibj/src/main/java/edu/wpi/first/wpilibj/shuffleboard/ContainerHelper.java

+ * A helper class for Shuffleboard containers to handle common child operations.
+ */
+final class ContainerHelper {
+


Why is wpiformat not catching this blank line @calcmogul?

That's a bug. I reproduced it locally, and it has to do with improper handling of comments (the comment contains the class keyword).

…berg/allwpilib into shuffleboard-integration

Forces buildMetadata to be called on first initialization. Makes layouts work Add a test to help prevent regressions

So we do not overwrite existing data in NetworkTables when robot code starts

Rename `data` parameter to `defaultValue` on some methods for clarity

Add a test for that case

AustinShalit · 2018-08-31T01:36:00Z

wpilibj/src/main/java/edu/wpi/first/wpilibj/shuffleboard/ShuffleboardInstance.java

+   * Applies the function {@code func} to all complex widgets in {@code container}. Helper method
+   * for {@link #applyToAllComplexWidgets}.
+   */
+  private void apply(ShuffleboardContainer container, Consumer<ComplexWidget> func) {


Can this be static?

Add shuffleboard C++ API

SamCarlberg · 2018-09-21T17:36:32Z

@PeterJohnson Any further review comments?

calcmogul · 2018-09-27T06:13:11Z

Here's a potential fix for clang:

diff --git a/wpilibc/src/main/native/cpp/shuffleboard/ShuffleboardInstance.cpp b/wpilibc/src/main/native/cpp/shuffleboard/ShuffleboardInstance.cpp
index ed815dcd1..41dcf301b 100644
--- a/wpilibc/src/main/native/cpp/shuffleboard/ShuffleboardInstance.cpp
+++ b/wpilibc/src/main/native/cpp/shuffleboard/ShuffleboardInstance.cpp
@@ -33,7 +33,7 @@ ShuffleboardInstance::~ShuffleboardInstance() {}
 
 frc::ShuffleboardTab& ShuffleboardInstance::GetTab(wpi::StringRef title) {
   if (m_impl->tabs.find(title) == m_impl->tabs.end()) {
-    m_impl->tabs.try_emplace(title, std::move(ShuffleboardTab(*this, title)));
+    m_impl->tabs.try_emplace(title, ShuffleboardTab(*this, title));
     m_impl->tabsChanged = true;
   }
   return m_impl->tabs.find(title)->second;
diff --git a/wpilibc/src/main/native/include/frc/shuffleboard/ShuffleboardComponentBase.h b/wpilibc/src/main/native/include/frc/shuffleboard/ShuffleboardComponentBase.h
index 41c195a97..3247d1e79 100644
--- a/wpilibc/src/main/native/include/frc/shuffleboard/ShuffleboardComponentBase.h
+++ b/wpilibc/src/main/native/include/frc/shuffleboard/ShuffleboardComponentBase.h
@@ -43,15 +43,15 @@ class ShuffleboardComponentBase : public virtual ShuffleboardValue {
  protected:
   wpi::StringMap<std::shared_ptr<nt::Value>> m_properties;
   bool m_metadataDirty = false;
-
- private:
-  ShuffleboardContainer& m_parent;
-  std::string m_type;
   int m_column = -1;
   int m_row = -1;
   int m_width = -1;
   int m_height = -1;
 
+ private:
+  ShuffleboardContainer& m_parent;
+  std::string m_type;
+
   /**
    * Gets the custom properties for this component. May be null.
    */

…rades (wpilibsuite#1022)

SamCarlberg added the wip label May 4, 2018

ThadHouse reviewed May 4, 2018

View reviewed changes

SamCarlberg mentioned this pull request May 18, 2018

Layouts should be able to be defined from robot code wpilibsuite/shuffleboard#473

Closed

SamCarlberg added 9 commits May 29, 2018 00:10

Add fluent builders for adding data to tabs

60da71f

Can specify a layout to place the data and its properties Can specify the widget and its properties 100% unit test coverage

Use robotPeriodic in example

be6ceb6

Make terminal stream lambda non-capturing

feeb9c9

Add dirty flag to avoid unnecessary NT updates

954e77c

Use setDefaultValue instead of forcePut

e75cc6b

Add Shuffleboard resource reporting

b3ea405

Uses resource ID 63

Force set default values if the entry exists with the wrong type

7235e25

NetworkTableInstance encapsulation, fix EntryBuilder using default inst

d2fb582

Minor API cleanup

25697bd

SamCarlberg force-pushed the shuffleboard-integration branch from 244dc58 to 25697bd Compare May 29, 2018 04:12

SamCarlberg mentioned this pull request Jun 14, 2018

Editable Fields in Shuffleboard wpilibsuite/shuffleboard#485

Closed

SamCarlberg added 2 commits June 15, 2018 11:25

Merge branch 'master' into shuffleboard-integration

5279c20

Address issues from new PMD, Checkstyle, JUnit

0eee2a2

AustinShalit reviewed Jun 15, 2018

View reviewed changes

SamCarlberg added 2 commits June 26, 2018 11:23

Merge branch 'master' into shuffleboard-integration

727373c

Call Shuffleboard.update() in main robot loop in iterative base

399aad7

Add comments to shuffleboard example robot

Run formatter

94e50fa

Remove usage reporting

02bd316

SamCarlberg removed the wip label Jun 26, 2018

SamCarlberg mentioned this pull request Jul 27, 2018

Add confirmation to SendableChooser #1217

Merged

SamCarlberg added 3 commits July 28, 2018 18:06

Merge branch 'master' into shuffleboard-integration

11fcba7

Disable complex widgets only if they're actuators

6e0c4e2

Only update tabs entry if tabs have been added since the last update

b1f891e

AustinShalit reviewed Jul 29, 2018

View reviewed changes

SamCarlberg added 3 commits July 29, 2018 15:04

Merge branch 'master' into shuffleboard-integration

d066453

Remove NOPMD comments from maps

fcb8841

Make sure builders are initialized when calling disableActuatorWidgets

63b89b4

AustinShalit reviewed Jul 30, 2018

View reviewed changes

auscompgeek mentioned this pull request Aug 2, 2018

Filename overflows file header in PRs StylishThemes/GitHub-Dark#724

Closed

SamCarlberg added 3 commits August 13, 2018 18:42

Merge branch 'shuffleboard-integration' of https://github.com/SamCarl…

83c540a

…berg/allwpilib into shuffleboard-integration

Initialize with dirty metadata

d3e6bb3

Forces buildMetadata to be called on first initialization. Makes layouts work Add a test to help prevent regressions

Format fix

048bbe6

SamCarlberg mentioned this pull request Aug 16, 2018

Allow recordings to be controlled via NetworkTables wpilibsuite/shuffleboard#502

Merged

Use setDefaultValue instead of forceSetValue when adding simple widgets

54bc722

So we do not overwrite existing data in NetworkTables when robot code starts

SamCarlberg mentioned this pull request Aug 23, 2018

Unable to add robot preferences wpilibsuite/shuffleboard#453

Closed

SamCarlberg added 2 commits August 24, 2018 19:02

Add an addPersistent() method

56bb3e4

Rename `data` parameter to `defaultValue` on some methods for clarity

Fix nested actuator widgets not getting disabled

29ff130

Add a test for that case

AustinShalit reviewed Aug 31, 2018

View reviewed changes

calcmogul and others added 2 commits September 16, 2018 13:01

Add shuffleboard C++ API

4a0defb

Merge pull request #1 from calcmogul/shuffleboard-cpp

5412aff

Add shuffleboard C++ API

Merge branch 'master' into shuffleboard-integration

2af00ac

Apply Tyler's Clang fix

7d86364

PeterJohnson approved these changes Sep 28, 2018

View reviewed changes

PeterJohnson merged commit 175c6c1 into wpilibsuite:master Sep 28, 2018

SamCarlberg deleted the shuffleboard-integration branch September 28, 2018 15:42

maniyar1 mentioned this pull request Nov 10, 2018

Make a logger to ensure things are going correctly in matches frc3324/OffSeason2018#3

Open

pjbuterbaugh pushed a commit to pjbuterbaugh/allwpilib that referenced this pull request Jun 15, 2023

Add --upgrade flag to note about frc-characterization pre-release upg…

ac9a8da

…rades (wpilibsuite#1022)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fluent builders for more flexibly adding data to Shuffleboard #1022

Add fluent builders for more flexibly adding data to Shuffleboard #1022

SamCarlberg commented May 4, 2018 •

edited

Loading

ThadHouse May 4, 2018

SamCarlberg May 4, 2018

ThadHouse May 4, 2018

SamCarlberg May 4, 2018

ThadHouse May 4, 2018

SamCarlberg May 4, 2018

ThadHouse May 4, 2018

SamCarlberg May 4, 2018 •

edited

Loading

AustinShalit Jun 15, 2018

SamCarlberg Jun 15, 2018

AustinShalit Jun 26, 2018

AustinShalit commented Jun 26, 2018

SamCarlberg commented Jun 26, 2018

AustinShalit commented Jun 26, 2018

SamCarlberg commented Jun 26, 2018

frcjenkins commented Jun 26, 2018 via email

SamCarlberg commented Jun 26, 2018

AustinShalit Jul 29, 2018

AustinShalit Jul 29, 2018

AustinShalit Jul 30, 2018

calcmogul Jul 31, 2018

AustinShalit Jul 31, 2018

AustinShalit Aug 31, 2018

SamCarlberg commented Sep 21, 2018

calcmogul commented Sep 27, 2018

Add fluent builders for more flexibly adding data to Shuffleboard #1022

Add fluent builders for more flexibly adding data to Shuffleboard #1022

Conversation

SamCarlberg commented May 4, 2018 • edited Loading

Overview

Motivation

Setup of the builders

Adding an entry

Adding a sendable

Keeping variables around

Example use

Still to do

Waiting on

Possible feature improvements

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamCarlberg May 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AustinShalit commented Jun 26, 2018

SamCarlberg commented Jun 26, 2018

AustinShalit commented Jun 26, 2018

SamCarlberg commented Jun 26, 2018

frcjenkins commented Jun 26, 2018 via email

SamCarlberg commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamCarlberg commented Sep 21, 2018

calcmogul commented Sep 27, 2018

SamCarlberg commented May 4, 2018 •

edited

Loading

SamCarlberg May 4, 2018 •

edited

Loading