Skip to content

[telemetry] Add new Telemetry implementation #7773

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

Draft
wants to merge 4 commits into
base: 2027
Choose a base branch
from

Conversation

PeterJohnson
Copy link
Member

@PeterJohnson PeterJohnson commented Feb 9, 2025

Significantly simplified version of #6453. The design approach here is to make Telemetry purely imperative/immediate and write only. Read capabilities would be added as a separate Tunable implementation.

Telemetry is a utility class with only static functions to allow simple use such as Telemetry.log("name", value); (ala System.out.println()), and is intended as the primary user-facing class. Nested (structured) telemetry is available via TelemetryTable, instances of which can be gotten by calling Telemetry.getTable() (or TelemetryTable.getTable() for further nesting).

The Sendable concept equivalent is TelemetryLoggable (not a great name, but naming is hard). Unlike the current Sendable / SmartDashboard.putData(), this is designed to be immediate, not callback-based. The updateTelemetry(TelemetryTable) function of a TelemetryLoggable should immediately log the desired data to the provided TelemetryTable and not store the table for later use.

While we aim to fast-path most types through specific overloads, we do have a generic Object-taking overload to avoid potential overload ambiguity (particularly between StructSerializable and ProtobufSerializable, where many types implement both) and provide a fallback toString path. There's a type registry mechanism to register specific type handlers; the intent here is to use this for things like Unit types, where you might want to both set a property (indicating the unit type) and provide the value as a double.

Backends can be configured at any level; the most specific backend is used based on longest prefix match of the telemetry path (this allows for e.g. setting up NT logging at the top level, but making some tables DataLog-only). Backends are provided for both NetworkTables and DataLog, and there's also a discard backend (that throws away any logged data) as well as a mock backend (for unit testing).

The initial backend implementation takes the type of the first log() call as the "forever" type for that particular name. Trying to later log to the same name with a different type is ignored and emits a warning. Changing types dynamically both significantly increases implementation complexity and will likely result in difficult-to-debug behavior in downstream tooling; it's hard to see the user benefits to supporting this.

Dependency wise, the telemetry library only depends on wpiutil.

TODO:

  • DataLog backend
  • NT backend
  • Discard backend
  • Mock backend
  • Introspection for struct/protobuf (to get .struct / .proto objects)
  • C++ implementation
  • Port current use cases of SmartDashboard/Sendable (and remove the implementations)
  • Implement int and short arrays for DataLog and NT
  • Registry should cache Entry so they are safe to use with ConcurrentMap and can be reset on reset()
  • Registry should potentially cache SendableTable objects too?
  • RobotBase set up NT as the default global backend
  • Unit tests and examples
  • Add early discard checks (avoid work if going to discard backend)
  • Make discard easier to use (table function instead of needing to use backend API?)
  • When new backend registered, remove matching cached entries from tables (or just invalidate all caches)
  • Implement type handler(s) for unit types
  • Implement warnings on type mismatch
  • Port current use of DataLog/NT APIs (in particular, DS joysticks) (TBR)
  • Add tunables (maybe separate PR?)
  • Design document
  • Update type strings for consistency (some have spaces between words, others don't)

Fixes #5513
Closes #5912
Closes #5481
Closes #5413

@github-actions github-actions bot added component: wpiutil WPI utility library component: wpilibj WPILib Java 2027 2027 target labels Feb 9, 2025
Copy link

@alan412 alan412 left a comment

Choose a reason for hiding this comment

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

I don't see any locking to make sure this works from multiple threads.

I also don't understand why there are a lot of individual functions with things like "log" naming.

@Gold856
Copy link
Contributor

Gold856 commented Feb 10, 2025

I also don't understand why there are a lot of individual functions with things like "log" naming.

What do you mean by this? Are you asking why everything is called log or why there's so many overloads? log was chosen because lots of other 3rd party telemetry libraries use that name, and people seem to prefer the shorter name. If you're asking about the overloads, we have them to prevent accidental casting from, say, a double to a float, or a long to an int.

@alan412
Copy link

alan412 commented Feb 10, 2025

I also don't understand why there are a lot of individual functions with things like "log" naming.

What do you mean by this? Are you asking why everything is called log or why there's so many overloads? log was chosen because lots of other 3rd party telemetry libraries use that name, and people seem to prefer the shorter name. If you're asking about the overloads, we have them to prevent accidental casting from, say, a double to a float, or a long to an int.

I wasn't clear here. This is in reference to TelemetryBackEnd.java

@PeterJohnson
Copy link
Member Author

PeterJohnson commented Feb 10, 2025

I also don't understand why there are a lot of individual functions with things like "log" naming.

I wasn't clear here. This is in reference to TelemetryBackEnd.java

I haven't finished implementing the backends, but basically all the backends have type safety and type-specific functionality. So writing a double to a datalog file or NetworkTables is different than writing a string (or an integer). If you're asking why they are uniquely named instead of overloaded, overloading is bad practice with virtual functions (particularly in C++). A bunch of overloaded "log" functions on the user-facing side makes sense for ease of use, but is a potential footgun on the backend.

@PeterJohnson
Copy link
Member Author

PeterJohnson commented Feb 10, 2025

I don't see any locking to make sure this works from multiple threads.

It should be thread safe as written already. The backends will have locking/atomics included as needed (there are some synchronized blocks there already in the start of the DataLogSendableBackend implementation). We use ConcurrentMap etc to avoid explicit locking on the frontend (e.g. TelemetryTable caching uses ConcurrentMap), at least in Java--in C++, we'll use explicit mutexes.

@github-actions github-actions bot added the component: ntcore NetworkTables library label Feb 12, 2025
@github-actions github-actions bot added the component: wpilibc WPILib C++ label Feb 19, 2025
@github-actions github-actions bot added the component: command-based WPILib Command Based Library label Feb 26, 2025
@PeterJohnson PeterJohnson changed the title [wpiutil] Add new Telemetry implementation [telemetry] Add new Telemetry implementation Feb 26, 2025
@PeterJohnson PeterJohnson self-assigned this Mar 11, 2025
@github-actions github-actions bot added component: hal Hardware Abstraction Layer component: examples labels Mar 12, 2025
@@ -158,3 +158,20 @@ class ADXL345_I2C : public nt::NTSendable,
};

} // namespace frc

template <>
struct wpi::Struct<frc::ADXL345_I2C::AllAxes> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this go into a separate file to match the other struct definitions?


TEST(Mechanism2dTest, Canvas) {
void TearDown() override { wpi::TelemetryRegistry::Reset(); }

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
template <typename T>
void ExpectLastValue(std::string_view path, const T& expected) {
auto value = mock->GetLastValue<T>(path);
ASSERT_TRUE(value);
EXPECT_EQ(expected, *value);
}

This should work and would reduce boilerplate, though it doesn't work for the types that gtest won't nicely format if the assertion fails. (If it doesn't recognize the type it outputs the hexademical of the raw memory contents- I remember encountering that for failing quaternion equality checks)

Copy link
Contributor

@spacey-sooty spacey-sooty left a comment

Choose a reason for hiding this comment

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

A log(name, measure) overload which includes unit metadata also be nice

@PeterJohnson
Copy link
Member Author

PeterJohnson commented Apr 1, 2025

Unit support will be implemented in a different way--the registry supports registering global type-specific handlers so we don't have to have telemetry depend on every library we want to provide overloads for.

@github-actions github-actions bot added component: wpimath Math library component: apriltag AprilTag library labels Apr 2, 2025
@PeterJohnson PeterJohnson moved this to In progress in 2027 Apr 2, 2025
@PeterJohnson
Copy link
Member Author

PeterJohnson commented Apr 2, 2025

Units support has been added.

In C++, it's integrated into units/base.h:

inline void LogEntry(wpi::TelemetryTable& table, std::string_view name, const nameSingular ## _t& value)
{
  table.SetProperty(name, "unit", " "#nameSingular);
  table.Log(name, value.value());
}

This will make it automatically work for all use cases.

In Java, it needs to be registered as a type handler, which is done in wpilibj RobotBase:

TelemetryRegistry.registerTypeEntry(Measure.class, (v, entry) -> {
  entry.setProperty("unit", v.unit().name());
  entry.logDouble(v.magnitude());
});

For Java, this means that user unit tests which want to log unit values in this way will need to similarly register the type entry. Unit tests will still compile and run without doing so, they'll just log a string value (via toString()) instead of a numeric value.

Currently, the callback is also a little different between C++ and Java; C++ gets the table and name, while Java gets the Entry. The reason for this is that we want to avoid referencing the virtual functions in Entry in C++, but it also adds a bit more flexibility and will let us unify the entry and table handler callbacks, so I'm considering changing the Java callback to also take the Table and name instead of the entry.

@KangarooKoala
Copy link
Contributor

KangarooKoala commented Apr 2, 2025

In C++, it's integrated into units/base.h:

inline void LogEntry(wpi::TelemetryTable& table, std::string_view name, const nameSingular ## _t& value)
{
  table.SetProperty(name, "unit", " "#nameSingular);
  table.Log(name, value.value());
}

This will make it automatically work for all use cases.

Before I forget, I think for mp-units we can use mp_units::unit_symbol(decltype(quantity)::unit) to get the unit symbol. I think we're also already planning to add a mp_units::quantity.value() (which can already be nicely expressed through the user-facing API), so logging the value shouldn't need to change.

Hard to say whether this or mp-units will happen first, but I just wanted to note this down before I forgot.

@@ -122,6 +122,10 @@
<Bug pattern="UC_USELESS_VOID_METHOD" />
<Class name="edu.wpi.first.wpilibj.templates.timesliceskeleton.Robot" />
</Match>
<Match>
<Bug pattern="UCF_USELESS_CONTROL_FLOW" />
<Class name="edu.wpi.first.wpilibj.examples.rapidreactcommandbot.subsystems.IntakeLogger" />
Copy link
Contributor

Choose a reason for hiding this comment

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

This class doesn't seem to exist?


/** Global registry for telemetry handlers (type handlers and telemetry backends). */
public final class TelemetryRegistry {
/** Handler for logging objects of specific type. Typically only one handler is specified, the */
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI that this is cut off.

*
* @return value
*/
T get();

This comment was marked as resolved.

Imported from https://github.com/nielsbasjes/prefixmap
with the following changes:
- Removed serialization
- Removed case-insensitive matching
- Formatting to placate spotbugs and spotless
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2027 2027 target component: apriltag AprilTag library component: command-based WPILib Command Based Library component: examples component: hal Hardware Abstraction Layer component: ntcore NetworkTables library component: wpilibc WPILib C++ component: wpilibj WPILib Java component: wpimath Math library component: wpiutil WPI utility library
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

6 participants