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

[hal, wpilib] Add DS OpMode support #7744

Open
wants to merge 1 commit into
base: 2027
Choose a base branch
from
Open

[hal, wpilib] Add DS OpMode support #7744

wants to merge 1 commit into from
+245 −3

Conversation

PeterJohnson
Copy link
Member

TODO:

  • High level sim support
  • NT publishing
  • Testing

@PeterJohnson PeterJohnson requested review from a team as code owners January 29, 2025 06:48
@github-actions github-actions bot added component: wpilibj WPILib Java component: wpilibc WPILib C++ component: hal Hardware Abstraction Layer 2027 2027 target labels Jan 29, 2025
ThadHouse
ThadHouse reviewed Jan 30, 2025
View reviewed changes
hal/src/main/native/include/hal/DriverStation.h
* @param len length of buffer (in), length of string (out)
* @return the error code, or 0 for success
*/
int32_t HAL_GetOpMode(char* buf, int32_t* len);
Copy link
Member

@ThadHouse ThadHouse Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Not the biggest fan of this being buf and length, when we have WPI_String. However, I'm also concerned about the allocations, since this is going to be on the hot path. Need to think about this one.

Copy link
Member Author

@PeterJohnson PeterJohnson Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I considered WPI_String, but this is called likely multiple times every loop. Can’t really help Java though, that’s always going to be an allocation. Maybe we should consider an approach for Java that returns null if it hasn’t changed since the last call, or something like that? Feels like micro-optimization though.

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

@ThadHouse ThadHouse ThadHouse left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
2027 2027 target component: hal Hardware Abstraction Layer component: wpilibc WPILib C++ component: wpilibj WPILib Java
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PeterJohnson @ThadHouse