Skip to content

[CI] Add uncrustify script and action #1532

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
Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open

[CI] Add uncrustify script and action #1532

wants to merge 37 commits into from

Conversation

jgromes
Copy link
Owner

@jgromes jgromes commented Jun 21, 2025 •
edited
Loading

This PR adds script to perform uncrustify formatting check and fix. Result of the script is a .patch file which can be applied to fix the issues.

A GitHub action is also added to trigger this on every push to master and every PR.

@jgromes jgromes self-assigned this Jun 21, 2025
@jgromes jgromes added the enhancement New feature or request label Jun 21, 2025
@jgromes jgromes mentioned this pull request Jun 21, 2025
Closed
Dazza0
Dazza0 reviewed Jul 7, 2025
View reviewed changes
jgromes added 24 commits July 7, 2025 20:51
@jgromes
[LR11x0] Chore: fixup firmware images
e975b2a
@jgromes
[CI] Force uncrustify 0.78.1
d2c876a
@jgromes
[HAL] Chore: uncrustify formatting
c14ef00
@jgromes
[MOD] Chore: uncrustify formatting
203ff5f
@jgromes
[CC1101] Chore: uncrustify formatting
09792bf
@jgromes
[LLCC68] Chore: uncrustify formatting
393fd8b
@jgromes
[LR11x0] Chore: uncrustify formatting
e1c7f51
@jgromes
[nRF24] Chore: uncrustify formatting
40e4de8
@jgromes
[RF69] Chore: uncrustify formatting
70137f8
@jgromes
[Si443x] Chore: uncrustify formatting
3f3f794
@jgromes
[SX123x] Chore: uncrustify formatting
dbe87cc
@jgromes
[SX126x] Chore: uncrustify formatting
5cad16a
@jgromes
[SX127x] Chore: uncrustify formatting
f96e6b3
@jgromes
[SX128x] Chore: uncrustify formatting
fa57770
@jgromes
[AFSK] Chore: uncrustify formatting
c4babb6
@jgromes
[APRS] Chore: uncrustify formatting
d471f76
@jgromes
[AX25] Chore: uncrustify formatting
c927127
@jgromes
[Bell] Chore: uncrustify formatting
e42cb80
@jgromes
[EXT] Chore: uncrustify formatting
2dddf71
@jgromes
[FSK4] Chore: uncrustify formatting
2765e1f
@jgromes
[Hell] Chore: uncrustify formatting
53f438b
@jgromes
[LoRaWAN] Chore: uncrustify formatting
e0c10a4
@jgromes
[Morse] Chore: uncrustify formatting
08f2807
@jgromes
[Pager] Chore: uncrustify formatting
9d51410
@jgromes
Copy link
Owner Author

jgromes commented Jul 19, 2025

So after some trial and error I managed to get an uncrustify configuration that somewhat matches the existing code style. It did find some inconsistencies, whitespaces at line ends etc., but overall I'm still not completely sure about this. I see the following problems:

  1. It will require contributors to run the check, and do it with a specific uncrustify version. On Linux that's reasonably easy with the script in extras/uncrustify/uncrustify.sh, but I don't know how many contributors here use other OSes. Also, a lot of people contribute very simple fixes (simple meaning 5 changed lines or less) - especially when getting started. This automated check makes it somewhat harder and increases the barrier to entry.
  2. The script does produce a diff file that can be applied with git diff, so technically it's not necessary to run uncrustify locally, however, it requires going to the build artifacts, downloading the diff file from there and applying it manually. Again though, that requires a bit more knowledge, so it has the potential of turning new contributors away.

What do you think @StevenCellist?

@StevenCellist
Copy link
Collaborator

I'd very, very much dislike having to do this. I'm a Windows user most of the time and it sounds terrible to have to do that every time. Let alone for those users that only want to commit a missing state assert and just learned to file an issue.

My personal preference would be to have it e.g. run as part of the release CI.

@StevenCellist
Copy link
Collaborator

It's killing some alignment choices in places where we added some extra whitespace. Which is a minor inconvenience. And probably have it ignore the firmware images?

@jgromes
Copy link
Owner Author

jgromes commented Jul 21, 2025

My personal preference would be to have it e.g. run as part of the release CI.

That is indeed a solution, we can run this on release (and also occasionaly by manual trigger).

It's killing some alignment choices in places where we added some extra whitespace

Could you point out where exactly? Like I said I wasn't able to fully match the existing style

And probably have it ignore the firmware images?

I don't expect any changes there, so I actually don't really care how the firmware-only headers are formatted. Also there's no readability required there.

StevenCellist
StevenCellist reviewed Jul 21, 2025
View reviewed changes
Copy link
Collaborator

@StevenCellist StevenCellist left a comment

Choose a reason for hiding this comment

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

Marked a couple of yours and one of mine as an example.

src/modules/SX126x/SX126x.cpp
Comment on lines 853 to -856
this->preambleDetLength = maxDetLen >= 32 ? RADIOLIB_SX126X_GFSK_PREAMBLE_DETECT_32 :
maxDetLen >= 24 ? RADIOLIB_SX126X_GFSK_PREAMBLE_DETECT_24 :
maxDetLen >= 16 ? RADIOLIB_SX126X_GFSK_PREAMBLE_DETECT_16 :
maxDetLen > 0 ? RADIOLIB_SX126X_GFSK_PREAMBLE_DETECT_8 :
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here's one

src/modules/SX126x/SX126x_LR_FHSS.cpp
Comment on lines -134 to -137
uint16_t pos = 0;
uint16_t st_idx = 0;
uint16_t st_idx_init = 0;
int16_t bits_left = nb_bits;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another

src/protocols/LoRaWAN/LoRaWAN.cpp
Comment on lines -523 to -531
this->rev = LoRaWANNode::ntoh<uint8_t>(&this->bufferSession[RADIOLIB_LORAWAN_SESSION_VERSION]);
this->lwClass = LoRaWANNode::ntoh<uint8_t>(&this->bufferSession[RADIOLIB_LORAWAN_SESSION_CLASS]);
this->homeNetId = LoRaWANNode::ntoh<uint32_t>(&this->bufferSession[RADIOLIB_LORAWAN_SESSION_HOMENET_ID]);
this->aFCntDown = LoRaWANNode::ntoh<uint32_t>(&this->bufferSession[RADIOLIB_LORAWAN_SESSION_A_FCNT_DOWN]);
this->nFCntDown = LoRaWANNode::ntoh<uint32_t>(&this->bufferSession[RADIOLIB_LORAWAN_SESSION_N_FCNT_DOWN]);
this->confFCntUp = LoRaWANNode::ntoh<uint32_t>(&this->bufferSession[RADIOLIB_LORAWAN_SESSION_CONF_FCNT_UP]);
this->rev = LoRaWANNode::ntoh<uint8_t>(&this->bufferSession[RADIOLIB_LORAWAN_SESSION_VERSION]);
this->lwClass = LoRaWANNode::ntoh<uint8_t>(&this->bufferSession[RADIOLIB_LORAWAN_SESSION_CLASS]);
this->homeNetId = LoRaWANNode::ntoh<uint32_t>(&this->bufferSession[RADIOLIB_LORAWAN_SESSION_HOMENET_ID]);
this->aFCntDown = LoRaWANNode::ntoh<uint32_t>(&this->bufferSession[RADIOLIB_LORAWAN_SESSION_A_FCNT_DOWN]);
this->nFCntDown = LoRaWANNode::ntoh<uint32_t>(&this->bufferSession[RADIOLIB_LORAWAN_SESSION_N_FCNT_DOWN]);
this->confFCntUp = LoRaWANNode::ntoh<uint32_t>(&this->bufferSession[RADIOLIB_LORAWAN_SESSION_CONF_FCNT_UP]);
this->confFCntDown = LoRaWANNode::ntoh<uint32_t>(&this->bufferSession[RADIOLIB_LORAWAN_SESSION_CONF_FCNT_DOWN]);
this->adrFCnt = LoRaWANNode::ntoh<uint32_t>(&this->bufferSession[RADIOLIB_LORAWAN_SESSION_ADR_FCNT]);
this->fCntUp = LoRaWANNode::ntoh<uint32_t>(&this->bufferSession[RADIOLIB_LORAWAN_SESSION_FCNT_UP]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some more

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

Reviewers

@StevenCellist StevenCellist StevenCellist left review comments

+1 more reviewer

@Dazza0 Dazza0 Dazza0 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jgromes jgromes

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jgromes @StevenCellist @pkendall64 @lyusupov @Dazza0