[wpilib] Tracer Overhaul #7099

oh-yes-0-fps · 2024-09-19T05:11:12Z

resolves #6583

rzblue · 2024-09-19T05:15:35Z

I don't think Tracer should be a static global, as it is used in multiple places that shouldn't have mixed outputs.

oh-yes-0-fps · 2024-09-19T05:26:56Z

I don't think Tracer should be a static global, as it is used in multiple places that shouldn't have mixed outputs.

It's being used differently here, it's similar to a flame graph with a cascading list of durations.
There are no mixed outputs, the tracer covers the whole robot and everything that happens during its runtime.
Also the tracer is more of ThreadLocal static and supports tracing multiple threads at once or restricting it to 1 thread.
It also has some tooling to attempt to isolate gc time from execution time but that can be turned off on a per thread basis.

oh-yes-0-fps · 2024-09-19T05:29:13Z

If performance is a concern we can make it so after a certain number of nests it is sent to only datalog instead.

rzblue · 2024-09-19T05:53:18Z

Ok, that's pretty cool.

A couple things:

Tracer is part of the public API, so we will need to do a deprecation cycle on the old interface
I think we should talk about how we want to handle telemetry like this; much like alerts we don't want it to be printed to the console, but it needs to be obvious how to access it. I also don't usually like tying things to NT directly, but maybe that's fine for now.
have you done any benchmarks on the rio? I doubt performance will be a significant regression from the current implementation but it'd be good to double check.

oh-yes-0-fps · 2024-09-19T06:00:32Z

Performance wise it's quite good, we ran a version close to this all of the 2024 season based on some cursory testing it seems to use less than a ms per loop. The current implementation is more refined with 1/3 as many string allocs so it should be retested.

I don't like doing 1 topic per duration but there isn't really a way around it.

In terms of displaying to user im unsure of better ways to convey information. The watchdog will still print when overruns happen but not the epoch times. With glass, ascope and maybe elastic being packaged with wpilib I think teams should be expected to be able to use a dashboard to take full advantage of wpilib.

oh-yes-0-fps · 2024-09-19T06:01:59Z

In terms of deprecation of the old tracer api, we could just name this something else while still removing tracer from watchdog. Profiler isn't a terrible name but also not great.

Whats the etiquette on just leaving the old api with deprecation warnings and just having all the functions no-op or smthn until we can remove it.

rzblue · 2024-09-19T18:03:33Z

Whats the etiquette on just leaving the old api with deprecation warnings and just having all the functions no-op or smthn until we can remove it.

That's worse than removing it without deprecation.

KangarooKoala · 2024-09-19T18:59:04Z

Generally the route for changing to a new API is that you make the new API with a different name and deprecate the old one (without doing any other changes to the old version)- The point of deprecation is that you can still use it without breakage. When it comes time to remove the old API (usually a year after deprecation), you completely remove it.
I can't think of any case where you'd want to change methods to no-op, since that's a silent breakage.

oh-yes-0-fps · 2024-09-19T20:08:14Z

So I guess this is a broader question but what is the worry of changing the public API for something when adding it in a new major version? Especially something that is currently not used very much by users.

calcmogul · 2024-09-19T23:15:17Z

So I guess this is a broader question but what is the worry of changing the public API for something when adding it in a new major version?

Ostensibly, it lets teams migrate their code gradually over the course of a year rather than breaking it all at once. In my experience though, teams either fix the deprecation warnings immediately because they think they're errors, or they don't migrate until we force them to by removing the old API.

At least deprecation warnings provide guidance on how to upgrade though; immediate removal or API change gives no guidance whatsoever and can be frustrating, especially if we broke working code from the previous year.

I'm not sure what we'd name the new class instead though, because Tracer is the most obvious name.

oh-yes-0-fps · 2024-09-19T23:30:45Z

This class's functionality is now static, it wouldn't be hard to make an instance of it to keep the functionality of the old tracer. It would just be confusing but using proper docs and deprecation warnings could help with that.

KangarooKoala

Instead of making addPeriodic(Runnable, String, Time, Time) the only non-deprecated overload, I'd prefer we add equivalents to all overloads that add the String name parameter. (That is, add addPeriodic(Runnable, String, double), addPeriodic(Runnable, String, double, double), and addPeriodic(Runnable, String, Time)). This way, users aren't forced into using the units library and don't need to specify the offset if they don't want one.

oh-yes-0-fps · 2024-10-01T17:40:01Z

Ok I added more overloads to addPeriodic

I need help porting the changes I made to TimedRobot.Callback over to C++

Another thought I had was that each periodic Callback now has a name. The name could be used when printing out errors to make debugging from which Callback the error is coming from easier.

KangarooKoala

I need help porting the changes I made to TimedRobot.Callback over to C++

You can use std::optional instead of java.util.Optional, with the exception that you can't use map() or ifPresentOrElse() (which aren't supported on the compiler versions we use)- Instead, you'll have to write the logic manually. (name.map(SubstitutiveTracer::new) -> name ? frc::Tracer::SubstitutiveTracer{name} : std::nullopt or m_tracer.ifPresentOrElse(sub -> sub.subWith(m_func), m_func) -> if (m_tracer) { sub.subWith(m_func); } else { m_func(); })

wpilibj/src/main/java/edu/wpi/first/wpilibj/TimedRobot.java

wpilibj/src/main/java/edu/wpi/first/wpilibj/TimesliceRobot.java

Co-authored-by: Joseph Eng <91924258+KangarooKoala@users.noreply.github.com>

Gold856 · 2024-10-03T04:39:44Z

wpilibNewCommands/src/main/native/include/frc2/command/Command.h

+   * Decorates this Command with a name. Is an inline function for
+   * Command::SetName(std::string_view);


Gold856 · 2024-10-03T04:41:52Z

wpilibc/src/main/native/cpp/IterativeRobotBase.cpp


  if constexpr (IsSimulation()) {
    HAL_SimPeriodicBefore();
-    SimulationPeriodic();
+    Tracer::TraceFunc("SimulationPeriodic",
+                      std::bind(&IterativeRobotBase::SimulationPeriodic, this));


This is techinically a behavior change. Previously, everything in the if statement would be timed, but now it's only SimulationPeriodic. Not necessarily bad, just noting.

Gold856 · 2024-10-03T04:42:36Z

wpilibc/src/main/native/cpp/IterativeRobotBase.cpp

-  // Warn on loop time overruns
-  if (m_watchdog.IsExpired()) {
-    m_watchdog.PrintEpochs();
-  }
 }


Is there functionality to alert the user when an overrun happens?

The watchdog already does that with the callback

Gold856 · 2024-10-03T05:15:44Z

wpilibc/src/main/native/cpp/Tracer.cpp

+}
+
+std::string Tracer::TracerState::PopTraceStack() {
+  m_stackSize > 0 ? m_stackSize-- : m_stackSize;


Could use an if statement here.

Gold856 · 2024-10-03T05:24:35Z

wpilibc/src/test/native/cpp/TracerTest.cpp

+  EXPECT_TRUE(test1Entry.GetDouble(0.0) - 500.0 < 1.0);
+  EXPECT_TRUE(test2Entry.GetDouble(0.0) - 400.0 < 1.0);


If the default value is zero, the value will be negative and will satisfy the condition. If you need some tolerance, use EXPECT_NEAR, or just EXPECT_EQ if you want to be exact.

Gold856 · 2024-10-03T05:24:58Z

wpilibc/src/test/native/cpp/TracerTest.cpp

+    auto test2Entry = nt::NetworkTableInstance::GetDefault().GetEntry(
+        "/Tracer/" + newThreadName + "/ThreadTest1");
+
+    EXPECT_TRUE(test2Entry.GetDouble(0.0) - 400.0 < 1.0);


Gold856 · 2024-10-03T05:25:08Z

wpilibc/src/test/native/cpp/TracerTest.cpp

+    auto test1Entry = nt::NetworkTableInstance::GetDefault().GetEntry(
+        "/Tracer/SingleThreadTest1");
+
+    EXPECT_TRUE(test1Entry.GetDouble(0.0) - 100.0 < 1.0);


wpilibc/src/test/native/cpp/TracerTest.cpp

Gold856 · 2024-10-03T05:26:42Z

wpilibj/src/main/java/edu/wpi/first/wpilibj/IterativeRobotBase.java

@@ -421,18 +432,11 @@ protected void loopFunc() {
      NetworkTableInstance.getDefault().flushLocal();
    }

-    // Warn on loop time overruns
-    if (m_watchdog.isExpired()) {
-      m_watchdog.printEpochs();


Is there functionality to alert the user of overruns?

wpilibj/src/main/java/edu/wpi/first/wpilibj/TimesliceRobot.java

Gold856

Is there any documentation for the time units Tracer publishes in?

wpilibc/src/main/native/cpp/Tracer.cpp

wpilibc/src/test/native/cpp/TracerTest.cpp

oh-yes-0-fps · 2024-10-03T22:03:04Z

Is there any documentation for the time units Tracer publishes in?

Where would the best place to document those be? maybe it should even be placed in NT?

Gold856 · 2024-10-03T22:25:13Z

Documentation in NT and the class doc would be good.

rework tracer

427d3c1

oh-yes-0-fps requested a review from a team as a code owner September 19, 2024 05:11

fixed CommandScheduler for now

2294b0b

oh-yes-0-fps requested a review from a team as a code owner September 19, 2024 05:12

oh-yes-0-fps added 3 commits September 19, 2024 20:06

reimplemented old tracer on class instances with deprecation warnings

890b7c4

added cpp version

2cc9aa4

Merge branch 'main' into tracer

b2d90b5

oh-yes-0-fps changed the title ~~[wpilibj] Tracer Overhaul~~ [wpilib] Tracer Overhaul Sep 23, 2024

oh-yes-0-fps added 11 commits September 22, 2024 22:00

Added robot thread naming

de78a0c

fix some compilation errors

5a23d75

ran fmt

1c9183b

it compiles 😳

7668034

added dprecation ignores in iterative robot base

4c545ec

Fixed Includes in Tracer.cpp

526403d

fixed deprecation warnings in watchdog

1fa81c8

class docs

52c74a3

deprecated scoped tracer and added new equivalent

c8d1a62

😭

6878049

fix deprecated errors in tracer

7832f67

added mechanism for substituting tracer state

c742e94

KangarooKoala reviewed Sep 30, 2024

View reviewed changes

oh-yes-0-fps added 2 commits September 30, 2024 17:01

fixed error with timeslice robot

d1a38f7

fixed deprecation warnings and added more addPeriodic overloads

7f68a91

ran fmt

06fb68a

KangarooKoala reviewed Oct 1, 2024

View reviewed changes

wpilibj/src/main/java/edu/wpi/first/wpilibj/TimedRobot.java Outdated Show resolved Hide resolved

wpilibj/src/main/java/edu/wpi/first/wpilibj/TimesliceRobot.java Outdated Show resolved Hide resolved

oh-yes-0-fps and others added 11 commits October 1, 2024 15:46

Update wpilibj/src/main/java/edu/wpi/first/wpilibj/TimedRobot.java

2000ef9

Co-authored-by: Joseph Eng <91924258+KangarooKoala@users.noreply.github.com>

Update wpilibj/src/main/java/edu/wpi/first/wpilibj/TimesliceRobot.java

e7b9e2c

Co-authored-by: Joseph Eng <91924258+KangarooKoala@users.noreply.github.com>

Fix epilogue test

3829f2b

fixed time robot docs

1721f3c

not sure why this isn't working

16e2adc

added more cpp tests and updated timed robot in cpp

ac78543

ran fmt

c88e29d

fixed deprecation warnings

bd8e779

ran fmt

e803048

fmt

322aeba

fixed missing tracer input

0cbbb01

oh-yes-0-fps requested a review from Gold856 October 3, 2024 03:57

oh-yes-0-fps added 2 commits October 3, 2024 00:01

rerun ci

b82fa6c

ran fmt

5d7f2e3

Gold856 suggested changes Oct 3, 2024

View reviewed changes

wpilibc/src/main/native/cpp/Tracer.cpp Outdated Show resolved Hide resolved

I'm so confused

07e643b

oh-yes-0-fps commented Oct 3, 2024

View reviewed changes

wpilibc/src/test/native/cpp/TracerTest.cpp Show resolved Hide resolved

oh-yes-0-fps added 2 commits October 3, 2024 15:46

removed ternary in PopTraceStack

2706c93

fixed tests

9272208

oh-yes-0-fps mentioned this pull request Oct 3, 2024

[wpilibj] Watchdog Alert Linking #7160

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wpilib] Tracer Overhaul #7099

[wpilib] Tracer Overhaul #7099

oh-yes-0-fps commented Sep 19, 2024 •

edited

Loading

rzblue commented Sep 19, 2024

oh-yes-0-fps commented Sep 19, 2024 •

edited

Loading

oh-yes-0-fps commented Sep 19, 2024

rzblue commented Sep 19, 2024

oh-yes-0-fps commented Sep 19, 2024

oh-yes-0-fps commented Sep 19, 2024 •

edited

Loading

rzblue commented Sep 19, 2024

KangarooKoala commented Sep 19, 2024

oh-yes-0-fps commented Sep 19, 2024

calcmogul commented Sep 19, 2024 •

edited

Loading

oh-yes-0-fps commented Sep 19, 2024

KangarooKoala left a comment

oh-yes-0-fps commented Oct 1, 2024

KangarooKoala left a comment

Gold856 Oct 3, 2024

Gold856 Oct 3, 2024

Gold856 Oct 3, 2024

oh-yes-0-fps Oct 3, 2024

Gold856 Oct 3, 2024

Gold856 Oct 3, 2024

Gold856 Oct 3, 2024

Gold856 Oct 3, 2024

Gold856 Oct 3, 2024

Gold856 left a comment

oh-yes-0-fps commented Oct 3, 2024

Gold856 commented Oct 3, 2024

		* Decorates this Command with a name. Is an inline function for
		* Command::SetName(std::string_view);

		EXPECT_TRUE(test1Entry.GetDouble(0.0) - 500.0 < 1.0);
		EXPECT_TRUE(test2Entry.GetDouble(0.0) - 400.0 < 1.0);

[wpilib] Tracer Overhaul #7099

Are you sure you want to change the base?

[wpilib] Tracer Overhaul #7099

Conversation

oh-yes-0-fps commented Sep 19, 2024 • edited Loading

rzblue commented Sep 19, 2024

oh-yes-0-fps commented Sep 19, 2024 • edited Loading

oh-yes-0-fps commented Sep 19, 2024

rzblue commented Sep 19, 2024

oh-yes-0-fps commented Sep 19, 2024

oh-yes-0-fps commented Sep 19, 2024 • edited Loading

rzblue commented Sep 19, 2024

KangarooKoala commented Sep 19, 2024

oh-yes-0-fps commented Sep 19, 2024

calcmogul commented Sep 19, 2024 • edited Loading

oh-yes-0-fps commented Sep 19, 2024

KangarooKoala left a comment

Choose a reason for hiding this comment

oh-yes-0-fps commented Oct 1, 2024

KangarooKoala left a 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

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

Gold856 left a comment

Choose a reason for hiding this comment

oh-yes-0-fps commented Oct 3, 2024

Gold856 commented Oct 3, 2024

oh-yes-0-fps commented Sep 19, 2024 •

edited

Loading

oh-yes-0-fps commented Sep 19, 2024 •

edited

Loading

oh-yes-0-fps commented Sep 19, 2024 •

edited

Loading

calcmogul commented Sep 19, 2024 •

edited

Loading