Add sum^2 calculation for PCAP module when the pcap_std_dev option is set #68

shu-soleil · 2021-11-09T10:02:17Z

No description provided.

Co-authored-by: Tom Cobb <[email protected]>

…ster

…on check

…rts pcap_std_dev fpga_option

…ests

shu-soleil · 2021-12-03T13:52:52Z

make_zpkg (xu5_st1-no-fmc) and make_zpkg (xu5_st1-fmc_acq430) passed locally

thomascobb · 2021-12-03T14:01:32Z

yes, this is a lack of RAM in the github runners, we are looking at a different solution

modified: modules/pcap/pcap_doc.rst

- pcap_core_wrapper.vhd : define pos_bus_i and extbus_i using PBUSW and EBUSW constants - pcap.timing.ini : update to the new version of pcap with sum of squared values function

- tests/hdl/regression_tests.tcl - tests/hdl/single_test.tcl

… (if exists)

- pcap.timing.ini : add test to capture Sum^2 - top_defines.vhd : set PCAP_SUPPORTS_STD_DEV to '1' ; define t_mode_group is array(8 downto 0) of .. modified: modules/pcap/pcap.timing.ini modified: tests/hdl/top_defines.vhd

Araneidae

This pull request seems to have a lot of unexpected changes, and this is making it harder to review than it ought to be.

Firstly, there is an alarming amount of formatting churn, layout changes, extra unrelated comments, file permission changes, etc.

Secondly, and more importantly, it strikes me that a lot of this pull request involves changes which have nothing to do with simply adding three more capture attributes. I don't feel happy taking this review any further until we understand why these extra changes are in this PR.

In fact most of the files seem to have significant changes which have nothing to do with the sum^2 calculation. Are these changes part of other bug fixes ... or are they errors introduced when merging master ... or something else I'm missing.

Araneidae · 2022-01-24T09:21:53Z

modules/pcap/hdl/pcap_bus_delay.vhd

+signal enable_r1      : std_logic;
+signal trig_r1        : std_logic;
+signal gate_r1        : std_logic;
+signal bit_bus_r1     : bit_bus_t;
+signal pos_bus_r1     : pos_bus_t;
+
+-- Block parameters pipeline registers
+signal TRIG_EDGE_r1   : std_logic_vector(1 downto 0);
+signal SHIFT_SUM_r1   : std_logic_vector(5 downto 0);


Just a minor coding style question: I don't understand why these intermediate signals have been declared? Why not just assign directly to the outputs?

To me this just makes the code even more verbose than it needs to be.

modules/pcap/hdl/pcap_buffer.vhd

modules/pcap/hdl/pcap_core.vhd

modules/pcap/hdl/pcap_frame.vhd

shu-soleil · 2022-01-24T11:12:24Z

I'll try to reply and make needed corrections. Our service provider Thierry is not working on this project anymore but he is still here and can answer questions if necessary.

Araneidae · 2022-01-24T13:46:49Z

To help with managing this review, I think I'll try and flag everything that catches my eye as not part of this pull request, I'll make a separate review entry for this. The problem is there are a lot of changes here, and probably most of them, maybe all of them, are doing something import ... but if they belong here it's not in this pull request!

I presume that this pull request should be adding the three extra sum of squares fields and nothing else. However, let's not lose all the other changes -- if they need to be done, let's make separate PRs for them and get them all in. I'm just trying to get this into a state where I can review it!

Araneidae

Ok, I've gone through all of the files. It strikes me that most of the changes are either fixes that are not part of this PR (there are some important looking and subtle changes), or are style changes.

If there are bug fixes in here that are needed for this PR and aren't already in master, can we please have a separate PR for them. Otherwise it's impossible to review this PR. I'm reviewing the changes as a whole rather than the 22 individual commits, which I don't think are structured for individual review.

Araneidae · 2022-01-24T13:48:51Z

common/scripts/build_ip.tcl

+# Set project properties
+if {[info exists BOARD_PART]} {
+    set_property "board_part" $BOARD_PART [current_project]
+}
+


I don't see board_part used anywhere else in this PR, so this looks like an orphan change.

modules/pcap/hdl/pcap_buffer.vhd

Araneidae · 2022-01-24T13:51:31Z

modules/pcap/hdl/pcap_core.vhd

@@ -54,21 +54,23 @@ end pcap_core;
 architecture rtl of pcap_core is




I don't think any of the changes in this file are part of this PR, are they?

Araneidae · 2022-01-24T13:52:37Z

modules/pcap/hdl/pcap_core_wrapper.vhd

        pos_bus(i) <= pos_bus_i(i*32+31 downto i*32);
    end loop;
 end process;

 process(extbus_i) begin
-    ext_bus: for i in 0 to 11 loop
+    ext_bus: for i in 0 to EBUSW-1 loop
        extbus(i) <= extbus_i(i*32+31 downto i*32);
    end loop;
 end process;


All the changes here are sound, but I'd rather have a style PR for this kind of thing. Not part of this PR, please.

Araneidae · 2022-01-24T13:53:03Z

modules/pcap/hdl/pcap_dma.vhd

+                    if (fifo_count = 0) then
+                        pcap_fsm <= IRQ;


Significant functional change, but definitely not part of this PR.

Araneidae · 2022-01-24T13:58:13Z

modules/pcap/hdl/pcap_frame_mode.vhd

+
+  -- Mode 6 / 7 / 8 : Sum^2 Low / Middle / High
+  ps_sum_squared_val: process(clk_i)
+  begin
+    if rising_edge(clk_i) then
+
+        -- Output the result
+        if trig_r1 = '1' then
+            -- Mode 6 Sum^2 byte 0
+            sum_sq_0_o <= std_logic_vector(sum_data_sq(31+(to_integer(unsigned(SHIFT_SUM))) downto (to_integer(unsigned(SHIFT_SUM)))));
+            -- Mode 7 Sum^2 byte 1
+            sum_sq_1_o <= std_logic_vector(sum_data_sq(63+(to_integer(unsigned(SHIFT_SUM))) downto 32+(to_integer(unsigned(SHIFT_SUM)))));
+            -- Mode 8 Sum^2 byte 2
+            sum_sq_2_o <= std_logic_vector(sum_data_sq(95+(to_integer(unsigned(SHIFT_SUM))) downto 64+(to_integer(unsigned(SHIFT_SUM)))));
+        end if;
+
+        -- Clear sum on disable or trigger with no gate high
+        if (enable_r1 = '0') or (trig_r1 = '1' and gate_r1 = '0') then
+            sum_data_sq <= (others => '0');
+        -- Set sum to current value on trigger with gate high
+        elsif (trig_r1 = '1' and gate_r1 = '1') then
+            sum_data_sq <= resize(value_sq_r1,sum_data_sq'length);
        -- Sum the received data whilst gate high
-        elsif (gate_i = '1') then
-            sum_data <= sum_data + signed(value_i);
+        elsif (gate_r1 = '1') then
+            --sum_data_sq <= sum_data_sq + value_sq_r1;
+            sum_data_sq <= sum_data_sq + resize(value_sq_r1,sum_data_sq'length);


At long last. The actual core of the change.

Rather a shame that all of the controlling signals around it have changed name, making review difficult.

Again, this file has so many other distracting changes that need to be pulled out into a separate PR, please!

Araneidae · 2022-01-24T13:59:56Z

modules/pcap/hdl/pcap_top.vhd

+pcap_bus_delay_inst : entity work.pcap_bus_delay
+port map (
+    clk_i               => clk_i,
+    -- Block parameters inputs
+    TRIG_EDGE_i         => TRIG_EDGE_reg(1 downto 0),
+    SHIFT_SUM_i         => SHIFT_SUM_reg(5 downto 0),
+    -- Block inputs
+    enable_i            => enable_from_bus,
+    trig_i              => gate_from_bus,
+    gate_i              => trig_from_bus,
+    bit_bus_i           => bit_bus_i,
+    pos_bus_i           => pos_bus_i,
+    -- Block parameters outputs
+    TRIG_EDGE_o         => TRIG_EDGE,
+    SHIFT_SUM_o         => SHIFT_SUM,
+    -- Block outputs
+    enable_o            => enable_i_dyd,
+    trig_o              => trig_i_dyd,
+    gate_o              => gate_i_dyd,
+    bit_bus_o           => bit_bus_i_dyd,
+    pos_bus_o           => pos_bus_i_dyd
+);
+


Ok, we need this for timing closure (personally I'd have just written a clocked process here instead, but never mind) ... but none of the other changes are helpful.

I'd be delighted to have a style cleanup PR ... but not as part of this one.

Araneidae · 2022-01-24T14:01:16Z

tests/hdl/regression_tests.tcl

@@ -42,7 +42,7 @@ set test_passed are;
 set test_failed are;

 foreach module [lrange $argv $MODULES_IND end] {
-        source $BUILD_DIR/hdl_timing/$module/$module.tcl
+    source $BUILD_DIR/hdl_timing/$module/$module.tcl
 }


All the changes in this file look like churn or style changes.

Araneidae · 2022-01-24T14:01:49Z

tests/hdl/single_test.tcl

@@ -18,14 +18,12 @@ create_project single_test single_test -force -part $FPGA_PART
 if {$argc > $MODULES_IND} {


Don't think this file is part of this PR

Araneidae · 2022-01-24T14:02:27Z

tests/hdl/top_defines.vhd

-type t_mode_group is array (5 downto 0) of std_logic_vector(31 downto 0);
-type t_mode is array (31 downto 0) of t_mode_group;
+-- Presence of PCAP_STD_DEV functionality
+constant PCAP_SUPPORTS_STD_DEV  : std_logic := '1';


This line should be the only change in this file for this PR, please.

EmilioPeJu · 2022-04-04T12:07:02Z

I have merged master because I fixed an issue related to this, the options field in the target file was not working due to a bug in the code that processes it

Araneidae · 2022-04-07T08:07:53Z

Corresponding PandABlocks-server issue #13 now closed with push of tag 3.0a2.

Shu ZHANG and others added 14 commits October 27, 2021 17:08

add pcap_std_dev option in target.ini for top_defines entry

111b034

Update common/hdl/reg_top.vhd

7cd922a

Co-authored-by: Tom Cobb <[email protected]>

Update common/templates/top_defines.vhd.jinja2

e4c95c8

Update common/hdl/reg_top.vhd: remove BIT_PCAP_STD_DEV constant

37ab11e

Update common/hdl/reg_top.vhd: add comments on FPGA_CAPABILITIES regi…

cc4d818

…ster

fix python_tests issue

05a44a6

Update common/python/generate_app.py

90d6be1

Update common/python/generate_app.py: add VALID_FPGA_OPTIONS for opti…

1203e90

…on check

Update common/python/generate_app.py: filter empty fpga options

654667c

update PCAP : sum of squared input values generated only if APP suppo…

eb81e55

…rts pcap_std_dev fpga_option

PCAP: add pcap_bus_delay module

c8dca3b

merge from master

d04aae8

delete modules/pcap/hdl/pcap_frame_test.vhd file, only used for test

889ade6

Merge branch 'master' into featurePCAPStdDev

fae4304

shu-soleil changed the title ~~Add std_dev calculation for PCAP module~~ Add sum^2 calculation for PCAP module when the pcap_std_dev option is set Nov 9, 2021

Update tests/hdl/top_defines.vhd: add PCAP_SUPPORTS_STD_DEV for hdl_t…

2a6aa74

…ests

shu-soleil requested review from thomascobb and Araneidae and removed request for thomascobb December 2, 2021 09:55

garrel and others added 5 commits December 16, 2021 10:33

pcap_doc.rst : add Mode 6/7/8 - Sum of Squared values Lo/Mid/Hi

a17645f

modified: modules/pcap/pcap_doc.rst

update pcap_core_wrapper.vhd and pcap.timing.ini

ec3f123

- pcap_core_wrapper.vhd : define pos_bus_i and extbus_i using PBUSW and EBUSW constants - pcap.timing.ini : update to the new version of pcap with sum of squared values function

Update tests TCL scripts

1328713

- tests/hdl/regression_tests.tcl - tests/hdl/single_test.tcl

Modified common/scripts/build_ip.tcl: set BOARD_PART project property…

6573509

… (if exists)

shu-soleil requested a review from glennchid January 7, 2022 14:18

Araneidae requested changes Jan 24, 2022

View reviewed changes

remove executable mode for mode-changed files

8e93f4a

Update pcap_core.vhd

bdb57e4

Araneidae requested changes Jan 24, 2022

View reviewed changes

Shu ZHANG and others added 3 commits January 24, 2022 16:00

merge from master commit d2d4f93

68db87e

remove not PR related changes

a2f00fa

Merge branch 'master' into featurePCAPStdDev

6448862

Fix wrong mapping of trig and gate signals from bus

9b543a8

Araneidae merged commit 2dc5f16 into master Jun 29, 2022

Araneidae deleted the featurePCAPStdDev branch June 29, 2022 09:46

tomtrafford mentioned this pull request Oct 19, 2022

Fixed the PCAP python simulation tests. #92

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sum^2 calculation for PCAP module when the pcap_std_dev option is set #68

Add sum^2 calculation for PCAP module when the pcap_std_dev option is set #68

shu-soleil commented Nov 9, 2021

shu-soleil commented Dec 3, 2021

thomascobb commented Dec 3, 2021

Araneidae left a comment

Araneidae Jan 24, 2022

shu-soleil commented Jan 24, 2022

Araneidae commented Jan 24, 2022

Araneidae left a comment

Araneidae Jan 24, 2022

Araneidae Jan 24, 2022

Araneidae Jan 24, 2022

Araneidae Jan 24, 2022

Araneidae Jan 24, 2022

Araneidae Jan 24, 2022

Araneidae Jan 24, 2022

Araneidae Jan 24, 2022

Araneidae Jan 24, 2022

EmilioPeJu commented Apr 4, 2022

Araneidae commented Apr 7, 2022

		@@ -54,21 +54,23 @@ end pcap_core;
		architecture rtl of pcap_core is

		@@ -18,14 +18,12 @@ create_project single_test single_test -force -part $FPGA_PART
		if {$argc > $MODULES_IND} {

Add sum^2 calculation for PCAP module when the pcap_std_dev option is set #68

Add sum^2 calculation for PCAP module when the pcap_std_dev option is set #68

Conversation

shu-soleil commented Nov 9, 2021

shu-soleil commented Dec 3, 2021

thomascobb commented Dec 3, 2021

Araneidae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shu-soleil commented Jan 24, 2022

Araneidae commented Jan 24, 2022

Araneidae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmilioPeJu commented Apr 4, 2022

Araneidae commented Apr 7, 2022