-
Notifications
You must be signed in to change notification settings - Fork 404
[ESI] Telemetry implementation exposing metrics over MMIO #9185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the telemetry service implementation to use MMIO (Memory-Mapped I/O) instead of channels. The telemetry class is renamed from Telemetry to Metric to better reflect its purpose.
Key changes:
- Renamed
TelemetryService::TelemetrytoTelemetryService::Metricacross all C++ and Python bindings - Replaced channel-based telemetry implementation with MMIO-based reads using register offsets
- Added
TelemetryMMIOservice implementation in Python that assigns each telemetry client a register in MMIO space
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/Dialect/ESI/runtime/python/esiaccel/esiCppAccel.cpp | Updated Python bindings to expose Metric class with new readInt method |
| lib/Dialect/ESI/runtime/cpp/tools/esitester.cpp | Updated to use Metric and new readInt() method for reading telemetry |
| lib/Dialect/ESI/runtime/cpp/tools/esiquery.cpp | Updated type references from Telemetry to Metric |
| lib/Dialect/ESI/runtime/cpp/lib/Services.cpp | Refactored telemetry service to use MMIO, added safer any_cast with pointer checks, implemented readInt() method |
| lib/Dialect/ESI/runtime/cpp/include/esi/Services.h | Updated class definition from Telemetry to Metric, added MMIO support fields |
| lib/Dialect/ESI/runtime/cpp/include/esi/Common.h | Added toString() helper method to AppID struct |
| lib/Dialect/ESI/runtime/cpp/include/esi/Accelerator.h | Moved ServiceTable typedef to Services.h for better modularity |
| lib/Dialect/ESI/runtime/cpp/lib/Manifest.cpp | Removed unused ServiceTable typedef |
| lib/Dialect/ESI/ESIStdServices.cpp | Changed telemetry 'get' channel width from 1 bit to 0 bits (void type) |
| frontends/PyCDE/src/pycde/esi.py | Added TelemetryMMIO implementation, updated channel widths, fixed type annotations |
| frontends/PyCDE/src/pycde/bsp/xrt.py | Added TelemetryMMIO service instantiation |
| frontends/PyCDE/src/pycde/bsp/cosim.py | Added TelemetryMMIO service instantiation |
| frontends/PyCDE/integration_test/esitester.py | Added instance names to module instantiations for better identification |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| table: Dict[int, AssignableSignal] = {} | ||
| for bundle in bundles.to_client_reqs: | ||
| # Only support 'report' port for telemetry. | ||
| if bundle.port == 'report': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice that these things are centralized/documented somewhere, so that we don't have these stray magic strings.
Instead of creating a DMA channel per metric, aggregate them and show them over MMIO.