search

wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.04k stars 605 forks source link

Example Best-Practices of Command-Based usage #6618

Open tom131313 opened 2 months ago

tom131313 commented 2 months ago

Add an Example Best-Practices of Command-Based usage. Issue #6616 Reference: CD thread Utilize an LED-based system to show various best-practices in coding Command-Based programs.

tom131313 commented 1 month ago

@oblarg I've completed the "best practice" example usage of command based (I haven't changed anything in 5 minutes now so it must be done!).

It is composed of the work of @illinar , @bovlb , @SamCarlberg plus I added the methods and tests I created for the "DisjointSequence" project. I think it demonstrates the ideas that were listed in the CD thread with some brief but far from complete commentary on why things are as they are.

There are some deficiencies as far as a WPILib submission goes: no C++, no Python, and only once did I dare to type "m_" variable prefix and sure enough my keyboard caught on fire and that was that.

I learned a lot and will use this for my team. I'm happy to share with anyone. If it misses the point as a WPILib example, I can close the PR and delete it as I have my own copy also in my GitHub repositories. I'm happy to do some more work on it if that is useful.

Thanks for your leadership on this subject.

tom131313 commented 1 month ago

@jasondaming I happened to see that my PR wants WPILib style so I've changed the code to satisfy at least most of the messages that I found. I see a wpiformat log that said at the end it was in error but I don't know how to find the errors in 3000 lines of output (tried to look but failed).

I don't know how to trigger the style checking again for round two of changing the style. Was that triggered by you? Thanks

calcmogul commented 1 month ago

I pushed a formatting fix for you. Here's what I did.

To fix C++:

pip install wpiformat
cd allwpilib
python -m wpiformat

To fix Java, remove the commented out import line in wpilibjExamples/src/main/java/edu/wpi/first/wpilibj/examples/commandbasedbestpracticeled/LEDPattern.java, then run ./gradlew wpilibjExamples:spotlessApply.

The large diff was caused by incorrect line endings on your side. There's a git config setting to avoid that issue on Windows, but I don't remember what it is.

tom131313 commented 1 month ago

@calcmogul

run ./gradlew wpilibjExamples:spotlessApply

I corrected (most) problems with the formatting from your running spotless. I also fixed the CR/LF issue but not sure if I did it in the right place.

I ran spotless as you said and can't see any issues and that cannot possibly be right. I looked at everything at the link to the gradle build scan.

Several days ago with lots of help I got the allwpilib fork for the PR to my github account. If there is something I need to change to give update access to all who need it, I'd be happy to follow more instructions.

More help is appreciated. Thanks.

My terminal output:

Microsoft Windows [Version 10.0.19045.4412] (c) Microsoft Corporation. All rights reserved.

C:\Users\RKT\frc\FRC2024\Code\allwpilib>gradlew wpilibjExamples:spotlessApply

Configure project : Tag is null. This does not match the expected version number pattern. No version number was generated. Skipping builds for arm32 (toolchain is marked optional) Skipping builds for arm64 (toolchain is marked optional)

BUILD SUCCESSFUL in 24s 13 actionable tasks: 2 executed, 11 up-to-date

Publishing build scan... https://gradle.com/s/kpdv2euzlyeqc

WispySparks commented 1 month ago

Why are all of the LED classes copied? (AddressableLED, AddressableLEDBuffer, AddressableLEDBufferView, Color, Color8Bit, LEDPattern, LEDReader, and LEDWriter)

tom131313 commented 1 month ago

Why are all of the LED classes copied? (AddressableLED, AddressableLEDBuffer, AddressableLEDBufferView, Color, Color8Bit, LEDPattern, LEDReader, and LEDWriter)

Illinar suggested using them and I agreed to that. They aren't part of the stable WPILib distribution that I have so I copied them from their PR. They would be removed from this example directory when they are available to import from their final resting place.

I would appreciate someone running spotless again to see if I caught all of its requirements. It appears the instructions I got are for Linux which I do not have. I installed spotless on my VSCode Windows PC and it gave different messages than the one from WPILib usage. It wanted all of the m_ name prefixes removed.

WispySparks commented 1 month ago

Illinar suggested using them and I agreed to that. They aren't part of the stable WPILib distribution that I have so I copied them from their PR. They would be removed from this example directory when they are available to import from their final resting place.

They've since been merged, you should merge main and delete the local versions.

tom131313 commented 2 weeks ago

I've taken this program with a few examples of using command-based as far as I can - it accomplishes all I intended. I can't go any further without WPILib reviewer input. If these examples are of value to WPILib, I'd be happy to recode whatever needs to make it a real WPILib example. The name of the folder should be changed at least to remove the "word" led from it as that is misleading and off-putting. I used LEDs in place of the PWM motor controllers or other devices merely to show actions of the examples.

If these examples aren't what is wanted, I'll close the PR and delete the repository from my account.