Cut the comments back to the genuinely subtle ones

fb.go

@@ -10,11 +10,10 @@ import (
 	"golang.org/x/sys/unix"
 )
 
-// Drawing lands in a plain memory buffer and is blitted to a scanout buffer the
+// Drawing lands in memory and is blitted to a buffer the display is not
-// display is not reading, which is then swapped in whole at a vertical blank.
+// reading, then swapped in whole at a vertical blank. Writing into the live
-// Writing into the live scanout buffer instead, as the fbdev interface invites,
+// scanout buffer instead, as fbdev invites, races the beam: the blit takes a
-// races the beam: the blit takes a few hundred microseconds, and whatever the
+// few hundred microseconds and the display reads half of each frame.
-// display reads during it is part of one frame and part of the next.
 const (
 	drmIoctlBase = 0x64
 
@@ -239,7 +238,6 @@ func crtcFor(fd int, c drmModeGetConnector, crtcs []uint32) (uint32, error) {
 	return 0, fmt.Errorf("connector %d has no usable crtc", c.connectorID)
 }
 
-// The connector to drive and a crtc that can drive it.
 func findDisplay(fd int) (connID, crtcID uint32, err error) {
 	crtcs, conns, err := cardResources(fd)
 	if err != nil {
@@ -507,9 +505,8 @@ func (fb *framebuffer) blend(x, y int, c rgb, cov uint8) {
 	fb.back[o+3] = byte(v >> 24)
 }
 
-// Copies into the scanout buffer furthest from being displayed and asks for it
+// The copy cannot tear because nothing is displaying that buffer, and the swap
-// at the next blank. The copy cannot tear because nothing is displaying that
+// cannot tear because the hardware does it between frames.
-// buffer, and the swap cannot tear because the hardware does it between frames.
 func (fb *framebuffer) flush() error {
 	next := (fb.front + 1) % scanoutBuffers
 	copy(fb.bufs[next].mem, fb.back)

harness/grab.go

@@ -11,12 +11,10 @@ import (
 	"golang.org/x/sys/unix"
 )
 
-// cabletest drives the panel through drm and page flips between two scanout
+// cabletest drives the panel through drm, so /dev/fb0 holds the kernel console
-// buffers of its own, so /dev/fb0 holds the kernel console and there is nothing
+// and is not worth reading. The displayed buffer is reachable from out here
-// in it worth reading. The buffer being displayed is reachable from out here
+// instead: GETCRTC names it, and root is allowed a handle to it without being
-// though: GETCRTC names it, and root is allowed a handle to it without being
+// drm master, so it can be read without the owning process cooperating.
-// drm master, so it can be mapped and read without the program that owns it
-// having to cooperate or even notice.
 const (
 	drmCardGlob = "/dev/dri/card*"
 
@@ -95,8 +93,6 @@ type grabber struct {
 	index int
 }
 
-// The card and crtc actually putting something on a display, which is whichever
-// one cabletest chose when it set its mode.
 func openGrabber() (*grabber, error) {
 	paths, err := filepath.Glob(drmCardGlob)
 	if err != nil {
@@ -145,11 +141,9 @@ func activeCrtc(fd int) (uint32, int, error) {
 
 func (g *grabber) close() { unix.Close(g.fd) }
 
-// Reading the buffer takes about as long as a frame, so where the read starts
+// Reading a buffer takes about as long as a frame, so where the read starts in
-// in the flip cycle is what decides whether it stays ahead of the writer. Woken
+// the flip cycle decides whether it stays ahead of the writer. Woken at a
-// at a blank, the buffer named next has just gone on screen, which leaves a
+// blank, the buffer GETCRTC then names has just gone on screen.
-// whole frame plus however long cabletest spends drawing before anything
-// touches it again.
 func (g *grabber) waitVblank() error {
 	v := drmWaitVblank{
 		typ:      drmVblankRelative | uint32(g.index<<drmVblankHighCrtcShft)&drmVblankHighCrtcMask,
@@ -164,8 +158,6 @@ func (g *grabber) waitVblank() error {
 	}
 }
 
-// Maps whichever buffer is on screen right now. The caller is expected to have
-// just woken at a blank, so this is the one with the longest life ahead of it.
 func (g *grabber) mapFront() (mem []byte, pitch, pw, ph int, err error) {
 	c := drmModeCrtc{crtcID: g.crtc}
 	if err := drmIoctl(g.fd, drmGetCrtc, unsafe.Pointer(&c)); err != nil {
@@ -200,14 +192,11 @@ func (g *grabber) mapFront() (mem []byte, pitch, pw, ph int, err error) {
 	return mem, pitch, pw, ph, nil
 }
 
-// Scanout memory is uncached, so reading a frame out of it costs a good
+// Scanout memory is uncached, so a read costs a good fraction of a frame and
-// fraction of a frame's time and could in principle be overtaken by the next
+// could be overtaken by the next redraw. Read twice and accept only if the two
-// redraw. Rather than trust that it was not, the buffer is read twice and the
+// agree: the writer leaves a buffer alone for several frames after displaying
-// pair only accepted if they agree: the writer cycles through its buffers in
+// it, which is long enough for both. Disagreement means that reasoning is
-// order and leaves this one alone for several frames after putting it on
+// wrong, which is worth hearing about rather than retrying past.
-// screen, which is comfortably long enough for both reads. Disagreement means
-// that reasoning is wrong somewhere, which is worth hearing about rather than
-// papering over with another go.
 func (g *grabber) frame() (*image.NRGBA, error) {
 	if err := g.waitVblank(); err != nil {
 		return nil, fmt.Errorf("wait for vblank: %w", err)

main.go

@@ -79,9 +79,8 @@ func (h *heldValue) get(now time.Time, cur uint64) uint64 {
 	return h.v
 }
 
-// Everything the display reads, taken at one instant: the worker counters plus
+// Everything the display reads, taken at one instant, so a pair of these
-// the two that are read rather than counted, so a pair of these describes both
+// describes both the rates and the errors over the span between them.
-// the rates and the errors over the span between them.
 type counterSet struct {
 	t     time.Time
 	s     sample
@@ -95,9 +94,7 @@ func (d *direction) capture(t time.Time) counterSet {
 }
 
 // What someone testing a cable is asking, rather than how each failure happened
-// to be noticed. Corruption arrives as three different symptoms and the kernel
+// to be noticed.
-// drops frames for reasons that are ours rather than the cable's, but none of
-// that is a distinction worth reading off a panel.
 type errs struct {
 	lost     uint64
 	corrupt  uint64
@@ -318,8 +315,8 @@ var intervalCols = []colSpec{
 	{title: "LEN m", width: 6, right: true},
 }
 
-// One interval's numbers, shared by the console table and the framebuffer so
+// Shared by the console table and the framebuffer so both show the same
-// both always show the same figures.
+// figures.
 type view struct {
 	txPPS, rxPPS    float64
 	txGbps, rxGbps  float64
@@ -349,8 +346,6 @@ func errsBetween(b, n counterSet) errs {
 	}
 }
 
-// Cumulative fields, which need no rate window and are identical for both the
-// console and the display.
 func (d *direction) counters(now counterSet) view {
 	return view{
 		rxFrames: now.s.rxFrames - d.base.s.rxFrames,
@@ -395,17 +390,14 @@ func (d *direction) view(t time.Time) view {
 	return v
 }
 
-// The one place the counters are read for the ring. Runs on its own ticker, so
-// what the buckets measure does not move when the drawing does.
 func (d *direction) sample(t time.Time) {
 	d.mu.Lock()
 	d.win.push(d.capture(t))
 	d.mu.Unlock()
 }
 
-// Draws whatever the sampler last put in the ring rather than reading the
+// Draws what the sampler last put in the ring rather than reading the counters
-// counters again, so the display is a consumer of the measurement and never a
+// again, so the display never participates in the measurement.
-// participant in it.
 func (d *direction) displayView(t time.Time) view {
 	d.mu.Lock()
 	n := d.win.count()
@@ -604,21 +596,17 @@ func main() {
 
 const (
 	reportInterval = time.Second
-	// What a bucket covers. Nothing to do with how often the panel is redrawn:
+	// Deliberately not tied to the refresh: letting a slow or blocked draw set
-	// how often the counters are read is a property of the measurement, and
+	// the sampling clock would stretch the window it reports.
-	// letting the refresh set it would let a slow or blocked draw stretch the
-	// window it reports. Short enough that a step lands promptly, long enough
-	// that a bucket holds tens of thousands of frames at line rate and is not
-	// itself noise.
 	sampleInterval = 16 * time.Millisecond
-	// Both how far back the shown errors reach and how many buckets the median
+	// How far back the shown errors reach and how many buckets the median runs
-	// runs over, so a step in the rate lands half this late.
+	// over, so a step in the rate lands half this late.
 	rateWindowSpan = time.Second
 	totalsHold     = 50 * time.Millisecond
 )
 
 // One sampler for both directions, so their buckets share an instant and the
-// cable length, which needs a figure from each, is never mixing two moments.
+// cable length, which needs a figure from each, never mixes two moments.
 type sampler struct {
 	dirs []*direction
 }

render.go

@@ -89,10 +89,9 @@ func humanBytes(b uint64) string {
 	return fmt.Sprintf("%.1f PB", v)
 }
 
-// A figure with the letter for its magnitude, so the unit itself can stay a
+// The magnitude letter goes with the figure so the unit can stay a fixed word
-// fixed word on the label and only the letter moves with the value. Below a
+// on the label. Below a thousand no letter is left dangling, since a trailing
-// thousand there is no letter and none is left dangling, since a trailing space
+// space would push the figure off centre.
-// would push the figure off centre.
 func scaleSI(v float64) string {
 	for _, mag := range []string{"", "k", "M", "G", "T"} {
 		if v < 1000 {
@@ -106,8 +105,7 @@ func scaleSI(v float64) string {
 	return fmt.Sprintf("%.2f P", v)
 }
 
-// The same shape for time, whose magnitudes are sixties and twenty-fours rather
+// The same shape for time, whose magnitudes are sixties and twenty-fours.
-// than thousands. The letter changes with the value; the label does not.
 func scaleTime(d time.Duration) string {
 	switch {
 	case d < time.Minute:
@@ -242,9 +240,8 @@ func renderBox(title string, headers []string, rights []bool, rows [][]string) s
 	return b.String()
 }
 
-// Counted exactly rather than scaled: these are whole frames, and the figure
+// Exact rather than scaled: scaled, one lost frame and a thousand both read as
-// that matters most is the small one. Scaled, a single lost frame and a
+// 1.00, separated only by a letter.
-// thousand of them both read as 1.00, separated only by a letter.
 func statusCell(v uint64) string {
 	s := commas(v)
 	if v == 0 {

text.go

@@ -27,12 +27,10 @@ type textFace struct {
 	ascent int
 	cache  map[rune]*glyph
 
-	// Where a line of text starts and stops as far as the eye is concerned: the
+	// The line as the eye reads it: top of a digit down to the baseline. The
-	// top of a digit or capital, down to the baseline. The cell is taller at
+	// cell is taller at both ends, holding accent space nothing here uses and
-	// both ends, reserving space above for accents nothing here uses and below
+	// descenders that hang past the line without being read as part of it, so
-	// for descenders, which hang past the line without being read as part of
+	// laying out by the cell puts more air around text than around a box.
-	// it. Laying out by the cell therefore puts visibly more air around text
-	// than around a bordered box the same distance away.
 	capTop int
 	lineH  int
 }

ui.go

@@ -19,21 +19,15 @@ var (
 	uiRed    = rgb{0xf0, 0x6b, 0x6b}
 )
 
-// Every gap is a multiple of one step, so the spacing carries meaning: things
+// Distances between ink, since layout measures a line from the top of a digit
-// a step apart belong together, things eight steps apart do not. Picking each
+// to the baseline rather than across a cell with accent and descender slack in
-// number for itself is what produced a panel where a label could have gone with
+// it. Values carried over from spacing cells will look too small here.
-// either the figure above it or the one below.
-//
-// These are distances actually seen, since layout measures a line of text from
-// the top of a digit to the baseline rather than across a cell with accent and
-// descender slack in it. Values that looked right when that slack was padding
-// them out are too small once it is gone.
 const (
 	step = 4
 
-	spaceTight = step * 2 // neighbouring chips
+	spaceTight = step * 2
-	spaceGroup = step * 4 // a figure and its label, chip padding, block to block
+	spaceGroup = step * 4
-	spaceRow   = step * 8 // one labelled pair and the next
+	spaceRow   = step * 8
 )
 
 const (
@@ -47,13 +41,9 @@ const (
 	btnH         = 80
 	holdDuration = time.Second
 
-	// Shared by the chips and the button, which are the same object drawn at
-	// different sizes.
 	chipRadius = spaceTight
 	chipBorder = 2
 
-	// One grid for the panel: the figures and the chips beneath them stand in
-	// the same columns because they are placed by the same arithmetic.
 	gridCols   = 2
 	chipPadY   = spaceGroup
 	chipGap    = spaceTight
@@ -111,21 +101,13 @@ func newDisplay() (*display, error) {
 		return nil, fmt.Errorf("grid faces disagree on cell width: %d vs %d",
 			d.grid.cellW, d.gridB.cellW)
 	}
-	// Guessed heights collide on a screen this small, so the layout follows what
-	// the loaded faces actually measure.
 	now := []int{d.statsH(d.big, 2), d.chipsH()}
 	since := []int{d.statsH(d.gridB, 4), d.countsH(), btnH}
 
-	// One gap for the whole screen rather than one per panel: whatever is left
+	// One gap for both panels, and vertically the frame is the border alone.
-	// after the blocks is divided between every gap in both of them, so the
+	// Insetting by uiPad as well would add it to the gaps at a panel's ends but
-	// space above the first figure, between each block, and below the last is
+	// not to the ones between blocks, which is not equal spacing however evenly
-	// the same distance everywhere. Each panel is then sized to exactly the
+	// the remainder is divided.
-	// blocks it holds plus its share, which is also what puts the button in the
-	// flow instead of pinned to the bottom with the remainder above it.
-	// Vertically the frame is the border and nothing else: the gap is the only
-	// whitespace there is. Insetting by uiPad as well would add it to the gaps
-	// at the top and bottom of a panel but not to the ones between blocks,
-	// which is not equal spacing however evenly the remainder is divided.
 	gaps := len(now) + len(since) + 2
 	spare := fb.h - 2*uiMargin - blockGap - 4*uiBorder - sum(now) - sum(since)
 	if spare < 0 {
@@ -168,9 +150,8 @@ func stack(y int, hs []int, gap int) []int {
 	return ys
 }
 
-// Tracks a press and hold on the reset button, returning true once it has been
+// Lifting or sliding off cancels, and the press has to be released before it
-// held long enough. Lifting or sliding off cancels, and the press has to be
+// can arm again.
-// released before it can arm again.
 func (d *display) holdReset(x, y int, down bool, now time.Time) bool {
 	if !down {
 		d.holdStart, d.holdFrac, d.fired = time.Time{}, 0, false
@@ -191,17 +172,14 @@ func (d *display) holdReset(x, y int, down bool, now time.Time) bool {
 	return true
 }
 
-// Built like the error chips, since it sits among them: a coloured outline
+// Cyan rather than the status colours because it is something to press, not
-// around a dark well. Cyan rather than the status colours because it is
+// something being reported.
-// something to press, not something being reported.
 func (d *display) drawResetButton() {
 	r := d.resetBtn
 	d.fb.roundRect(r.x, r.y, r.w, r.h, chipRadius, uiCyan)
 	d.fb.roundRect(r.x+chipBorder, r.y+chipBorder, r.w-2*chipBorder, r.h-2*chipBorder,
 		chipRadius-chipBorder, uiBg)
 
-	// The hold fills the well rather than the whole button, so the outline stays
-	// put and it reads as the button filling up.
 	split := r.x + chipBorder
 	if d.holdFrac > 0 {
 		w := int(float64(r.w-2*chipBorder) * math.Min(d.holdFrac, 1))
@@ -229,8 +207,8 @@ func (d *display) close() {
 	d.fb.close()
 }
 
-// y is the top of the line as read, so text and a bordered box placed the same
+// y is the top of the line as read, not the top of the cell, so text and a
-// distance apart are the same distance apart to look at.
+// bordered box placed the same distance apart look it.
 func (d *display) centerIn(f *textFace, x, w, y int, s string, col rgb) int {
 	f.draw(d.fb, x+(w-len([]rune(s))*f.cellW)/2, y-f.capTop, s, col)
 	return y + f.lineH + pairGap
@@ -252,9 +230,7 @@ func (d *display) statsH(vf *textFace, n int) int {
 	return gridRows(n)*(d.statPairH(vf)+statRowGap) - statRowGap
 }
 
-// Where cell i of n falls in the panel's grid. A last row that does not fill
+// A last row that does not fill the grid is centred.
-// the grid is centred, so the odd one out balances the rows above rather than
-// hanging off the left of them.
 func gridCell(i, n, x, w int) (cx, cw int) {
 	cw = (w - (gridCols-1)*chipGap) / gridCols
 	inRow := min(n-(i/gridCols)*gridCols, gridCols)
@@ -262,10 +238,8 @@ func gridCell(i, n, x, w int) (cx, cw int) {
 	return cx, cw
 }
 
-// A figure with its label directly underneath, two to a row. The gap between
+// An empty value takes its space without drawing, so nothing below it moves
-// rows is wider than the one inside a pair, so which label belongs to which
+// when it arrives.
-// figure is a matter of spacing rather than of guessing. An empty value takes
-// its space without drawing, so nothing below moves when it arrives.
 func (d *display) stats(vf *textFace, x, w, y int, cells []statCell) int {
 	for i, c := range cells {
 		if c.value == "" {
@@ -291,7 +265,6 @@ var errRows = []struct {
 
 func (d *display) chipH() int { return d.grid.lineH + 2*chipPadY }
 
-// Taller by a line, since these carry the count under the kind.
 func (d *display) countChipH() int { return d.chipH() + d.gridB.lineH + pairGap }
 
 func (d *display) chipsH() int {
@@ -302,24 +275,20 @@ func (d *display) countsH() int {
 	return gridRows(len(errRows))*(d.countChipH()+chipGap) - chipGap
 }
 
-// Shared by both panels so they are demonstrably the same object, one carrying
+// Outlined by drawing the border colour and sinking a smaller well of
-// a count and one not.
+// background into it, so both curves get the same antialiasing.
 func (d *display) chipAt(i, x, w, y, h int, c rgb) (int, int, int) {
 	cx, cw := gridCell(i, len(errRows), x, w)
 	cy := y + (i/gridCols)*(h+chipGap)
 
-	// Outlined by drawing the border colour and then sinking a smaller well of
-	// background into it, so both curves get the same antialiasing.
 	d.fb.roundRect(cx, cy, cw, h, chipRadius, c)
 	d.fb.roundRect(cx+chipBorder, cy+chipBorder,
 		cw-2*chipBorder, h-2*chipBorder, chipRadius-chipBorder, uiBg)
 	return cx, cw, cy
 }
 
-// The same kinds as errBlock, but answering whether rather than how many, and
+// Whether rather than how many: over a window this short a count changes faster
-// carrying their own labels so nothing has to be matched up across a row. Over
+// than it can be read.
-// a window this short a count is a number nobody can read before it changes;
-// the only thing worth knowing at a glance is which kinds are happening now.
 func (d *display) errChips(x, w, y int, e errs) int {
 	for i, r := range errRows {
 		c := errColor(r.get(e))
@@ -340,8 +309,6 @@ func (d *display) errCounts(x, w, y int, e errs) int {
 	return y + d.countsH()
 }
 
-// Draws the frame and hands back the writable width inside it. Where the blocks
-// sit within it was settled once at startup, since it never changes.
 func (d *display) panel(p rect, e errs) (int, int) {
 	fill, edge := uiOKFill, uiOKEdge
 	if e.total() > 0 {