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

Migra o template para CPP #46

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open

Migra o template para CPP #46

wants to merge 14 commits into from

Conversation

Eduardo-Barreto
Copy link
Member

@Eduardo-Barreto Eduardo-Barreto commented Oct 21, 2023

Com o recente movimento de migração dos códigos da equipe para C++ e a criação das libs, acho que faz total sentido migrarmos esse template aqui também, tanto para facilitar a criação de novos projetos quanto para os testes da lib mesmo

O que foi feito?

  • Mudanças no CMakeLists.txt pra compilar em C++ (bem trivial)
  • Migração do MCU, main e testes para C++
  • Adição do Uncrustify como formatador padrão do vscode (não diretamente do escopo da PR, mas vi e não entendi pq nao tava antes)

@Eduardo-Barreto
Copy link
Member Author

Ah, só uma coisa, existe uma branch antiga de migração, mas fui orientado na gaiola de simplesmente abrir outra (desculpa xin)

Copy link

@n-honda2 n-honda2 left a comment

Choose a reason for hiding this comment

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

Acho q tá td check, vc falou q compilou certinho então sucesso

Copy link

@Bruno1406 Bruno1406 left a comment

Choose a reason for hiding this comment

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

Para além dos comentários q eu fiz, fiquei preocupado se nn faltou isso aqui no cmake
Screenshot from 2023-10-22 00-13-27
Nn manjo nada dessas opções de linker nn, mas foi só com elas que eu consegui buildar a lib de IMU.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
inc/mcu.hpp Show resolved Hide resolved
inc/mcu.hpp Outdated

namespace hal
{
class mcu

Choose a reason for hiding this comment

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

Isso aqui é só um comentário. Nn sei se vejo muito sentido nisso aqui ser uma classe nn. Se a ideia for encapsulamento, "mcu" poderia ser um namespace. Por outro lado, eu entendo que está seguindo um padrão e piriri pororó.

Copy link
Member Author

Choose a reason for hiding this comment

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

po foi 100% pelo padrão

a ideia era manter como nas futuras libs, mcu ser uma classe no namespace hal

src/mcu.cpp Show resolved Hide resolved
src/mcu.cpp Outdated Show resolved Hide resolved
src/mcu.cpp Outdated Show resolved Hide resolved
src/mcu.cpp Outdated Show resolved Hide resolved
src/mcu.cpp Show resolved Hide resolved
Eduardo-Barreto and others added 4 commits October 22, 2023 16:22
Co-authored-by: Bruno1406 <bruno.lossmachado@gmail.com>
Co-authored-by: Bruno1406 <bruno.lossmachado@gmail.com>
@Eduardo-Barreto Eduardo-Barreto requested review from Bruno1406 and a team October 22, 2023 20:02
@Eduardo-Barreto Eduardo-Barreto self-assigned this Oct 22, 2023
Copy link

@Erick-DAS Erick-DAS left a comment

Choose a reason for hiding this comment

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

Só deixei uma pergunta sobre padrões ali no CMake, mas fora isso parece tudo check

CMakeLists.txt Outdated Show resolved Hide resolved
*****************************************/

extern "C"
{
Copy link
Member

Choose a reason for hiding this comment

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

O uncrustify deixa essa chave aqui mesmo? Estranho ahushuashuashu

Copy link
Member Author

Choose a reason for hiding this comment

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

aisduhas de fato estranho, mas o make format nao mudou isso aí

void SystemClock_Config(void);
}

namespace hal {
Copy link
Member

Choose a reason for hiding this comment

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

Então, não sei sobre deixar esse namespace hal aqui, acho que pode deixar mais simples pro template. Não é porque existem namespaces que a gente tem que colocar vários, tem que saber dosar. Mas assim batata também.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, na minha cabeça fazia sentido (principalmente considerando as libs), só botei pq nao vi pq nao botar sinceramente

Copy link
Collaborator

Choose a reason for hiding this comment

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

Acho que eu gosto de um namespace hal, ele ajuda a segmentar o que é baixo do que é alto. Se algo tem escrito hal:: antes, a gente sabe que é uma abstração de algo bem baixo nível, coisa de função da ST.

}

namespace hal {
class mcu {
Copy link
Member

Choose a reason for hiding this comment

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

Tenho alguns mixed feeling sobre deixar isso como classe no template. Tipo não é porque suporta classe que a gente tem que usar classe em tudo. O sleep e o init até fazem sentindo estarem numa classe assim, tanto que eles já tinham o prefixo do mcu_, mas aí o led toggle deixa meio estranho. Como não tem muito também onde colocar, batata também ahushuauahas.

Esse e o comentário do namespace são meio batatas, é só pra pensarem quando tiverem as coisas de usar com moderação as features da linguagem.

Copy link
Member Author

Choose a reason for hiding this comment

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

o sleep e o init foram por esse motivo q vc citou, o led_toggle eu imaginei que se referia ao led da placa asudhasiudh no caso da bluepill é o C13, mas no código estar o A5 deixa mais esquisito mesmo asduihasiu

Copy link
Collaborator

Choose a reason for hiding this comment

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

Também tenho mixed feelings mas acho que tem que ficar como classe sim. Na minha opinião, numa alternativa sem classe, ficaria meeeio estranho de acessar, ficaria algo do tipo hal::sleep(50), daí ainda mais com RTOS, é um sleep do HAL? É um Sleep do RTOS? Sei lá asdhuadhuasd acho que hal::mcu::sleep(50) é mais bonito. Posteriormente poderíamos ter um hal::rtos::sleep(50) ou sei lá hal::rtos::publish(mensagem), enfim, daí vai, mas é tudo hal né

inc/mcu.hpp Outdated Show resolved Hide resolved
inc/mcu.hpp Outdated Show resolved Hide resolved
Eduardo-Barreto and others added 3 commits October 30, 2023 20:07
Co-authored-by: Lucas Haug <39196309+LucasHaug@users.noreply.github.com>
Co-authored-by: Lucas Haug <39196309+LucasHaug@users.noreply.github.com>
Copy link
Collaborator

@pedroazeredo04 pedroazeredo04 left a comment

Choose a reason for hiding this comment

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

Cara eu boto fé, achei bem legal do jeito que tá, com namespace pras coisinhas do HAL e tals.

Idealmente, essa abstração das funções do HAL viriam de uma lib?

void SystemClock_Config(void);
}

namespace hal {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Acho que eu gosto de um namespace hal, ele ajuda a segmentar o que é baixo do que é alto. Se algo tem escrito hal:: antes, a gente sabe que é uma abstração de algo bem baixo nível, coisa de função da ST.

}

namespace hal {
class mcu {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Também tenho mixed feelings mas acho que tem que ficar como classe sim. Na minha opinião, numa alternativa sem classe, ficaria meeeio estranho de acessar, ficaria algo do tipo hal::sleep(50), daí ainda mais com RTOS, é um sleep do HAL? É um Sleep do RTOS? Sei lá asdhuadhuasd acho que hal::mcu::sleep(50) é mais bonito. Posteriormente poderíamos ter um hal::rtos::sleep(50) ou sei lá hal::rtos::publish(mensagem), enfim, daí vai, mas é tudo hal né

@Eduardo-Barreto
Copy link
Member Author

@pedroazeredo04: Idealmente, essa abstração das funções do HAL viriam de uma lib?

não sei se entendi o que vc quis dizer, é sobre as HAL_* dentro das funções no mcu? se sim, fiquei meio confuso de como fazer isso sem obrigar quem for usar o template de usar nossas libs (tudo bem que seria só mudar esse arquivo mas sei la asudhausdh) e coloquei direto as funções do hal, mas é mutável pra deixar com a lib se for o caso, como o led_toggle usar a gpio, apesar de ficar meio alubaubdsalub sobre isso kkkkkkkk na minha cabeça tudo bem a galera que ta no namespace hal usar o hal da st mas entendo o ponto

@Eduardo-Barreto
Copy link
Member Author

Revendo aqui, removi o led_toggle do mcu pq realmente não faz muito sentido auihashdauis mas acho que de resto ta tudo check

@Eduardo-Barreto Eduardo-Barreto requested a review from a team February 23, 2024 13:28
@LucasHaug
Copy link
Member

LucasHaug commented Feb 23, 2024

Não revisei de novo agora, mas antes de dar merge de qualquer coisa, acho que seria bom fazer umas releases e atualizações. Primeiro diria pra finalizar a PR #37 e aí fazer uma release da main com a versão final do projeto com Makefile e depois fazer merge da main na develop com as coisas novas, aí diria pra fazer merge da develop na main com as coisas de CMakeLists e aí fazer outra release. Aí acho que dá pra ver de mergear essa PR na develop, só pra gente ter os estágios das coisas diferentes salvos em releases e as coisas não se misturarem de uma vez e a PR da develop pra main virar um monstro.

@Eduardo-Barreto
Copy link
Member Author

Top top @LucasHaug, a ideia era mergear as coisas atuais pra develop e depois mandar pra main mesmo, mas não tinha pensado no release, vou fazer aqui

sobre essa outra PR, ia comentar AGORA aqui na gaiola pra fechar ela, inclusive vi esse seu comentário aqui pq vim de lá kkkkk

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.

None yet

6 participants