Added LIFT SDPFW review notes.

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..
+