Skip to content

Phase 1017: Tag/Event auto-wiring (registry-default + dual-key)#99

Open
HanSur94 wants to merge 11 commits intomainfrom
claude/gifted-mestorf-549915
Open

Phase 1017: Tag/Event auto-wiring (registry-default + dual-key)#99
HanSur94 wants to merge 11 commits intomainfrom
claude/gifted-mestorf-549915

Conversation

@HanSur94
Copy link
Copy Markdown
Owner

Summary

  • Registry-default EventStore. New TagRegistry.setEventStore(store) / getEventStore() static API. After one call at setup, MonitorTag ctor, FastSense, FastSenseWidget, EventTimelineWidget, and TableWidget(events) all auto-discover the store — no per-instance 'EventStore' NV-pairs needed.
  • Dual-key event regression-protected. Event.TagKeys is set to {monitor.Key, parent.Key} on emission (already in code at 3 sites — now grep-guarded by tests so it can't silently regress). Closes a hidden bug where EventStore.getEventsForTag(parentSensorKey) returned nothing because events were only filed under the monitor's own key.
  • Backward compatible. Explicit per-instance 'EventStore' / 'EventStoreObj' always wins. Existing scripts and tests run unchanged. No new error IDs, no deprecation warnings.

Why

The industrial-plant demo shipped without event markers visible on FastSense plots even though the EventStore was correctly wired to every MonitorTag. Root cause was the key mismatch (sensor key vs monitor key) and the absence of a registry-level fallback. This phase fixes both at the API layer rather than papering over the demo.

After this PR, the canonical recipe collapses to:

store = EventStore(eventFile);
TagRegistry.setEventStore(store);
% ...register sensors, monitors, composites — no 'EventStore' NV-pairs
% addWidget('fastsense', 'Tag', sensorTag) — events render automatically

demo/industrial_plant/private/registerPlantTags.m, buildEventsPage.m, and examples/example_event_markers.m are migrated to the new pattern as the canonical references.

Test plan

  • octave --no-gui --eval "addpath('.'); install(); test_dashboard_events_toggle" — 22/22 pass (8 pre-existing + 14 new across plans 01–04)
  • octave --no-gui --eval "addpath('.'); install(); example_event_markers" — smoke green (only pre-existing PostSet warnings, unrelated to this PR)
  • octave --no-gui --eval "addpath('.'); install(); test_demo_industrial_plant_smoke" — green
  • test_tag, test_tag_registry, test_monitortag, test_monitortag_events — all green, no regression
  • Phase verifier ran goal-backward checks against the codebase: 12/12 must-haves verified
  • Manual: close all force; clear all; clear classes; cd demo/industrial_plant; run_demo — confirm event markers appear on FastSense plots and the Events toolbar button toggles them off/on (this also resolves the original "Events button missing" question — likely was just a stale class cache)

Files Touched

API (libs/):

  • libs/SensorThreshold/TagRegistry.m (+47 lines: setEventStore, getEventStore, eventStoreRef_, clear() extension)
  • libs/SensorThreshold/MonitorTag.m (+9 lines: ctor fallback after NV-pair loop)
  • libs/FastSense/FastSense.m (registry tail in renderEventLayer_)
  • libs/Dashboard/FastSenseWidget.m (esForward local pattern)
  • libs/Dashboard/EventTimelineWidget.m (esObj local + new private helper eventStoreToStructsFrom_)
  • libs/Dashboard/TableWidget.m (esObj local; also fixed pre-existing Octave `contains()` incompatibility)

Demo + example migration:

  • demo/industrial_plant/private/registerPlantTags.m (–4 NV-pairs, +1 setEventStore)
  • demo/industrial_plant/private/buildEventsPage.m (–1 NV-pair, fixed misleading comment)
  • examples/example_event_markers.m (canonical migration)

Tests (dual-target MATLAB suite + Octave function pair):

  • tests/suite/TestDashboardEventsToggle.m (+14 methods)
  • tests/test_dashboard_events_toggle.m (Octave parity blocks)

🤖 Generated with Claude Code

HanSur94 added 11 commits April 28, 2026 17:25
…Ref_() helper

- Add setEventStore(store) public static: stores in containers.Map handle singleton
- Add getEventStore() public static: returns [] if unset (safe before first call)
- Add eventStoreRef_() private static: persistent containers.Map (handle) for mutation safety
- Extend clear() to also remove 'store' key from eventStoreRef_ map (Pitfall 5 fix)
- Pass [] to setEventStore() to clear the default without calling clear()
… overwrite tests

- TestDashboardEventsToggle.m: 5 new methods (Phase 1017)
  testTagRegistryEventStoreRoundTrip, testTagRegistryEventStoreEmptyDefault,
  testTagRegistryEventStoreOverwrite, testTagRegistryClearResetsEventStore,
  testTagRegistryEventStoreSetEmptyClears
- Add deleteIfExists local helper for temp file cleanup
- test_dashboard_events_toggle.m: 5 Octave parity test blocks (tests 9-13)
  matching MATLAB names for grep-verified parity
- All 13 tests green in Octave (8 existing + 5 new)
…entStore()

- Insert 3-line registry-default fallback after NV-pair for-loop
- Fallback only runs when no explicit 'EventStore' NV-pair was provided
- Explicit per-instance store always wins (fallback gated on isempty)
- Falls back to [] when registry default unset (pre-1017 behavior preserved)
- Dual-key stamping at 3 sites (lines 738-741, 762-765, 877-879) untouched
…ission tests

- testMonitorTagRegistryDefaultFallback: verifies constructor uses registry default
- testMonitorTagExplicitOverridesRegistry: verifies explicit NV-pair wins
- testMonitorTagDualKeyEmission: regression-protects dual-key stamp at 3 sites
- All three tests added to both MATLAB suite and Octave flat test file
- 16 total Octave tests pass (0 failures)
…th registry-default fallback

- FastSense.renderEventLayer_: append TagRegistry.getEventStore() tail after bound-tag loop
- FastSenseWidget.render: use esForward local var resolved from registry default before inner FastSense forwarding
- Both sites: explicit per-instance store wins (unchanged), registry only consulted when slot is empty
- Second render path (rerender method in FastSenseWidget) also updated
…idget registry-default fallback

- testRegistryDefaultFastSense: verifies renderEventLayer_ uses TagRegistry.getEventStore() when bound tag's EventStore is empty
- testRegistryDefaultFastSenseWidget: verifies FastSenseWidget forwards registry-default store to inner FastSense
- testFastSenseWidgetExplicitWinsOverRegistry: explicit EventStore NV-pair wins over registry default
- Octave parity: Tests 17-19 in test_dashboard_events_toggle.m (all 19 pass)
- Added closeIfValid helper to TestDashboardEventsToggle.m
…lineWidget + TableWidget

- EventTimelineWidget.resolveEvents: public; uses local esObj resolved from
  obj.EventStoreObj then TagRegistry.getEventStore() (Phase 1017, no obj mutation)
- New private helper eventStoreToStructsFrom_(esObj) avoids re-entrancy risk
  (RESEARCH Pitfall 6) — body is verbatim copy of eventStoreToStructs with esObj arg
- TableWidget refresh events branch: condition narrowed to strcmp(Mode,'events');
  local esObj resolves explicit slot then registry default (Phase 1017)
…eWidget registry-default fallback

- testRegistryDefaultEventTimeline: no EventStoreObj -> registry default resolves
- testRegistryDefaultTableWidget: Mode=events, no EventStoreObj -> refresh via registry
- testEventTimelineExplicitWinsOverRegistry: explicit store wins over registry default
- Also fix pre-existing Octave incompatibility: replace contains() with strfind in TableWidget
- Add TagRegistry.setEventStore(store) after EventStore construction
- Drop 'EventStore', store NV-pair from all four MonitorTag constructors
- MonitorTag instances now pick up EventStore via registry-default fallback
- Drop 'EventStoreObj', ctx.store NV-pair from EventTimelineWidget call
- Fix misleading comment claiming FastSense 'auto-discovers EventStore
  from any bound MonitorTag' — replaced with accurate description of the
  registry-default fallback and dual-key MonitorTag event emission
- Update function header comment to reflect registry-default approach
…t pattern

- Add TagRegistry.clear(); EventBinding.clear(); at top to prevent singleton pollution
- Add TagRegistry.setEventStore(es) once after EventStore construction
- Register both SensorTags and MonitorTags via TagRegistry.register()
- Drop 'EventStore', es NV-pairs from both MonitorTag constructors
- Drop 'EventStore', es NV-pairs from both addWidget calls
- Retain 'ShowEventMarkers', true on both widgets (explicit feature toggle)
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'FastSense Performance'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.10.

Benchmark suite Current: d05b012 Previous: 483cbff Ratio
Downsample mean std(1M) 0.07 ms 0.015 ms 4.67
Instantiation mean std(5M) 1.222 ms 0.58 ms 2.11
Render mean std(5M) 3.028 ms 1.356 ms 2.23
Zoom cycle mean std(5M) 0.867 ms 0.649 ms 1.34
Zoom cycle mean std10M) 0.829 ms 0.549 ms 1.51
Render mean ( std00M) 3.58 ms 2.157 ms 1.66
Zoom cycle mean ( std00M) 1.17 ms 0.506 ms 2.31
Downsample mean ( std00M) 0.801 ms 0.683 ms 1.17
Instantiation mean ( std00M) 91.712 ms 62.827 ms 1.46
Zoom cycle mean ( std00M) 1.173 ms 0.506 ms 2.32
Dashboard create+render stdmean 35.08 ms 17.252 ms 2.03
Dashboard live tick stdmean 0.038 ms 0.029 ms 1.31
Dashboard broadcastTimeRange stdmean 0.027 ms 0.013 ms 2.08

This comment was automatically generated by workflow using github-action-benchmark.

CC: @HanSur94

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant