latency/histo binstream: fix FIFO sizing and fail visibly on error#4102
Open
grandixximo wants to merge 3 commits into
Open
latency/histo binstream: fix FIFO sizing and fail visibly on error#4102grandixximo wants to merge 3 commits into
grandixximo wants to merge 3 commits into
Conversation
The FIFO held 2*MAXBINNUMBER+1+2 records, but the hal_stream ring buffer reserves one slot, so usable capacity is depth-1. At 1000 bins the last of the 2003 records overruns, the reader times out, and latency-histogram fails with --sbins 1000. Add one slot so a full set fits. Fixes LinuxCNC#4101
Same off-by-one as latencybinstream: the FIFO held MAXBINNUMBER+2 records, but the hal_stream ring buffer reserves one slot, so usable capacity is depth-1. At the max 200 bins the last of the 202 records overruns and hal-histogram times out with --nbins 200. Add one slot.
The stream read loop only printed to the console on error and returned, so a hard FIFO sizing error left the window open and silently non-updating while spamming the console every cycle (the symptom in issue LinuxCNC#4101 before the FIFO fix). Treat the two error kinds differently: - stream-error (FIFO too small) is unrecoverable, so pop up a dialog and quit via a new fatal proc, matching the existing availablebins check. - a plain timeout may be transient, so warn once with a popup after it persists rather than only logging to the console.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4101
FIFO sizing
The
hal_streamring buffer keeps one slot permanently empty to tell full from empty apart, so its usable capacity isdepth - 1. Both histogram stream components sized their FIFO to exactly one snapshot, so at the maximum bin count the last record overran and the reader waited forever for a depth that never arrived:2*MAXBINNUMBER+1+2= 2003--sbins 1000MAXBINNUMBER+2= 202nbins+2= 202--nbins 200This is why
--sbins 999worked but--sbins 1000failed with:Each component now allocates one extra slot so a complete set always fits.
Error handling
Following rodw's note that the tool should not open in a broken state: the stream read loop previously only printed to the console and returned, leaving the window open, blank and non-updating while spamming the console every cycle.
stream-error(FIFO too small) is unrecoverable, so it now pops up a dialog and quits via a newfatalproc, matching the existingavailablebinsstartup check.Both
latency-histogramandhal-histogramget the matching changes.