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.

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

Add more driver station feedback and diagnostic logging #25

Merged
merged 5 commits into from
Mar 9, 2024

Conversation

garrettsummerfi3ld
Copy link
Member

@garrettsummerfi3ld garrettsummerfi3ld commented Mar 3, 2024

Added

Added Git warnings to the driver station alerting the drivers that the robot is running uncommitted code.

Added CTRE logging for the CANcoders.

Added writing the logs to a formatted USB device plugged into an open RIO USB port.

Fixed an issue that where if an AdvantageKit logfile was not provided or was null during the robot simulation startup there would be a simulator crash. This is now fixed.

Added Git warnings to the driver station. This will notify the driver if there are currently uncommitted changes on the robot as well as if the robot is currently not on the "main" branch. This will show up as errors as we are treating these warnings as errors.
@garrettsummerfi3ld garrettsummerfi3ld added the enhancement New feature or request label Mar 4, 2024
@garrettsummerfi3ld garrettsummerfi3ld self-assigned this Mar 4, 2024
Added logging for CTRE devices such as CANcoders.

Added a log writer that will log to a USB drive plugged into the RIO.
@garrettsummerfi3ld garrettsummerfi3ld marked this pull request as ready for review March 9, 2024 06:01
"Event" branches are now logged for driver input as well as logging events.
Copy link
Member

@EuphoniumPlayer EuphoniumPlayer left a comment

Choose a reason for hiding this comment

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

Only thing I'd say is that maybe just a warning is okay (for the git warning), as it makes it seem like using a branch other than main and uncommitted changes are not okay, when in reality branches are experimental/work in progress areas, and uncommitted changes deployed to the robot are being tested. Maybe change it so it just works as a heads up with instructions so it doesn't send the wrong idea, especially seeing as our club policy is that changes need to be tested before commits to major branches (especially if it is an adjustment to a major system) and pull request are only to be made when one or more mentors has seen the code work, and merges can only be done by you after review.

@garrettsummerfi3ld
Copy link
Member Author

@EuphoniumPlayer some thoughts from your review. Genuine thoughts.

Only thing I'd say is that maybe just a warning is okay (for the git warning), as it makes it seem like using a branch other than main and uncommitted changes are not okay, when in reality branches are experimental/work in progress areas, and uncommitted changes deployed to the robot are being tested.

My only counterpoint with that is simply any change needs to be documented and logged somewhere not on one machine.

We have had time and time again with changes written to the robot and the changes that are deployed that have no commits attached is astoundingly bad. Code is deployed without really any concern on whether or not those changes are at least saved in a commit for someone to remember to push later. The warning is treated as an error as its critical to have at least some breadcrumbs to retrace steps on what was deployed and what wasn't deployed.

This will not cause the robot to stop working, and there is a change where event branches are considered less so as its for an event and a gentle reminder that after an event merge it into main and push those commits.

Maybe change it so it just works as a heads up with instructions so it doesn't send the wrong idea

The instructions are hard to input as the console is only such a small size on the driver station, but it is there. What other information would be needed to be added to satisfy this requirement?

... our club policy is that changes need to be tested before commits to major branches (especially if it is an adjustment to a major system) and pull request are only to be made when one or more mentors has seen the code work, and merges can only be done by you after review.

We currently are implementing a policy that is very loose, however it should be at least down in writing.

All commits should be done in a branch not on main, no one can push to main, at all. All changes must be pull requested into main for a review of myself or other mentors (I only added myself to the required reviewers as I didn't want to bother other mentors times as they are valuable and mine is not), and once we read that the code is good or is documented, it gets merged. Whether or not it works we have the status checks for if it just passes a basic sanity test. After a review has passed, it is merged into main.

@EuphoniumPlayer
Copy link
Member

@garrettsummerfi3ld I understand the reasoning and counter point, I just though it was a little suggestive that being on a branch that is not main is a bad thing, and that all changes should be committed before testing them, that's all. By instructions I meant what you currently have on there about how to commit and create a pull request

Copy link
Contributor

Choose a reason for hiding this comment

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

@ShaneHopkins11 approves changes.

@garrettsummerfi3ld
Copy link
Member Author

@garrettsummerfi3ld I understand the reasoning and counter point, I just though it was a little suggestive that being on a branch that is not main is a bad thing, and that all changes should be committed before testing them, that's all. By instructions I meant what you currently have on there about how to commit and create a pull request

The only suggestion is main is considered a stable version, where anything else isn't considered stable, and would be unfortunate to not run main or event branches during an actual game.

For uncommitted changes, that is an actual sin I cannot have slide by. Changes that aren't saved and are modified causes headaches for debugging as a commit that has dirty changes may not be the same on other machines and can be easily as cross referenced.

@EuphoniumPlayer
Copy link
Member

@garrettsummerfi3ld Fair enough

Copy link
Member

@EuphoniumPlayer EuphoniumPlayer left a comment

Choose a reason for hiding this comment

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

Fair enough

@garrettsummerfi3ld garrettsummerfi3ld merged commit cd3c54d into main Mar 9, 2024
7 checks passed
@garrettsummerfi3ld garrettsummerfi3ld deleted the feature/driver-station-feedback branch March 9, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants