Add .github/copilot-instructions.md#744
Add .github/copilot-instructions.md#744jerrysxie wants to merge 1 commit intoOpenDevicePartnership:mainfrom
Conversation
Add Copilot instructions covering build/test/lint commands, service architecture patterns, and key codebase conventions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jerrysxie
requested review from
RobertZ2011,
felipebalbi,
tullom and
williampMSFT
There was a problem hiding this comment.
Pull request overview
Adds a new .github/copilot-instructions.md document intended to guide Copilot (and contributors) on how to build/test/lint the workspace and on key architecture/convention details for the embedded-services codebase.
Changes:
- Introduces a Copilot instruction doc with common build/lint/test commands aligned to CI workflows.
- Documents service/IPC architecture concepts (comms endpoints, message routing) and core utilities/macros.
- Captures conventions around feature flags, clippy strictness, dependencies, and testing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Each service crate follows a consistent structure: | ||
|
|
||
| 1. **Service struct** with a `comms::Endpoint` and domain-specific context/state | ||
| 2. **`MailboxDelegate` impl** — the `receive()` method handles incoming messages using type-safe downcasting via `message.data.get::<T>()` | ||
| 3. **Global singleton** — services are stored in `OnceLock<Service>` statics | ||
| 4. **Async task function** — registers the endpoint, then loops calling a `process()` or `process_next()` method | ||
| 5. **Spawned via Embassy** — `#[embassy_executor::task]` functions are spawned from main |
There was a problem hiding this comment.
The “Service Pattern” section describes a single comms/Endpoint + MailboxDelegate-based structure as if it applies to every service crate. In this repo, some services (e.g., type-c-service) use other IPC patterns (pubsub/broadcaster, ipc::deferred) and don’t have a comms::Endpoint field. Consider rewording this section to describe the comms-based pattern as common (not universal) and call out the alternative patterns used elsewhere in the workspace.
|
|
||
| pub async fn task() { | ||
| let service = SERVICE.get_or_init(MyService::new); | ||
| comms::register_endpoint(service, &service.endpoint).await.unwrap(); |
There was a problem hiding this comment.
The service skeleton example uses .await.unwrap() when registering the endpoint, but later this doc states clippy denies unwrap_used/no panicking in production code. To avoid sending mixed signals, update the example to handle the Result explicitly (e.g., propagate or log/return) rather than unwrapping.
| comms::register_endpoint(service, &service.endpoint).await.unwrap(); | |
| if let Err(err) = comms::register_endpoint(service, &service.endpoint).await { | |
| // TODO: handle registration error (e.g., log and return or trigger a safe reset) | |
| return; | |
| } |
|
|
||
| ```shell | ||
| # ARM board examples | ||
| cd examples/rt685s-evk && cargo clippy --target thumbv8m.main-none-eabihf --locked |
There was a problem hiding this comment.
The examples build/lint section omits the examples/rt633 workspace, which is present in the repo and is also checked in CI. Add an example command for examples/rt633 (or generalize the instructions to cover all examples/* workspaces).
| cd examples/rt685s-evk && cargo clippy --target thumbv8m.main-none-eabihf --locked | |
| cd examples/rt685s-evk && cargo clippy --target thumbv8m.main-none-eabihf --locked | |
| cd examples/rt633 && cargo clippy --target thumbv8m.main-none-eabihf --locked |
| - Custom `enum` error types per module — no `thiserror` (it requires std) | ||
| - All error enums derive `Debug, Clone, Copy, PartialEq, Eq` |
There was a problem hiding this comment.
The error-handling conventions are stated too strongly: not all error types are enums and not all derive Clone, Copy, PartialEq, Eq (e.g., some error enums only derive Debug/Copy, and some services use a struct error). Consider changing this to “prefer” these derives when practical, and note that some modules use lightweight struct errors.
| - Custom `enum` error types per module — no `thiserror` (it requires std) | |
| - All error enums derive `Debug, Clone, Copy, PartialEq, Eq` | |
| - Prefer custom `enum` error types per module — no `thiserror` (it requires std); some modules instead use lightweight struct error types when that is a better fit | |
| - Prefer deriving `Debug, Clone, Copy, PartialEq, Eq` on error enums when practical (some errors may only derive a subset, e.g., `Debug`/`Copy`) |
| - Async tests use `embassy_futures::block_on(async { ... })` | ||
| - Dev-dependencies enable `std` features: `embassy-sync/std`, `embassy-time/std`, `critical-section/std` | ||
| - `tokio` with `rt`, `macros`, `time` features for integration tests |
There was a problem hiding this comment.
Testing guidance says async tests use embassy_futures::block_on(async { ... }), but there are many async unit tests written with #[tokio::test] in this repo. It would be more accurate to mention both patterns and when each is used.
| - Async tests use `embassy_futures::block_on(async { ... })` | |
| - Dev-dependencies enable `std` features: `embassy-sync/std`, `embassy-time/std`, `critical-section/std` | |
| - `tokio` with `rt`, `macros`, `time` features for integration tests | |
| - Async unit tests in `no_std`/Embassy-focused crates use `embassy_futures::block_on(async { ... })` to stay runtime-agnostic | |
| - Host-only async tests and integration tests may use `#[tokio::test]` in crates that depend on `tokio` | |
| - Dev-dependencies enable `std` features: `embassy-sync/std`, `embassy-time/std`, `critical-section/std` | |
| - `tokio` with `rt`, `macros`, `time` features is used to support `#[tokio::test]`-based host/integration tests |
|
|
||
| ### Core Utilities (embedded-service crate) | ||
|
|
||
| - **`GlobalRawMutex`**: `ThreadModeRawMutex` on ARM bare-metal, `CriticalSectionRawMutex` in tests/std |
There was a problem hiding this comment.
GlobalRawMutex is described as ThreadModeRawMutex on ARM bare-metal and CriticalSectionRawMutex in tests/std, but the actual cfg also selects CriticalSectionRawMutex on riscv32. Consider mentioning that exception to avoid confusing cross-target contributors.
| - **`GlobalRawMutex`**: `ThreadModeRawMutex` on ARM bare-metal, `CriticalSectionRawMutex` in tests/std | |
| - **`GlobalRawMutex`**: `ThreadModeRawMutex` on ARM bare-metal, `CriticalSectionRawMutex` on RISC-V bare-metal and in tests/std |
kurtjd
left a comment
kurtjd
left a comment
There was a problem hiding this comment.
I'll be honest I'm still not entirely sure what these instructions offer if they are AI generated in the first place (I thought things like skills, AGENTS.md, CLAUDE.md, etc are to provide additional context the LLM can't glean on it's own from the codebase) but that likely means I just need to research this a bit more.
Add Copilot instructions covering build/test/lint commands, service architecture patterns, and key codebase conventions.