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

Add/Fix pdp.GetAllCurrents() #6025

Merged
merged 7 commits into from
May 24, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions hal/src/main/native/cpp/jni/PowerDistributionJNI.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -179,15 +179,17 @@ JNIEXPORT void JNICALL
Java_edu_wpi_first_hal_PowerDistributionJNI_getAllCurrents
(JNIEnv* env, jclass, jint handle, jdoubleArray jarr)
{
double storage[16];
int32_t status = 0;
// TODO fix me
HAL_GetPowerDistributionAllChannelCurrents(handle, storage, 16, &status);
wpi::SmallVector<double, 24> storage;
storage.resize_for_overwrite(24);

HAL_GetPowerDistributionAllChannelCurrents(handle, storage.data(), 24,
&status);
if (!CheckStatus(env, status, false)) {
return;
}

env->SetDoubleArrayRegion(jarr, 0, 16, storage);
env->SetDoubleArrayRegion(jarr, 0, 24, storage.data());
}

/*
Expand Down
22 changes: 19 additions & 3 deletions wpilibc/src/main/native/cpp/PowerDistribution.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,13 @@ PowerDistribution::~PowerDistribution() {
}
}

int PowerDistribution::GetNumChannels() const {
int32_t status = 0;
int32_t size = HAL_GetPowerDistributionNumChannels(m_handle, &status);
FRC_ReportError(status, "Module {}", m_module);
return size;
}

double PowerDistribution::GetVoltage() const {
int32_t status = 0;
double voltage = HAL_GetPowerDistributionVoltage(m_handle, &status);
Expand All @@ -87,6 +94,17 @@ double PowerDistribution::GetCurrent(int channel) const {
return current;
}

std::vector<double> PowerDistribution::GetAllCurrents() const {
Copy link
Member

Choose a reason for hiding this comment

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

@PeterJohnson should we make this be a SmallVector too? This could be a pretty hot path.

Copy link
Contributor Author

@scarmain scarmain Dec 10, 2023

Choose a reason for hiding this comment

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

I choose std::vector as it was the return type of all the NetworkTables get__Array() like GetDoubleArray(). I'm neutral either way.

The API call here should be the vector/SmallVector though, as I don't see anywhere in the C++ APIs that we return arrays.

Honestly, if speed was a concern, I think in the JNI service, we could go back to my initial prototype of just a straight double[24] array. We don't need a vector there as we could just allocate to the biggest PDP size, and the HAL filling the array means it doesn't need to be initialized too. Since it doesn't leave the function, we don't need the dynamic sizing there. I just didn't see as much array support in C++ (and I'm not a native C++ developer).

Copy link
Member

@PeterJohnson PeterJohnson Dec 11, 2023

Choose a reason for hiding this comment

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

So typically how we do this is we have both a vector and a SmallVector overload of the same function. So you would keep the current function and add a SmallVector-taking one: std::span<double> GetAllCurrents(wpi::SmallVectorImpl<double>& buf).

int32_t status = 0;
int32_t size = GetNumChannels();
std::vector<double> currents(size);

HAL_GetPowerDistributionAllChannelCurrents(m_handle, currents.data(), size,
&status);
FRC_ReportError(status, "Module {}", m_module);
return currents;
}

double PowerDistribution::GetTotalCurrent() const {
int32_t status = 0;
double current = HAL_GetPowerDistributionTotalCurrent(m_handle, &status);
Expand Down Expand Up @@ -188,9 +206,7 @@ PowerDistribution::StickyFaults PowerDistribution::GetStickyFaults() const {

void PowerDistribution::InitSendable(wpi::SendableBuilder& builder) {
builder.SetSmartDashboardType("PowerDistribution");
int32_t status = 0;
int numChannels = HAL_GetPowerDistributionNumChannels(m_handle, &status);
FRC_ReportError(status, "Module {}", m_module);
int numChannels = GetNumChannels();
// Use manual reads to avoid printing errors
for (int i = 0; i < numChannels; ++i) {
builder.AddDoubleProperty(
Expand Down
14 changes: 14 additions & 0 deletions wpilibc/src/main/native/include/frc/PowerDistribution.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,13 @@ class PowerDistribution : public wpi::Sendable,
PowerDistribution(PowerDistribution&&) = default;
PowerDistribution& operator=(PowerDistribution&&) = default;

/**
* Gets the number of channels for this power distribution object.
*
* @return Number of output channels (16 for PDP, 24 for PDH).
*/
int GetNumChannels() const;

/**
* Query the input voltage of the PDP/PDH.
*
Expand All @@ -62,6 +69,13 @@ class PowerDistribution : public wpi::Sendable,
*/
double GetCurrent(int channel) const;

/**
* Query all currents of the PDP.
*
* @return The current of each channel in Amperes
*/
std::vector<double> GetAllCurrents() const;

/**
* Query the total current of all monitored PDP/PDH channels.
*
Expand Down
22 changes: 22 additions & 0 deletions wpilibc/src/test/native/cpp/simulation/PDPSimTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -79,4 +79,26 @@ TEST(PowerDistributionSimTest, SetCurrent) {
EXPECT_TRUE(callback.GetLastValue());
}
}

TEST(PowerDistributionSimTest, GetAllCurrents) {
HAL_Initialize(500, 0);
PowerDistribution pdp{2, frc::PowerDistribution::ModuleType::kRev};
PowerDistributionSim sim(pdp);

// setup
for (int channel = 0; channel < pdp.GetNumChannels(); ++channel) {
const double kTestCurrent = 24 - channel;
sim.SetCurrent(channel, kTestCurrent);
}

// run it
std::vector<double> currents = pdp.GetAllCurrents();

// verify
for (int channel = 0; channel < pdp.GetNumChannels(); ++channel) {
const double kTestCurrent = 24 - channel;
EXPECT_EQ(kTestCurrent, currents[channel]);
}
}

} // namespace frc::sim
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,17 @@ public double getCurrent(int channel) {
return PowerDistributionJNI.getChannelCurrent(m_handle, channel);
}

/**
* Query all currents of the PDP.
*
* @return The current of each channel in Amperes
*/
public double[] getAllCurrents() {
double[] currents = new double[getNumChannels()];
PowerDistributionJNI.getAllCurrents(m_handle, currents);
return currents;
}

/**
* Query the current of all monitored channels.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright (c) FIRST and other WPILib contributors.
// Open Source Software; you can modify and/or share it under the terms of
// the WPILib BSD license file in the root directory of this project.

package edu.wpi.first.wpilibj;

import static org.junit.jupiter.api.Assertions.assertEquals;

import edu.wpi.first.hal.HAL;
import edu.wpi.first.wpilibj.PowerDistribution.ModuleType;
import edu.wpi.first.wpilibj.simulation.PDPSim;
import org.junit.jupiter.api.Test;

class PowerDistributionTest {
@Test
void testGetAllCurrents() {
HAL.initialize(500, 0);
PowerDistribution pdp = new PowerDistribution(1, ModuleType.kRev);
PDPSim sim = new PDPSim(pdp);

for (int i = 0; i < pdp.getNumChannels(); i++) {
sim.setCurrent(i, 24 - i);
}
double[] currents = pdp.getAllCurrents();
for (int i = 0; i < pdp.getNumChannels(); i++) {
assertEquals(24 - i, currents[i]);
}
}
}
Loading