hal-histogram: drop BLT for Tcl/Tk 9, add race-free histobinstream comp#4090
Conversation
BLT has no Tcl/Tk 9 port. Extract the canvas-based lh_chart widget and window-capture helper into lib/hallib/lh_chart.tcl, shared by both tools. latency-histogram now sources it instead of carrying an inline copy; hal-histogram drops blt::barchart, blt::bitmap and blt::winop snap along with the hard BLT and Tclx requirements, so both run on Tcl/Tk 8.6 and 9 alike. Also remove the obsolete base_thread_fp advice from hal-histogram: the HAL uses_fp flag is deprecated and ignored, so all threads are FP-capable and no motmod option is needed.
histobins transfers one bin per cycle, so its snapshot is smeared across many milliseconds and the statistics can desync from the bins. Add histobinstream, the streaming counterpart modeled on latencybinstream: on the rising edge of the stream pin it pushes the whole histogram (negative tail, bins, positive tail) through a HAL stream and latches the scalar statistics, giving a consistent atomic snapshot. hal-histogram uses it by default and falls back to histobins with --legacy. The bin classification compares invalue against minvalue for the negative tail, because integer truncation of a negative quotient rounds toward zero and would otherwise land such samples in bin 0. It tests the computed bin index against nbins for the positive tail, avoiding the bin[nbins] out-of-bounds write that histobins performs when invalue == maxvalue.
|
Your implementation of histobinstream is not one that I'd accept. Quite a bit of legacy implementation that has been wrong for some time... Let me hack one together that fixes most of the issues I see in your version ;-) |
|
Had a feeling it wouldn't fly... |
Wasn't too bad, but I'm picky :-) Try to take a look at the attached version. See if that works. (attached with extra |
|
And please fix my "Cache som pins" typo (make it 'some'). |
Replace the comp implementation with the version Bertho Stultiens posted in review of PR LinuxCNC#4090: cache the volatile input pins once per cycle, use real_t, keep input_error sticky until reset, and update the last_* trackers right after the accumulate block. The bin classification and first-sample seeding are unchanged. Also fixes his "som" -> "some" comment typo.
|
Adopted your version as a commit on top so the rework is easy to diff against the original, and fixed the "som" typo. Verified it builds and the classification still holds (25 to bin 2, 50 to the positive tail with no overflow, -3 to the negative tail), and tested it in RIP against a running machine. should have caught the |
2 participants
Summary
Two related changes for
hal-histogram, with one shared piece reused bylatency-histogram:histobinstream, a streaming histogram comp that takes an atomicsnapshot, removing the RT/non-RT data race of the existing
histobins.See the individual commit messages for the details of each change.
Motivation
BLT has no Tcl/Tk 9 port, so
hal-histogramcould not run on Tcl 9 systems(e.g. Fedora 42+).
latency-histogramwas already ported to a canvas widget;I extract that widget into
lib/hallib/lh_chart.tclso both tools share it.histobinstransfers one bin per servo cycle over theindex/checkhandshake, so its histogram is smeared across many milliseconds and its
statistics can drift out of sync with the bins.
latencybinstreamalreadysolved this for
latency-histogram;histobinstreambrings the same HALstream approach to
hal-histogram, which now uses it by default and fallsback to
histobinswith--legacy.histobins left unchanged on purpose
I deliberately leave
histobinsas it is, including two latent bugs, so the--legacypath stays a faithful contrast to the corrected streaming comp:invalue == maxvaluelands inbin[nbins], a write past the array atnbins == 200and reachable with integer pins.histobinstreamindex-teststhe high tail and counts it as positive overflow instead.
meanstillseeded to
0, biasing the running statistics.histobinstreamseeds fromthe first sample.
Testing
Tested locally:
lh_chartrenders under Xvfb on Tk 8.6; screenshot capture works through theImageMagick fallback (the native
image -format windowpath is Tk 8.7+/9).histobinstreamvalidated end-to-end underhalrun: input25falls in theexpected bin, input
50(== maxvalue) goes to the positive tail with noout-of-bounds write, input
-3goes to the negative tail;stream_errorstays clear and the statistics are correct.
hal-histogramrun against a live LinuxCNC instance with the streaming compon a real-time thread; the GUI updates correctly.
histobinstream.compcarries the same-Wextrahygiene as the recent
histobinscleanup.