[C] [Sprawdź mój kod] AS5040 - biblioteka

[Hudyvolt]

Wpadł mi do głowy taki pomysł, aby jakimś stopniu poprawić swoje kodzenie. Każdy z nas uczy się programowania na swój sposób i napisany kod jest lepszy albo gorszy. Zazwyczaj poprzestaje się na etapie kiedy kod działa. Warto się jednak rozwijać, aby kod nie tylko działał, ale także był optymalny oraz czytelny dla innych. Aby to osiągnąć najlepszą chyba metodą jest dać ten kod komuś do oceny. Lepsi programiści mogą wspomóc konstruktywną krytyką i poradami, a słabsi pytaniami, które mogą podsunąć jakieś pomysły jak kod ulepszyć.

Zacznę od siebie, ostatnio napisałem prostą bibliotekę do odczytu kąta absolutnego z enkodera AS5040.

uC: XMEGA 32MHz

język: C

środowisko: Atmel Studio

Na początek datasheet układu: www.ams.com/eng/content/download/1285/7214

Kod wygląda tak:

AS5040.h

/*
* AS5040.h
*
* Created: 2014-11-01 14:13:43
*  Author: Patryk Antończyk
*/ 


#ifndef AS5040_H_
#define AS5040_H_

#define CSnPORT		PORTC
#define CSnPIN		PIN0_bm
#define CLKPORT		PORTC
#define CLKPIN		PIN1_bm
#define DOPORT		PORTC
#define DOPIN		PIN2_bm

#define CSn_LO		CSnPORT.OUTCLR = CSnPIN
#define CSn_HI		CSnPORT.OUTSET = CSnPIN
#define CLK_LO		CLKPORT.OUTCLR = CLKPIN
#define CLK_HI		CLKPORT.OUTSET = CLKPIN

#define tCSn		0.05
#define tCLK_FE		0.6
#define tCLK2		0.2
#define tCLK2_1		0.2
#define tCLK2_2		0.33

void AS5040_Init();
uint16_t AS5040_Read10bit();

#endif /* AS5040_H_ */

AS5040.c

/*
* AS5040.c
*
* Created: 2014-11-01 14:13:34
*  Author: Patryk Antończyk
*/ 

#include <avr/io.h>
#include <util/delay.h>
#include "AS5040.h"

void AS5040_Init()
{
CSnPORT.DIRSET = CSnPIN;
CLKPORT.DIRSET = CLKPIN;
DOPORT.DIRCLR = DOPIN;

CSn_HI;
CLK_HI;
}

uint16_t AS5040_Read10bit()
{
uint16_t angle=0;
CLK_HI;
CSn_HI;
_delay_us(tCSn);
CSn_LO;
_delay_us(tCLK_FE);
CLK_LO;
_delay_us(tCLK2);
for (uint8_t i=0; i<10; i++)
{
	CLK_HI;
	_delay_us(tCLK2_1);
	if (PORTD.IN & DOPIN) angle |= (1 << (9-i));
	CLK_LO;
	_delay_us(tCLK2_2);
}
CSn_HI;
CLK_HI;
return angle;
}

Ostatnio też bawiłem się troszkę nowo nabytym oscyloskopem i dobrałem wartości opóźnień tak aby uzyskać jak najkrótsze dopuszczalne czasy przebiegów (stąd dziwne wartości stałych w delayach).

Osiągnięty czas odczytu 10bitów kąt to 12.4us.

OK. Mam nadzieję, że łapiecie ideę. Liczę na wszelkie komentarze dotyczące kodu, od tabulacji, enterów itd. (pliki w załączniku), przez nazwy zmiennych, po algorytm itd.

AS5040.c

AS5040.h

[MirekCz]

Skoro nikt nie podjął tematu to może ja spróbuję:

+ Stylistyka kodu

+ Ogólny podział na funkcje itd.

- Kompletny brak komentarzy

-- Skąd ktoś ma wiedzieć co robią poszczególne funkcje?

-- Jak wrócisz do kodu za rok będziesz pamiętał co to jest tCLK2, tCLK2_1, tCLK2_2 i skąd tam takie wartości?

- Nie rozumiem konstrukcji z delay_us. Skoro masz procesor 32MHz (nie znam xmega, założe w najprostszy sposób że wykonuje 32mln. instrukcji i każda instrukcja to jeden cykl zegara).

Co teraz ma zrobić instrukcja _delay_us(0,05) ? Zegar 32mhz/0,05us=1,8 instrukcji zegara.

Do tego dolicz, że sam delay musi ustawić licznik i potem zrobić pętle, więc pewnie w najmniejszej wersji zajmuje 5-6 instrukcji... to co Ty robisz w tej chwili to zgadywanie.

Większość Twoich delay jest bardzo krótkich, więc możesz po prostu zastąpić je serią instrukcji nop o ile zależy Tobie na minimalnym czasie odczytu.

- Masz konstrukcję

       CLK_HI; 
       _delay_us(tCLK2_1); 
       if (PORTD.IN & DOPIN) angle |= (1 << (9-i)); 
       CLK_LO; 
       _delay_us(tCLK2_2); 

zwróć uwagę, że jak warunek jest spełniony to wykonujesz dużo dodatkowego kodu, a potem delay jest taki sam. Jak zależy Tobie na czasie możesz to zoptymalizować:

       CLK_HI; 
       _delay_us(tCLK2_1); 
       if (PORTD.IN & DOPIN) 
       {
            CLK_LO; 
            angle |= (1 << (9-i)); 
            nop; //wywołanie assemblera - tyle ile trzeba
       }
       else
       {
            CLK_LO; 
            nop;nop;nop; //wywołanie assemblera - tyle ile trzeba
       }

No i ostatnia uwaga - nigdzie w kodzie nie piszesz, że to jest zoptymalizowane dla zegara 32MHz...

Reasumując - komentarze, komentarze i jeszcze raz komentarze. Reszta to już detale techniczne do przemyślenia, ale nie mają w tym wypadku krytycznego znaczenia skoro kod działa 😉

[Hudyvolt]

MirekCz, dziękuję za odpowiedź.

- Kompletny brak komentarzy

-- Skąd ktoś ma wiedzieć co robią poszczególne funkcje?

-- Jak wrócisz do kodu za rok będziesz pamiętał co to jest tCLK2, tCLK2_1, tCLK2_2 i skąd tam takie wartości?

Komentarzy brak to prawda. Po wysłaniu posta się o tym zorientowałem i stwierdziłem, że już nie będę poprawiał. Mam też zły nawyk właśnie ich nie pisania, bo wydaje mi się, że kod jest na tyle prosty, że będę wiedział o co chodzi.

Nazwy tych czasów są takie same jak w DataSheecie, ale racja, warto dodać opisy.

- Nie rozumiem konstrukcji z delay_us. Skoro masz procesor 32MHz (nie znam xmega, założe w najprostszy sposób że wykonuje 32mln. instrukcji i każda instrukcja to jeden cykl zegara).

Co teraz ma zrobić instrukcja _delay_us(0,05) ? Zegar 32mhz/0,05us=1,8 instrukcji zegara.

Do tego dolicz, że sam delay musi ustawić licznik i potem zrobić pętle, więc pewnie w najmniejszej wersji zajmuje 5-6 instrukcji... to co Ty robisz w tej chwili to zgadywanie.

Tak jak wspomniałem, właśnie w tej kwestii bawiłem się oscyloskopem. Tak dobierałem wartości tych delayów aby czasy spełniały minimum z datasheeta. Nie zastanawiałem się też nad przeliczaniem tego na ilość instrukcji. Muszę to przeanalizować.

I rzecz najważniejsza, na którą zwróciłeś uwagę i pozwoliła mi dostrzec poważny błąd, który popełniłem. Chodzi o wyrównanie czasów, w zależności od wyniku instrukcji warunkowej.

Na początek drobne usprawiedliwienie. Jak widać w początkowym komentarzu bibliotekę napisałem w listopadzie. Wtedy jeszcze na zegarze 2MHz i do sprawdzania sygnału używałem taniego analizatora stanów logicznych. Wtedy widziałem właśnie te różnice w czasach poszczególnych sygnałów w zależności czy enkoder zwracał jedynkę czy zero. W grudniu kupiłem sobie oscyloskop i zacząłem się nim "bawić" właśnie analizując ten kod, ale już przy 32MHz i przede wszystkim przy podłączeniu enkodera do innych pinów (w międzyczasie układ był rozebrany).

Teraz problem różnych czasów nie występował i pomyślałem, że są tak małe, że ich nie widać z powodu zwiększenia częstotliwości zegara.

Tak nie było, a czasy były równe, ponieważ IF zawsze odczytywał zero. Ja głupi też nie sprawdzałem czy funkcja zwraca dobre wartości tylko patrzyłem na oscyloskop.

Dlaczego?

if (PORTD.IN & DOPIN) angle |= (1 << (9-i)); 

Odczyt jest z pinu na porcie D. Enkoder mam teraz podłączony do portu C. Zamiast PORTD.IN, powinno być DOPORT.IN (z definicji zawierającej odpowiedni port).

Po poprawce sygnał wygląda tak.

Widać tutaj znaczące różnice w długości stanu wysokiego i niskiego sygnał CLK (niebieski) w zależności od przesyłanego bitu z enkodera (DO - różowy sygnał). Różnica jest także w zależności od tego, który to bit z kolei (zmienna ilość przesunięć bitowych).

Po dodaniu komentarzy, poprawieniu IF, kod wygląda następująco:

/*
* AS5040.h
*
* Created: 2014-11-01 14:13:43
*  Author: Patryk Antończyk
*/ 

//UWAGA! Biblioteka przystosowana do pracy z zegarem 32MHz

#ifndef AS5040_H_
#define AS5040_H_

//Definicje portów i pinów, do którym podłączone są linie CS, CLK oraz DO
//Należy zmienić zgodnie z rzeczywistym układem
#define CSn_PORT	PORTC
#define CSn_PIN		PIN0_bm						
#define CLK_PORT	PORTC						
#define CLK_PIN		PIN1_bm						
#define DO_PORT		PORTC
#define DO_PIN		PIN2_bm

//Definicje do ustawiania stanów linii wyjściowych
#define CSn_LO		CSn_PORT.OUTCLR = CSn_PIN
#define CSn_HI		CSn_PORT.OUTSET = CSn_PIN
#define CLK_LO		CLK_PORT.OUTCLR = CLK_PIN
#define CLK_HI		CLK_PORT.OUTSET = CLK_PIN

//Definicje czasów 
#define tCSn		0.05			//stan wysoki sygnału CS (min. 500ns)
#define tCLK_FE		0.6				//opóźnienie pomiędzy zboczem opadającym CS, a opadającym CLK (min. 500ns)
#define tCLK2		0.2				//pierwszy stan niski CLK (min. 500ns)
#define tCLK2_1		0.2				//stan wysoki CLK (min. 500ns)
#define tCLK2_2		0.33			//stan niski CLK (min. 500ns)

void AS5040_Init();					//funkcja inicjalizująca porty mikrokontrolera 
uint16_t AS5040_Read10bit();		//funkcja zwraca 10bitowy kąt enkodera

#endif /* AS5040_H_ */

/*
* AS5040.c
*
* Created: 2014-11-01 14:13:34
*  Author: Patryk Antończyk
*/ 

#include <avr/io.h>
#include <util/delay.h>
#include "AS5040.h"

void AS5040_Init()
{
//ustaw linie CS oraz CLK jako wyjścia, DO jako wejście
CSn_PORT.DIRSET = CSn_PIN;
CLK_PORT.DIRSET = CLK_PIN;				
DO_PORT.DIRCLR = DO_PIN;

//ustaw stan wysoki na liniach CS oraz CLK
CSn_HI;
CLK_HI;
}

uint16_t AS5040_Read10bit()
{
uint16_t angle=0;
//rozpoczęcie transmisji
CLK_HI;
CSn_HI;
_delay_us(tCSn);
CSn_LO;
_delay_us(tCLK_FE);
CLK_LO;
_delay_us(tCLK2);
//generowanie sygnału CLK oraz odczyt bitów
for (uint8_t i=0; i<10; i++)
{
	CLK_HI;
	_delay_us(tCLK2_1);
	if (DO_PORT.IN & DO_PIN) angle |= (1 << (9-i));
	CLK_LO;
	_delay_us(tCLK2_2);
}
CSn_HI;
CLK_HI;
return angle;
}

Jak będzie czas poprawię to wyrównane sygnały zegarowego, bo jak zauważyłeś chciałbym uzyskać jak najkrótszy czas odczytu.

Jeszcze raz dziękuję za uwagi i zachęcam do dalszej dyskusji.

AS5040.c

AS5040.h