Led Matrix - Verbesserung/Tip/Rat zu Design

[kleiens Licht (Gast)]

Ich arbeite mich langsam weiter in vhdl ein.
Mein momentanes Ziel ist Pong auf einem VGA Monitor - da ist der Weg 
noch weit.
Um weiter Erfahrung zu sammeln, habe ich vor eine 4x4 ledmatrix zu löten 
und dort Muster zeitgerecht drauf auszugeben. Die ist noch nicht fertig, 
somit wird erst simuliert.
Das Design unten sieht auch gut aus, ABER eine Frage habe ich. -daß- 
Variablen für Einsteiger Probleme darstellen habe ich des öfteren 
gelesen. Daß sie besser vermieden werden sollen auch, aber wie würde ich 
mein Mini Design angehen, damit ich in den steig komplexeren Designs 
(wenn das überhaupt geht) Fallen vermeide?

library IEEE;
use IEEE.STD_LOGIC_1164.ALL;
use IEEE.NUMERIC_STD.ALL;


entity leddisp is
   Generic(clkdelay : integer := 2);
    Port ( clk : in  STD_LOGIC;
           a : out  STD_LOGIC_VECTOR(3 downto 0);
           k : out  STD_LOGIC_VECTOR(3 downto 0));
end leddisp;

architecture Behavioral of leddisp is
signal ai : STD_LOGIC_VECTOR(3 downto 0) := "0111";
signal ki : STD_LOGIC_VECTOR(3 downto 0) := "1000";
signal clkcnt : integer range 0 to clkdelay := 0;
signal state : integer range 0 to 3 := 0;
begin

main_proc : process(clk) 
variable cnt : integer range 0 to 3 := 0;
begin
  if( rising_edge(clk) ) then
    if(clkcnt < clkdelay) then
      clkcnt <= clkcnt + 1;
    else
      clkcnt <= 0;  
      cnt := cnt + 1;
      
      case state is
        when 0 => ai <= ai(0)&ai(3 downto 1); if(cnt=3)then state<=1; cnt := 0; end if;
        when 1 => ki <= ki(0)&ki(3 downto 1); if(cnt=3)then state<=2; cnt := 0; end if;
        when 2 => ai <= ai(2 downto 0)&ai(3); if(cnt=3)then state<=3; cnt := 0; end if;
        when 3 => ki <= ki(2 downto 0)&ki(3); if(cnt=3)then state<=0; cnt := 0; end if;
      end case;
    
    end if;
  end if;
end process main_proc;

a <= ai;
k <= ki;

end Behavioral;

Ich mache das allein zu Hause, kein Team, keine Kollegen kein Mentor - 
mit meinen Problemen wäre ich allein, würde sie folglich ohne externes 
Wissen auch nciht klären können und daher bin ich sehr um Prävention 
bemüht....

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

kleiens Licht schrieb:
> Ich arbeite mich langsam weiter in vhdl ein.
Du brauchst keine Variable cnt. Warum nimmst du sie trotzdem?

> Das Design unten sieht auch gut aus
Das kann nichts kompliziertes machen, aber ich kapiere das nicht auf 
Anhieb. Und damit sieht es für mich nicht GUT aus... :-/
Nur weil alles in einer Zeile steht, wird es nicht besser.

> aber wie würde ich mein Mini Design angehen
Du solltest diese Vorteiler (clkcnt + cnt) aus der FSM herausnehmen und 
in einem eigenen Prozess nur ein Clock-Enable Flag erzeugen.

Du kannst unsigned Vektoren verwenden und die rotate_left() bzw. 
rotate_right() Funktionen aus der numeric_std.

Benenne deine Zustände nicht einfach mit Zahlen durch, sondern gib ihnen 
lesbare Namen.

Du solltest nicht jeden Zähler einfach cnt oder clkcnt bennnen. Denn 
was soll ein Zähler anderes zählen als Takte?

Einrückungen machen den Quelltext überschaubar. Das sieht aber soweit 
schon gut aus.

Schreib in einem Kommentar, was das Modul machen soll bzw. macht. Und 
auch, wo es Probleme gibt oder gegeben hat. Das hilft dir selber als 
Gedankenstütze.

[kleiens Licht (Gast)]

Ja stimmt, wenn man nicht dazu sagt, was beabsichtigt ist, kommt es 
etwas kryptisch daher.
Der Plan war als erste Übung die äußeren Leds rundlaufen zu lassen. Ein 
2d Lauflicht sozusagen.
Die Variablen benötige ich nicht. So weit versprochen - was tun? Wenn 
ich am Ende der Zeilen angekommen bin, zieht das dann eine 
Richtungsänderung nach sich, also anoden 8 mal schieben, dann wert nicht 
mehr ändern, kathoden 8 mal schieben, dann Anoden wieder zurück usw.
Die Anoden steuern pnp Transistoren und die Kathoden npn Transistoren 
darum wird eine 0 bzw 1 für die aktive Reihe durchgeschoben.
Also ich werde heute Abend versuchen, 2 Prozesse zu schreiben miteinbem 
der nur Takt teilt und einem der Zählschritte macht. Mir wäre aber an 
einer Art Bausatz-Wissen gelegen, um in Zukunft die Probleme 
kontrolliert in bekannte Teile zu zerlegen. Ich hoffe ihr, speziell 
Lothar, versteht was ich meine... Den weisen Professor ersetzen ;-)

In diesem Fall könnte ich mir vielleicht vorstellen nur einen Prozess 
zur Taktteilung und einen fürs schieben... denn wenn ich statt der 
Variable ein Signal nehme, bin ich ins schwimmen gekommen, wie oft denn 
nun zählen wegen des resultierenden Versatzes.
Außerdem habe ich unglücklicherweise das Buch zum nexys1200 Board und 
dort wird extrem viel mit Variablen gearbeitet - andere Punkte beißen 
sich z.B. auch mit den z.B. von Lothar stoisch genannten Punkten. Da 
muss ich ziemlich lange probieren, um die Beispiele umzusetzen. Und 
scheiter doch oft wegen einem Brett vorm Kopf.
Dennoch ist das Buch sehr schön, denn praktische Beispiele, wie 
Algorithmen umgesetzt werden, schnittstellen oder Ram sind rar gesäht.

[kleiens Licht (Gast)]

"Du solltest diese Vorteiler (clkcnt + cnt) aus der FSM herausnehmen und
in einem eigenen Prozess nur ein Clock-Enable Flag erzeugen.

Du kannst unsigned Vektoren verwenden und die rotate_left() bzw.
rotate_right() Funktionen aus der numeric_std.
"

also diesen rat für mich oben zusammengefasst - werde ich also in 
Angriff nehmen.

[kleiens Licht (Gast)]

Der dritte Post in Folge.... mit der Bitte um Entschuldigung.

Nun voll Tatendrang zu Hause angekommen und grandios gescheitert. Ich 
rudere GANZ weit an den Anfang denn folgendes wird durch ein "parse 
error, unexpected IDENTIFIER, expecting SEMICOLON" quittiert.
Ich komme einer Überarbeitung meines codes also nicht wirklich näher.

library IEEE;
use IEEE.STD_LOGIC_1164.ALL;
use IEEE.NUMERIC_STD.ALL;

entity led_matrix is
    Port ( spalten :out STD_LOGIC_VECTOR(7 downto 0));
end led_matrix;

architecture Behavioral of led_matrix is
signal spalten_s : unsigned (7 downto 0) := "01111111";
begin
  
spalten <= (STD_LOGIC_VECTOR)spalten_s;

end Behavioral;

P.S. Wenn ich meine Hausaufgaben erledigt habe, überlege ich dann ob ich 
das Ergebnis poste. Hilft sicher anderen Anfängern auch weiter. Und 
hoffentlich wäre das dann cniht mein 4. post in Folge

[kleiens Licht (Gast)]

library IEEE;
use IEEE.STD_LOGIC_1164.ALL;
use IEEE.NUMERIC_STD.ALL;

entity led_matrix is
   Generic(TAKT_SHIFT : integer := 2);
    Port ( spalte : out STD_LOGIC_VECTOR(7 downto 0);
        zeile : out STD_LOGIC_VECTOR(7 downto 0);
        clk   : in STD_LOGIC);
end led_matrix;

architecture Behavioral of led_matrix is
signal spalte_s : unsigned (7 downto 0) := "01111111";
signal zeile_s  : unsigned (7 downto 0) := "10000000";
-- Takterzeugung
signal shift_clk : integer range 0 to TAKT_SHIFT := 0;
signal shift_en  : bit := '0';
--Zustände
signal geshiftet : integer range 0 to 7 := 0;
type states is(ZEILE_UNTEN, SPALTE_RECHTS, ZEILE_OBEN, SPALTE_LINKS);
signal state : states := ZEILE_UNTEN;
begin

clk_enable:process(clk) begin
  if( rising_edge(clk) ) then
    if( shift_clk < TAKT_SHIFT ) then
      shift_clk <= shift_clk + 1;
      shift_en <= '0';
    else
      shift_clk <= 0;
      shift_en <= '1';
      if(geshiftet < 7) then        --ich zähle die shift operationen mit und ändere alle 8 mal den Zustand
        geshiftet <= geshiftet + 1; --warum es nicht funktioniert, z.B. in zeile_s das unterste bit anzugucken
      else                   --und wenn dieses 0 wird in SPALTE_RECHTS zu gehen weiß ich noch nicht
        geshiftet <= 0;
      end if;
    end if;
  end if;
  end process clk_enable;

leds_shiften : process(clk, shift_en) begin
  if(rising_edge(clk) and shift_en = '1') then
    case state is
      when ZEILE_UNTEN => zeile_s <= rotate_right(zeile_s, 1);
                    --if(zeile_s(0)='0') then
                    if(geshiftet = 7) then
                      state <= SPALTE_RECHTS;
                    end if;
      when SPALTE_RECHTS => spalte_s <= rotate_right(spalte_s, 1);
                    --if(spalte_s(0)='1') then
                    if(geshiftet = 7) then
                      state <= ZEILE_OBEN;
                    end if;
      when ZEILE_OBEN    => zeile_s <= rotate_left(zeile_s, 1);
                    --if(zeile_s(7)='0') then
                    if(geshiftet = 7) then
                      state <= SPALTE_LINKS;
                    end if;
      when SPALTE_LINKS  => spalte_s <=  rotate_left(spalte_s, 1);
                    --if(spalte_s(7)='0') then
                    if(geshiftet = 7) then
                      state <= ZEILE_UNTEN;
                    end if;
    end case;
  end if;
  end process leds_shiften;


zeile  <= STD_LOGIC_VECTOR(zeile_s);
spalte <= STD_LOGIC_VECTOR(spalte_s);

end Behavioral;

Ich habe versucht die Anregungen umzusetzen. Wie ihr am Code seht, bin 
ich  noch nicht rundum zufrieden, aber ich probiere weiter.
Vielen, vielen Dank an Lothar ich habe mich in diesem Thread wirklich 
nciht gerade empfohlen und werde mich jetzt erst mal in ein ganz tiefes 
Loch verkriechen :-(

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

kleiens Licht schrieb:
> Ich habe versucht die Anregungen umzusetzen.
Na bitte: geht doch...
Und jetzt das Ganze hübsch simulieren... ;-)
Dann wirst du das auch noch herausfinden:
>> warum es nicht funktioniert, z.B. in zeile_s das unterste bit anzugucken
>> und wenn dieses 0 wird in SPALTE_RECHTS zu gehen weiß ich noch nicht
Als Tipp: das hat sicher was mit der Latency und dem shift_en zu tun...

kleiens Licht schrieb:
> leds_shiften : process(clk, shift_en) begin
>   if(rising_edge(clk) and shift_en = '1') then
Schreib das besser wie "der Rest der Welt":

leds_shiften : process(clk) begin -- nur der Takt in der Sensitivliste, dann sieht jeder: voll synchron das Ding
  if rising_edge(clk) then
     if (shift_en = '1') then ...

[kleiens Licht (Gast)]

Ich habe das Design nun lauffähig bekommen. Nicht umständlich Schritte 
zu zählen ist doch definitiv schöner: einmal intuitiver und auch 
Ressourcensparender. Ich habe nur ein cpld vor mir liegen.
Die ordentlichere Variante hat zwar ungefähr 4 mal so lange wie das 
hingefetzte gedauert, aber das wird der lerneffekt hoffenltich 
mittelfristig wieder wett machen.

Dank noch einmal, aber ich habe es soeben auch mit gemischten Gefühlen 
gestartet. Einmal froh, daß es nun klappt. (Stichwort Latenz ich leite 
den Zustandswechsl nun einfach eine Shiftposition vor Endposition ein) 
aber auch mit einem schalen Gefühl, weil es mir eben (noch?) nicht 
gelingt, dieses Problem zu 100% zu durchschauen.
Das kann ja nicht die Praxis sein einfach mal aus Gefühl früher oder 
später etwas zu erledigen, weil sich da sicher irgendwas beim Ablauf 
beißt...

[Boris (Gast)]

Hallo "kleines Licht",

zeig doch mal deinen Code! Dann kann man dir vielleicht noch den ein 
oder anderen Tip geben, bzw. oder dir sagen warum es klappt.

>Das kann ja nicht die Praxis sein einfach mal aus Gefühl früher oder
>später etwas zu erledigen, weil sich da sicher irgendwas beim Ablauf
>beißt...

Da hast du Recht, dass ist nicht die Praxis... Aber als "Anfänger" (so 
wie ich bestimmt auch noch einer bin) macht man hin und wieder 
Denkfehler. Das bleicbt nicht aus.

PS: Weißt du wie dein CPLD intern Aufgebaut ist? Wenn nicht gebe ich dir 
den Tip das heraus zu finden. So kannst du Ressourcen sparen in dem du 
synthesefreundlichen VHDL Code schreibst. Auch das RTL-Schematic, 
welches du dir mit der ISE anschauen kannst sagt dir ob du das bekommst 
was du in VHDL umschrieben hast...

Gruß Boris

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

kleiens Licht schrieb:
> Ich habe das Design nun lauffähig bekommen.
Zeig doch mal deinen Code, um die Sache hier abzurunden... ;-)

> Das kann ja nicht die Praxis sein einfach mal aus Gefühl früher oder
> später etwas zu erledigen
Du wist das einfach verinnerlichen, dann ist das kein Gefühl mehr... ;-)

[kleiens Licht (Gast)]

sehr gern obwohl so 98% das Selbe ;-)

library IEEE;
use IEEE.STD_LOGIC_1164.ALL;
use IEEE.NUMERIC_STD.ALL;

entity led_matrix is
   Generic(TAKT_SHIFT : integer := (2**17)-1);
    Port ( spalte : out STD_LOGIC_VECTOR(7 downto 0);
        zeile : out STD_LOGIC_VECTOR(7 downto 0);
        clk   : in STD_LOGIC);
end led_matrix;

architecture Behavioral of led_matrix is
signal spalte_s : unsigned (7 downto 0) := "01111111";
signal zeile_s  : unsigned (7 downto 0) := "10000000";
-- Takterzeugung
signal shift_clk : integer range 0 to TAKT_SHIFT := 0;
signal shift_en  : bit := '0';
--Zustände
type states is(ZEILE_UNTEN, SPALTE_RECHTS, ZEILE_OBEN, SPALTE_LINKS);
signal state : states := ZEILE_UNTEN;
begin

clk_enable:process(clk) begin
  if( rising_edge(clk) ) then
    if( shift_clk < TAKT_SHIFT ) then
      shift_clk <= shift_clk + 1;
      shift_en <= '0';
    else
      shift_clk <= 0;
      shift_en <= '1';
    end if;
  end if;
  end process clk_enable;

leds_shiften : process(clk, shift_en) begin
  if(rising_edge(clk) and shift_en = '1') then
    case state is
      when ZEILE_UNTEN => zeile_s <= rotate_right(zeile_s, 1);
                    if(zeile_s(1)='1') then
                      state <= SPALTE_RECHTS;
                    end if;
      when SPALTE_RECHTS => spalte_s <= rotate_right(spalte_s, 1);
                    if(spalte_s(1)='0') then
                      state <= ZEILE_OBEN;
                    end if;
      when ZEILE_OBEN    => zeile_s <= rotate_left(zeile_s, 1);
                    if(zeile_s(6)='1') then
                      state <= SPALTE_LINKS;
                    end if;
      when SPALTE_LINKS  => spalte_s <=  rotate_left(spalte_s, 1);
                    if(spalte_s(6)='0') then
                      state <= ZEILE_UNTEN;
                    end if;
    end case;
  end if;
  end process leds_shiften;


zeile  <= STD_LOGIC_VECTOR( zeile_s);
spalte <= STD_LOGIC_VECTOR( spalte_s);

end Behavioral;

P.S. Der Buchstabendreher ist nicht -ganz- unbeabsichtigt, aber dennoch 
nett gemeinter Hinweis.

P.P.S sehr gute Hilfe auch für Trottel wie mich, da muss ich mich 
anmelden

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

Du bekommst das "gefühlte" Verhalten weg, wenn du quasi einen 
"voreilenden Gehorsam" in jeden State einbaust:

      when ZEILE_UNTEN => 
                    if(zeile_s(0)='1') then
                      state    <= SPALTE_RECHTS;
                      spalte_s <= rotate_right(spalte_s, 1);
                    else
                      zeile_s  <= rotate_right(zeile_s, 1);
                    end if;
      when SPALTE_RECHTS => 
                    if(spalte_s(0)='0') then
                      state    <= ZEILE_OBEN;
                      zeile_s  <= rotate_left(zeile_s, 1);
                    else 
                      spalte_s <= rotate_right(spalte_s, 1);
                    end if;
      when ZEILE_OBEN    => 
                    if(zeile_s(7)='1') then
                      state    <= SPALTE_LINKS;
                      spalte_s <=  rotate_left(spalte_s, 1);
                    else
                      zeile_s  <= rotate_left(zeile_s, 1);
                    end if;

Aber da gefällt mir deine Version eigentlich besser.

Eine andere wesentlich elegantere Möglichkeit wäre, den gesamten Prozess 
mit voller Geschwindigkeit laufen zu lassen und mit dem shift_en nicht 
den gesamten Prozess zu beeinflussen, sondern nur das Schieben (denn 
daher hat shift_en ja auch den Namen):

leds_shiften : process(clk, shift_en) begin
  if rising_edge(clk) then
    case state is
      when ZEILE_UNTEN => 
                    if (shift_en = '1') 
                      zeile_s <= rotate_right(zeile_s, 1);
                    end if;
                    if (zeile_s(0)='1') then
                      state <= SPALTE_RECHTS;
                    end if;
      when SPALTE_RECHTS => 
                    if (shift_en = '1') 
                      spalte_s <= rotate_right(spalte_s, 1);
                    end if;
                    if (spalte_s(0)='0') then
                      state <= ZEILE_OBEN;
                    end if;
      when ZEILE_OBEN    => 
                    if (shift_en = '1') 
                      zeile_s <= rotate_left(zeile_s, 1);
                    end if;
                    if (zeile_s(7)='1') then
                      state <= SPALTE_LINKS;
                    end if;
      when SPALTE_LINKS  => 
                    if (shift_en = '1') 
                      spalte_s <=  rotate_left(spalte_s, 1);
                    end if;
                    if (spalte_s(7)='0') then
                      state <= ZEILE_UNTEN;
                    end if;
    end case;
  end if;
  end process leds_shiften;

Ich würde das so machen, und das deckt sich auch eher mit deiner 
Denkweise. Und mit ein wenig Übung wird auch aus einem kleien Licht ein 
Großes...  ;-)

[kleiens Licht (Gast)]

Bei deiner letzten Lösung verschiebt sich der Zustandswechsel dann aber 
auch einen Takt hinter das shiften auf bit 0, oder?
Macht nur nichts, weil die shift_en Signale ohnehin mehrere Takte 
auseinander liegen.
Ich frage nur, um etwas mehr Verständnis reinzubekommen. Es könnte ja 
auch sein, daß nicht ein so derart runtergeteilter Takt für Aktionen 
genutzt wird, sondern z.B. eben ein Takt was ausmacht.

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

kleiens Licht schrieb:
> Bei deiner letzten Lösung verschiebt sich der Zustandswechsel dann aber
> auch einen Takt hinter das shiften auf bit 0, oder?
Ja.

> Macht nur nichts, weil die shift_en Signale ohnehin mehrere Takte
> auseinander liegen.
Sehr viele sogar...  ;-)
Mindestens 1 wäre aber wirklich nötig.

> eben ein Takt was ausmacht.
Klar: das muss man sich dabei im Hinterkopf behalten.