Phase 1017: Tag/Event auto-wiring (registry-default + dual-key)#99
Open
Phase 1017: Tag/Event auto-wiring (registry-default + dual-key)#99
Conversation
…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)
Contributor
There was a problem hiding this comment.
⚠️ 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
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
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.
Summary
EventStore. NewTagRegistry.setEventStore(store)/getEventStore()static API. After one call at setup,MonitorTagctor,FastSense,FastSenseWidget,EventTimelineWidget, andTableWidget(events)all auto-discover the store — no per-instance'EventStore'NV-pairs needed.Event.TagKeysis 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 whereEventStore.getEventsForTag(parentSensorKey)returned nothing because events were only filed under the monitor's own key.'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:
demo/industrial_plant/private/registerPlantTags.m,buildEventsPage.m, andexamples/example_event_markers.mare 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"— greentest_tag,test_tag_registry,test_monitortag,test_monitortag_events— all green, no regressionclose 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