-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
base: master
Are you sure you want to change the base?
Conversation
Atualizações 04/04
Criação firmware V4 com base no V3 + redução.
Adicionado razão I:E e calculo de frequencia
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 @@ | |||
|
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prints de debug.
//lcd.print("1:"); //entrada relação invies medico | ||
//lcd.setCursor(5, 1); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Queremos tirar o whatsapp?
No description provided.