Resolve L2SDP-557
Closes L2SDP-557
Merge request reports
Activity
requested review from @kooistra
assigned to @walle
192 192 ); 193 193 194 194 bsn_at_sync <= bs_sosi.bsn WHEN bs_sosi.sync = '1' ELSE bsn_at_sync_reg; -
Kun je deze bsn_at_sync, bsn_ok en out_valid niet beter in een PROCESS met IF THEN ELSE coderen. Als een WHEN statement groter wordt vind ik het erg moeilijk om na te gaan of de condities wel kloppen en alle mogelijkheden wel volledig afgedekt worden.
Bijv. waarom wordt voor bsn_at_sync (nog) niet gechecked op g_check_channel?
changed this line in version 2 of the diff
196 out_valid <= '1' WHEN in_sosi.sop = '1' AND TO_UINT(in_sosi.channel) /= g_check_channel ELSE 197 bsn_ok WHEN in_sosi.sop = '1' AND TO_UINT(in_sosi.channel) = g_check_channel ELSE 198 out_valid_reg; 193 p_bsn_check : PROCESS(bs_sosi, in_sosi, bsn_at_sync_reg, bsn_ok_reg) 194 VARIABLE v_bsn_ok : STD_LOGIC; 195 VARIABLE v_bsn_at_sync : STD_LOGIC_VECTOR(g_bsn_w-1 DOWNTO 0); 196 BEGIN 197 v_bsn_at_sync := bsn_at_sync_reg; 198 v_bsn_ok := bsn_ok_reg; 199 out_valid <= out_valid_reg; 200 201 IF bs_sosi.sync = '1' THEN 202 v_bsn_at_sync := bs_sosi.bsn; 203 END IF; 199 204 205 IF in_sosi.sync = '1' AND TO_UINT(in_sosi.channel) = g_check_channel THEN -
Deze conditie komt steeds voor: TO_UINT(in_sosi.channel) = g_check_channel. Kun je die niet als buitenste IF conditie doen, om duidelijker te maken dat er niks gebeurt als deze conditie niet TRUE is.
Zowiezo vind ik IF condities met a AND b lastig. Het kan wel, maar te overwegen alternatief is of IF a THEN if b te doen, dan kun je ook makkelijker eventueel commentaar kwijt per contitie a en conditie b.
Edited by Eric Kooistra changed this line in version 3 of the diff
187 186 -- MM registers in st_clk domain 188 187 reg_wr_arr => OPEN, 189 188 reg_rd_arr => OPEN, 190 in_reg => count_reg, -- read only 189 in_reg => count_reg, -- read only 191 190 out_reg => OPEN -- no write 192 191 ); 193 192 194 bsn_at_sync <= bs_sosi.bsn WHEN bs_sosi.sync = '1' ELSE bsn_at_sync_reg; 195 bsn_ok <= bsn_ok_reg WHEN in_sosi.sync = '0' ELSE '1' WHEN in_sosi.bsn = bsn_at_sync ELSE '0'; 196 out_valid <= '1' WHEN in_sosi.sop = '1' AND TO_UINT(in_sosi.channel) /= g_check_channel ELSE 197 bsn_ok WHEN in_sosi.sop = '1' AND TO_UINT(in_sosi.channel) = g_check_channel ELSE 198 out_valid_reg; 193 p_bsn_check : PROCESS(bs_sosi, in_sosi, bsn_at_sync_reg, bsn_ok_reg) changed this line in version 3 of the diff
202 v_bsn_at_sync := bs_sosi.bsn; 203 END IF; 199 204 205 IF in_sosi.sync = '1' AND TO_UINT(in_sosi.channel) = g_check_channel THEN 206 IF in_sosi.bsn = v_bsn_at_sync THEN 207 v_bsn_ok := '1'; 208 ELSE 209 v_bsn_ok := '0'; 210 END IF; 211 END IF; 212 213 IF in_sosi.sop = '1' THEN 214 IF TO_UINT(in_sosi.channel) = g_check_channel THEN 215 out_valid <= v_bsn_ok; 216 ELSE 217 out_valid <= '1'; changed this line in version 3 of the diff
189 in_reg => count_reg, -- read only 191 190 out_reg => OPEN -- no write 192 191 ); 193 192 194 bsn_at_sync <= bs_sosi.bsn WHEN bs_sosi.sync = '1' ELSE bsn_at_sync_reg; 195 bsn_ok <= bsn_ok_reg WHEN in_sosi.sync = '0' ELSE '1' WHEN in_sosi.bsn = bsn_at_sync ELSE '0'; 196 out_valid <= '1' WHEN in_sosi.sop = '1' AND TO_UINT(in_sosi.channel) /= g_check_channel ELSE 197 bsn_ok WHEN in_sosi.sop = '1' AND TO_UINT(in_sosi.channel) = g_check_channel ELSE 198 out_valid_reg; 193 p_bsn_check : PROCESS(bs_sosi, in_sosi, bsn_at_sync_reg, bsn_ok_reg) 194 VARIABLE v_bsn_ok : STD_LOGIC; 195 VARIABLE v_bsn_at_sync : STD_LOGIC_VECTOR(g_bsn_w-1 DOWNTO 0); 196 BEGIN 197 v_bsn_at_sync := bsn_at_sync_reg; 198 v_bsn_ok := bsn_ok_reg; 199 out_valid <= out_valid_reg; changed this line in version 4 of the diff
193 192 194 bsn_at_sync <= bs_sosi.bsn WHEN bs_sosi.sync = '1' ELSE bsn_at_sync_reg; 195 bsn_ok <= bsn_ok_reg WHEN in_sosi.sync = '0' ELSE '1' WHEN in_sosi.bsn = bsn_at_sync ELSE '0'; 196 out_valid <= '1' WHEN in_sosi.sop = '1' AND TO_UINT(in_sosi.channel) /= g_check_channel ELSE 197 bsn_ok WHEN in_sosi.sop = '1' AND TO_UINT(in_sosi.channel) = g_check_channel ELSE 198 out_valid_reg; 193 p_bsn_check : PROCESS(bs_sosi, in_sosi, bsn_at_sync_reg, bsn_ok_reg) 194 VARIABLE v_bsn_ok : STD_LOGIC; 195 VARIABLE v_bsn_at_sync : STD_LOGIC_VECTOR(g_bsn_w-1 DOWNTO 0); 196 BEGIN 197 v_bsn_at_sync := bsn_at_sync_reg; 198 v_bsn_ok := bsn_ok_reg; 199 out_valid <= out_valid_reg; 200 201 IF bs_sosi.sync = '1' THEN 202 v_bsn_at_sync := bs_sosi.bsn; -
Ik snap het gebruik van v_bsn_at_sync zo niet goed.
Kun je de variabelen uitleggen? Je gebruikt ze, omdat je niet nog een clock slag wilt wachten op de _reg value en die dan te gebruiken.
Kun je de outputs uitleggen, wat doet/is: . cnt_discarded? . out_valid? . bsn_ok? . bsn_at_sync ?
Edited by Eric Kooistra changed this line in version 4 of the diff
198 VARIABLE v_bsn_ok : STD_LOGIC; 199 -- Using v_bsn_at_sync to ensure correct behavior when in_sosi has no latency compared to bs_sosi. 200 VARIABLE v_bsn_at_sync : STD_LOGIC_VECTOR(g_bsn_w-1 DOWNTO 0); 201 BEGIN 202 v_bsn_at_sync := bsn_at_sync_reg; 203 v_bsn_ok := bsn_ok_reg; 204 out_valid <= out_valid_reg; 205 206 IF bs_sosi.sync = '1' THEN 207 v_bsn_at_sync := bs_sosi.bsn; 208 END IF; 209 210 IF TO_UINT(in_sosi.channel) = g_check_channel THEN 211 -- Compare bsn at sync of in_sosi to bsn at sync of bs_sosi. 212 IF in_sosi.sync = '1' THEN 213 IF in_sosi.bsn = v_bsn_at_sync THEN -
Liever UNSIGNED(in_sosi.bsn) = UNSIGNED(v_bsn_at_sync)
of
in_sosi.bsn(g_bsn_w-1 DOWNTO 0) = v_bsn_at_sync
Want als g-bsn_w < c_dp_stream_bsn_w = 64 dan weet ik niet zeker of slv = slv wel goed gaat voor ongelijke lengtes. Volgens mij vergelijkt UNSIGNED() waardes, ongeacht de lengte. Daarom ook liever UNSIGNED() dan:
in_sosi.bsn(g_bsn_w-1 DOWNTO 0) = v_bsn_at_sync
Edited by Eric Kooistra changed this line in version 4 of the diff
211 -- Compare bsn at sync of in_sosi to bsn at sync of bs_sosi. 212 IF in_sosi.sync = '1' THEN 213 IF in_sosi.bsn = v_bsn_at_sync THEN 214 v_bsn_ok := '1'; 215 ELSE 216 v_bsn_ok := '0'; 217 END IF; 218 END IF; 219 220 -- Setting out_valid to control the discarding at packet level. 221 IF in_sosi.sop = '1' THEN 222 out_valid <= v_bsn_ok; 223 END IF; 199 224 225 -- set cnt_discarded_en to control u_discarded_counter. 226 cnt_discarded_en <= in_sosi.sync AND NOT v_bsn_ok; -
Deze regel kan bij IF in_sosi.sync = '1' THEN deel in.
De cnt_discarded_en <= '0'; bij de ELSE hieronder moet dan naar boven als default assignment boven in het process.
Op deze manier is nog iets duidelijke dat cnt_discarded_en nooit een latch kan worden en dat het alleen voor packets met channel = g_check_channel gebruikt wordt.
changed this line in version 4 of the diff
64 63 CONSTANT c_gap_size : NATURAL := 4; 65 64 CONSTANT c_nof_sync : NATURAL := 5; 66 65 CONSTANT c_nof_blk : NATURAL := g_nof_blocks_per_sync * c_nof_sync; 66 -- choosing c_check_channel such that it corresponds with a channel from stimuli on a sync because the channel field is generated by a counter. 67 CONSTANT c_check_channel : NATURAL := g_nof_blocks_per_sync; -- can be c_check_channel MOD g_nof_blocks_per_sync = 0 but not larger than c_nof_blk. mentioned in commit 397cdaf9
unassigned @walle