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

Create NTLogAppender #1297

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Create NTLogAppender #1297

wants to merge 2 commits into from

Conversation

mcm001
Copy link
Contributor

@mcm001 mcm001 commented Mar 22, 2024

image

I don't think this adds much bandwidth, and massively helps debug photon when looking at wpilogs of NT data

based on #1298

@mcm001 mcm001 requested a review from a team as a code owner March 22, 2024 01:18
@mcm001 mcm001 force-pushed the nt-debug branch 2 times, most recently from fb5f529 to e7dd94e Compare March 22, 2024 02:24
Copy link
Member

@srimanachanta srimanachanta left a comment

Choose a reason for hiding this comment

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

Looks good to me but I think debug options should have the ability to be disabled. That is a lot of work though so...

@mcm001
Copy link
Contributor Author

mcm001 commented Mar 22, 2024

It's super easy to must not send debug prints to NT, if we want?

@srimanachanta
Copy link
Member

It's super easy to must not send debug prints to NT, if we want?

I meant more adding a UI toggle to handle logging to NT

@mcm001
Copy link
Contributor Author

mcm001 commented Mar 23, 2024

Yeah idk. Unless teams see a legit value add from having photon logs in their wpilogs, I may punt on this one

@srimanachanta
Copy link
Member

Yeah idk. Unless teams see a legit value add from having photon logs in their wpilogs, I may punt on this one

I think it would be useful, especially for teams that do log

@TheTripleV
Copy link
Contributor

On 2nd thought, maybe the rio nt server will get bogged down. Logging rio console to wpilog via advantagekit slowed things down for us.

Instead, maybe it would be better for photonlib to publish the name of the wpilog and have photon live rename the log file to match what the wpilog is called.

@mcm001
Copy link
Contributor Author

mcm001 commented May 13, 2024

That's decently clean. I do think we can reevaluate this come 2027 (ROSconsole, for example, is a thing and exists). Wanna make an issue to track auto-renaming logs instead? Or at the very least print in the log when valid match number is sent?

@mcm001
Copy link
Contributor Author

mcm001 commented Nov 2, 2024

I'm inclined to merge this -- if nobody subscribes to the topic, nothing should happen I believe? Need to confirm with @PeterJohnson that a topic published by a client with no subscribers doesn't actually get sent to the server.

@PeterJohnson
Copy link

Currently it still gets sent to the server, although I’m looking at fixing that soon.

@mcm001
Copy link
Contributor Author

mcm001 commented Nov 2, 2024

Happy to cut an issue to track that in allwpilib

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants