-
Notifications
You must be signed in to change notification settings - Fork 631
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
[hal, wpilib] PWM Rewrite #7845
base: 2027
Are you sure you want to change the base?
Conversation
@@ -4,7 +4,6 @@ nativeUtils { | |||
groupId = "edu.wpi.first.thirdparty.frc2024.ceres" | |||
artifactId = "ceres-cpp" | |||
headerClassifier = "headers" | |||
sourceClassifier = "sources" |
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.
Unrelated change?
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.
Yes, but #7844 will likely be merged beforehand and make this OBE.
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.
Was the change missed in 7844?
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.
We haven't merged main into 2027 since then.
m_rawCbKey = REGISTER(PulseMicrosecond, "<raw", int32_t, int); | ||
m_periodScaleCbKey = REGISTER(PeriodScale, "<period_scale", int32_t, int); | ||
m_zeroLatchCbKey = REGISTER(ZeroLatch, "<zero_latch", bool, boolean); | ||
m_outputPeriodCbKey = REGISTER(OutputPeriod, "<output_period", int32_t, int); |
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.
Need to update the websocket doc to match (both hardware_ws_api.md and wpilib-ws.yaml). Note this will break the Romi motor interface as well, but that should probably be changed to use a SimDevice.
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.
Theres already a decent chunk of romi breakages in 2027. But I'll update that for now.
void SetSpeed(double val) override { HALSIM_SetPWMSpeed(m_index, val); } | ||
void SetSpeed(double val) override { | ||
HALSIM_SetPWMPulseMicrosecond(m_index, val); | ||
} |
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.
Naming of Speed functions should be updated to be PulseMicrosecond or similar. Parameter/return types should also be changed from double to int32_t. But most likely this just needs a major update to use the SimDevice pieces instead? That can be a separate PR, we should just open an issue to track it.
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.
Yeah the 2nd one is what is really needed. I just wanted it compiling.
The HAL will only contain the output period and the raw microseconds. Higher level things such as SimDevice can handle everything else.