-
Notifications
You must be signed in to change notification settings - Fork 665
[wpilib] Add a few unit overloads #8231
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
base: main
Are you sure you want to change the base?
Conversation
wpilibj/src/main/java/edu/wpi/first/wpilibj/event/BooleanEvent.java
Outdated
Show resolved
Hide resolved
wpilibj/src/main/java/edu/wpi/first/wpilibj/event/BooleanEvent.java
Outdated
Show resolved
Hide resolved
fyi, these changes can be ported to C++ as well, despite the issue specifically mentioning Java. |
Co-authored-by: Sam Carlberg <[email protected]>
Co-authored-by: Sam Carlberg <[email protected]>
Co-authored-by: Sam Carlberg <[email protected]>
Co-authored-by: Sam Carlberg <[email protected]>
wpilibj/src/main/java/edu/wpi/first/wpilibj/event/BooleanEvent.java
Outdated
Show resolved
Hide resolved
@calcmogul I see this: allwpilib/wpilibc/src/main/native/cpp/TimedRobot.cpp Lines 106 to 107 in ee3d55e
|
Linux definitely does work. However by default in allwpilib, the project intellisense generation doesn't run. You can run |
That makes more sense. There's probably a reason that it doesn't when I open up the project that I don't know. Maybe bad stuff happens if I close the project before the command completes? |
@calcmogul |
You can just use the SI units ( |
On the C++ tests, I'm getting this message:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First: Please make the following changes to the C++ overload set to match Java.
- Add
void StartPeriodic(units::hertz_t frequency);
toNotifier.h
. (Don't forget to copy-paste the doc comment abovevoid StartPeriodic(units::second_t period);
and to replace period with frequency.) The implementation can beStartPeriodic(1.0 / frequency);
. - Add
explicit TimedRobot(units::hertz_t frequency = 1 / kDefaultPeriod);
. (Again, copy-paste the doc comment from theunits::second_t
version and replace period with frequency.) For this one, you'll want to use the following syntax in theTimedRobot.cpp
file to delegate to the main constructor:TimedRobot::TimedRobot(units::hertz_t frequency) : TimedRobot{1 / frequency} {}
- Remove
Timer::HasElapsed(units::hertz_t)
- The Java equivalent would beTimer.hasElapsed(Frequency)
, but that is intentionally not present, since it's not useful. - Remove
Watchdog::SetTimeout(units::hertz_t)
- Similar to the above note forTimer::HasElapsed()
. - Remove
BooleanEvent::Debounce(units::hertz_t, frc::Debouncer::DebounceType)
- Similar to the above note forTimer::HasElapsed()
.
Second: The compilation error seems to be a problem with the C++ units library we're currently using. I'll work on a fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Co-authored-by: Joseph Eng <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems okay to me for what it's worth, though you should probably run wpiformat -f wpilibc/src/main/native/cpp/TimedRobot.cpp
to make sure the long constructor line doesn't need to be broken up. (If you haven't installed wpiformat
, just run python3 -m pip install wpiformat
)
This PR is blocked on #8267. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me, for what it's worth.
Closes #8191