Ho passato tre anni su un progetto dove ogni pull request passava attraverso esattamente due revisori, un periodo di attesa obbligatorio di 24 ore, e una checklist di quarantasette voci. Il codice era ancora pieno di bug. Il modulo safety-critical aveva una race condition che è sopravvissuta a quattordici revisioni. Lo sviluppatore junior passava più tempo ad aspettare approvazioni che a scrivere codice. E il team odiava il processo così tanto che la gente ha iniziato a fare commit in batch alla fine degli sprint solo per evitare l'overhead.
Quell'esperienza mi ha insegnato qualcosa di scomodo: la maggior parte dei processi di code review sono progettati per far sentire i manager al sicuro, non per migliorare il codice. I rituali — revisori obbligatori, numero minimo di commenti, gate di approvazione — creano l'illusione della qualità consumando enormi quantità di tempo di engineering.
Negli anni successivi, ho spogliato il processo, l'ho ricostruito dai principi fondamentali e l'ho testato su quattro diversi team firmware. Ecco cosa è sopravvissuto.
Il Primo Principio: Le Review Trovano Bug Diversi Dai Test
L'errore più grande che vedo nei team firmware è trattare la code review come un sostituto del testing, o il testing come un sostituto della review. Trovano cose diverse:
| Cosa trovano i test | Cosa trova la review |
|---|---|
| Errori logici, violazioni di timing, casi limite nell'esecuzione | API fraintese, assunzioni sbagliate sul comportamento dell'hardware |
| Valori di registro e maschere di bitfield errati | Configurazione di registro che funziona ma non è quello che il datasheet raccomanda per il caso d'uso |
| Corruzione della catena di descrittori DMA sotto carico | Layout del buffer DMA che sarà un incubo di manutenzione tra sei mesi |
| Stack overflow da catene di chiamate profonde | Qualificatori volatile mancanti su variabili condivise tra ISR e main loop |
Ho visto team investire in elaborati banchi di test hardware-in-the-loop mentre la loro code review consisteva in un singolo commento "LGTM" dalla persona più junior del team. E ho visto team con review obbligatorie a due persone su ogni commit in un test harness interno, mentre il bootloader che andava su diecimila dispositivi riceveva un timbro di gomma perché "era già stato revisionato in fase di prototipo."
La soluzione è semplice: abbina la profondità della review al profilo di rischio del codice, e non confondere mai il completamento del processo con la garanzia di qualità.
Il Modello a Tre Livelli
Uso tre livelli di review, applicati in base alla criticità del codice:
| Livello | Quando | Chi | Profondità |
|---|---|---|---|
| Livello 1 — Occhiata | Strumenti interni, test harness, script di build, documentazione | Una persona, qualsiasi anzianità (anche auto-merge dopo 2 ore) | Controlla errori evidenti e naming. Nessun commento di stile. |
| Livello 2 — Standard | Logica applicativa, implementazioni di protocolli, nuovi driver di periferiche | Una persona con conoscenza del dominio, entro 24 ore | Correttezza, design delle API, assunzioni di concorrenza, sanità a livello di registro. Stile solo se influisce sulla leggibilità. |
| Livello 3 — Approfondita | Bootloader, moduli safety-critical, sequenze di power management, configurazioni watchdog | Due persone: un esperto del dominio, un architetto/senior che non ha lavorato al modulo | Riga per riga se necessario. Analizza ogni transizione di stato, ogni percorso di errore, ogni assunzione di timing. Documenta l'esito della review nel commit message. |
Il punto chiave: la maggior parte del codice firmware è Livello 2. Trattare tutto il codice come Livello 3 esaurisce i revisori e rallenta il team. Trattare il codice del bootloader come Livello 1 è così che i dispositivi si brickano in campo.
Comunico questo modello a livelli esplicitamente all'inizio di ogni incarico — nel contratto o nel kick-off del progetto. Il cliente paga per le review di Livello 3 sui moduli critici e Livello 2 sul resto. Tutti sanno dove è tracciata la linea, e nessuno discute su "perché questa modifica ha bisogno di due revisori."
Gli Anti-Pattern Che Vieto dal Giorno Uno
Questi quattro pattern distruggono il valore delle code review nei team firmware. Li segnalo nella prima riunione di team:
1. Commenti di stile travestiti da problemi di correttezza.
"Per favore rinomina i2c_transfer() in i2c_transfer_data() perché trasferisce anche il byte dell'indirizzo." — Questa è una preferenza di stile vestita da pignoleria. Se la funzione funziona, è documentata e ha un contratto chiaro, lascia perdere. I dibattiti di stile nelle review sono il più grande spreco di tempo, e frustrano sproporzionatamente gli sviluppatori junior. Usa un formatter (clang-format), impostalo in CI, e passa oltre.
2. Il commento "Io l'avrei fatto diversamente."
Se l'approccio è corretto, abbastanza performante e sicuro, la preferenza per un'implementazione alternativa non è actionable. Il revisore dovrebbe chiedersi "questo approccio ha un problema di correttezza?" non "lo avrei scritto in questo modo?"
3. Revisori obbligatori N + 1.
La modalità di fallimento più comune: un progetto richiede due revisori, quindi ogni PR aspetta due persone. Nel frattempo, il revisore più competente ha già approvato, e il secondo revisore è una formalità che scorre veloce e clicca Approva. La review aggiunge zero valore ma raddoppia la latenza. La soluzione: un revisore con la giusta conoscenza del dominio vale più di due revisori casuali.
4. Revisionare codice generato o di configurazione.
Codice di inizializzazione generato da STM32CubeMX, linker script copiati dall'esempio SDK, header di configurazione CMSIS — perché degli umani stanno revisionando queste cose? Se l'output di CubeMX compila e l'hardware si avvia, la review è teatro. Concentra il tempo dei revisori sulla logica che l'ingegnere ha effettivamente scritto.
La Checklist Specifica per l'Embedded
Oltre ai principi generali di code review, il codice firmware ha modalità di fallimento che le review di software puro non incontrano mai. Ecco le cose specifiche che controllo in ogni review di Livello 3:
- Semantica volatile. Ogni registro memory-mapped e ogni variabile condivisa tra ISR e main loop è qualificata con
volatile? Il qualificatore è applicato correttamente al puntatore target, non al puntatore stesso? (Ho perso il conto di quante dichiarazionivolatile uint32_t*ho visto che avrebbero dovuto essereuint32_t volatile*.) - Priorità e nidificazione degli interrupt. Il codice assume un certo schema di priorità degli interrupt? C'è un lock (disabilitazione interrupt) che potrebbe causare un hard fault se il controller degli interrupt è configurato diversamente su una variante diversa del chip? La sezione critica maschera solo gli interrupt giusti, non tutti?
- Alimentazione del watchdog. Il watchdog viene alimentato dal main loop o da un timer ISR? Se alimentato dal main loop, c'è un percorso dove il loop si blocca (
HAL_Delaycon una periferica bloccata, un bus I2C locked) e il watchdog non scatta? - Test dei percorsi di errore. Cosa succede quando
HAL_I2C_Master_Transmit()restituisceHAL_ERROR? La maggior parte del codice firmware assume successo. La review dovrebbe verificare che ogni chiamata API abbia almeno un log o un fallback sul fallimento — e se non ce l'ha, la giustificazione deve essere esplicita ("questo è un percorso solo di sviluppo", "la macchina a stati riprova dall'inizio"). - Assunzioni sul clock tree. Il codice hardcoda un delay o un timeout basato su una frequenza di clock specifica? Se il progetto passa da HSI a HSE o cambia la configurazione del PLL, quei delay si rompono silenziosamente.
- Endianness e allineamento. Il firmware comunica spesso su protocolli byte-oriented (I2C, SPI, UART). I campi multi-byte sono impacchettati correttamente? C'è un potenziale fault di allineamento quando si casta un buffer di byte a un puntatore a struct?
L'Infrastruttura Che Rende le Review Veloce
Nessuna quantità di ottimizzazione del processo risolverà un workflow di review dove il revisore deve fare checkout del branch, compilare, flashare ed eseguire manualmente per capire la modifica. È qui che la maggior parte dei team dovrebbe investire prima di ritoccare la politica di review:
- CI che compila ogni PR. Banale — ma rimarresti sorpreso da quanti progetti firmware eseguono
makesolo sulla macchina dello sviluppatore. La CI dovrebbe compilare, eseguire analisi statica e fallire rapidamente sui warning. - Un diff che ignora spazi bianchi e formattazione. Il
?w=1di GitHub è il tuo migliore amico. Se il PR è all'80% reformattazione, questa è una preoccupazione della CI, non della review. - Smoke test hardware-in-the-loop sui PR critici. Non una regressione completa — solo "la UART stampa ancora?" e "la sequenza LED funziona ancora?" Abbastanza per cogliere la rottura ovvia prima che un umano guardi il codice.
- Un screenshot condiviso di logic analyzer o oscilloscopio per modifiche timing-critical. Un'immagine della transazione SPI vale cento righe di commenti di review.
Cosa Ha Funzionato per Me
Quattro cose hanno fatto la differenza più grande nei team che ho guidato:
- Scrivere le aspettative di review nella team charter. Un documento di una pagina che dice: "Le review di Livello 2 devono iniziare entro 24 ore. Le review di Livello 3 sono programmate come riunione. I commenti di stile sono vietati a meno che non esista una regola del formatter. I revisori approvano o richiedono modifiche — niente 'optional nitpick' lasciati in sospeso per giorni."
- Bloccare tempo per le review. Programmo due blocchi di review da 45 minuti al giorno, primo mattino e subito dopo pranzo. Niente riunioni, niente Slack, niente email. Se il team sa che il revisore ha tempo dedicato, inviano PR più piccoli e più mirati — il che rende le review più veloci, il che spinge le persone a inviare PR ancora più piccoli. Un circolo virtuoso.
- Revisionare il processo di review. Ogni trimestre, chiedo al team due domande: "Su cosa stiamo passando troppo tempo nelle review?" e "Quali bug sono sfuggiti alla review che avremmo voluto fossero stati catturati?" Le risposte guidano i cambiamenti di processo. Un team ha scoperto di passare il 40% del tempo di review su modifiche ai linker script che nessuno capiva veramente — così hanno scritto un generatore di linker script e hanno revisionato il generatore una volta, non l'output ogni volta.
- Pair programming sui moduli critici invece della review. Per il bootloader e la macchina a stati del power management, mi siedo con lo sviluppatore e scriviamo il codice insieme. Richiede lo stesso tempo di una review di Livello 3 ma produce codice migliore e trasferisce più conoscenza. Poi il PR riceve un'occhiata di Livello 1 solo per traccia.
Checklist Pratica
- Classifica il codice in tre livelli (Occhiata / Standard / Approfondita) e abbina la profondità al rischio
- Vieta i commenti di stile — usa clang-format in CI invece
- Un revisore esperto del dominio vale più di due revisori casuali
- Non revisionare codice generato (CubeMX, esempi SDK, linker script del vendor)
- Controlla semantica volatile, priorità interrupt, alimentazione watchdog in ogni review firmware
- Verifica i percorsi di errore — la maggior parte del codice embedded assume successo
- Aggiungi build CI + analisi statica prima di aspettarti che umani facciano review
- Programma tempo dedicato per le review; blocca Slack ed email durante
- Rivedi il processo di review trimestralmente con il team
Fonti
- Guida alle Code Review di Google — il principio "cerca correttezza, non stile"
- Michael Fagan, "Design and Code Inspections to Reduce Errors in Program Development" (1976) — il documento originale sulle ispezioni formali, ancora rilevante per il Livello 3
- Esperienza personale su 8 team firmware tra il 2018 e il 2026
📬 Commenti / discussione
Preferisci email: comments@carrese.eu — includi l'URL dell'articolo così posso seguire. Per correzioni o domande più approfondite, di solito rispondo entro 48 ore.