allwpilib
allwpilib copied to clipboard
wpilibsuite
Refactor constants from kConstant to ALL_CAPS
Fixes #7943
This is a MASSIVE PR, but I tried to break it up into separate commits per subproject. They were all tested locally with ./gradlew :<project>:build, but this is my first contribution here, so it's entirely plausible (and even likely) I've overlooked some aspect.
The one thing I wanted to call out is the use of kP/kI/kD/etc. Do we want to leave it as-is? Update it to P/I/D? KP/KI/KD? something else? For now, I left them alone, but that's easy enough to revisit.
I'm also certain documentation will need some updates to match the new style
Okay, I have everything fixed except a few things from wpiformat, which is odd because running it locally doesn't produce any required changes. I can manually make those changes if you'd like, unless there's a better way to do so
You can download a patch file from the wpiformat action and apply that. https://github.com/wpilibsuite/allwpilib/actions/runs/14823407025?pr=7954
Since all the wpiformat changes are in python, do you have black installed?
Since all the wpiformat changes are in python, do you have black installed?
Hmm... good observation. I likely don't. I was effectively mimicking what this job is doing: https://github.com/wpilibsuite/allwpilib/blob/main/.github/workflows/lint-format.yml#L21-L58. Beyond that, wpiformat is a bit of a black box to me
Also, my browser gets sad trying to handle all 1226 files, so I'm just going to list all the spots here so the page doesn't crash.
Yeah... I'm considering posting a script that generates all of the easily generatable changes so that people could do git restore --source $(git merge-base 2027 HEAD); ./make_changes.py; git diff to view just the non-easily generatable changes.
FWIW github.dev (i.e. press . to open VS Code web) seems to fare okay at viewing this PR.
Having said that though, if scripting it isn't too hard, it might be easier to review such a script and land whatever changes it makes first.
SysId has a bunch of changes where
Kwas removed from feedforward gains. I assume that's unintentional?
Yes that was unintentional. I started making changes then realized it was going to become problematic. I reverted most, but apparently missed these. I can either revert these or change the rest. Is there a consensus on how we want to handle those cases?
LTVDifferentialDriveControllerTest.java has kX and KY
Whoops...
Struct.java has kSizeDouble in the Javadoc
I couldn't find where that one is defined, but just took another look and realized I need to stop working on code after midnight.... Committing the change momentarily
MappedFileRegion has a MapMode enum with kConstant style
This one was intentional as I wasn't sure how certain things related. This enum is used by code in the thirdparty directory. I tried to avoid touching those, which meant leaving this alone. Easy enough to go back and update if thirdparty is safe to edit. Just didn't want to cause issues with later upgrades if that code is intended to be a local copy of external projects.
We frequently patch libraries under thirdparty directories to make them fit our needs. Typically, we manage patches with upstream_utils, but it looks like MemoryBuffer, the only consumer of MapMode, is exempt and can just be modified directly.
Something I'm just going to throw out here- Based on the https://github.com/orgs/wpilibsuite/projects/17/views/1 project board, this might be held off until fall anyways to reduce how much teams need to translate during the alpha test. (Though maybe we could set up a separate 2027-post-alpha branch for this and other P2 PRs to target? There's a decent amount of stuff to consider for that decision though and I'm not the one to make it.)
I think that's just ranking relative importance. Ideally, we'd have all breaking changes done by alpha so we get feedback on all those changes. We only have so much labor to throw at things though.
Based on this Discord message from 5/7/25, it seems that we want to limit the amount of changes teams and vendors need to make:
For broader community awareness as this affects PR velocity in certain areas: I’ve updated the priorities in the 2027 project on github to better reflect our aim points for the alpha test, which is (particularly initially) a hardware-focused test where we want teams to port existing code to the new system and shake out the hardware, so we want to limit the amount of changes teams and vendors need to make (plus we have limited time to make changes before it starts).
P0 is what we need in to get done in the next month to support the start of alpha. P1 is things we want to target releasing during alpha to get some early testing on (but not at the expense of hardware testing). P2 is items we should be looking at integrating this fall.
https://github.com/orgs/wpilibsuite/projects/17
SysId has a bunch of changes where K was removed from feedforward gains. I assume that's unintentional?
Yes that was unintentional. I started making changes then realized it was going to become problematic. I reverted most, but apparently missed these. I can either revert these or change the rest. Is there a consensus on how we want to handle those cases?
Constants like kP can keep the k prefix since they're part of the math notation rather than a Hungarian notation thing. For example, kₚ for the P gain in controls academia. Another option is just writing P for the constant since it's for a PID controller, and we want ALL_CAPS.
Here is a Python script that reproduces nearly all of the changes made in this PR, and the accompanying generate.sh script will apply the changes, with the end result that afterwards, running git diff will show the changes on the PR branch relative to the generated changes. Some changes that don't seem right are marked with # NOTE in the generate.py script, and running ./generate.sh; git diff wpilibj wpilibjExamples with the branch checked out will also show some other typos or inconsistent renames.
Now that the 2027 alpha has been released and the package reorg has been merged, this PR should be unblocked. For best results resolving the merge conflicts, we recommend squashing fixup commits, then doing a rebase. Git seems to do a good job tracking the file renames, and most of the conflicts are just import/include updates.
Impeccable timing. I was just looking at this earlier today and haven't had a chance to comment asking if now was a good time to revisit this.