Skip to content

Merge to master - USP/3D Tech#9

Open
HPparanhos wants to merge 82 commits intoInspire-Poli-USP:masterfrom
HPparanhos:master
Open

Merge to master - USP/3D Tech#9
HPparanhos wants to merge 82 commits intoInspire-Poli-USP:masterfrom
HPparanhos:master

Conversation

@HPparanhos
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Obrigado por traduzir!

README.md Outdated
- Tere.Red
- Ocotea Filmes

wpp: +55 11 994840571
Copy link
Copy Markdown
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