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

[wpilibj] PowerDistribution Logger #7131

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

oh-yes-0-fps
Copy link
Contributor

Adds a PowerDistributionLogger class that can log data to nt or datalog in the form of 2 structs. The Stats struct contains transient data like voltage, temp and current. Stats are intended to be continually updated over the lifetime of the robot program. The Meta struct contains static information like canId, module type and version info. Meta info is only logged once on logger instantiation.

I decided to add a new class over modifying the PowerDistribution class to keep PowerDistribution completely immutable.

I plan on doing more pr to add struct support for pneumatics, robot controller and more but this is a good start.

@oh-yes-0-fps oh-yes-0-fps requested review from a team as code owners September 26, 2024 19:57
@rzblue
Copy link
Member

rzblue commented Sep 26, 2024

I think we really need to discuss how this (and other automatic telemetry facilities) fit in to the overall ecosystem. There's currently several different ways that wpilib enables logging like this (sendable, DS logging, implicit FMS data logging, epilogue), and it's going to be confusing for users.

Comment on lines +85 to +99
/**
* Sets the log windup value, this value determines how many calls to {@link #log()} are required
* before the data is logged. This is available to prevent needless JNI calls when no new data is
* available.
*
* <p>For example, if this value is set to 5, the data will only be logged every 5 calls to {@link
* #log()}.
*
* <p>The {@link PowerDistribution} refreshes it's data every 80ms.
*
* @param windup The number of calls to {@link #log()} required before the data is logged
*/
public void setLogWindup(int windup) {
m_logWindup = windup;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is way premature optimization implemented in a confusing way; updating the log is a handful of JNI functions so it really shouldn't be that much overhead.

If reducing JNI overhead is a concern the entire thing could be shunted to JNI.

Another interesting option would be to utilize the CAN streaming APIs available for power distribution. This would allow all data to be logged as it arrived, with no wasted log entries.

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.

2 participants