Refactor of monitors for v1. Way cleaner Monitoring API.#868
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the monitoring API by consolidating initialization parameters into structured metadata objects, eliminating the need for post-construction setters and fixing bridge component mapping issues.
Changes:
- Replaced multiple initialization methods (
new,set_topology,set_copperlist_info,set_execution_probe) with a singlenew(metadata, runtime)constructor - Introduced
CuMonitoringMetadataandCuMonitoringRuntimeto bundle static and dynamic monitoring context - Fixed bridge component monitoring by computing proper component-to-slot mappings at code generation time
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/cu_monitoring/src/main.rs | Updated example monitor to use new metadata-based initialization |
| core/cu29_runtime/src/monitoring.rs | Added metadata/runtime structures and refactored CuMonitor trait |
| core/cu29_runtime/src/curuntime.rs | Updated runtime construction to build and pass monitoring metadata |
| core/cu29_runtime/src/config.rs | Extracted DEFAULT_MISSION_ID constant |
| core/cu29_derive/src/lib.rs | Updated codegen to build component metadata and mapping arrays |
| components/monitors/cu_safetymon/tests/safety_monitoring.rs | Updated tests to construct metadata objects |
| components/monitors/cu_safetymon/src/std_impl.rs | Refactored to use metadata-based initialization |
| components/monitors/cu_safetymon/src/nostd_impl.rs | Updated signature to match new trait |
| components/monitors/cu_logmon/src/lib.rs | Refactored to use metadata-based initialization |
| components/monitors/cu_logmon/examples/demo.rs | Updated example to construct metadata |
| components/monitors/cu_consolemon/src/lib.rs | Refactored to use metadata and removed manual mapping logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let task_ids = task_specs.ids.clone(); | ||
| let ids = build_monitored_ids(&task_ids, &mut culist_bridge_specs); |
There was a problem hiding this comment.
The variable task_ids is cloned and immediately passed to build_monitored_ids, but the original task_specs.ids could be used directly on line 1092. This clone appears unnecessary unless task_specs.ids is borrowed elsewhere.
| let task_ids = task_specs.ids.clone(); | |
| let ids = build_monitored_ids(&task_ids, &mut culist_bridge_specs); | |
| let ids = build_monitored_ids(&task_specs.ids, &mut culist_bridge_specs); |
| #sim_tasks_instanciator | ||
|
|
||
| pub const TASKS_IDS: &'static [&'static str] = &[#( #ids ),*]; | ||
| pub const TASK_IDS: &'static [&'static str] = &[#( #task_ids ),*]; |
There was a problem hiding this comment.
The constant was renamed from TASKS_IDS to TASK_IDS, but all the string references in debug messages still use the old plural form 'tasks' (e.g., 'Task' instead of 'tasks'). Consider whether this naming should be TASKS_IDS for consistency with the terminology used throughout the codebase, or update comments/messages to match the singular form.
| pub const TASK_IDS: &'static [&'static str] = &[#( #task_ids ),*]; | |
| pub const TASKS_IDS: &'static [&'static str] = &[#( #task_ids ),*]; |
Summary
The structure was kind of shitty, wrong and not extensible: instead of multiple shots injection of static info through the trait and new to pass a the config + list of taskids
Now we only pass a Metadata structure so consolemon doesn't have to recompute a lot of task mapping etc that we already do at runtime creation.
And it was buggy with the bridges, it was spotted with the flight controller task mapping.
Related issues
Changes
Testing
just fmtjust lintjust testjust std-ci(if std/runtime paths are impacted)just nostd-ci(if embedded/no_std paths are impacted)pro-tip:
justwith no parameters in the root defaults tojust fmt,just lint, andjust test.Checklist
Breaking changes (if any)
Additional context