Ottimizziamo il codice del core di Arduino

PaoloP: ... Guglielmo hai fatto la modifica anche per il compilatore cpp?

Si, ho tolto la -w ad entrambi e ... ora ho aggiunto la -Wall al entrambi ;)

Guglielmo

-Wall abilita tutte le warning. Le -Dxx sono macro utente visibili in ogni unit compile, come appunto la versione dell'ide -DARDUINO=155

Ciao.

clonato il repo: https://github.com/lestofante/Arduino/blob/ide-1.5.x

già rimossa l'infame -w, se mi fate un riassunto delle modifiche le carico, graaaazie :)

partiamo dalla toolchain originale, arduino 1.5.4 (la 1.0.x lasciamola perdere per favore)

con -Wall ecco i warning di un codice base:

void setup() {
  // put your setup code here, to run once:

}

void loop() {
  // put your main code here, to run repeatedly: 
  
}
/home/mauro/arduino-1.5.4/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp: In function 'void __vector_18()':
/home/mauro/arduino-1.5.4/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp:89: warning: unused variable 'c'
/home/mauro/arduino-1.5.4/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp: In member function 'void HardwareSerial::begin(long unsigned int, byte)':
/home/mauro/arduino-1.5.4/hardware/arduino/avr/cores/arduino/HardwareSerial.cpp:329: warning: unused variable 'current_config'
/home/mauro/arduino-1.5.4/hardware/arduino/avr/cores/arduino/Tone.cpp:119: warning: only initialized variables can be placed into program memory area
/home/mauro/arduino-1.5.4/hardware/arduino/avr/cores/arduino/Print.cpp: In member function 'size_t Print::print(const __FlashStringHelper*)':
/home/mauro/arduino-1.5.4/hardware/arduino/avr/cores/arduino/Print.cpp:44: warning: '__progmem__' attribute ignored

Non c’è l’ho operativa la 1.5.4, anzi se mi sai dire quella che ho scaricato dal repo chiamata 1.5.x che versione è?
In allegato trovi tutti i file, devi metterli tu nel posto giusto.

Comunque le modifiche sono poche, quindi ti basta fare un diff e in 10 minuti risolvi anche con la 1.5.4.

Aggiornamento zip:
Incluso HardwareSerial.cpp

Note: Il codice proviene da arduino-1.0.5 quindi, non copiare direttamente nel ramo 1.5x ma usa un programma diff e applica le modifiche.

Perché 1.0.5? Io operativa ho solo la 1.0.5, non sono in grado di fare il build della 1.5x, se avete info precise postate pure.

ciao.

arduinochanged.zip (10.4 KB)

Le versioni attualmente in download (http://arduino.cc/en/Main/Software) sono la 1.0.5 e la 1.5.5.

Questa --> https://github.com/arduino/Arduino/tree/ide-1.5.x è la 1.5.5 con 1 o 2 modifiche effettuate dopo la pubblicazione. In pratica è la versione 1.5.6 in sviluppo.

per la prima parte di warning: rimosse le 3 variabili.

UDR0 e UDR mi hannolasciato un po perplesso, essndo registrinon vorrei che la loro lettura scatenasse qualcosa. Credo di no, anzi c'è il riscio che l'istruzione sia rimossa dal compilatore, vedendo che è inutile, come un ciclo for finito ma vuoto, no?

per la parte 2 di tone, non so, c'è la progmem di mezzo e un array di un valore.. e tra l'altro mi sembra errata lal ibreria pèerchè fa

tone_pin_to_timer_PGM + i

invece io mi aspetterei fosse un tone_pin_to_timer_PGM[i] ovvero che legge il prossimo

ma poi in realtà l'uso di più timer cozza con le definizioni multiple ehh. bho è una schifezza di codice, scritto metà per supportare più timer e metà per funzionare con un timer solo. Meriterebbe una riscrittura.

MauroTec opinioni sul da farsi?

per la 3° parte se ho capito bene avete già risolto, giusto? mi posti il codice corretto? graaazie :)

per chi volesse clonare, giusto per testare e senza portarsi appresso tutto il mondo, ecco il comando git:

git clone --depth 1 -b ide-1.5.x https://github.com/lestofante/Arduino.git

git: comando e ci siamo clone: crea un clone del repo --depth 1: non impora la storia, ma soloilmomento attuale -b ide-1.5.x: clona solo il branch ide-1.5.x https://github.com/lestofante/Arduino.git: da dove pescare il codice

ps. nei preferencies.txt ho tolto il -w, ma NON ho messo il -Wall. Anche quì, sono aperto a suggerimenti pps. e ai SAM nessuno ci pensa? mi aspeto avvenga di conseguenza,ma essendo parti del codice diverse...bhe vedremo poi

edit: la mia versione si basa sull'attuale 1.5.x, questo per evitareun pò di casini eventuali quando faremo la pull-request :)

Per Tone.cpp non ho risolto, io credevo si trattasse di un bug del compilatore risolto con la 4.7.2, almeno così ho letto in rete.

Per UDR ecc, non ti seguo non vedo warning, comunque i warning unused lasciamoli stare al momento.

#if defined(UDR0)
    if (bit_is_clear(UCSR0A, UPE0)) {
      unsigned char c = UDR0;
      store_char(c, &rx_buffer);
    } [color=brown]else {
      unsigned char c = UDR0;[/color]
    };

Che senso ha questo codice in HardwareSerial?
Il ; dopo chiusura blocco. Il codice in brown color si deve eliminare, in quanto non ha senso creare c se poi nella funzione ISR non viene usata. Ho modificato anche altre cose in HardwareSerial che non ho incluso nello zipfile, l’ho dimenticato.

Rivalutazione
Il codice in brown color deve essere mantenuto, in quanto ha la funzione di scartare il carattere, nel caso in cui PE0 in UCSRnA is set, però io non vedo abilitato (UPMn1 = 1) in UCSRnC. Comunque mi sembra di aver risolto l’unused di c così:

 #if defined(UDR0)
    unsigned char c;    
    if (bit_is_clear(UCSR0A, UPE0)) {
      c = UDR0;
      store_char(c, &rx_buffer);
    } else {
      c = UDR0;
    }
  #elif defined(UDR)
    unsigned char c;
    if (bit_is_clear(UCSRA, PE)) {
      c = UDR;
      store_char(c, &rx_buffer);
    } else {
      c = UDR;
    }
  #else
    #error UDR not defined

Ciao.

nid69ita: wiring.c (nella delay() richiama la yield() !?!)

La Due ha un proto-scheduler di tipo cooperativo basato sul SysTick, un timer interno, e sull'uso della funzione yeld per passare il controllo da un task all'altro. Non so però il motivo per cui la trovi nel core Avr, dovrebbe essere solo nel core Arm.

Ah, ho capito. Nel wiring.c del core Avr la funzione yield è una macro che serve ad attendere esattamente 1 microsecondo eseguendo una particolare istruzione assembly, ed è usata nella funzione delay per contare il tempo non usando più millis. In teoria quindi delay dovrebbe funzionare anche dentro una ISR.

Invece in wiring.c del core Sam la funzione di yield è proprio quella di eseguire altri task mentre il core è in un delay.

MauroTec: Le -Dxx sono macro utente visibili in ogni unit compile, come appunto la versione dell'ide -DARDUINO=155

Si @Mauro è chiaro. Trovo strano però che nelle 2 versioni IDE vengano poi dichiarate altre -D diverse tra i due. A che serviranno? Influenzano qualche lib? Magari causano compilazioni diverse? Non credo sia importante però un ma, mi sovviene.

@Lesto Per i SAM si usa la toolchian ARM che mi pare sia l'ultima.

Si @Mauro è chiaro. Trovo strano però che nelle 2 versioni IDE vengano poi dichiarate altre -D diverse tra i due. A che serviranno? Influenzano qualche lib? Magari causano compilazioni diverse? Non credo sia importante però un ma, mi sovviene.

Non ho avuto il tempo per indagare, sarebbe todo, se puoi cerca quelle macro in tutto il codice con find e vedi cosa fanno.

Per Tone.cpp non ho risolto, io credevo si trattasse di un bug del compilatore risolto con la 4.7.2, almeno così ho letto in rete.

Mi autoquoto. Il problema è stato risolto come pensavo a partire dalla 4.7.2 infatti nel file degli errori allegato da gpb01 non c'è quel warning.

Ciao.

MauroTec: Per Tone.cpp non ho risolto, io credevo si trattasse di un bug del compilatore risolto con la 4.7.2, almeno così ho letto in rete.

io ho letto il codice della tone e mi pare una porcheria per i motivi cui sopra. Che poi un array dia warning starni sul progmem che cmq da warning anche sulle variabili ci sta.

MauroTec: Rivalutazione Il codice in brown color deve essere mantenuto, in quanto ha la funzione di scartare il carattere, nel caso in cui PE0 in UCSRnA is set, però io non vedo abilitato (UPMn1 = 1) in UCSRnC.

immaginavo, in oltre è obbligatorio leggere PRIMA UCSRA e poi UDR, però vedo che la cosa un pò varia, in oltre non è tenuto conte l'errore FE (Frame Error, bit 4) e DOR (Data OverRun, bit 3)

ora mi studio un pò il datasheet, vediamo che salta fuori

da datasheet:

unsigned int USART_Receive( void )
{
unsigned char status, resh, resl;
/* Wait for data to be received */
while ( !(UCSRnA & (1<<RXCn)) )
;
/* Get status and 9th bit, then data */
/* from buffer */
status = UCSRnA;
resh = UCSRnB;
resl = UDRn;
/* If error, return -1 */
if ( status & (1<<FEn)|(1<<DORn)|(1<<UPEn) )
return -1;
/* Filter the 9th bit, then return */
resh = (resh >> 1) & 0x01;
return ((resh << 8) | resl);
}

nid69ita: ```

/avr/bin/avr-g++ -c -g -Os -w -fno-exceptions -ffunction-sections -fdata-sections -MMD -mmcu=atmega328p -DF_CPU=16000000L -DARDUINO=155 -DARDUINO_AVR_UNO -DARDUINO_ARCH_AVR -I/...

\avr\bin\avr-g++ -c -g -Os -Wall -fno-exceptions -ffunction-sections -fdata-sections -MMD -mmcu=atmega328p -DF_CPU=16000000L  -DARDUINO=105 -DUSB_VID=null -DUSB_PID=null  -I/... ```

Le altre define che fanno? I vari -D qualcosa ?

Dalla versione 1.5.3 sono cambiate le define passate al compilatore per definire l'hardware per cui si compila. https://github.com/arduino/Arduino/issues/308 Dalla 1.5.3 si usa -DARDUINO_{board} e poi -DARDUINO_ARCH_AVR oppure ARDUINO_ARCH_SAM Mentre nella 1.0.x venivano usate USB_VID=xxx -DUSB_PID=xxx Secondo me non è poi così banale, ma forse poco importante. Nel core e nelle librerie distribuite dal Team si saranno adeguati. Chissà nelle librerie di terze parti se il tutto funziona ancora (se si basano su quelle define USB qualcosa).

Esempio nella libreria fornita dal Team TFT, c'era:

#if (USB_VID == 0x2341) && (USB_PID == 0x803C) // are we building for Esplora?

ora c'e':

#if ARDUINO_AVR_ESPLORA // are we building for Esplora?

Stranamente in 1.5.5 è poco presente questo ARDUINO_xxx e di fisso han messo USB_VID = 0x2341 in Arduino.h se SAM (usata questa macro in USBcore.cpp)

#define USB_VID            0x2341 // arduino LLC vid

esatto, così facendo invece che compilare per atmega328 o atmega328p, visto che la atmel (e/o gcc?) ha fatto il favore che i nomi dei registri sono gli stessi per gli stessi hardware, non serve più verificare il TIPO di MCU ma se SUPPORTA quell'hardware, aka se possiete quei registri.

In questo modo, se la atmel rialscia una nuova MCU i cui HW sono gli stessi di altre MCU già scritte, il suo supporto è automagico!!

lesto: esatto, così facendo invece che compilare per atmega328 o atmega328p, visto che la atmel (e/o gcc?) ha fatto il favore che i nomi dei registri sono gli stessi per gli stessi hardware, non serve più verificare il TIPO di MCU

Non diciamo cavolate :) Il core del 328 è diverso da quello del 328p, il compilatore DEVE sapere per quale compila, solo per il 1284 e il 1284p è possibile fare una cosa unificata perché i due core sono identici.

il compilatore deve saperlo, ma non gli #IFDEF che scelgono se e quali registri usare.

se la atmel rialscia una nuova MCU i cui HW sono gli stessi di altre MCU già scritte

rettifico aggiungendo: e che sia supportata da GCC

edit: esempio: io posso scrivere HardwareSerial per atmega238p, ma siamo d'accordo che il codice è uguale per atmgea8 e varie altre. Quindi o faccio uina bella #ifdef con millemila cpu, oppure una bella #ifdef nomeRegistroUsart poi ri-edito quando trovo l'issue che spiega bene

Secondo me non è poi così banale, ma forse poco importante. Nel core e nelle librerie distribuite dal Team si saranno adeguati.

Non è né banale né poco importante e solo che in questa fase non serve sapere cosa sono quelle #define. Non vorrei ci si lanciassi in modifiche pesanti al core per almeno un motivo: Il core bene o male fa il proprio lavoro, sistemiamo quello che si può senza stravolgere il funzionamento, ottenuta una compilazione senza warning o con solo unused è l'obiettivo e mi sembra che ci siamo arrivati o quasi, quasi perché io non ho la toolchain 3.4.3, quindi chi l'ha è pregato di apportare le modifiche e postare il risultato della compilazione con i warning abilitati.

Stasera vedo di postare la HardwareSerial modificata per compilare senza warning.

Riguardo alla gestione della HardwareSerial c'è da fare delle considerazioni e in base a queste prendere delle decisioni. Al momento non ho ben chiaro come si debbano gestire gli errori quando di mezzo c'è il buffer rx, scartare il carattere non è risolutivo ma fa comprendere che qualcosa non funziona. Se spedisco alimortacci e ricevo alimortaci il significato si perde no. :grin:

Comunque gli interventi sul core o lib che aggiungono caratteristiche o migliorano il funzionamento lo si deve fare in seconda istanza, al momento ne possiamo solo parlare, perché mettere mano pesantemente al codice lo può portare facilmente al non funzionamento per cui ogni intervento deve essere testato con un codice di test.

Ciao.