Enkla men fatala buggar

PIC, AVR, Arduino, Raspberry Pi, Basic Stamp, PLC mm.
Användarvisningsbild
TomasL
EF Sponsor
Inlägg: 45168
Blev medlem: 23 september 2006, 23:54:55
Ort: Borås
Kontakt:

Re: Enkla men fatala buggar

Inlägg av TomasL »

Nja, håller inte med dig.
Det är inte fel att förklara vad koden gör, det blir oerhört mycket tydligare då, dessutom, så kan man faktiskt skilja mellan vad den bär göra och vad den gör.

Min devis, Kommentareran skall vara 10x Koden, typ, eller så.
Användarvisningsbild
stekern
Inlägg: 453
Blev medlem: 2 november 2008, 08:24:18
Ort: Esbo, Finland

Re: Enkla men fatala buggar

Inlägg av stekern »

TomasL skrev:Nja, håller inte med dig.
Det är inte fel att förklara vad koden gör, det blir oerhört mycket tydligare då, dessutom, så kan man faktiskt skilja mellan vad den bär göra och vad den gör.
Jag håller med, men det beror ju på vilken nivå du kommentarar vad koden gör.
Kommentarer som beskriver saker man uppenbart kan se från koden är bara överflödiga.
TomasL skrev:Min devis, Kommentareran skall vara 10x Koden, typ, eller så.
Det låter alldeles för mycket, men beror givetvis på var kommentarerna är.
Om du har 10 rader kommentarer för varje rad kod, låter det väldigt jobbigt att läsa.
100 rader kommentarer för 10 rader kod kan vara ok, men låter fortfarande för mycket.
Om du måste skriva så mycket kommentarer för att beskriva koden du skriver,
borde du nog revidera din kod istället.
Användarvisningsbild
jesse
Inlägg: 9233
Blev medlem: 10 september 2007, 12:03:55
Ort: Alingsås

Re: Enkla men fatala buggar

Inlägg av jesse »

Det handlar ju inte bara om kod eller kommentarer. Som stekern säger handlar det också om hur koden ser ut. Då tänker jag främst på hur den är organiserad, i form av funktioner, varibler med vettiga namn organiserade i struct:er och att man använder beskrivande MACRO (om man inte har någon princip om att aldrig använda MACRO).

Exempel på hur MACROn kan användas: Här säger MACROTs namn det man behöver veta

Kod: Markera allt

// analoga ingångar - ADC
		#define AINP_temp	6 // Extern tempsensor mcp9700a som ger 500 mV vid noll grader plus 10 mV per grad C.
		#define AINP_batt	7 // mäter 12-volten på ingången via 22k/10k spänningsdelare
	// digitala utgångar			
		#define FluidPumpOn()  PORTD |=  OUT_RELAY_1_VPUMP
		#define FluidPumpOff() PORTD &= ~OUT_RELAY_1_VPUMP
		
		#define KontaktorOn()  PORTD |=  OUT_RELAY_0_KONTAKTORER
		#define KontaktorOff() PORTD &= ~OUT_RELAY_0_KONTAKTORER
		
		#define LED_on()  PORTD |=  OUT_RELAY_3
		#define LED_off() PORTD &= ~OUT_RELAY_3
	// PWM-utgång	
		#define SetPWM(val) OCR1A = (val) // 0-1023
Jag skulle aldrig skriva t.ex. PORTD &= ~(1 << PD5); direkt i koden. Antingen använder jag en funktion eller ett MACRO så det blir istället KontaktorOff(); - betydligt mer lättläst och mindre risk för småfel t.ex. ett tecken som kommit på fel ställe.

Exempel på struct: Jag vill gärna hålla variabelnamn korta utan att skapa obegripliga förkortningar. Kommentar obligatorisk för varje variabel.

Kod: Markera allt

	
		typedef struct
		{
			/* inlästa rådata från ADC - filtreras under inläsning */
			u16 raw_HV;		// driftbatteri spänning - ADC-data * 8 =  0-32767
			s16 raw_curr;	// strömsensor read data - aDC-data * 8 = -32768 till +32767
			u16 raw_batt;	// 12V system read data, 0-1023 * 4
			u16 raw_temp;	// temperatur externt read data 0-1023 * 4
			/* behandlade data från ADC */
			u16 high_volt;	// driftbatteri spänning * 10 - t.ex. 130.0V = 1300
			u16 batt_volt;	// 12V-batteri spänning * 100 - avläst 12.35 volt = 1235
			s16 current;	// avläst ström från strömsensor * 100, t.ex. I = 24.55A = 2455
			s16 temperatur;	// temperatur externt - avläst = grader C * 10
			s16 high_volt_temp_adjusted; //driftbatteri spänning * 10, justerad för teperatur.
			s32 ampHours_fas1;	// antal inladdade amperetimmar * 10 << 16 , 2400<<16 = 240.0 Ah: totalt under fas1
			s32 ampHours_fas2;	// antal inladdade amperetimmar * 10 << 16 , 2400<<16 = 240.0 Ah: totalt under fas2
			s32 ampHours_diff;	// antal inladdade amperetimmar * 10 << 16 , 2400<<16 = 240.0 Ah: fas1 - fas2/0.15
		} analogTyp;
		
		typedef struct
		{
			bo		AC_on;		// om nätspänning är ansluten
			bo		ladd_run;	// styrning av laddare, på/av.
			State	state;		// laddtillstånd
			Orsak	stop_orsak;		// felkod
			u16		time;		// tid i sekunder
			u8		counter;	// räknar andra tider (sekunder)
			s16		current_output; // laddarens utström, sätts med setLaddCurrent(nn)
			u16		current_limit; // max strömgräns för laddare
			u16		current_outlim; // utström begränsad
			u16		fas1_volt_max; // omslag från Fas1 till fas2 - beroende av temperatur. U*10
			u8		err_state;	// lagrar state vid error.
			u16		err_time;	// lagrar tid vid "orsak", sekunder
			u8		blink;		// antal blink med LED per sekund 0 - 15 * 16
			bo		AC_off_Flag;// flagga som sätts om kontakten dragits ur under laddning eller efter laddning
				
		} systemTyp;
				
Med rätt variablenamn och uppgifterna uppdelade i funktioner blir koden sedan lättläst:

Kod: Markera allt

/*** Analog read ***/
	
	analog.batt_volt = read12Vbatt();
	analog.high_volt = readHighVolt();
	analog.temperatur = readTemp();
	
Användarvisningsbild
TomasL
EF Sponsor
Inlägg: 45168
Blev medlem: 23 september 2006, 23:54:55
Ort: Borås
Kontakt:

Re: Enkla men fatala buggar

Inlägg av TomasL »

Nu menar jag naturligtvis inte att man skall kommentera rader såsom A = B + C;, typ.
Dock skall det finnas förklarande texter vad Konstanterna är för något, Vad variablerna är (Oavsett snygga förklarande namn) samt vilka värden de förväntas anta osv, till detta kommer att förklara vad själva modulen gör samt vad respektive funktion gör och en kommentar i if/while/for satser om vad de gör och varför.

Det kvittar hur förklarander och tydlig koden är, efter ett halvår kommer man inte ihåg hur man tänkte, tyvärr.
Dessutom skall någon annan kunna göra nått, så är det bra för denne andra att veta hur tankarna gick.
Användarvisningsbild
TomasL
EF Sponsor
Inlägg: 45168
Blev medlem: 23 september 2006, 23:54:55
Ort: Borås
Kontakt:

Re: Enkla men fatala buggar

Inlägg av TomasL »

Jag skulle aldrig skriva t.ex. PORTD &= ~(1 << PD5); direkt i koden. Antingen använder jag en funktion eller ett MACRO så det blir istället KontaktorOff(); - betydligt mer lättläst och mindre risk för småfel t.ex. ett tecken som kommit på fel ställe.
Det gör inte vi heller, vi enumrerar istället, då makron bara skapar problem, samt att vi även använder inversregistren och de biblioteksfunktioner som finns tillgängliga.
Så det kan till exempel bli PortSetBits(SPICSPort, ADC1); till exempel
Där SPICSPort och ADC1 är enumererade istället för #define'ade, typ.

Fördelen är ju att kompilatorn ger fel, om man klantar till det och försöker omdefiniera av misstag (Vilket är lätt hänt när kodmassan börjar närma sig någon halvmiljon rader), och med #define får man på sin höjd en varning, typ.
Användarvisningsbild
stekern
Inlägg: 453
Blev medlem: 2 november 2008, 08:24:18
Ort: Esbo, Finland

Re: Enkla men fatala buggar

Inlägg av stekern »

TomasL skrev:Det gör inte vi heller, vi enumrerar istället, då makron bara skapar problem [...]
enum är bra, men min mening är att de passar endast för konstanter i följande ordning.
Dvs, för t.ex. bit-definitioner eller adress-definitioner ser jag hellre en macro definition.

Kod: Markera allt

#define SOMEPERIPHERAL_SOMEREGISTER_ADDR 0x10001000
#define SOMEPERIPHERAL_SOMEREGISTER_SOMEBIT (1<<5)
istället för

Kod: Markera allt

enum {
  SOMEPERIPHERAL_SOMEREGISTER_ADDR = 0x10001000,
};
enum {
  SOMEPERIPHERAL_SOMEREGISTER_SOMEBIT = (1<<5),
};
Omdefinitioner undviks lättast genom att använda tillräckligt beskrivande namn,
om man har problem med att man lyckas använda samma namn på flera ställen,
så är det igen dags att titta över hur man skriver sin kod.

Sen det som Jesse sade om att mycket är beroende på hur koden ser ut och hur den är organiserad,
detta gäller inte bara kommentarer, utan även t.ex. variabelnamn.
Hur beskrivande ett variabelnamn behöver vara beror helt på vilket 'scope' (vad heter detta på svenska?)
variabeln har.
Ju större scope en variabel har, desto mer beskrivande behöver den vara och vice versa.
I min mening kan ner till enbokstavsvariabler vara helt ok när scopet är minimalt och meningen med dem är uppenbar.
Typexemplet är i,j,k för loop-variabler, men även andra fall kan inkluderas, t.ex. när variablen är av en typ där typen gör det uppenbart vad variabelns funktion är, t.ex.

Kod: Markera allt

void init_device(struct device *d)
{
  d->reset_regs();
  d->bus->init();
  d->irq_init();
}
persika
EF Sponsor
Inlägg: 1336
Blev medlem: 31 juli 2006, 22:14:37
Ort: Österlen, Skåne

Re: Enkla men fatala buggar

Inlägg av persika »

Google översätter scope med tillämpningsområde.
Stämmer ju bra i detta fall, men klumpigt tycker jag.
Användarvisningsbild
Icecap
Inlägg: 26105
Blev medlem: 10 januari 2005, 14:52:15
Ort: Aabenraa, Danmark

Re: Enkla men fatala buggar

Inlägg av Icecap »

I programmering anser jag att "scope" kan kallas "arbetsområde".
extradrajven
Inlägg: 18
Blev medlem: 1 juni 2013, 18:07:48

Re: Enkla men fatala buggar

Inlägg av extradrajven »

Jag föredrar det typsäkra:

Kod: Markera allt

#include <stdint.h>
static const uint32_t SOMEPERIPHERAL_SOMEREGISTER_SOMEBIT = (1<<5);
och undviker funktionsmakron:

Kod: Markera allt

static inline void KontaktorOn(void) {
  PORTD |=  OUT_RELAY_0_KONTAKTORER;
}
Onödigt att blanda in preprocessorn i onödan.
Användarvisningsbild
adent
Inlägg: 4094
Blev medlem: 27 november 2008, 22:56:23
Ort: Utanför Jönköping
Kontakt:

Re: Enkla men fatala buggar

Inlägg av adent »

Ingen bugg, bara avsaknad av databladsläsning...

SPI på en AVR: Använde gpio som CS. AVR är master. SS-pinnen är oanvänd ingång, flytande för tillfället. (ja jag vet, ska inte)
all SPI-kommunikation dör efter någon sekund, ibland mitt i en byte. Tydligen är SS-aktiv tillsammans med SPI
även i Master-mode, ifall man kör multimaster, om den dras låg anser AVR:en att nån annan vill prata på bussen och tvärtystnar.
Satte man SS till utgång funkade allt som det skulle :)
Användarvisningsbild
jesse
Inlägg: 9233
Blev medlem: 10 september 2007, 12:03:55
Ort: Alingsås

Re: Enkla men fatala buggar

Inlägg av jesse »

Jag har en känska av arr den här tråden skulle passa bättre i "Programmering"....?

Anyway, här en kul sida med "månadens bugg", och lösningen på den, så klart.
"Lösningen" i det här fallet genereras av analysverktyget PC-lint.

Använder ni några kod-analys-verktyg , och vilka i så fall? Vilka tycker ni är bra / dåliga? Varför?

Bug of the Month Samples
superx
Inlägg: 1127
Blev medlem: 19 juni 2012, 23:28:16
Ort: Linköping

Re: Enkla men fatala buggar

Inlägg av superx »

Jag använder clangs statiska kodanalys ibland. Ganska bra faktiskt! Framförallt bra på att hitta vägar genom stora härken av switch-satser och preprocessor-defines som leder till odefinierade variabler etc.

Valgrind är bra också, men det är ju inte kodanalys utan runtime-test.
Användarvisningsbild
jesse
Inlägg: 9233
Blev medlem: 10 september 2007, 12:03:55
Ort: Alingsås

Re: Enkla men fatala buggar

Inlägg av jesse »

Gjorde en 'bugg' idag som jag aldrig hade kommit på om jag inte råkade ha indata som utlöste buggen.

hur ofta skriver man inte t.ex. när man vill skriva ut en sträng (som inte är konstant):

Kod: Markera allt

char text[];
...
printf(text);
DET ÄR FEL !

I mitt fall råkade strängen innehålla c-kod, och då blev det så här:

Original: sprintf( Buffer, " %2.2d", Tid.Sec );
Utskrift: sprintf( Buffer, " 1980043360", Tid.Sec ); :shock:

Det här var jag inte alls beredd på.... man måste alltså använda ett annat sätt att skicka ut informationen på... kanske printf("%s", text); ?

Hur många har gjort fel på den, räck upp en hand! :jimmyhacker:
Användarvisningsbild
lillahuset
Gått bort
Inlägg: 13969
Blev medlem: 3 juli 2008, 08:13:14
Ort: Norrköping

Re: Enkla men fatala buggar

Inlägg av lillahuset »

GCC varnar.
Användarvisningsbild
jesse
Inlägg: 9233
Blev medlem: 10 september 2007, 12:03:55
Ort: Alingsås

Re: Enkla men fatala buggar

Inlägg av jesse »

inte min GCC:
-------------- Build: Debug in wave (compiler: GNU GCC Compiler)---------------

mingw32-gcc.exe -Wall -g -c E:\CodeBlocks\filfix\main.c -o obj\Debug\main.o
mingw32-g++.exe -o bin\Debug\wave.exe obj\Debug\main.o
Output file is bin\Debug\wave.exe with size 32.80 KB
Process terminated with status 0 (0 minute(s), 0 second(s))
0 error(s), 0 warning(s) (0 minute(s), 0 second(s))
Skriv svar