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.

Sign up for GitHub

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

Jump to bottom

[wpilib] Add on/off, forward/reverse/off boolean methods to Solenoid and DoubleSolenoids #7079

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

[wpilib] Add on/off, forward/reverse/off boolean methods to Solenoid and DoubleSolenoids #7079

wants to merge 34 commits into from

Conversation

narmstro2020
Copy link
Contributor

This PR is intended to make it easier to use the Java (::) operator for Solenoids and Double Solenoids for command based programming in regards to Triggers and Boolean Suppliers and provides an option to not use lambdas in those cases. It also provides void methods to turn solenoids on/off or forward/reverse/off in the case of double solenoids. Thereby reducing the need to pass in a boolean argument.

@narmstro2020 narmstro2020 marked this pull request as ready for review September 14, 2024 15:21
@narmstro2020 narmstro2020 requested a review from a team as a code owner September 14, 2024 15:21
@narmstro2020
Copy link
Contributor Author

/format

@narmstro2020
Copy link
Contributor Author

/format

@narmstro2020 narmstro2020 requested a review from rzblue September 14, 2024 18:02
@narmstro2020 narmstro2020 reopened this Sep 21, 2024
@narmstro2020
Copy link
Contributor Author

/format

@narmstro2020
Copy link
Contributor Author

@rzblue Anything left on this one.

rzblue
rzblue requested changes Sep 26, 2024
View reviewed changes
wpilibc/src/main/native/include/frc/simulation/SolenoidSim.h Outdated Show resolved Hide resolved
wpilibj/src/main/java/edu/wpi/first/wpilibj/simulation/DoubleSolenoidSim.java Show resolved Hide resolved
wpilibj/src/main/java/edu/wpi/first/wpilibj/simulation/SolenoidSim.java Outdated
@@ -50,16 +50,44 @@ public SolenoidSim(PneumaticsModuleType moduleType, int channel) {
*
* @return the solenoid output
*/
public boolean getOutput() {
public boolean get() {
Copy link
Member

Choose a reason for hiding this comment

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

ditto re: deprecation

Copy link
Member

Choose a reason for hiding this comment

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

See other comment re: deprecating getOutput in favor of isOn instead

@narmstro2020
Copy link
Contributor Author

@rzblue Okay I think I've made the requested changes.

@narmstro2020
Copy link
Contributor Author

/format

@narmstro2020
Copy link
Contributor Author

@rzblue
Okay. Sounds good. Let me know if I missed anything

@narmstro2020
Copy link
Contributor Author

This has been sitting for a bit. Anything that needs to happen on my end.

@narmstro2020
Copy link
Contributor Author

Anything else?

@Starlight220
Copy link
Member

Solenoid.get should be deprecated in Java as well.

Starlight220
Starlight220 previously approved these changes Dec 24, 2024
View reviewed changes
@Starlight220 Starlight220 requested a review from rzblue December 24, 2024 17:11
@sciencewhiz
Copy link
Contributor

sciencewhiz commented Dec 24, 2024 •
edited
Loading

Typically, I don't like deprecating items this close to release, as there's no time for feedback during beta. Particularly since there isn't really a problem with the old methods, my recommendation would be not to deprecate them.

With that being said, deprecation notice should be more clear that isOn is exactly the same as get, and the C++ ones should also use the doxygen deprecated annotation

@narmstro2020
Copy link
Contributor Author

Typically, I don't like deprecating items this close to release, as there's no time for feedback during beta. Particularly since there isn't really a problem with the old methods, my recommendation would be not to deprecate them.

With that being said, deprecation notice should be more clear that isOn is exactly the same as get, and the C++ ones should also use the doxygen deprecated annotation

Okay. So remove the deprecations across the board?

@narmstro2020
Copy link
Contributor Author

Typically, I don't like deprecating items this close to release, as there's no time for feedback during beta. Particularly since there isn't really a problem with the old methods, my recommendation would be not to deprecate them.

With that being said, deprecation notice should be more clear that isOn is exactly the same as get, and the C++ ones should also use the doxygen deprecated annotation

I just want to make sure I've got it clear before I start editing some more

@Starlight220
Copy link
Member

To sum up:

  • remove deprecations
  • have comments indicating that get and isOn are aliases

@narmstro2020
Copy link
Contributor Author

Hows this?

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

@Starlight220 Starlight220 Starlight220 approved these changes

@rzblue rzblue Awaiting requested review from rzblue

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
component: wpilibc WPILib C++ component: wpilibj WPILib Java
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@narmstro2020 @rzblue @Starlight220 @sciencewhiz