Skip to content

Conversation

Eduardo-Barreto
Copy link
Member

@Eduardo-Barreto Eduardo-Barreto commented Mar 17, 2025

Salve galera! Ja faz um tempo que estamos comentando que o template não refletia tao bem o jeito que faziamos as coisas na equipe

essa pr aqui basicamente junta tudo que havia de ruim e passa o caminhao

Resumidamente:

  • Migra pra C++
  • Migra pra Clang format
  • Adiciona Linter
  • Adiciona Docker
  • Adiciona Doxygen
  • Migra pro CMake do CubeMX ao inves do obko
  • Muda o uC pra o da bluepill
  • Corrige outros erros

@Eduardo-Barreto
Copy link
Member Author

Ou não usar o cmake do cube rsrsrs

morrer

@Eduardo-Barreto Eduardo-Barreto requested a review from a team March 29, 2025 22:27
@Eduardo-Barreto Eduardo-Barreto requested a review from Copilot April 3, 2025 11:16
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors several parts of the project by migrating to C++, integrating Clang Format, adding a linter and Docker configuration, updating CMake configuration, and switching the target microcontroller.

  • Introduces new GitHub Actions workflows for release and CI processes
  • Adds configuration files for Clang-Tidy and Clang-Format to enforce code quality
  • Updates project infrastructure including Docker, Doxygen, and microcontroller settings

Reviewed Changes

Copilot reviewed 30 out of 46 changed files in this pull request and generated 1 comment.

File Description
.github/workflows/release.yaml Adds a new release workflow that builds the project in Docker and attaches binaries to releases
.github/workflows/ci.yaml Introduces a CI workflow for pull requests and pushes with cache, formatting, building, and linting
.clang-tidy Provides a comprehensive linting configuration for C++ code
.clang-format Establishes a consistent C++ formatting style using Clang-Format
Files not reviewed (16)
  • .devcontainer.json: Language not supported
  • .docker/.dockerignore: Language not supported
  • .docker/Dockerfile: Language not supported
  • .docker/scripts/build.sh: Language not supported
  • .docker/scripts/check_format.sh: Language not supported
  • .editorconfig: Language not supported
  • .githooks/pre-commit: Language not supported
  • .gitmodules: Language not supported
  • .vscode/extensions.json: Language not supported
  • .vscode/settings.json: Language not supported
  • .vscode/tasks.json: Language not supported
  • CMakeLists.txt: Language not supported
  • Doxyfile: Language not supported
  • LICENSE: Language not supported
  • PreLoad.cmake: Language not supported
  • cmake/config_validation.cmake: Language not supported

Comment on lines +52 to +58
- name: 🔨 Build project
if: steps.cache.outputs.cache-hit != 'true'
run: docker run --rm project:build /bin/bash /project/.docker/scripts/build.sh

- name: 🚨 Lint project
if: steps.cache.outputs.cache-hit != 'true'
run: docker run --rm project:build /bin/bash /project/.docker/scripts/build.sh -DLINTER_MODE=ON
Copy link
Member

Choose a reason for hiding this comment

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

Não olhei tão a fundo, mas precisa desses dois estágios? O o build já não é feito junto do linting?

Copy link
Member Author

Choose a reason for hiding this comment

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

verdade, acho q eh melhor juntar nesse caso

a única diferença de resultado eh que quando roda com lint não tem binários de saída, mas como o CI não usa eu acho legal, já eh bom que diminui o tempo do check

Copy link
Member Author

Choose a reason for hiding this comment

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

a gnt tbm perde a clareza da informação se falhou na compilação ou no lint, mas acho q tá suave

Copy link
Member

Choose a reason for hiding this comment

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

Pra ambos os casos teria que olhar os logs pra ver o que falhou, então acho que não é tanto um problema. E aí economizar tempo na CI sempre ajuda, ainda mais pra projetos grandes aushhshshshsu

Copy link
Member

Choose a reason for hiding this comment

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

Eu ainda digo que esse if de cache em todos os steps não faz sentido nenhum e tem que ter uma forma melhor

Copy link
Member Author

Choose a reason for hiding this comment

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

Analisando melhor, acho que vale destacar uma troca importante entre clareza e performance aqui.

Atualmente, temos dois estágios separados: um para o build puro e outro para o build com lint. A proposta de juntar os dois ajuda a economizar tempo no CI, mas tem algumas implicações.

Comparativo entre os cenários

Build sem linter (estágios separados):

⏱️ Build puro: ~23s

🧹 Lint (com build): ~3min

🔍 Time-to-fail se o código não compila: 23s

🔍 Time-to-fail se o lint falha: ~3min 23s

Build com linter somente (estágios unificados):

🔄 Tudo junto: ~3min

🔍 Time-to-fail para qualquer falha: sempre até 3min

Ou seja, se o código não compila, o CI gasta 3 minutos para descobrir algo que poderia ser pego em 23 segundos. Isso pode ser relevante principalmente quando estamos em uma iteração rápida ou PRs mais simples.

Além disso, com os estágios separados, a gente ganha um pouco mais de clareza visual no resultado do CI, já que conseguimos ver diretamente se falhou no build ou no lint pelo nome do step (com o emojizinho bonitinho :p). Se unificarmos, isso se perde, aí todo erro vai parecer um erro do "build+lint"

Por outro lado…

É justo dizer que falhas de compilação são menos comuns em branches que sobem PRs, então há um argumento forte pra otimizar pro caso comum (sucesso), e evitar rodar o build puro à toa.

Eu particularmente acho que tanto faz kkkkkkkkkkkkkkkk mas como eu gosto dos emojis tendo a manter como está. De qualquer modo, é uma coisa extremamente simples que pode ser configurada pelo projeto se ele nao gostar do jeito que está

else()
message(FATAL_ERROR "CUBE_PATH not defined")
# Check if host system is Linux
if(NOT CMAKE_HOST_SYSTEM_NAME STREQUAL "Linux")

Choose a reason for hiding this comment

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

Suggested change
if(NOT CMAKE_HOST_SYSTEM_NAME STREQUAL "Linux")
if(NOT CMAKE_HOST_UNIX)

Acho que se deixar assim ele suporta MacOS também.

Comment on lines +28 to +29
message(STATUS "Linux detected")
set(CUBE_DEFAULT_PATH "/usr/local/STMicroelectronics/STM32Cube/STM32CubeMX")

Choose a reason for hiding this comment

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

Suggested change
message(STATUS "Linux detected")
set(CUBE_DEFAULT_PATH "/usr/local/STMicroelectronics/STM32Cube/STM32CubeMX")
if (CMAKE_HOST_APPLE)
message(STATUS "MacOS detected")
set(CUBE_DEFAULT_PATH "/Applications/STMicroelectronics/STM32CubeMX.app")
else()
message(STATUS "Linux detected")
set(CUBE_DEFAULT_PATH "/usr/local/STMicroelectronics/STM32Cube/STM32CubeMX")
endif()

Tem que pedir pra alguém com Mac testar se só isso é suficiente.

Comment on lines +37 to +38
if: steps.cache.outputs.cache-hit != 'true'
uses: docker/build-push-action@v6

Choose a reason for hiding this comment

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

Suggested change
if: steps.cache.outputs.cache-hit != 'true'
uses: docker/build-push-action@v6
if: steps.cache.outputs.cache-hit != 'true'
timeout-minutes: 10
uses: docker/build-push-action@v6

Comment on lines +22 to +23
- name: 🐋 Build Docker image
uses: docker/build-push-action@v6

Choose a reason for hiding this comment

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

Suggested change
- name: 🐋 Build Docker image
uses: docker/build-push-action@v6
- name: 🐋 Build Docker image
timeout-minutes: 10
uses: docker/build-push-action@v6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants