Skip to content
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

Allow loop epochs to be logged to DataLog and published to NT #5375

Closed
wants to merge 27 commits into from
Closed

Allow loop epochs to be logged to DataLog and published to NT #5375

wants to merge 27 commits into from

Conversation

Gold856
Copy link
Contributor

@Gold856 Gold856 commented Jun 2, 2023

Includes methods to enable DataLog logging or NT publishing. The first time an epoch is added, an NT publisher is created and a DataLog entry is started (if they're enabled), both of which are cached for future use. Everytime an epoch is added, it gets published to NT and logged to DataLog (if they're enabled). Addresses a part of #5314. Resolves #609.

@Gold856 Gold856 requested a review from a team as a code owner June 2, 2023 22:21
@Gold856

This comment was marked as outdated.

@PeterJohnson PeterJohnson requested review from PeterJohnson and a team as code owners June 2, 2023 22:25
@Gold856
Copy link
Contributor Author

Gold856 commented Jun 2, 2023

Huh. How did Actions screw up that badly?

@Gold856

This comment was marked as outdated.

@PeterJohnson
Copy link
Member

PeterJohnson commented Jun 2, 2023

Yeah something is really messed up with that action... there was some changes made to wpiformat recently that may have broken it.

@Gold856
Copy link
Contributor Author

Gold856 commented Jun 2, 2023

Emergency PR time!

@rzblue
Copy link
Member

rzblue commented Jun 3, 2023

Do you need help with the c++ implementation?

@Gold856
Copy link
Contributor Author

Gold856 commented Jun 3, 2023

A little bit, the thing with std::string_view is messing with me. How do I concatenate a string literal with std::string_view? Trying to separate epochs into sub keys by adding "/".

@Gold856

This comment was marked as resolved.

@Gold856
Copy link
Contributor Author

Gold856 commented Jun 4, 2023

/format
Let's make sure this is fixed

We're good!

@rzblue
Copy link
Member

rzblue commented Jun 4, 2023

A little bit, the thing with std::string_view is messing with me. How do I concatenate a string literal with std::string_view? Trying to separate epochs into sub keys by adding "/".

You can't directly concatenate std::string_view- why do you need to use std::string_view?

If you upload what code you have I can help out better.

@Gold856

This comment was marked as resolved.

Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

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

Just some naming notes: publishToNetworkTables and startDataLog is unclear for CommandScheduler and IterativeRobotBase because it isn't immediately obvious what is being published or logged. (I don't think this isn't a problem for Tracer or Watchdog because those classes should provide enough context, whereas the other cases have too many other possibilities)

@rzblue rzblue added the component: telemetry High level telemetry functionality label Jun 21, 2023
@Gold856
Copy link
Contributor Author

Gold856 commented Jun 24, 2023

Just gonna leave this note here so I don't forget and for other people to contemplate: it should be possible to make Tracer implement Sendable because Tracer objects need to be reset at the start of a method for accurate epoch logging; since SmartDashboard.updateValues() (or whatever will replace it in the telemetry improvements) is one of the last things called in IterativeRobotBase, the tracers will have their epochs before they're cleared, so we can add them as properties. The only problem is that it would be difficult to publish the amount of time SmartDashboard.updateValues() itself takes; you can't publish data that doesn't exist yet.
Alternatively, we can cheat a little and still make it Sendable by grabbing the topic name from the builder in initSendable, and calling publishToNetworkTables with the builder's topic name. That allows all the epochs to be grouped under one topic per Tracer, bypasses SmartDashboard.updateValues(), and should still be compatible with the upcoming telemetry changes.

@Gold856
Copy link
Contributor Author

Gold856 commented Jul 9, 2023

I've done some more thinking, and I think that making Tracer implement Sendable might not work. Any epoch can be added to a Tracer with an arbitrary name. As a result, a topic/entry is created for each new epoch in addEpoch and is cached for subsequent loops. However, Sendable requires you to declare all properties in initSendable, and new properties can't be added after the fact. Since epochs aren't known until runtime, this means the topics corresponding to epochs can't be created in initSendable, making it difficult for Tracer to implement Sendable. The core issue is that Sendable doesn't support dynamic topic creation, which is pretty reasonable, considering dynamically creating topics is not something you need to do often. NTSendable would allow dynamically created topics with setUpdateTable, but I think we're also trying to deprecate NTSendable, and pulling setUpdateTable into Sendable doesn't make for very futureproof code (it would need to account for all backends). We could try and make Sendable support dynamic topic creation, but that's far out of scope for this PR and probably not worth it.

@rzblue rzblue added the state: blocked Something is blocking action. label Aug 4, 2023
@Gold856
Copy link
Contributor Author

Gold856 commented Aug 4, 2023

What's blocking this PR?

@Starlight220
Copy link
Member

Perhaps use the structured data support that #5391 will add to encode the epochs as a single topic/property?

@PeterJohnson
Copy link
Member

Multiple topics is the right approach here. While structured data could be used, you would have to use protobuf (higher overhead) and send the string names on every update (a lot higher overhead). It does feel like we need to figure out a good api shape for nesting dynamic topics or even other Sendables within Sendable without requiring NTSendable.

@Gold856
Copy link
Contributor Author

Gold856 commented Aug 4, 2023

Maybe SendableBuilder could have a getPath method. It would function similar to getTopic, but would return a string representation of the path where the properties will be placed, e.g. /Epochs/Scheduler or /SmartDashboard/Scheduler/Epochs. It would be up to the implementing class to support all backends (I don't think delgating dynamic topics to SendableBuilder is the right path). The implementing class would store the path in a member variable, and would use it with NT publishers and DataLog to publish properties and their values.

It also conveniently allows values to be updated independently without needing SmartDashboard.updateValues, which is important for Tracer (since it otherwise can't measure the time SmartDashboard.updateValues takes.

@Starlight220
Copy link
Member

other Sendables within Sendable

#5180? It still has some design decisions needed, though.

Copy link
Member

@auscompgeek auscompgeek left a comment

Choose a reason for hiding this comment

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

Should we have corresponding unpublish/stop methods?

Gold856 and others added 3 commits September 17, 2023 00:57
Co-authored-by: David Vo <auscompgeek@users.noreply.github.com>
@calcmogul
Copy link
Member

I've fixed the CMake failure in #6403.

Copy link
Member

@Starlight220 Starlight220 left a comment

Choose a reason for hiding this comment

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

The technical debt here isn't great, but we don't have a better solution at the moment so I agree this is worth it.
Most of this looks good, just has some resource closing issues.

wpilibc/src/main/native/cpp/Tracer.cpp Outdated Show resolved Hide resolved
wpilibj/src/main/java/edu/wpi/first/wpilibj/Tracer.java Outdated Show resolved Hide resolved
Copy link
Member

@Starlight220 Starlight220 left a comment

Choose a reason for hiding this comment

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

Watchdog should close its tracer

wpilibj/src/main/java/edu/wpi/first/wpilibj/Tracer.java Outdated Show resolved Hide resolved
@Gold856 Gold856 closed this by deleting the head repository Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: telemetry High level telemetry functionality state: blocked Something is blocking action.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add loop timing telemetry
7 participants