Skip to content

Added default PID values#8895

Open
siddharth-shah121 wants to merge 12 commits into
wpilibsuite:mainfrom
siddharth-shah121:main
Open

Added default PID values#8895
siddharth-shah121 wants to merge 12 commits into
wpilibsuite:mainfrom
siddharth-shah121:main

Conversation

@siddharth-shah121
Copy link
Copy Markdown

For the specific files, I have added the default PID values, since zach told me to.

@siddharth-shah121 siddharth-shah121 requested a review from a team as a code owner May 13, 2026 23:36
@github-actions github-actions Bot added the component: wpilibj WPILib Java label May 13, 2026
@ThadHouse
Copy link
Copy Markdown
Member

This needs to be done in c++ too.

m_dPublisher.set(d);
m_pPublisher.set(1);
m_iPublisher.set(0);
m_dPublisher.set(0.01);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Leave the setPID method alone, just set the default values in the constructor. DoublePublisher even has a setDefault method.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

So just add below got it

m_dPublisher.set(d);
m_pPublisher.set(1);
m_iPublisher.set(0);
m_dPublisher.set(0.01);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See my comment for PositionConstants.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yep

Update PID coefficients in setPID method.
Update PID coefficients to fixed values.
Updated PID default values and modified setPID method to accept parameters.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

where did the rest of this file go

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

alr i think i did it

@github-actions github-actions Bot added the component: wpilibc WPILib C++ label May 14, 2026
.Publish(options);

// Default PID values
m_pPublisher.SetDefault(1.0);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't use SetDefault here, just use Set

.GetDoubleTopic(fmt::format("/rhsp/{}/motor{}/constants/velocity/kd",
hubNumber, motorNumber))
.Publish(options);
m_pPublisher.SetDefault(1.0);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same thing here, use Set

.publish(options);

// Default PID values
m_pPublisher.setDefault(1.0);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here, use set

"/rhsp/" + hubNumber + "/motor" + motorNumber + "/constants/velocity/kd")
.publish(options);

m_pPublisher.setDefault(1.0);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here, use set

@ThadHouse
Copy link
Copy Markdown
Member

You're going to need to undo all the formatting changes made here. Everything was already correctly formatted.

@siddharth-shah121
Copy link
Copy Markdown
Author

I only need to revert formatting for the cpp, correct?

@ThadHouse
Copy link
Copy Markdown
Member

Java has some bad formatting too. You can set up wpiformat and it will format it for you. Theres instructions in the repo.

@siddharth-shah121
Copy link
Copy Markdown
Author

I could just do it manually at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants