[CPLD] [VHDL] UART Modul - Code Durchsicht

[TokyoDrift (Gast)]

Hallo,

ich bin ganz neu in VHDL und habe mir zum üben ein UART Modul 
geschrieben. Man kann nicht viel einstellen, nur die Baudrate, ansonsten 
arbeitet das ganze mit 8N1. Es ist Synthetisierbar und läuft auf meinem 
Pollin CPLD Board wunderbar (Xilinx XC95144XL).
Allerdings bin ich mir SEHR unsicher ob mein VHDL Stil angemessen ist 
oder ob das ganze nur aus Glück funktioniert und beim Synthetisieren 
eigentlich viel zu viel Hardware benötigt. Es wäre also sehr nett wenn 
jemand meinen Code überfliegen könnte und mir sagen ob das so wie es ist 
passt.

Vielen Dank :)
TD

http://pastebin.com/EJGsivx0 - UART_RX
http://pastebin.com/cT6QdFbL - UART_TX
http://pastebin.com/jyBumkNz - CLK_GEN

Zu UART_RX: POP geht für einen Takt auf 1 wenn ein Byte empfangen wurde.
Zu UART_TX: PUSH muss kurz auf 1 geschaltet werden damit der Inhalt von 
INPUT gesendet wird.

[Falk B. (falk)]

@  TokyoDrift (Gast)

>arbeitet das ganze mit 8N1. Es ist Synthetisierbar und läuft auf meinem
>Pollin CPLD Board wunderbar (Xilinx XC95144XL).

Schon mal gut.

>http://pastebin.com/EJGsivx0 - UART_RX

Etwas unorthodox aber OK.

outbuf(bit_cnt) <= RX_PIN;

Das wird ein Multiplexer. Kann man machen, ist aber nicht wirklich 
notwendig. Ein Schieberegister ist einfacher, schneller, sinnvoller

outbuf <= outbuf(7 downto 1) & RX_PIN;

>SIGNAL outbuf  : STD_LOGIC_VECTOR (8 downto 1);

Was soll das? Vektoren gehen meistens bis 0 runter. Wir sind Ingenieure, 
die wissen dass Null eine sinnvolle Zahl ist.

>http://pastebin.com/cT6QdFbL - UART_TX

Hier das Gleiche. Ein Schieberegister ist sinnvoller.

>http://pastebin.com/jyBumkNz - CLK_GEN

Ganz falsch. Siehe Taktung FPGA/CPLD.

>Zu UART_RX: POP geht für einen Takt auf 1 wenn ein Byte empfangen wurde.

Ist meistens nicht sinnvoll. Besser ein Handshake. Sprich, das Signal 
wird gesetzt, wenn ein Byte empfangen wurde. Das nachfolgende Modul 
quittiert das Signal und übernimmt die Daten, dabei wird das Signal 
wieder gelöscht.

>Zu UART_TX: PUSH muss kurz auf 1 geschaltet werden damit der Inhalt von
>INPUT gesendet wird.

Ist OK. Aber hier fehlt ein Statussignal, ob der Sender neue Daten 
annehmen kann oder nicht.

Aber für den Anfang ist das alles erstmal OK.

MFG
Falk

[TokyoDrift (Gast)]

outbuf <= outbuf(7 downto 1) & RX_PIN;

Das ist gut, ich wusste nicht dass das geht :).

Falk Brunner schrieb:
>>SIGNAL outbuf  : STD_LOGIC_VECTOR (8 downto 1);
>
> Was soll das? Vektoren gehen meistens bis 0 runter. Wir sind Ingenieure,
> die wissen dass Null eine sinnvolle Zahl ist.

Das habe ich so gemacht weil beim ersten bit (sprich eigentlich 
outbuf(0)) bit_cnt schon 1 ist weil ich ja das startbit mit zähle. So 
muss ich von bit_cnt nicht wieder eins abziehen. Mit dem code von oben 
kann ich das ja ändern :).

Falk Brunner schrieb:
>>http://pastebin.com/jyBumkNz - CLK_GEN
>
> Ganz falsch. Siehe Taktung FPGA/CPLD.

Werde ich mir durchlesen.

Falk Brunner schrieb:
>>Zu UART_RX: POP geht für einen Takt auf 1 wenn ein Byte empfangen wurde.
> Ist meistens nicht sinnvoll. Besser ein Handshake. Sprich, das Signal
> wird gesetzt, wenn ein Byte empfangen wurde. Das nachfolgende Modul
> quittiert das Signal und übernimmt die Daten, dabei wird das Signal
> wieder gelöscht.

klingt logisch, werde ich einbinden

Falk Brunner schrieb:
>>Zu UART_TX: PUSH muss kurz auf 1 geschaltet werden damit der Inhalt von
>>INPUT gesendet wird.
>Ist OK. Aber hier fehlt ein Statussignal, ob der Sender neue Daten
>annehmen kann oder nicht.

auch klar, werde ich auch machen

Vielen Dank für die schnelle Antwort.
TD

[TokyoDrift (Gast)]

Jetzt muss ich nochmal nachfragen, was in dem Artikel steht ist ja alles 
einleuchtend aber wenn ich den Code anschaue:

process(clk)
begin
  if rising_edge(clk) then
    if cnt=cnt_div-1 then
      ce  <= '1';
      cnt <= 0;
    else
      ce  <= '0';
      cnt <= cnt +1 ;
    end if;
  end if;
end process;

dann meine ich, dass ce für 14  (schnelle) Takte low und nur für einen 
high ist, dann hätte ich ja immer nur ganz kurze high phasen. Stimmt das 
oder habe ich mich da jetzt vertan?
Wenn ja, wie wärs mit

process(clk)
begin
  if rising_edge(clk) then
    if cnt=cnt_div-1 then
      ce <= NOT(ce);
      cnt <= 0;
    else
     cnt <= cnt + 1;
    end if;
  end if;
end process;

?
TD

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

> Das wird ein Multiplexer. Kann man machen, ist aber nicht wirklich
> notwendig. Ein Schieberegister ist einfacher, schneller, sinnvoller
In einem CPLD nicht unbedingt...
Ich habe das für SPI mal ausprobiert. Fazit: durch die mächtige Logik 
kann u.U. ein MUX effizienter implementiert werden.

> Es ist Synthetisierbar und läuft auf meinem
> Pollin CPLD Board wunderbar (Xilinx XC95144XL).
Wieviel Platz brauchst du? Ich hätte da eine Schieberegister-Lösung, die 
über ein Xilinx XC9536 ein 8-Bit-IO-Modul darstellt: 
http://www.lothar-miller.de/s9y/categories/49-RS232-IO

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

> dann hätte ich ja immer nur ganz kurze high phasen.
Richtig, das ganze Design wird mit dem einen globalen Takt angesteuert, 
und aus dem Taktgenerator kommen nur jeweils 1 Taktzyklus lang gültige 
Impulse.

Wenn also irgendwo rising_edge() sthet, dann steht in der Klammer 
immer der gleiche Takt. Und die Auswertung der Clock-Enables sieht 
dann so aus:

process(clk)
begin
  if rising_edge(clk) then
    if ce='1' then
      -- tu was
    end if;
  end if;
end process;

[TokyoDrift (Gast)]

>> Das wird ein Multiplexer. Kann man machen, ist aber nicht wirklich
>> notwendig. Ein Schieberegister ist einfacher, schneller, sinnvoller
>In einem CPLD nicht unbedingt...
>Ich habe das für SPI mal ausprobiert. Fazit: durch die mächtige Logik
>kann u.U. ein MUX effizienter implementiert werden.
Sprich, das muss man letztendlich ausprobieren? Mhm werd ich mir merken.

>Richtig, das ganze Design wird mit dem einen globalen Takt angesteuert,
>und aus dem Taktgenerator kommen nur jeweils 1 Taktzyklus lang gültige
>Impulse.
Ja genau, ich will aber doch meistens 50% high und 50% low phase in 
meinem takt oder?

>Wenn also irgendwo rising_edge() sthet, dann steht in der Klammer
>immer der gleiche Takt. Und die Auswertung der Clock-Enables sieht
>dann so aus:
Ja das ist mir klar.

TD

[Falk B. (falk)]

@  TokyoDrift (Gast)

>Ja genau, ich will aber doch meistens 50% high und 50% low phase in
>meinem takt oder?

Das hast du auch, nämlich in deinem schnellen Systemtakt mit ??? MHz. 
Das Clock Enable hat ein beliebig geringes Tastverhältnis und das ist 
voll OK.

MfG
Falk

[TokyoDrift (Gast)]

Falk Brunner schrieb:
>>Ja genau, ich will aber doch meistens 50% high und 50% low phase in
>>meinem takt oder?
>
> Das hast du auch, nämlich in deinem schnellen Systemtakt mit ??? MHz.
> Das Clock Enable hat ein beliebig geringes Tastverhältnis und das ist
> voll OK.

ja stimmt
ich stell mir n taktsignal nur immer so als 50:50 Signal vor. Aber da 
muss ich ja eigentlich nichtmal umdenken weil die rising_edges sind ja 
gleich weit voneinander entfernt, egal wie das verhältnis von high zu 
low ist...
Gut, ich werde meinen code dann morgen entsprechend überarbeiten :)
Vielen Dank nochmal für die ganze Hilfe, ich hoffe dass das mit VHDL 
noch was wird für mich ^^
TD

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

> Aber da muss ich ja eigentlich nichtmal umdenken weil die
> rising_edges sind ja gleich weit voneinander entfernt,
> egal wie das verhältnis von high zu low ist...
Doch, du bist von der richtigen Denkweise noch recht weit weg...  :-o
Die Flanke des Enable-Signals ist absolut uninteressant. Interessant 
ist der Pegel des Enable-Signals zum Zeitpunkt der Flanke des
Taktsignals. Hier mal der Ablauf eines Zählers, der mit einem 
Enable-Signal getaktet wird. Der Zählerstand ändert sich niemals mit 
der Enable-Flanke.

          __    __    __    __    __    __    __    __    __    __    __
clk    __|  |__|  |__|  |__|  |__|  |__|  |__|  |__|  |__|  |__|  |__|  |
                     ____    _____          __            _________ 
en    ______________|    |__|     |________|  |__________|         |___
                     *           *           *                 * 
      ______________  ___________ ____________ _________________ ________
cnt   ___0___________X___1_______X___2________X____3____________X___4____

> die rising_edges sind ja gleich weit voneinander entfernt
eine Abfrage auf rising_edge() findet sowieso immer auf den selben 
(System-)Takt statt, niemals auf einen heruntergeteilten Takt. Du 
handelst dir mit dieser rising_edges()-Denkweise sogar sofort einen Takt 
Latency ein, denn in der Praxis dauert das Enable-Signal immer (ziemlich 
genau) einen Taktzyklus lang. Und die steigende Flanke des 
Enable-Signals kommt aber sofort nach der Flanke eines Taktsignals (#). 
Der Pegel des Enable-Signals wird aber erst mit der nächsten aktiven 
Taktflanke des Systemtakts abgefragt und ausgewertet (*)...

          __    __    __    __    __    __    __    __    __    __    __
clk    __|  |__|  |__|  |__|  |__|  |__|  |__|  |__|  |__|  |__|  |__|  |
                 _____                   _____             _____ 
en    __________|     |_________________|     |___________|     |___
                #    *                  #    *            #    * 
      ______________ ________________________ _________________ ________
cnt   ___0___________X___1___________________X____2____________X___3____

[TokyoDrift (Gast)]

>> Aber da muss ich ja eigentlich nichtmal umdenken weil die
>> rising_edges sind ja gleich weit voneinander entfernt,
>> egal wie das verhältnis von high zu low ist...
>Doch, du bist von der richtigen Denkweise noch recht weit weg...  :-o
>Die Flanke des Enable-Signals ist absolut uninteressant. Interessant
>ist der Pegel des Enable-Signals zum Zeitpunkt der Flanke des
>Taktsignals. Hier mal der Ablauf eines Zählers, der mit einem
>Enable-Signal getaktet wird. Der Zählerstand ändert sich niemals mit
>der Enable-Flanke.

Tut mir leid, ich habe mich wohl falsch ausgedrückt.
Was ich mit rising_edge gemeint habe ist nicht die rising_edge(ce) 
sondern quasi RISING_EDGE(ce AND rising_edge(clk)).

Also:

process(clk)
begin
  if rising_edge(clk) then
    if cnt=cnt_div-1 then
      ce  <= '1';
      cnt <= 0;
    else
      ce  <= '0';
      cnt <= cnt +1 ;
    end if;
  end if;
end process;

process(clk)
begin
  if rising_edge(clk) then
    if ce='1' then
      OUT <= '1';
    else
      OUT <= '0';
    end if;
  end if;
end process;

Dann sieht das OUT signal ja im grunde so aus:

           _            _            _            _
__________| |__________| |__________| |__________| |

Aber die Periode ist ja gleich wie bei

       _____        _____        _____        _____
______|     |______|     |______|     |______|     |

Sprich die Abstände der RISING_EDGEs der Kombination von CLK und CE sind 
gleich. :)


Und der eine Takt Latenz wurde ja schon durch "if cnt=cnt_div-1 
then...".
Ich hoffe ich hab das so richtig verstanden :).
TD

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

> sondern quasi RISING_EDGE(ce AND rising_edge(clk)).
Wie definierst du die steigende Flanke einer steigenden Flanke?  :-o

> sondern quasi RISING_EDGE(ce AND rising_edge(clk)).
Dieser Fall wird niemals eintreten, weil niemals zwei Flanken 
gleichzeitig kommen werden. Vielmehr gilt nur das, was in der Klammer 
steht und so könnte man das tatsächlich auch schreiben:

process(clk)
begin
  if rising_edge(clk) and enable='1' then
     -- mach was, wenn du dazu enabled/befähigt bist...
  end if;
end process;

> Dann sieht das OUT signal ja im grunde so aus:
Das stimmt. Der aktive Pegel des CE-Signals dauert genau 1 Taktzyklus 
lang.

[TokyoDrift (Gast)]

Das mit dem Takt ist klar. Jetzt habe ich aber ein anderes Problem. Ich 
krieg das mit dem Handshake nicht hin.
Also ich hab 2 Signale:

DATA_RECEIVED : out STD_LOGIC --1 IF A BYTE WAS RECEIVED
DATA_READ : inout STD_LOGIC; --SET TO 1 IF BYTE READ FROM OUTPUT

Wenn jetzt das letzte Bit gesendet wurde setze ich DATA_RECEIVED auf 1, 
das ist auch klar. Jetzt soll von extern DATA_READ auf 1 gesetzt werden 
und wenn das der fall ist sollen beide auf 0 gesetzt werden.
Also in der RX Architecture habe ich jetzt

IF DATA_READ = '1' THEN
  DATA_RECEIVED <= '0';
  DATA_READ <= '0';
END IF;

Das Problem ist jetzt wenn ich von außen DATA_READ manipulieren will 
meint das Programm nur "Multi source unit blahblah;  this signal is 
connected to multiple drivers.".
Ich versteh schon dass es nicht geht das eine Signal gleichzeitig auf 1 
und 0 zu ziehen. Ich weiß nur nicht wie ich es sonst machen soll.
Ich meine, ich muss das DATA_READ Signal ja auch wieder löschen, sonst 
kann ich ja das nächste byte nicht quittieren.
TD

[Falk B. (falk)]

@TokyoDrift (Gast)

>DATA_RECEIVED : out STD_LOGIC --1 IF A BYTE WAS RECEIVED
>DATA_READ : inout STD_LOGIC; --SET TO 1 IF BYTE READ FROM OUTPUT

Innerhalb eines FPGAs verwendet man keine INOUTs, nur IN oder OUT.

>das ist auch klar. Jetzt soll von extern DATA_READ auf 1 gesetzt werden
>und wenn das der fall ist sollen beide auf 0 gesetzt werden.

Nöö, nur dein DATA_RECEIVED wird auf Null gesetzt. DATA_READ ist ein IN.
Oder sollte es zumindest sein.

MfG
Falk

[TokyoDrift (Gast)]

>Nöö, nur dein DATA_RECEIVED wird auf Null gesetzt. DATA_READ ist ein IN.
>Oder sollte es zumindest sein.

Das funktioniert aber dann nicht so wie ich das grad denke o_O:
- received und read ist 0
- byte wird empfangen
- received wird auf 1 gesetzt
- von außen wird read auf 1 gesetzt
- received wird auf 0 gesetzt
Wenn ich READ nicht auch auf 0 zurücksetze bleibt es ja 1 und beim 
nächsten mal wenn RECEIVED 1 wird, wird es sofort wieder 0 weil READ ja 
immernoch auf 1 steht und die Daten so scheinbar gelesen wurden, was 
aber nicht stimmt.

Irgendwo ist da ein Denkfehler, aber ich weiß nicht wo.
TD

[Falk B. (falk)]

@  TokyoDrift (Gast)

>Wenn ich READ nicht auch auf 0 zurücksetze bleibt es ja 1 und beim

Eben das macht man nicht. Read wird nur für einen Takt auf 1 gesetzt.

MFG
Falk

[TokyoDrift (Gast)]

>>Wenn ich READ nicht auch auf 0 zurücksetze bleibt es ja 1 und beim
>Eben das macht man nicht. Read wird nur für einen Takt auf 1 gesetzt.
Achso. Das ist ja dann relativ umständlich. Ich kenns nur vom AVR zB. 
so, dass ich garnichts setzen muss, sondern er automatisch beim Auslesen 
oder Schreiben eines Registers das merkt und entsprechend eine Aktion 
durchführt. Da ich hier ja letztendlich auch Hardware bastle dachte ich 
das kann man auch so oder so ähnlich machen.
Aber gut, dann geht das ja ganz flott zum Umsetzen.
TD

[TokyoDrift (Gast)]

So, jetzt hab ich alles überarbeitet, mit Handshake im RX Modul, READY 
Signal im TX Modul, verbessertem CLK_GEN.
Wär super wenn nochmal jemand drübergucken könnte, geht grundsätzlich, 
allerdings wird das erste gesendete Byte falsch gesendet, ab dem 2. 
funktionierts wunderbar.

CLK_GEN: http://pastebin.com/zLFVNqiX
UART_RX: http://pastebin.com/kFwQwvwK
UART_TX: http://pastebin.com/rQFsY8Ms

Danke :)
TD

[Falk B. (falk)]

@  TokyoDrift (Gast)

>CLK_GEN: http://pastebin.com/zLFVNqiX

Soweit OK, aber CLK_OUt ist ein irreführender Name. Besser CE_OUT.
PRESCALE sollte man besser als generic nutzen, das vereinfacht die Logik 
(Vergleich mit Konstante anstatt Variable).

>UART_RX: http://pastebin.com/kFwQwvwK

>IF RISING_EDGE(CLK) AND CE='1' THEN

Geht das bei deinem VHD-Compiler? Das musste ich früher immer in zwei 
If-Anweisungen packen.

Deine Handhabung von data_received ist noch unsauber. Was passiert, wenn 
ein weiteres empfangen wird, das alte aber noch nicht abgeholt ist? Dann 
wird das alte Zeichen langsam überschrieben. Wenn dann irgendwann 
zwischendrin Daten abgeholt werden, kommt totaler Datenmüll raus. 
Ausserdem ist sie falsch platziert, sie steht ausserhalb des IF 
RISING_EDGE(CLK) Blocks. Aha, also kein fehlerfrei compilierter Code, 
sondern nur hingeschrieben. Deine Formatierung ist ausserdem defekt, 
zumindest auf der Website. DATA_RECEIVED als integer ist nicht sinnvoll, 
std_logic_vector ist OK.

>UART_TX: http://pastebin.com/rQFsY8Ms

Hier ähnlich.

Mfg
Falk

[TokyoDrift (Gast)]

>PRESCALE sollte man besser als generic nutzen, das vereinfacht die Logik
>(Vergleich mit Konstante anstatt Variable).
Oh, das kenne ich noch nicht, werde ich mir anschauen.

>>IF RISING_EDGE(CLK) AND CE='1' THEN
>Geht das bei deinem VHD-Compiler? Das musste ich früher immer in zwei
>If-Anweisungen packen.
Ja das geht bei mir. Macht aber ja auch keinen großen Unterschied ob man 
das nun in 2 Anweisungen packt oder nicht.

>Deine Handhabung von data_received ist noch unsauber. Was passiert, wenn
>ein weiteres empfangen wird, das alte aber noch nicht abgeholt ist? Dann
>wird das alte Zeichen langsam überschrieben. Wenn dann irgendwann
>zwischendrin Daten abgeholt werden, kommt totaler Datenmüll raus.
Ja stimmt, das habe ich vergessen, am besten lasse ich ihn neue daten 
einfach verwerfen wenn die Daten nicht abgeholt wurden.

>Ausserdem ist sie falsch platziert, sie steht ausserhalb des IF
>RISING_EDGE(CLK) Blocks.
Ja genau. Ich wusste nicht wie ich das sonst machen soll. Der teil hat 
ja eigentlich nichts mit dem Takt zu tun, wenn ich ihn aber in einen 
eigenen Prozess schreibe meint mein Compiler dass das DATA_RECEIVED 
Signal mehrere Treiber hätte und bricht ab.

>Aha, also kein fehlerfrei compilierter Code,
>sondern nur hingeschrieben.
Naja, grundsätzlich gehts ja, nur das mit dem ersten Byte, allerdings 
bekomme ich auch keine Warnungen oder so als Hinweis, und ich versteh 
nicht wieso die Daten das erste mal falsch sind. Wenn ich ein Signal 
erstelle, aber keinen Wert zuweise dann ist der Wert doch automatisch 0 
oder?

>Deine Formatierung ist ausserdem defekt,
>zumindest auf der Website.
Ja das tut mir leid, in meiner IDE siehts gut aus, dann werde ich das 
nächste mal einfach die vhd Dateien hochladen.

>DATA_RECEIVED als integer ist nicht sinnvoll,
>std_logic_vector ist OK.
DATA_RECEIVED ist doch ein STD_LOGIC. Meinst du OUTPUT? Okay, das ist ja 
nicht schwer zu ändern. :)

mfg
TD

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

@ Falk
>>IF RISING_EDGE(CLK) AND CE='1' THEN
> Geht das bei deinem VHD-Compiler? Das musste ich früher immer in zwei
> If-Anweisungen packen.
Die Compiler bekommen heute sogar noch abstrusere Kostrukte gebacken:
http://www.lothar-miller.de/s9y/categories/6-Clock-Enable

Sehr elegant und ganz ohne Prozess schreibt sich z.B. ein 
Schieberegister auch so:

   sr <= sr(sr'left-1 downto 0) & Input when rising_edge(clk);