Merge de cambios realizados #4

royconejo · 2018-04-06T21:18:57Z

Como hablamos en la lista embebidos32, les envio los cambios propuestos y otra serie de cambios menores como consecuencia:

Se incluye codigo funcional de board Edu-CIAA en LPCOpen.
Cambio propuesto de .s a .S en makefile, los fundamentos estan en el issuel makefile y archivos .s #3
Se modifica system.c y dos archivos de LPCOpen en board para obtener compilacion limpia con -Wall. Las modificaciones solo marcan correctamente variables no usadas para no generar warnings.
Nuevo ejemplo utilizando interfaz de input de botones en la nueva board LPCOpen (blinky_input). Este es un ejericio que realize en el CESE.

En cuanto al punto 3, considero que -Wall es una herramienta fundamental. Los cambios realizados permiten compilar limpio proyectos LPCOpen con -Wall. Ignoro si proyectos con sAPI compila limpios con -Wall, pero podria ejercerse esa practica si lo consideran correcto.

…t/ e inclusion de ejemplo LPCOpen "blinky-input"

…pararlo al resto de ejemplos (se uso para comprobar compilacion limpia con -Wall)"

martinribelotta

Veo bien lo de -Wall (siempre y cuando se pueda deshabilitar por variable de makefile)

Te hice algunos comentarios sobre el PR. El mas fundamental es el de la licencia. No podemos poner codigo GPL aca... Primero porque es virico y segundo porque es incompatible con lpcopen

martinribelotta · 2018-04-07T16:50:02Z

libs/vendor_libs/lpc_board_ciaa_edu_4337/src/board.c

+    This program is free software: you can redistribute it and/or modify
+    it under the terms of the GNU General Public License as published by
+    the Free Software Foundation, either version 3 of the License, or
+    (at your option) any later version.


La licencia de LPCOpen y la GPL son incompatibles. Los aportes al proyecto se hacen con licencia BSD.

Ok, la cambio!

Si, esto está aclarado en al docuwiky de la CIAA, es BSD 3 clausulas.

El tema es que si no les vuelve GPL todo el repo y queremos que lo pueda usar cualquier empresa sin obligarlos a abrir el código.

Entiendo, gracias Eric. Ya tengo todos los cambios acordados, solo resta la resolucion de un par en donde aguardo comentarios.

¿Falta aclarar algo más?

Propongo un Pull request con las modificaciones al board (sacando lo de pragma y con comentarios en ingles). Luego vayamos discutiendo las otras cosas.

Otra vez gracias por el aporte!

martinribelotta · 2018-04-07T16:52:35Z

libs/vendor_libs/lpc_board_ciaa_edu_4337/inc/board.h

-
-#ifndef __BOARD_H_
-#define __BOARD_H_
+#pragma once


Esto no es codigo conforme. Es C99 pero no es aceptable existiendo una forma legacy perfectamente compatible con todos los compiladores. Ademas, pragma once, al ser implementation-defined puede andar mal en distintas plataformas de compilación. Si existe una forma portable y segura como son los guard-defined debemos usarla

Entiendo que en el contexto actual, los compiladores soportados por LPCOpen son Kei, IAR, Code Red y por extension GCC, no habria otros. En todos hace mas de una decada que #pragma once funciona perfecto. Igual veo que es extendido el uso de la forma clasica. Lo modifico, no hay problema.

martinribelotta · 2018-04-07T16:54:39Z

libs/vendor_libs/lpc_board_ciaa_edu_4337/src/board.c

+
+    ----------------------------------------------------------------------------
+
+    Objetivo y alcance:


No hacer comentarios en español en el codigo. Usar el ingles de lengua franca.

Crei haber leido que el lenguaje oficial del proyecto CIAA era Castellano? Eso no aplica al codigo? Que raro... Hay dos opciones que puedo proponer: puedo eliminar los comentarios o bien, podria alguien prestarse a traducirlos aclarando siempre que es una traduccion del castellano y el nombre del traductor?

Es así el tema, en el proyecto CIAA acordamos que la documentación sería en castellano y en lo posible en inglés también, mientras que en el código fuente los comentarios sean en inglés, eso es para ayudar a contribuciones extrangeras.

Algunas cosa de la documentación están en ambos idiomas, pero no contamos con traductores oficiales dentro del proyecto CIAA con lo cual escribimos ingles lo mejor que podemos cada uno. A lo sumo si alguien nos propone un mejor uso del idioma lo cambiamos.

Te propongo traducirlos voslo mejor que te parezca y lo miramos después. No tengas verguenza...

Ok aclarado entonces! pense que esta vez podria usar Castellano jaja :)
Ingleseo los comentarios.

martinribelotta · 2018-04-07T16:58:19Z

libs/vendor_libs/lpc_board_ciaa_edu_4337/src/board.c

+   Board_SPI_Init       ();
+   Board_GPIO_Init      ();
+   Board_I2C_Init       ();
+   Board_ADC_Init       ();


El identado luego del nombre de la funcion rompe el estilo usado. Es discutible pero visualmente no aporta nada.
En especial la critica va por el lado de uniformidad de codigo. Si usamos este estilo de codigo tenemos que reformatear todos los archivos, ademas, no hay una regla clara de identado.

Ok, lo modifico. Como comentario al margen (sin debatir estilos) a mi en particular me ayuda mucho el indentado vertical.

En esto estamos usando el estilo de código de Kernigan & Ritchie.

En astyle el parámetro es --style=k&r

En Eclipse el template de codigo se llama de igual manera: Kernigan & Ritchie.

En embedded IDE usamos el estilo Linux.

En otros IDE puede variar, pero siempre esta relacionado al estilo de llaves java, es decir, attached braces.

La única desviación del estándar es el uso de 3 espacios en lugar de 1 tab.

Ver la documentacion de aspell:
http://astyle.sourceforge.net/astyle.html#_Brace_Style_Options

martinribelotta · 2018-04-07T17:08:33Z

libs/vendor_libs/lpc_board_ciaa_edu_4337/src/system.c

-#define UNUSED(...) ((void) (__VA_ARGS__))
+#define UNUSED1(x) (void)x
+#define UNUSED2(x,y) UNUSED1(x); UNUSED1(y)
+#define UNUSED3(x,y,z) UNUSED2(x,y); UNUSED1(z)


No estoy convencido que sea la mejor manera de hacer esto, mas que nada porque fuerza a usar varias macros en vez de una.
Prefiero usar el UNUSED(exp) varias veces

A mi tampoco me gusto del todo. Cambio a UNUSED(exp) por cada una.
(La solucion correcta con VA_ARGS tokenizando parametros demanda 6 o 7 macros y me parecio exagerado para este uso)

Tratamos de no tocar tanto LPCOpen, la capa board está bien que la cambiemos, acá deberíamos tener 2, una para la CIAA-NXP y otra para la EDU-CIAA. Luego el tema dle arranque del clock. Peor más allá de eso no queremos cambiarla tanto porque si no en cada nueva versión que sale hay que tomarse el trabajo de hacerle estos cambios.

Los de la variable tmp:

(void) temp;

están para forzar que se lea la variable. No la pusieron en vano. Nosotros le agregamos el void para que no aparezca como warning.

Eric, las variables tmp funcionan exactamente igual que antes. Las dos lineas agregadas "(void) tmp;" solo sirven para marcar que tu intencion realmente es usarla como la usaste y asi el compilador no tira warnings en modo -Wall. Esto no tiene efectos secundarios en la generacion del codigo.

Lo correcto seria avisar a la gente de LPCOpen para que no tengamos que hacerlo de vuelta en cada nuevo release de ellos. Hace rato hice lo propio en ST y hubo muy buena respuesta.

Habría que probar según las opciones de optimización que no rompa en alguna combinación...

martinribelotta · 2018-04-07T17:14:54Z

Makefile

-ASRC=$(wildcard $(PROJECT_PATH_AND_NAME)/src/*.s)
-ASRC+=$(foreach m, $(MODULES), $(wildcard $(m)/src/*.s))
+ASRC=$(wildcard $(PROJECT_PATH_AND_NAME)/src/*.S)
+ASRC+=$(foreach m, $(MODULES), $(wildcard $(m)/src/*.S))


Esto puede traer problemas en los sistemas de archivos case-insensitive. Podriamos usar en su lugar la extención *.sx que segun la documentacion de gcc hace lo mismo.
De todas formas, no encuentro problemas al compilar los archivos *.s del proyecto actual. Solo tienen comentarios que son parte de gnu-as (aunque parescan de cpp)

La cosa no seria al reves? O sea, incluir .s o .S en case insensitive no traeria ningun problema ya que GAS siempre asume .S en esos sistemas?

Pero en Linux, por ejemplo, examples/lpcOpen/asm/first/src/zeros.S no compila con la makefile actual. Y si uno cambia .S por .s, tampoco compila ya que utiliza directivas del preprocesador.

Y como comente en el issue #3, en clase se vio justo el ejemplo que comento que no compila. Y se tuvo que hacer exactamente este cambio en la makefile, .s por .S.

La verdad, .sx no suena muy standard. Pero queda a tu decision.

Estoy de acuerdo, usemos .sx y .s para evitar confusiones.

Igual cundo pueda voy a probar bien que pasa si rompe algo en windows tener .s y .S.
También por qué compiló bien los .s con el cambioa .S en el makefile.

Bueno, descarto este cambio. Si desean pueden dejar el issue #3 para hacer el seguimiento.

royconejo · 2018-04-07T19:35:11Z

Aclaro lo de -Wall: Actualmente, en este framework no hay forma estandar de elegirlo o no, eso no cambio con mis aportes. Si quiero -Wall, lo que hago en mi codigo es OPT=g -Wall en el config.mk del proyecto.

epernia · 2018-04-07T21:10:48Z

Lo de -Wall deberíamos pensar la forma de ponerlo elegantemente.

Ese archivo config.mk esta hecho como está para que sea fácilmente generable en forma automática como hacemos con Embedded IDE.

royconejo · 2018-04-07T21:54:37Z

Ahi pushie modificaciones, creo que estarian resueltas todas las observaciones.

martinribelotta · 2018-04-08T04:01:29Z

Yo lo veo perfecto! Por mi podemos mergear sin problema. Las modificaciones de -Wall parametrizable y otros detalles (P.Ej *.s vs *.S vs *.sx) pueden quedar para futuros commits.

royconejo · 2018-04-08T05:17:59Z

Si Martin, aquello quedara para definir a futuro.

Sobre -Wall, algo raro que note es que la makefile de firmware_v2 siempre compilo por defecto con -Wall (y expone unos cuantos warnings de LPCOpen cuando se compila para el core M0!)

royconejo added 5 commits April 6, 2018 14:47

board Edu-CIAA en LPCOpen y makefile (.s -> .S)

b4afecc

Agrego ejemplo y compilacion limpia con -Wall

0150e11

Merge https://github.com/epernia/cese-edu-ciaa-template

6477f16

"Modificacion de .gitignore para ignorar efectivamente directorios ou…

3dc6416

…t/ e inclusion de ejemplo LPCOpen "blinky-input"

"Modificacion de config.mk del nuevo ejemplo "blinky_input" para equi…

b2cc55e

…pararlo al resto de ejemplos (se uso para comprobar compilacion limpia con -Wall)"

martinribelotta reviewed Apr 7, 2018

View reviewed changes

epernia mentioned this pull request Apr 7, 2018

makefile y archivos .s #3

Closed

Done changes according to merge discussion in epernia#4

725475a

epernia merged commit 9807f86 into epernia:master Apr 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge de cambios realizados #4

Merge de cambios realizados #4

royconejo commented Apr 6, 2018

martinribelotta left a comment

martinribelotta Apr 7, 2018

royconejo Apr 7, 2018

epernia Apr 7, 2018

royconejo Apr 7, 2018

epernia Apr 7, 2018

martinribelotta Apr 7, 2018

royconejo Apr 7, 2018

martinribelotta Apr 7, 2018

royconejo Apr 7, 2018

epernia Apr 7, 2018

royconejo Apr 7, 2018

martinribelotta Apr 7, 2018

royconejo Apr 7, 2018

epernia Apr 7, 2018

martinribelotta Apr 7, 2018

royconejo Apr 7, 2018

epernia Apr 7, 2018

royconejo Apr 7, 2018 •

edited

Loading

epernia Apr 7, 2018

martinribelotta Apr 7, 2018

royconejo Apr 7, 2018

epernia Apr 7, 2018

royconejo Apr 7, 2018

royconejo commented Apr 7, 2018

epernia commented Apr 7, 2018

royconejo commented Apr 7, 2018

martinribelotta commented Apr 8, 2018

royconejo commented Apr 8, 2018


		----------------------------------------------------------------------------

		Objetivo y alcance:

Merge de cambios realizados #4

Merge de cambios realizados #4

Conversation

royconejo commented Apr 6, 2018

martinribelotta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

royconejo Apr 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

royconejo commented Apr 7, 2018

epernia commented Apr 7, 2018

royconejo commented Apr 7, 2018

martinribelotta commented Apr 8, 2018

royconejo commented Apr 8, 2018

royconejo Apr 7, 2018 •

edited

Loading