feat(dispatcher): configurable module ids for every service and example module - #799
Conversation
…le module The dispatcher module byte is a routing key, not an identity, but most espp services hard-coded theirs (static kModule). Add `Config::module` (default = the published id) to OtaService (0), Telemetry (3) and CoreDumpService (4), matching Mcp266Service (6), and route EVERY use of the id through module_id(): frame filtering in handle()/feed() and the build_frame() calls for replies/notifications (feed()/build() in Telemetry and CoreDumpService used kModule directly, so a moved instance would have replied on the wrong module). The ota_stream make_* builders take an optional trailing module argument for the same reason. Example modules keep their id in one named constant at the top of the example (kHapticsModule, kCanBridgeModule; kCrashModule already was), with the protocol headers' kModule/kModuleId as the default. Defaults are unchanged; the hosted web consoles and the espp_ota CLI assume them, which the docs (custom_modules.rst, dispatcher pages, component READMEs/rst) now state explicitly. Adds a host test that the OTA builders stamp a given module id. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
|
✅Static analysis result - no issues found! ✅ |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
This PR makes dispatcher module IDs configurable per service instance (via Config::module) and updates examples + documentation to emphasize that module IDs are routing keys with unchanged defaults.
Changes:
- Added
Config::modulesupport to services (Telemetry/CoreDump/Ota) and ensured inbound filtering + outbound replies/notifications use the configured module consistently. - Updated OTA stream-protocol builders to accept an optional module ID and added host tests validating stamping behavior.
- Refreshed docs/READMEs/examples to clarify “defaults vs configurable IDs” and show how to move modules safely.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| doc/en/telemetry/telemetry.rst | Notes module id 3 is the default and documents Config::module behavior + host tooling expectations. |
| doc/en/ota/ota.rst | Documents OTA module default (0) and implications of moving it via Config::module. |
| doc/en/dispatcher/dispatcher.rst | Clarifies the module table lists defaults and points to custom module docs. |
| doc/en/dispatcher/custom_modules.rst | Adds detailed explanation + examples for configurable module IDs across services and examples. |
| doc/en/coredump/coredump.rst | Documents CoreDump module default (4) and moving via Config::module. |
| components/telemetry/include/telemetry.hpp | Adds Config::module, stores per-instance module id, and uses it for filtering + building frames. |
| components/telemetry/README.md | Documents Telemetry default module id and moving it via configuration. |
| components/ota/test/ota_protocol_host_test.cpp | Adds a host test verifying builders stamp an explicit module id and defaults remain unchanged. |
| components/ota/include/ota_service.hpp | Adds Config::module and uses it when building all replies/status/errors. |
| components/ota/include/detail/ota_stream_protocol.hpp | Updates all OTA builders to accept an optional module id (defaulting to 0). |
| components/ota/README.md | Documents OTA Config::module and builder module argument for non-default routing. |
| components/mcp266/README.md | Documents module 6 as default and notes configurability via Config::module. |
| components/dispatcher/README.md | Clarifies default IDs and documents that services use Config::module. |
| components/coredump/include/coredump_service.hpp | Adds Config::module, stores per-instance module id, and uses it for filtering + building frames. |
| components/coredump/example/main/coredump_example.cpp | Clarifies crash-trigger module routing and host-console default expectations. |
| components/coredump/README.md | Documents default module id and moving via Config::module. |
| components/canopen/can_bridge_example/main/can_bridge_protocol.hpp | Clarifies module id 5 is the protocol default and how the example derives its chosen id. |
| components/canopen/can_bridge_example/main/can_bridge_example.cpp | Centralizes module id in kCanBridgeModule and ensures all frames are stamped with it. |
| components/canopen/can_bridge_example/README.md | Documents that module id 5 is the default and the single constant to change it in the example. |
| components/bldc_haptics/example/main/haptics_usb_protocol.hpp | Makes build() accept a module id so replies follow the registered module id. |
| components/bldc_haptics/example/main/bldc_haptics_example.cpp | Centralizes haptics module id in kHapticsModule and routes all replies/telemetry through it. |
| components/bldc_haptics/example/README.md | Documents which module IDs are defaults and where to configure them. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
New fields alter positional initialization of three public aggregate configuration types.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
components/ota/include/detail/ota_stream_protocol.hpp:88
- This block makes the OTA module configurable, but the same public header still says “OTA occupies module 0” at line 23 and describes
MessageTypeas being “within module 0” at line 91. Those wire-protocol annotations are false for a non-defaultConfig::module; describe 0 as the default and refer to the configured module where appropriate.
/// Default dispatcher module id of the OTA protocol (0) -- the id the OTA
/// console / CLI expect. Every frame builder below takes an optional `module`
/// argument (defaulting to this) so a device that serves OTA on another id
/// (espp::OtaService::Config::module) builds its replies on that id.
components/coredump/include/coredump_service.hpp:8
- The service can now move off module 4, but this header still labels both wire-direction tables (lines 14 and 20) and the
Msgenum (line 117) as module 4. For a non-defaultConfig::module, those authoritative protocol comments are incorrect; describe 4 as the default and refer to the configured module where appropriate.
// framing spec). The core-dump protocol occupies dispatcher MODULE 4 by
// default (CoreDumpService::Config::module can move it), so it can share one
components/bldc_haptics/example/main/haptics_usb_protocol.hpp:42
- The override documented here leaves the protocol documentation internally inconsistent: this header still says the protocol occupies module 2 and that
Msgis “within module 2” (lines 10 and 48), whilecomponents/bldc_haptics/example/PROTOCOL.mdcalls 2 mandatory at lines 17 and 29. After changingkHapticsModule, those statements are false; describe 2 as the default and point readers to the configured constant.
/// Default dispatcher module id of the haptics protocol (the frame `module`
/// byte): the id the hosted haptics console expects. The example registers the
/// protocol under `kHapticsModule` (bldc_haptics_example.cpp), which defaults
/// to this; build() takes the id to stamp so replies follow whatever the app
/// registered.
components/canopen/can_bridge_example/main/can_bridge_protocol.hpp:30
- The override documented here leaves the published protocol documentation inconsistent. This header still says every frame sets and routes to module 5 (lines 5–8), and
doc/en/buses/canopen.rst:44presents module 5 as fixed. After changingkCanBridgeModule, those statements no longer describe the example; identify 5 as the default, reference the configured constant, and note that host tooling must use the same id.
/// Default dispatcher module id of the CAN bridge protocol: the id the hosted
/// CAN console expects. The example registers under `kCanBridgeModule`
/// (can_bridge_example.cpp), which defaults to this.
- Files reviewed: 22/22 changed files
- Comments generated: 3
- Review effort level: Balanced
… header still called them fixed Review follow-ups on #799: the OTA, coredump, haptics and CAN-bridge protocol headers (plus PROTOCOL.md, the dispatcher README snippet and the CANopen doc) now describe their module id as the default and point at the Config::module / constant that moves it; grammar fix in the OTA host test comment. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
…eter; const module_ Review follow-ups on #799: CoreDumpService::build() and Telemetry::build() are static again, taking the module id to stamp (kModule by default) so existing callers compile unchanged; the services pass module_id() at every call site. CoreDumpService::module_ is const like Telemetry's. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
There was a problem hiding this comment.
🟡 Changes recommended
The dispatcher guide still documents a build signature that differs from the public API.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
doc/en/dispatcher/custom_modules.rst:95
- These two instances have independent
send_mutex_members, so sharing the samesendcallback does not serialize writes between them. Iffast.emit()andslow.emit()run concurrently, a non-thread-safe transport callback can be entered concurrently even though Telemetry’s class documentation promises per-instance serialization; document that this shared sender must provide cross-instance serialization (or show it wrapping the transport with one mutex).
components/mcp266/README.md:67
- The public API documentation was updated here for
Config::module, butdoc/en/motor_control/mcp266.rststill has no mention of the configurable module id or the fact that the console assumes the default. Please update the component RST as well so the API/reference navigation does not contradict the README and the repository's component-documentation convention.
- `mcp266_protocol.hpp` — the wire protocol (dispatcher **module 6** by
default — `Mcp266Service::Config::module` moves an instance, but the console
looks for 6 until told otherwise; `stream_frame` framing): request/reply
types, the payload structs
- Files reviewed: 24/24 changed files
- Comments generated: 2
- Review effort level: Balanced
…le) helper; telemetry example says module 3 is the default Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
Motivation
The dispatcher
modulebyte is a routing key, not an identity: it only decides which registered handler receives a frame, while what a module is comes from its handler and its discoveryModuleInfo. Until now most espp services and example modules hard-coded their id (static constexpr uint8_t kModule), so an application could not, e.g., run twoespp::Telemetryemitters on one link, or keep its own protocol on4next to aCoreDumpService.espp::Mcp266Servicealready did this right (Config::module); this PR brings every other dispatcher module to the same pattern.Defaults are unchanged, so nothing moves unless an application opts in.
What changed
Every service now takes its id from
Config::module(placed with the other identity-ish fields;log_levelstays the last member) and uses it for everything — the frames it accepts inhandle()/feed()and themodulebyte it stamps on every reply / notification — so request and reply sides can never disagree. ThekModuleconstants stay as the documented defaults.espp::OtaServiceConfig::module(new);make_*builders indetail/ota_stream_protocol.hpptake an optional trailingmodule(defaultkModule) so replies are stamped with the configured idespp::TelemetryConfig::module(new); fixesfeed()andbuild()which usedkModuledirectlyespp::CoreDumpServiceConfig::module(new); fixesfeed()andbuild()which usedkModuledirectlyespp::Mcp266ServiceConfig::module— verified only, no changekHapticsModuleconstant at the top ofbldc_haptics_example.cpp(defaults tohaptics_proto::kModule);haptics_proto::build()takes the id, and all replies/telemetry route through itkCanBridgeModuleconstant at the top ofcan_bridge_example.cpp(defaults tocan_bridge::kModuleId), used for registration and every built framekCrashModuleconstant — comment onlyDocs:
doc/en/dispatcher/custom_modules.rstnow explains routing-vs-identity, that the id table lists defaults, shows moving a service / running twoTelemetryinstances, and updates theCoreDumpServiceanatomy (Config carriesmodule,build()usesmodule_id(),module_id()resolved on the object at registration).dispatcher.rst, the dispatcher/ota/coredump/telemetry/mcp266 READMEs and rst pages, and the haptics / CAN-bridge example READMEs note theConfig::moduleknob.Host tooling assumes the defaults
The hosted web consoles (
ota_console.html,coredump_console.html,telemetry.html,mcp266_console.html, the haptics and CAN consoles) and theespp_otaPython CLI are written against the default ids and are intentionally not changed here. A device that moves a service off its default will not be found by the stock console until that console / your own host tool is told the new id; the docs say so explicitly.Verification
components/ota/test/ota_protocol_host_test.cppgainedtest_builders_take_a_module_id(every builder stamps the given id; default still 0) — passes;components/dispatcher/test/dispatcher_host_test.cpppasses.IDF_COMPONENT_MANAGER=0, esp32s3):ota/example,coredump/example,telemetry/example,mcp266/webapp_example,canopen/can_bridge_example,bldc_haptics/example— all build.cppcheck --std=c++17 --check-level=exhaustive --enable=all --inconclusive --inline-suppr --force --platform=mips32 --suppressions-list=suppressions.txton the changed headers: clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU