use astyle for cformatting

[BBBmau]

Fixes #1919

Needs more cleanup, will work more on this tomorrow.

[BBBmau]

I had spent more time on this and clang-format really isn't thorough as I thought it would be for custom styling.

This is what I was able to bring it down to when focusing on just board_v1.h

board/jungle/boards/board_v1.h:36:42: error: code should be clang-formatted [-Wclang-format-violations]
      print("Invalid CAN transciever ("); puth(transciever); print("): enabling failed\n");
                                         ^
board/jungle/boards/board_v1.h:36:61: error: code should be clang-formatted [-Wclang-format-violations]
      print("Invalid CAN transciever ("); puth(transciever); print("): enabling failed\n");
                                                            ^
board/jungle/boards/board_v1.h:41:19: error: code should be clang-formatted [-Wclang-format-violations]
void testingClang(   uint8_t test) {
                  ^
board/jungle/boards/board_v1.h:41:37: error: code should be clang-formatted [-Wclang-format-violations]
void testingClang(   uint8_t test) {
                                    ^
board/jungle/boards/board_v1.h:74:52: error: code should be clang-formatted [-Wclang-format-violations]
      print("Tried to set unsupported CAN mode: "); puth(mode); print("\n");
                                                   ^
board/jungle/boards/board_v1.h:74:64: error: code should be clang-formatted [-Wclang-format-violations]
      print("Tried to set unsupported CAN mode: "); puth(mode); print("\n");
                                                               ^
board/jungle/boards/board_v1.h:103:66: error: code should be clang-formatted [-Wclang-format-violations]
      print("Tried to set an unsupported harness orientation: "); puth(orientation); print("\n");
                                                                 ^
board/jungle/boards/board_v1.h:103:85: error: code should be clang-formatted [-Wclang-format-violations]
      print("Tried to set an unsupported harness orientation: "); puth(orientation); print("\n");
                                                                                    ^
board/jungle/boards/board_v1.h:129:19: error: code should be clang-formatted [-Wclang-format-violations]
  UNUSED(channel); UNUSED(sbu);
                  ^
board/jungle/boards/board_v1.h:143:6: error: code should be clang-formatted [-Wclang-format-violations]
  for(uint8_t i = 1; i <= 4; i++) {
     ^
board/jungle/boards/board_v1.h:162:27: error: code should be clang-formatted [-Wclang-format-violations]
void board_v1_tick(void) {}

Their aren't options available to cover the rest of these errors that appear when setting everything to default. I've gone through the docs for styling a good amount and I'm surprised by the fact that there's SpaceBeforeParens but nothing for SpaceAfterParens.

Would like to know where we go from here.

[adeebshihadeh]

So we just want very basic style checks, like the 2 space indent. I'm open to other tools for this.

[BBBmau]

This could potentially be a good alternative. I'll look more into it later.

https://github.com/uncrustify/uncrustify

[BBBmau]

uncrustify looks to be a lot better for dealing with simple checks. This is what I'm using for the 2 space indent in the config file. Tested with safety_mazda.h with no issues:

indent_columns                  = 2
indent_with_tabs                = 0

uncrustify also has a nice feature where it can detect and give you the config of a file if you want all files to match the style of a specific file done. This is done by uncrustify --detect -f board/safety/safety_mazda.h

This is all good and all but from the original issue set the idea is to have this be a pre-commit hook right?

[adeebshihadeh]

Yes, and we only want it to check, not fix.

[BBBmau]

last step is to get it running in a hook. the check can be done by just adding a --check flag. this shows what is outputted on a pass/fail:

┌─(~/Dev/panda-1)───────────────────────────────────────────────────────────────────────────────────────────────────────────────(mau@mau-JKDT676NCP:s041)─┐
└─(11:53:15 on setup-clang-format ✹ ✭)──> uncrustify -c test.cfg -f board/safety/safety_mazda.h --check                                     ──(Fri,Apr05)─┘
do_source_file: Parsing: board/safety/safety_mazda.h as language C-Header
FAIL: board/safety/safety_mazda.h (File size changed from 3635 to 3622)
┌─(~/Dev/panda-1)───────────────────────────────────────────────────────────────────────────────────────────────────────────────(mau@mau-JKDT676NCP:s041)─┐
└─(12:02:54 on setup-clang-format ✹ ✭)──> uncrustify -c test.cfg -f board/safety/safety_mazda.h --check                                 1 ↵ ──(Fri,Apr05)─┘
do_source_file: Parsing: board/safety/safety_mazda.h as language C-Header
PASS: board/safety/safety_mazda.h (3622 bytes)

[BBBmau]

Wasn't happy with the output since this would fail at the first file and also not show how to fix it.

Found a user that worked on a more elaborate uncrustify hooks implementation that shows what to fix to follow style. This is done on staged changes only:

┌─(~/Dev/panda-1)────────────────────────────────────────────────────────────────────────────────────────(mau@mau-JKDT676NCP:s041)─┐
└─(14:41:24 on setup-clang-format ✹)──> git commit -m "show all hooks"                                           1 ↵ ──(Fri,Apr05)─┘
Running hook: pre-commit-uncrustify
do_source_file: Parsing: board/safety/safety_mazda.h as language C-Header

The following differences were found between the code to commit and the uncrustify rules:

--- "a/board/safety/safety_mazda.h"     2024-04-05 14:41:24
+++ "b/board/safety/safety_mazda.h"     2024-04-05 14:41:38
@@ -14,9 +14,9 @@

 const SteeringLimits MAZDA_STEERING_LIMITS = {
   .max_steer = 800,
-.max_rate_up = 10,
-.max_rate_down = 25,
-.max_rt_delta = 300,
+  .max_rate_up = 10,
+  .max_rate_down = 25,
+  .max_rt_delta = 300,
   .max_rt_interval = 250000,
   .driver_torque_factor = 1,
   .driver_torque_allowance = 15,

You can apply these changes with:
 git apply /tmp/pre-commit-uncrustify-2024-04-05_14h41m38s.patch
(may need to be called from the root directory of your repository)
Aborting commit. Apply changes and commit again or skip checking with --no-verify (not recommended).

The thing to note is that this will force an abort on the commit, resulting in all the other pre-hooks set to not run. Once the changes have been made it will go through the rest of the hooks normally.

┌─(~/Dev/panda-1)────────────────────────────────────────────────────────────────────────────────────────(mau@mau-JKDT676NCP:s041)─┐
└─(14:42:04 on setup-clang-format ✹)──> git commit -m "show all hooks"                                               ──(Fri,Apr05)─┘
Running hook: pre-commit-uncrustify
do_source_file: Parsing: board/safety/safety_mazda.h as language C-Header
Files in this commit comply with the uncrustify rules.
check python ast.....................................(no files to check)Skipped
check yaml...........................................(no files to check)Skipped
check for merge conflicts................................................Passed
check for broken symlinks............................(no files to check)Skipped
check that executables have shebangs.................(no files to check)Skipped
check that scripts with shebangs are executable..........................Passed
mypy.................................................(no files to check)Skipped
ruff.................................................(no files to check)Skipped
[setup-clang-format f5ccace3] show all hooks
 1 file changed, 3 insertions(+), 3 deletions(-)

[adeebshihadeh]

https://github.com/pocc/pre-commit-hooks?tab=readme-ov-file#example-usage

[BBBmau]

https://github.com/pocc/pre-commit-hooks?tab=readme-ov-file#example-usage

awesome find, I didn't come across this when first researching uncrustify hooks. How it looks now after a pre-commit run when style doesn't match:

└─(15:10:05 on setup-clang-format ✚)──> pre-commit run                                                               ──(Fri,Apr05)─┘
[WARNING] The 'rev' field of repo 'https://github.com/pocc/pre-commit-hooks' appears to be a mutable reference (moving tag / branch).  Mutable references are never updated after first install and are not supported.  See https://pre-commit.com/#using-the-latest-version-for-a-repository for more details.  Hint: `pre-commit autoupdate` often fixes this.
check python ast.....................................(no files to check)Skipped
check yaml...............................................................Passed
check for merge conflicts................................................Passed
check for broken symlinks............................(no files to check)Skipped
check that executables have shebangs.................(no files to check)Skipped
check that scripts with shebangs are executable..........................Passed
mypy.................................................(no files to check)Skipped
ruff.................................................(no files to check)Skipped
uncrustify...............................................................Failed
- hook id: uncrustify
- exit code: 1

board/safety/safety_mazda.h
====================
--- original

+++ formatted

@@ -17,7 +17,7 @@

   .max_rate_up = 10,
   .max_rate_down = 25,
   .max_rt_delta = 300,
-.max_rt_interval = 250000,
+  .max_rt_interval = 250000,
   .driver_torque_factor = 1,
   .driver_torque_allowance = 15,
   .type = TorqueDriverLimited,

[BBBmau]

from latest commit:

┌─(~/Dev/panda-1)────────────────────────────────────────────────────────────────────────────────────────(mau@mau-JKDT676NCP:s041)─┐
└─(15:12:33 on setup-clang-format ✚)──> git commit -m "add uncrustify into pre-commit-config.yaml file"          1 ↵ ──(Fri,Apr05)─┘
Running hook: pre-commit-uncrustify
Files in this commit comply with the uncrustify rules.
g[WARNING] The 'rev' field of repo 'https://github.com/pocc/pre-commit-hooks' appears to be a mutable reference (moving tag / branch).  Mutable references are never updated after first install and are not supported.  See https://pre-commit.com/#using-the-latest-version-for-a-repository for more details.  Hint: `pre-commit autoupdate` often fixes this.
pcheck python ast.....................................(no files to check)Skipped
check yaml...............................................................uPassed
check for merge conflicts................................................Passed
check for broken symlinks............................(no files to check)Skipped
check that executables have shebangs.................(no files to check)Skipped
check that scripts with shebangs are executable..........................sPassed
mypy.................................................(no files to check)Skipped
ruff.................................................(no files to check)Skipped
uncrustify...........................................(no files to check)Skipped
[setup-clang-format 53c217ec] add uncrustify into pre-commit-config.yaml file
 1 file changed, 7 insertions(+)

[BBBmau]

it's failing because of this non-zero default values: https://github.com/uncrustify/uncrustify/blob/787adc57ad95b3961581f2287dcbdff7058b746d/src/options.cpp#L2176-L2220

I'll set them to zero/false and give it another run.

[BBBmau]

I've tried different configurations but run into a blocker with &. specifically:

Before uncrustify

  ((((TypeExtField) << MPU_RASR_TEX_Pos) & MPU_RASR_TEX_Msk)                  | \
   (((IsShareable)  << MPU_RASR_S_Pos)   & MPU_RASR_S_Msk)                    | \
   (((IsCacheable)  << MPU_RASR_C_Pos)   & MPU_RASR_C_Msk)                    | \
   (((IsBufferable) << MPU_RASR_B_Pos)   & MPU_RASR_B_Msk))

After Uncrustify

        ((((TypeExtField) << MPU_RASR_TEX_Pos)& MPU_RASR_TEX_Msk)                  | \
  (((IsShareable)  << MPU_RASR_S_Pos)& MPU_RASR_S_Msk)                    | \
  (((IsCacheable)  << MPU_RASR_C_Pos)& MPU_RASR_C_Msk)                    | \
  (((IsBufferable) << MPU_RASR_B_Pos)& MPU_RASR_B_Msk))

The removing of spaces around & is something I have been unable to solve. The 2 space indents are working however.

Where do we go from here? I've looked through the docs a bit and unsure of what else can be done with the current styling in the codebase.

It's worth knowing that the tests in GHA will fail no matter what since it's detecting areas in the code that are not two space indents. Something to consider with whether or not we are wanting this merged or not.

[BBBmau]

This is odd since I don't get anything like this when running locally. I'll look more into this.

precommit_format.cfg:3: unknown symbol 'pp_space_after'
precommit_format.cfg:5: unknown symbol 'align_nl_cont_spaces'
Option<BOOL>: at precommit_format.cfg:7: Expected one of 'true', 'false'for 'indent_shift'; got '1'
precommit_format.cfg:11: unknown symbol 'sp_between_ptr_ref'
Option<IARF>: at precommit_format.cfg:12: Expected one of 'ignore', 'add', 'remove', 'force'for 'sp_before_byref'; got 'not_defined'
Option<IARF>: at precommit_format.cfg:13: Expected one of 'ignore', 'add', 'remove', 'force'for 'sp_after_byref'; got 'not_defined'
precommit_format.cfg:15: unknown symbol 'sp_byref_paren'

Problem with uncrustify: Unexpected Stderr/return code received when analyzing board/drivers/registers.h.
Args: ['uncrustify', '-c', 'precommit_format.cfg', '-q', '-f', 'board/drivers/registers.h']

[BBBmau]

precommit_format.cfg:3: unknown symbol 'pp_space_after'
precommit_format.cfg:5: unknown symbol 'align_nl_cont_spaces'
Option<BOOL>: at precommit_format.cfg:7: Expected one of 'true', 'false'for 'indent_shift'; got '1'
precommit_format.cfg:11: unknown symbol 'sp_between_ptr_ref'
Option<IARF>: at precommit_format.cfg:12: Expected one of 'ignore', 'add', 'remove', 'force'for 'sp_before_byref'; got 'not_defined'
Option<IARF>: at precommit_format.cfg:13: Expected one of 'ignore', 'add', 'remove', 'force'for 'sp_after_byref'; got 'not_defined'
precommit_format.cfg:15: unknown symbol 'sp_byref_paren'

Problem with uncrustify: Unexpected Stderr/return code received when analyzing board/drivers/registers.h.
Args: ['uncrustify', '-c', 'precommit_format.cfg', '-q', '-f', 'board/drivers/registers.h']

These errors come from using an old version of uncrustify that's used for the precommit hooks. I updated the .cfg to match the version, checks are still the same.

[BBBmau]

It seems like it might be easier to just create a super barebones uncrustify just for comma use to make this possible. I'm still surprised that something as simple as just checking for 2 space indentation is such a pain for things like clang and uncrustify. I'll get back into this one soon.

[BBBmau]

went back to looking for solutions and stumbled across astyle formatting. I tried the cli and it was super simple and is exactly what we needed. I just needed to make it into a pre-commit-hook that we can use. Their was none that had been made from what I saw. The shell script is a modified gist that added support for pre-commits but was very old and needed updating. The original can be found here

The repo where i implemented the pre-commit-hook is here, the code itself isnt too crazy: https://github.com/BBBmau/astyle-pre-commit

this came from committing the change on body.h when the format was found to be wrong and after the changes were applied:

(base) ┌─(~/Dev/panda)─────────────────────────────────────────────────────────────────────────────────────────────────────────────────(mau@mau-JKDT676NCP:s076)─┐
└─(18:50:48 on setup-clang-format ✚)──> pre-commit run                                                                                      ──(Wed,May15)─┘
[INFO] Initializing environment for https://github.com/BBBmau/astyle-pre-commit.
check python ast.....................................(no files to check)Skipped
check yaml...............................................................Passed
check for merge conflicts................................................Passed
check for broken symlinks............................(no files to check)Skipped
check that executables have shebangs.................(no files to check)Skipped
check that scripts with shebangs are executable..........................Passed
mypy.................................................(no files to check)Skipped
ruff.................................................(no files to check)Skipped
astyle...................................................................Failed
- hook id: astyle
- exit code: 1
- files were modified by this hook

[!] board/safety/safety_body.h does not respect the agreed coding standards.

(base) ┌─(~/Dev/panda)─────────────────────────────────────────────────────────────────────────────────────────────────────────────────(mau@mau-JKDT676NCP:s076)─┐
└─(18:53:59 on setup-clang-format ✹ ✚)──> pre-commit run                                                                                1 ↵ ──(Wed,May15)─┘
check python ast.....................................(no files to check)Skipped
check yaml...............................................................Passed
check for merge conflicts................................................Passed
check for broken symlinks............................(no files to check)Skipped
check that executables have shebangs.................(no files to check)Skipped
check that scripts with shebangs are executable..........................Passed
mypy.................................................(no files to check)Skipped
ruff.................................................(no files to check)Skipped
astyle...................................................................Passed

[BBBmau]

missing a commit, one sec

[BBBmau]

i stumbled upon this which we could use, it supports a bit more but for either one is fine since this is for a simple 2 space indent check. The choice is yours

https://github.com/igrr/astyle_py

[BBBmau]

@adeebshihadeh let me know your thoughts

[adeebshihadeh]

I don't really have time to investigate these alternative solutions, so I'm gonna just take down that bounty.

If you want to keep looking, we'll still pay out the bounty if you get something nice merged. We're looking to enforce very light style guidelines (two spaces, whitespace for stuff like if (...) {, etc.).

[BBBmau]

the PR would be covering the two space indentation style requirement since that's what's stated in the original issue. If whitespace on conditional statements to make them look like if (...) {, switch (...) { is the second requirement then it should be a simple add with astyle. the argument for checkking two space indentation is -s2 so i can imagine something similar is there for whitespaces, I'll look into it later.

through using all three astyle is by far the easiest and most straightforward formatter to use when it comes to super simple stuff. I'll reopen this when i can get whitespaces added.

[BBBmau]

I don't really have time to investigate these alternative solutions, so I'm gonna just take down that bounty.

If you want to keep looking, we'll still pay out the bounty if you get something nice merged. We're looking to enforce very light style guidelines (two spaces, whitespace for stuff like if (...) {, etc.).

closest I could do to get the desired format with astyle was with: args: ["-s2", "-H", "-U"] but doesn't catch this: if (bus == 0) {

I'll leave this for now and maybe look into the astyle code to see if this could be supported.