From 43b8f0b6aec05314f37d7828da340b4d2da9ebad Mon Sep 17 00:00:00 2001
From: Eric Kooistra <kooistra@astron.nl>
Date: Mon, 7 Aug 2023 16:43:20 +0200
Subject: [PATCH] Added LIFT SDPFW review notes.
---
.../station2_sdp_transient_buffer.txt | 94 +++++++++++++++++++
1 file changed, 94 insertions(+)
diff --git a/applications/lofar2/doc/prestudy/station2_sdp_transient_buffer.txt b/applications/lofar2/doc/prestudy/station2_sdp_transient_buffer.txt
index b46f13b09e..d86e420c6d 100644
--- a/applications/lofar2/doc/prestudy/station2_sdp_transient_buffer.txt
+++ b/applications/lofar2/doc/prestudy/station2_sdp_transient_buffer.txt
@@ -13,6 +13,7 @@ Detailed design: Transient Buffer (TBuf) function for LIFT project
10) Planning
11) Transient detection (TDet) Design
12) Development planning
+13) Review comments
References:
@@ -574,6 +575,91 @@ The CP FPGA_beamlet_output_nof_beamlets_RW is not supported in SDPTR and SDPFW y
. write FPGA_tbuf_dump_enable_RW = False to stop dumping
+List of points:
+FPGA_tbuf_record_all_RW[N_pn]
+FPGA_tbuf_record_enable_RW[N_pn]
+FPGA_tbuf_dump_nof_bps_RW[N_pn]
+FPGA_tbuf_dump_start_timestamp_RW[N_pn]
+FPGA_tbuf_dump_time_interval_RW[N_pn]
+FPGA_tbuf_dump_enables_RW[N_pn]
+FPGA_tbuf_clear_counts_RW[N_pn]
+
+FPGA_tbuf_signal_input_rsn_R[N_pn]
+FPGA_tbuf_signal_input_nof_blocks_R[N_pn]
+FPGA_tbuf_signal_input_nof_samples_R[N_pn] - debug
+FPGA_tbuf_nof_samples_per_page_R[N_pn] - debug
+FPGA_tbuf_nof_pages_in_buffer_R[N_pn] - debug
+FPGA_tbuf_recorded_nof_pages_R[N_pn] - debug
+FPGA_tbuf_recorded_first_page_R[N_pn] - debug
+FPGA_tbuf_recorded_last_page_R[N_pn] - debug
+FPGA_tbuf_recorded_time_interval_R[N_pn]
+FPGA_tbuf_recorded_first_timestamp_R[N_pn]
+FPGA_tbuf_recorded_last_timestamp_R[N_pn] - debug
+FPGA_tbuf_dump_start_page_R[N_pn] - debug
+FPGA_tbuf_dump_nof_packets_R[N_pn] - debug
+FPGA_tbuf_dump_done_R[N_pn]
+FPGA_tbuf_memory_read_nof_packets_R[N_pn]
+FPGA_tbuf_memory_read_nof_crc_errors_R[N_pn]
+FPGA_tbuf_memory_read_nof_rsn_errors_R[N_pn]
+FPGA_tbuf_memory_dumped_nof_packets_R[N_pn]
+FPGA_tbuf_output_nof_packets_R[N_pn]
+
+FPGA_tbuf_ddr_gigabytes_R[N_pn] - debug
+FPGA_tbuf_ddr_calibrated_R[N_pn]
+FPGA_tbuf_ddr_wr_fifo_nof_full_R[N_pn]
+FPGA_tbuf_ddr_rd_fifo_nof_full_R[N_pn]
+FPGA_tbuf_ddr_wr_fifo_usedw_R[N_pn]
+FPGA_tbuf_ddr_rd_fifo_usedw_R[N_pn]
+
+FPGA_tbuf_ring_nof_transport_hops_RW[N_pn]
+FPGA_tbuf_ring_rx_clear_total_counts_RW[N_pn]
+
+FPGA_tbuf_ring_rx_total_nof_packets_received_R[N_pn]
+FPGA_tbuf_ring_rx_total_nof_packets_discarded_R[N_pn]
+
+FPGA_tbuf_output_hdr_eth_destination_mac_RW[N_pn]
+FPGA_tbuf_output_hdr_ip_destination_address_RW[N_pn]
+FPGA_tbuf_output_hdr_udp_destination_port_RW[N_pn]
+FPGA_tbuf_output_hdr_app_reserved_RW[N_pn] - debug
+FPGA_tbuf_output_enable_RW[N_pn]
+
+ICD review comments JDM (3 aug 2023):
+* FPGA_tbuf_recorded_last_page_R, FPGA_tbuf_recorded_last_timestamp_R
+ In software intervals typically are half open, [first, last). This is to be able to represent 0 pages in the buffer.
+ Maybe we should adopt that here as well, make "last" exclusive? How is 0 pages represented now?
+ EK: Ok. With circular buffer this means that last = first indicates buffer empty or full dependend on
+ nof_pages_in_buffer_R.
+
+* Clear upon read of FPGA_tbuf_ddr_wr_fifo_full_R, FPGA_tbuf_ddr_rd_fifo_full_R.
+ This behaviour is difficult to work with, since it assumes only one client will read the MP. Can we turn
+ this into a failure counter?
+ EK: Yes, changed into FPGA_tbuf_ddr_wr_fifo_nof_full_R, FPGA_tbuf_ddr_rd_fifo_nof_full_R
+
+Same with rd fifo, obviously.
+* So this variable FPGA_tbuf_clear_counts_RW basically is always True, but we have to write True to it
+ regardless to clear. That's confusing to me. It also does not allow us to see when the counters were cleared.
+ Same for the one FPGA_tbuf_ring_rx_clear_total_counts_RW below it obviously.
+ We could either have a command or have a variable that is a counter we must change.
+ EK: Ok I understand. How does a command work with OPC-UA? We have not used that yet with SC-SDP interface.
+
+* FPGA_tbuf_dump_done_R.
+ There is a risk of a race condition here where dump_enables_RW > 0 but dump_done_R is still True from a
+ previous dump. Cached by any of the layers (f.e. SDPTR or SC). We could have dump_done_R := False
+ explicitly when dump_enables_RW = 0 is written.
+ For often changing conditions, we should consider increasing counters that allow us to detect changes over
+ any time period, instead of booleans. But I suppose we don't dump that often.
+ EK: SDPFW guarantees that dump_done_R = False when dump is disabled, see Figure 3.6b in L5 SDPFW Design
+ Document: Transient buffer raw data
+
+ The whole problem with booleans for state is being vigilant (waakzaamheid) that we can
+ still infer behaviour if we don't poll often enough. We could miss TFT or FTF transitions.
+ The general solution is to introduce a counter instead. F.e. the monitoring system archives all MPs every 10s.
+ We need any dump to take >10s or we'd risk missing them entirely in our monitoring system. So, if we agree
+ dumps take >>10s, the booleans are fine. If we need to keep track of faster changes, we need counters...
+ Of course we have abilities to monitor some points more often. It's just that I like designs that are not
+ dependent on that interval (big grin)
+
+
7) TBuf ICD STAT/SDP-CEP
- LOFAR1:
@@ -947,3 +1033,11 @@ Extend tbuf_dump script with support for record all or half
Verify tbuf record all or half on hardware
+13) Review comments
+
+* 3 aug 2023
+https://support.astron.nl/confluence/display/L2M/2023-08-03+LIFT+firmware+design+review
+
+Assume > 10 s between dumps, so that default 10s monitor interval will not mis MP changes.
+LOFAR2 station will have second 10GbE port on SDP for dumping, because to CEP 100GbE will become available..
+
--
GitLab