Skip to content
Snippets Groups Projects
Commit ca29f870 authored by Paolo Baesso's avatar Paolo Baesso
Browse files

More tidying up. Expanded trigger logic section.

parent 36362ca6
Branches
Tags
No related merge requests found
Showing with 83 additions and 54 deletions
......@@ -5,30 +5,33 @@
1
Main_TLU.tex
12
10
12
6
Main_TLU.tex
TeX
135278587 0 -1 1149 -1 1153 75 75 1448 573 0 1 193 323 -1 -1 0 0 43 -1 -1 43 1 0 1153 -1 0 -1 0
ch_TLU_triggerInputs.tex
TeX
17838075 0 -1 22273 -1 22523 100 100 1473 598 0 1 825 714 -1 -1 0 0 48 -1 -1 48 1 0 22523 -1 0 -1 0
135278587 0 -1 4564 -1 1364 75 75 1448 573 0 1 49 680 -1 -1 0 0 43 -1 -1 43 1 0 1364 -1 0 -1 0
ch_TLU_Preparation.tex
TeX
1060859 0 -1 2896 -1 2869 150 150 1523 648 0 1 49 408 -1 -1 0 0 43 -1 -1 43 1 0 2869 -1 0 -1 0
1060859 0 -1 2711 -1 2715 150 150 1523 648 0 1 577 357 -1 -1 0 0 43 -1 -1 43 1 0 2715 -1 0 -1 0
O:\LatexFiles\Glossary\myGlossary.tex
TeX
1060859 0 -1 638 -1 660 150 150 2063 827 0 1 401 -358 -1 -1 0 0 61 -1 -1 61 1 0 660 -1 0 0 0
1060859 0 -1 638 -1 660 150 150 2063 827 0 1 401 187 -1 -1 0 0 61 -1 -1 61 1 0 660 -1 0 0 0
ch_TLU_Hardware.tex
TeX
1060859 1 -1 441 -1 433 200 200 1573 698 0 1 345 68 -1 -1 0 0 41 -1 -1 41 1 0 433 -1 0 -1 0
1060859 0 -1 5277 -1 5288 200 200 1573 698 0 1 777 493 -1 -1 0 0 41 -1 -1 41 1 0 5288 -1 0 -1 0
ch_TLU_clock.tex
TeX
1060859 0 -1 914 -1 1840 100 100 1473 598 0 1 89 255 -1 -1 0 0 31 -1 -1 31 1 0 1840 -1 0 -1 0
1060859 0 -1 914 -1 1896 100 100 1473 598 0 1 1721 204 -1 -1 0 0 31 -1 -1 31 1 0 1896 -1 0 -1 0
DUT_signals.tex
TeX
269496315 0 -1 291 -1 313 75 75 1448 573 0 1 105 34 -1 -1 0 0 42 -1 -1 42 1 0 313 -1 0 -1 0
1060859 0 -1 291 -1 313 75 75 1448 573 0 1 105 34 -1 -1 0 0 42 -1 -1 42 1 0 313 -1 0 -1 0
ch_TLU_triggerInputs.tex
TeX
286273531 0 -1 26319 -1 26326 100 100 1473 598 0 1 1185 918 -1 -1 0 0 48 -1 -1 48 1 0 26326 -1 0 -1 0
ch_EventBuffer.tex
TeX
1060859 0 -1 96 -1 565 125 125 1498 623 0 1 41 221 -1 -1 0 0 44 -1 -1 44 1 0 565 -1 0 -1 0
ch_TLU_Functions.tex
TeX
1060859 0 -1 1975 -1 3888 0 0 1386 472 0 1 233 561 -1 -1 0 0 39 -1 -1 39 1 0 3888 -1 0 -1 0
......@@ -37,13 +40,10 @@ TeX
17838075 0 -1 9482 -1 9218 25 25 1411 497 0 1 169 510 -1 -1 0 0 45 -1 -1 45 1 0 9218 -1 0 -1 0
ch_EUDAQParameters.tex
TeX
1060859 0 -1 1968 -1 1990 175 175 2088 852 0 1 1865 323 -1 -1 0 0 -1 -1 -1 -1 1 0 1990 -1 0 -1 0
1060859 1 -1 33 -1 44 175 175 2088 852 0 1 393 0 -1 -1 0 0 45 -1 -1 45 1 0 44 -1 0 -1 0
ch_TLU_Appendix.tex
TeX
1060859 4 -1 36 -1 37 175 175 1548 673 0 1 337 0 -1 -1 0 0 37 -1 -1 37 1 0 37 -1 0 -1 0
ch_EventBuffer.tex
TeX
1060859 0 -1 545 -1 563 125 125 1498 623 0 1 41 204 -1 -1 0 0 44 -1 -1 44 1 0 563 -1 0 -1 0
1060859 0 -1 189 -1 194 175 175 1548 673 0 1 377 51 -1 -1 0 0 37 -1 -1 37 1 0 194 -1 0 -1 0
*Main_TLU.tex
>
......
File added
File added
Documentation/Latex/Images/tlu_trigger_logic.png

15.2 KiB

This diff is collapsed.
No preview for this file type
......@@ -38,6 +38,7 @@
\usepackage{listings} %Include code
\usepackage{multirow}
\usepackage{datetime}
%\usepackage{svg}
\definecolor{infobackground}{RGB}{217,237,247}
\definecolor{infoforeground}{RGB}{58,135,173}
......@@ -180,10 +181,10 @@ Paolo Baesso - \monthname, \the\year\newline paolo.baesso@bristol.ac.uk
\include{DUT_signals}
\include{ch_TLU_triggerInputs}
\include{ch_EventBuffer}
\include{ch_TLU_Appendix}
\include{ch_TLU_Functions}
\include{ch_TLU_IPBusRegs}
\include{ch_EUDAQParameters}
\include{ch_TLU_Appendix}
%\begin{figure}[h]
% \centering
......
......@@ -10,3 +10,27 @@ Reading from \verb|EventFifoCSR| returns the following:
\item bit 4: \gls{fifo} programmable full flag
\item other bits: 0
\end{itemize}
The status register (SerdesRst) is as follows:
\begin{itemize}
\item bit 0: reset the ISERDES
\item bit 1: reset the trigger counters
\item bit 2: calibrate IDELAY: This seems to be disconnected at the moment.
\item bit 3: fixed to 0
\item bit 4, 5: status of \verb|thresholdDeserializer(Input0)|. When the IDELAY modules (prompt, delayed) have reached the correct delay, these two bits should read 00.
\item bit 6, 7: status of \verb|thresholdDeserializer(Input1)|
\item bit 8, 9: status of \verb|thresholdDeserializer(Input2)|
\item bit 10, 11: status of \verb|thresholdDeserializer(Input3)|
\item bit 12, 13: status of \verb|thresholdDeserializer(Input4)|
\item bit 14, 15: status of \verb|thresholdDeserializer(Input5)|
\item bit 16, 19: fixed to 0
\item bit 20: \verb|s_deserialized_threshold_data(Input0)(7)|
\item bit 21: \verb|s_deserialized_threshold_data(Input1)(7)|
\item bit 22: \verb|s_deserialized_threshold_data(Input2)(7)|
\item bit 23: \verb|s_deserialized_threshold_data(Input3)(7)|
\item bit 24: \verb|s_deserialized_threshold_data(Input4)(7)|
\item bit 25: \verb|s_deserialized_threshold_data(Input5)(7)|
\end{itemize}
9 bits are used to determine trigger edges. 8 are from the deserializers, 1 is added as the LSB and is the MSB from the previous word.
\ No newline at end of file
\chapter{Appendix}\label{ch:appendix}
\includepdf[link,pages={1}]{./Docs/PM3TopView.pdf}
\includepdf[link,pages=-, angle=90]{./Docs/Connections.pdf}
\ No newline at end of file
\includepdf[link,pages=-, angle=90]{./Docs/Connections.pdf}
\includepdf[link,pages=-, angle=90]{./Docs/schematics.pdf}
\ No newline at end of file
......@@ -3,11 +3,11 @@ Board \brd is an evolution of the miniTLU designed at the \gls{uob}. The board s
\section{Inputs and interfaces}\label{ch:hwDUT}
\subsubsection{FMC}
The board must be plugged onto a \gls{fmc} carrier board with an \gls{fpga} in order to function correctly. The connection is achieved using a low pin count \gls{fmc} connector. The list of the pins used is provided in appendix at page~\pageref{ch:appendix}.\\
The board must be plugged onto a \gls{fmc} carrier board with an \gls{fpga} in order to function correctly. The connection is achieved using a low pin count \gls{fmc} connector. The list of the pins used and the corresponding signal within the \gls{fpga} are provided in appendix at page~\pageref{ch:appendix}.\\
\subsubsection{Device under test}\label{ch:dut}
The \gls{dut}s are connected to the \gls{tlu} using standard size \gls{hdmi} connectors\footnote{In the miniTLU hardware there were mini\gls{hdmi} connectors.}.\\
In this version of the hardware, up to four \gls{dut}s can be connected to the board. In this document the connectors will be referred to as \verb|HDMI1|, \verb|HDMI2|, \verb|HDMI3| and \verb|HDMI4|.\\
In the current version of the hardware, up to four \gls{dut}s can be connected to the board. In this document the connectors will be referred to as \verb|HDMI1|, \verb|HDMI2|, \verb|HDMI3| and \verb|HDMI4|.\\
The connectors expect 3.3~V \gls{lvds} signals and are bi-directional, i.e. any differential pair can be configured to be an output (signal from the TLU to the DUT) or an input (signals from the DUT to the TLU) by using half-duplex line transceivers. Figure~\ref{fig:LVDSTransceiver} illustrates how the differential pairs are connected to the transceivers.
\begin{alertinfo}{Note}
The input part of the transceiver is configured to be always on. This means that signals going \emph{into} the \gls{tlu} are always routed to the logic (\gls{fpga}). By contrast, the output transceivers have to be enabled and are off by default: signal sent from the logic to the \gls{dut}s cannot reach the devices unless the corresponding enable signal is active.
......@@ -47,7 +47,7 @@ The enable signals can be configured by programming two \gls{gpio} bus expanders
\centering
\includegraphics[width=.80\textwidth]{./Images/LVDS_transceiver.pdf}
\caption{Internal configuration of the HDMI pins for the DUTs. The path from the DUT to the FPGA is always active. The path from the FPGA to the DUT can be enabled or disabled by the user.}\label{fig:LVDSTransceiver}
\end{figure}
\end{figure}\\
In terms for functionalities, the four \gls{hdmi} connectors are identical with one exception: the clock signal from \verb|HDMI4| can be used as reference for the clock generator chip mounted on the hardware. For more details on this functionality refer to section~\ref{ch:clock}.
\section{Clock LEMO}
......@@ -78,7 +78,8 @@ The correspondence between DAC slave and thresholds is shown in table~\ref{tab:D
\section{I$^{2}$C slaves}\label{ch:i2c}
The \gls{i2c} interface on the \brd can be used to configured several features of the board. Table~\ref{tab:I2C addresses} lists all the valid addresses and the corresponding slave on the board. The Enclustra lines refer to slaves located on the PM3 board; these slaves can be ignored with the exception of the bus expander. The Enclustra expander is used to enable/disable the \gls{i2c} lines going to the \gls{fmc} connector.
The \gls{i2c} interface on the \brd can be used to configured several features of the board.\\
Table~\ref{tab:I2C addresses} lists all the valid addresses and the corresponding slave on the board. The Enclustra lines refer to slaves located on the PM3 board; these slaves can be ignored with the exception of the bus expander. The Enclustra expander is used to enable/disable the \gls{i2c} lines going to the \gls{fmc} connector.
\begin{alertinfo}{Note}
After a power cycle the Enclustra expander is configured to disable the \gls{i2c} interface pins. This means that it is impossible to communicate to any \gls{i2c} slave on the \gls{tlu} until the expander has been enabled.\\
The interface is enable by setting bit 7 to 0 on register 0x01 of the Enclustra expander.
......@@ -95,7 +96,8 @@ The \gls{i2c} interface on the \brd can be used to configured several features o
IC5 & 24AA025E48T & EEPROM & 0x50 \\ \hline
IC6 & PCA9539PW & I2C Expander1 & 0x74 \\ \hline
IC7 & PCA9539PW & I2C Expander2 & 0x75 \\ \hline
IC8\_9 & Si5345A & Clock Generator & 0x68 \\ \hline
IC8 & ADN2814ACPZ & CDR & 0x60 \\ \hline
IC8\_9 & Si5345A & Clock Generator & 0x68 \\ \hline
\multicolumn{4}{|l|}{Enclustra slaves} \\ \hline
& & Enclustra Bus Expander & 0x21 \\ \hline
& & Enclustra System Monitor & 0x21 \\ \hline
......@@ -117,6 +119,9 @@ The identifier is always in the form: \verb|0xD8 80 39 XX XX XX| with the top th
\subsubsection{Bus expander}
The expanders are used as electronic switched to enable and disable individual lines. Each expander has two 8-bit banks; the values of the bits, as well as their direction (input/output) can be configured via the \gls{i2c} interface. For the purpose of the \gls{tlu}, all the expander pins should be configured as outputs since they must drive the enable signals on the \gls{dut} transceivers.
\subsubsection{Clock and data recovery chip}
The \gls{cdr} is used in conjunction with the \gls{sfp} cage to recover data and clock from the incoming bit stream. The functionality has not yet been implemented in the firmware so the \gls{i2c} slave can be ignored for now.
\subsubsection{Clock generator}
The clock for \brd can be generated using various external or internal references (see section~\ref{ch:clock} for further details). In order to reduce any jitter from the clock source and to provide a stable clock, the board hosts a Si5345 clock generator that needs to be configured via \gls{i2c} interface.\\
The configuration involves writing $\thicksim$380 register values. A configuration file, containing all the register addresses and the corresponding values, can be generated using the ClockBuilder tool available from \href{http://www.enclustra.com/en/home/}{Silicon Labs}.\\
......
......@@ -16,7 +16,7 @@ At the time of writing this work\footnote{Oct 2017} the AX3 is the only \gls{fpg
\section{Power}
The \gls{tlu} requires 12~V to operate. Power can be provided using the circular jack on the back panel of the unit.\\
During normal operation the current drawn by the unit is about ??A.
During normal operation the current drawn by the unit is about 0.5~A.
%\section{Preparation}
%Before powering the \gls{tlu} it is necessary to follow a few steps to ensure the board and the \gls{fpga} work correctly.\\
%
......
......@@ -6,7 +6,7 @@ The \gls{dut}s can receive the clock either from the Si5435A or directly from th
The firmware uses the clock generated by the Si5345A except for the block \verb|enclustra_ax3_pm3_infra| which relies on a crystal mounted on the Enclustra board to provide the IPBus functionalities (in this way, at power up the board can communicate via IPBus even if the Si5345A is not configured).
\section{Input selection}
The Si5345 has four inputs that can be selected to provide the clock alignment; the selection can be automatic or user-defined.
The Si5345 has four inputs that can be selected to provide the clock alignment; the selection can be automatic or user-defined. For further details on this aspect the user should consult the chip documentation.
\begin{table}[]
\small
......
\section{Trigger inputs}\label{ch:triggerinputs}
The status register (SerdesRst) is as follows:
\begin{itemize}
\item bit 0: reset the ISERDES
\item bit 1: reset the trigger counters
\item bit 2: calibrate IDELAY: This seems to be disconnected at the moment.
\item bit 3: fixed to 0
\item bit 4, 5: status of \verb|thresholdDeserializer(Input0)|. When the IDELAY modules (prompt, delayed) have reached the correct delay, these two bits should read 00.
\item bit 6, 7: status of \verb|thresholdDeserializer(Input1)|
\item bit 8, 9: status of \verb|thresholdDeserializer(Input2)|
\item bit 10, 11: status of \verb|thresholdDeserializer(Input3)|
\item bit 12, 13: status of \verb|thresholdDeserializer(Input4)|
\item bit 14, 15: status of \verb|thresholdDeserializer(Input5)|
\item bit 16, 19: fixed to 0
\item bit 20: \verb|s_deserialized_threshold_data(Input0)(7)|
\item bit 21: \verb|s_deserialized_threshold_data(Input1)(7)|
\item bit 22: \verb|s_deserialized_threshold_data(Input2)(7)|
\item bit 23: \verb|s_deserialized_threshold_data(Input3)(7)|
\item bit 24: \verb|s_deserialized_threshold_data(Input4)(7)|
\item bit 25: \verb|s_deserialized_threshold_data(Input5)(7)|
\end{itemize}
9 bits are used to determine trigger edges. 8 are from the deserializers, 1 is added as the LSB and is the MSB from the previous word.
\chapter{Trigger inputs}\label{ch:triggerinputs}
The six inputs on the \gls{tlu} can be used to generate a global trigger that is then issued to all the \gls{dut}s.\\
Each input has a programmable voltage discriminator that can be configured in the range [-1.3 : 1.3]~V.\\
All the inputs are protected by clamping diodes that limit the input voltage in the range [-5 : +5]~V.
\section{Trigger logic}\label{ch:triggerLogic}
The TLU has six trigger inputs than can be used to generate a valid trigger event. The number of possible different trigger combinations is $2^6= 64$ so a 64-bit word can be used to decide the valid combinations. In the hardware the 64-bit word is split into two 32-bit words (indicated as \gls{msb} and \gls{lsb} word) and the rules to generate the trigger can be specified by the user by writing in the two 32-bit registers \verb|TriggerPattern_highW| and \verb|TriggerPattern_lowW|: the first stores the 32 most significative bits of the trigger word, the latter stores the least significative bits.\\
......@@ -127,7 +108,7 @@ DEC & I5 & I4 & I3 & I2 & I1 & I0 & PATTERN & \multicolumn{1}{l|}{\begin{tabular
\end{alertinfo}
\subsubsection{Example}
In this example we have connected a pulser to two inputs of the \gls{tlu}, namely input 0 and input 4. The inputs fire with a small, random delay with respect to each other.\\
In this example we have connected a pulser to two inputs of the \gls{tlu}, namely \verb|IN_1| and \verb|IN_5|. The inputs fire with a small, random delay with respect to each other.\\
In order to ensure that the signals overlap adequately, we use the \emph{stretch} register (see chapter~\ref{ch:triggerLogic}) to increase the length of the pulses: we extend \emph{in0} to 10 clock cycles and \emph{in4} to 8 clock cycles, where the clock has a frequency of 160~MHz. The resulting signals are shown in figure~\ref{Fig:exampleExtendedTriggers}.
\begin{figure}
\centering
......@@ -137,26 +118,39 @@ In order to ensure that the signals overlap adequately, we use the \emph{stretch
\end{figure}\\
We can now define the trigger logic to be used to assert a valid trigger: we only consider the lower 32-bits of the trigger word and see how different values can produce very different results.
\begin{itemize}
\item Trigger \gls{lsb} word= 0x00020000. This indicates that the only valid trigger combination occurs when both \emph{in0} and \emph{in4} are high. The valid trigger goes high 1 clock cycle after this condition is met and remains high up to 1 clock cycle after the condition is no longer valid. This is illustrated in figure~\ref{Fig:exampleTrig00020000}.
\item Trigger \gls{lsb} word= 0x00020000. This indicates that the only valid trigger combination occurs when both \verb|IN_1| and \verb|IN_5| are high. The valid trigger goes high 1 clock cycle after this condition is met and remains high up to 1 clock cycle after the condition is no longer valid. This is illustrated in figure~\ref{Fig:exampleTrig00020000}.
\begin{figure}
\centering
\includegraphics[width=.90\textwidth]{./Images/Trigger0x00020000.png}
\caption{Trigger configuration 0x00020000. The valid trigger (blue) is asserted only when both signals are high. This condition occurs at frame 39. The trigger is asserted on the following frame.}
\label{Fig:exampleTrig00020000}
\end{figure}\\
\item Trigger \gls{lsb} word= 0x00020002. This indicates that a valid trigger is achieved in two separated configurations (in logic OR): when both inputs are high at the same time (as in the previous case) or if \emph{in0} is active on its own. This is illustrated in figure~\ref{Fig:exampleTrig00020002}. It can be seen that the valid trigger is asserted immediately one clock cycle after \emph{in0} is high and remains high as long as this condition is met. One might assume that specifying the combination with input 4 is redundant, but the following example should show that this is not the case.
\item Trigger \gls{lsb} word= 0x00020002. This indicates that a valid trigger is achieved in two separated configurations (in logic OR): when both inputs are high at the same time (as in the previous case) or if \verb|IN_1| is active on its own. This is illustrated in figure~\ref{Fig:exampleTrig00020002}. It can be seen that the valid trigger is asserted immediately one clock cycle after \verb|IN_1| is high and remains high as long as this condition is met. One might assume that specifying the combination with \verb|IN_5| is redundant, but the following example should show that this is not the case.
\begin{figure}
\centering
\includegraphics[width=.90\textwidth]{./Images/Trigger0x00020002.png}
\caption{Trigger configuration 0x00020002. The valid trigger (blue) is asserted if \emph{in0} is high OR when \emph{in0} and \emph{in4} are both high at the same time.}
\caption{Trigger configuration 0x00020002. The valid trigger (blue) is asserted if \texttt{IN\_1} is high OR when \texttt{IN\_1} and \texttt{IN\_5} are both high at the same time.}
\label{Fig:exampleTrig00020002}
\end{figure}\\
\item Trigger \gls{lsb} word= 0x00000002. This indicates that the only valid configuration is the one where only \emph{in0} is high. It is important to understand that in this configuration all other inputs act as veto. This might produce unexpected results if the user is not careful\footnote{Specifically, pulse stretch, pulse delay and trigger logic must be configured correctly to avoid unwanted results.}.\\
In figure~\ref{Fig:exampleTrig00000002} it is possible to see that the logic produces two separated trigger valid pulses, both shorter than the ones in previous examples: the first one is due to \emph{in0} going high while \emph{in4} is low. As soon as \emph{in4} goes high, the trigger condition is no longer met. When \emph{in4} returns low, a trigger condition is met again because \emph{in0} is still high. In this specific case, the double pulse is caused by the different width of the pulses.
\item Trigger \gls{lsb} word= 0x00000002. This indicates that the only valid configuration is the one where only \verb|IN_1| is high. It is important to understand that in this configuration all other inputs act as veto. This might produce unexpected results if the user is not careful\footnote{Specifically, pulse stretch, pulse delay and trigger logic must be configured correctly to avoid unwanted results.}.\\
In figure~\ref{Fig:exampleTrig00000002} it is possible to see that the logic produces two separated trigger valid pulses, both shorter than the ones in previous examples: the first one is due to \verb|IN_1| going high while \verb|IN_5| is low. As soon as \verb|IN_5| goes high, the trigger condition is no longer met. When \verb|IN_5| returns low, a trigger condition is met again because \verb|IN_1| is still high. In this specific case, the double pulse is caused by the different width of the pulses.
\begin{figure}
\centering
\includegraphics[width=.90\textwidth]{./Images/Trigger0x00000002.png}
\caption{Trigger configuration 0x00000002. The valid trigger (blue) is asserted only when \emph{in0} is active on its own. As such, two separated trigger pulses are produced because \emph{in4} goes high and returns low before \emph{in0}.}
\caption{Trigger configuration 0x00000002. The valid trigger (blue) is asserted only when \texttt{IN\_1} is active on its own. As such, two separated trigger pulses are produced because \texttt{IN\_5} goes high and returns low before \texttt{IN\_1}.}
\label{Fig:exampleTrig00000002}
\end{figure}\\
\end{itemize}
\section{Stretch and delay}
The trigger logic is designed to detect edge transitions\footnote{Currently only negative edges are registered.} at the trigger inputs and produce a pulse for each transition detected. The pulse has an initial duration of one clock cycle (f= 160~MHz, one cycle 6.25~ns) and occurs on the next rising edge of the 160~MHz internal clock.\\
Each pulse can be stretched and delayed in integer numbers of clock cycles to compensate for differences in cable length. Two separate 5-bit registers are used for the task: the value written in the registers will stretch/delay the pulse by a corresponding number of clock cycles.\\
Diagram~\ref{Fig:trigger_stretchdelay} shows the effect of the delay and stretch words on the trigger logic.
\begin{figure}
\centering
\includegraphics[width=.90\textwidth]{./Images/tlu_trigger_logic.pdf}
%\includesvg[width=.90\textwidth]{./Images/tlu_trigger_logic.svg}
\caption{Effect of the stretch and delay values. In1 is delayed by 2 clock cycles (t1= 12.5~ns) and stretched by 5 clock cycles (t2= 31.25~ns) to create a coincidence window with in5 and produce the resulting trigger signal.}
\label{Fig:trigger_stretchdelay}
\end{figure}\\
Further details on how to configure the stretch and delay values are provided in section~\ref{ch:EUDAQPar}.
\ No newline at end of file
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment