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

Merge to master - USP/3D Tech #9

Open
wants to merge 82 commits into
base: master
Choose a base branch
from

Conversation

HPparanhos
Copy link

No description provided.

Copy link
Collaborator

@breno-helf breno-helf left a comment

Choose a reason for hiding this comment

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

Ufa! Olhei todos os 96 arquivos. Primeiramente muito obrigado por uma contribuição tão grande! Acho que o ideal para essa contribuição seria quebrar em pequenas contribuições que são fáceis de revisar. Deixei comentários mais específicos em cada linha mas de maneira geral o que eu acho que precisa para essa contribuição é o seguinte (PR significa Pull-Request):

  • Tem vários arquivos binários nesse PR, não sei se isso é intencional (por exemplo arquivos do solid works). Acredito que arquivos que demorem para ser gerados, talvez seja uma boa, mas por via de regra a gente nunca quer incluir arquivos "Gerados" no repositório. Isso é simples de arrumar se foi não intencional, só remover os arquivos.

  • Tem vários prints de debug nos códigos dos firmwares, acredito que não seja ideal incluir prints de debug na master pois ser majotariamente noise e pode atrapalhar a leitura do código. Entendo que esse projeto é Emergencial e as coisas tem que andar rápido, então pode acabar passando um ou outro, mas acho que tirar eles é prática boa.

  • Tem algumas partes do firmware que acho que poderiam ser extraídas para um outro arquivo (header + implementação) onde podemos incluir eles em diferentes arquivos (Temos várias partes de código repetidas nos diferentes firmwares).

  • Para PRs grandes assim acho que vale a pena tentar quebrar em PRs menores. Podemos fazer uma branch para cada alteração (A exemplo, poderíamos criar uma branch firmware onde incluiríamos todas as mudanças de firmware), isso ajudaria muito na revisão e as mudanças poderiam ser incluídas de forma mais ágil.

  • Esse Pull Request está tentando atualizar a parte "velha" da master, o que quer dizer que ele tem conflitos com mudanças mudanças feitas antes de você submeter esse PR. Você teria que fazer um "rebase" para atualizar o pull request. Para entender mais sobre rebase recomendo esse link: https://blog.algolia.com/master-git-rebase/ . Me disponibilizo a explicar isso também.

  • Para futuros pull requests podemos explicar as mudanças no título e corpo dele, posso ajudar nisso também.

Enfim, é isso por agora. Desculpa a série de comentários, mas dado o tamanho do Pull request, tinha que comentar bastante também. Mais uma vez Obrigado @HPparanhos !

@@ -0,0 +1,3 @@
# These are supported funding model platforms

custom: https://www.vakinha.com.br/vaquinha/acelerar-o-desenvolvimento-do-respirador-mecanico-opensource
Copy link
Collaborator

Choose a reason for hiding this comment

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

Queremos isso no repositório da Poli também? @OttoHeringer Acho que pode dar um insight aqui.

@@ -0,0 +1 @@

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 esse arquivo é um typo, mas não tenho certeza.

@@ -0,0 +1,266 @@
ISO-10303-21;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Além desse aquivo a gente tem um binário de mesmo nome (3D model/Cilindro guia/Alongador_guia.SLDPRT). Acredito que não queremos incluir binários no repositório, mas não tenho certeza dado o contexto. @OttoHeringer Queremos incluir um arquivo com extensão .SLDPRT?

@@ -0,0 +1,231 @@
ISO-10303-21;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mesma coisa, temos um arquivo binário de mesmo nome, não tenho certeza se devemos incluir ele.



int velocidade() {
//Serial.println("Entrou velocidade");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Acredito que podemos tirar os prints de debugs para o submissão na master. Podemos deixar ele como um comentário se achar necessário.

// Última atualização : 24/03/2020
//
// O programa em questão deve permitir que um usuário consiga controlar um motor de passo, sua velocidade e o avanço,
// para isso ele utiliza duas entradas variáveis (potenciometros), e converte sua leitura dentro de um range estipulado.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Outra coisa que não entendi, mas isso é mais para eu ganhar contexto, por que esse firmware é tão parecido (se não igual) ao OpenLung-firmware/Firmware de controle do equipamento/OpenLung-firmwarev2.ino ? São versões que foram evoluindo? Eles tem várias partes repetidas que a gente poderia abstrair para outro arquivo.

lcd.setCursor(9, 0);
lcd.print("ml:"); //volume medido por ciclo
lcd.setCursor(12, 0);
//lcd.print(analogRead(volMed)); //entrada volume sensor
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prints de debug.

Comment on lines +151 to +152
//lcd.print("1:"); //entrada relação invies medico
//lcd.setCursor(5, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prints de debug.

@@ -1,74 +1,204 @@
# CITI-OpenLung
Copy link
Collaborator

Choose a reason for hiding this comment

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

Obrigado por traduzir!


wpp: +55 11 994840571
Copy link
Collaborator

Choose a reason for hiding this comment

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

Queremos tirar o whatsapp?

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.

5 participants