Skip to content

Commit 2700e72

Browse files
HaoboGuHaoboGu
committed
refactor(rynk): dispatch propagates handler errors via Result
`dispatch` no longer encodes handler errors into the payload itself. Each match arm uses `await?` so any `RynkError` (from `msg.cmd()`, `payload_mut()`, the handler body, or the topic-CMD arms) propagates out as the function's `Err`. The dispatcher stays a pure "request bytes → response bytes or error" function with no buffer mutation on the failure path. Adds `write_error_response(msg, err)` next to `dispatch` and `write_topic`. Transports call it on the dispatch `Err` arm to encode `Err::<(), RynkError>(err)` into the same buffer's payload region and patch LEN, then send the reply. The split keeps the wire contract ("every response is a `Result<T, RynkError>` envelope") while pulling the error-encoding policy out of the protocol core. `cmd` and `seq` in the header are not touched by `write_error_response` — the host correlates the reply with its outgoing request by `seq`, regardless of the protocol-level error. Signed-off-by: Haobo Gu <haobogu@outlook.com>
1 parent e1d2a1b commit 2700e72

5 files changed

Lines changed: 83 additions & 165 deletions

File tree

‎rmk-types/src/protocol/rynk/message.rs‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,7 @@ use super::cmd::Cmd;
1616
/// Size in bytes of the fixed Rynk header.
1717
pub const RYNK_HEADER_SIZE: usize = 5;
1818

19-
/// Header-field accessors over an in-place buffer.
20-
///
21-
/// Every method validates that the underlying buffer is at least
22-
/// `RYNK_HEADER_SIZE` bytes long and returns `Err(InvalidRequest)`
23-
/// otherwise — there's no separate "must verify length first" step.
19+
/// Message operations for Rynk
2420
pub trait RynkMessage {
2521
fn cmd(&self) -> Result<Cmd, RynkError>;
2622
fn seq(&self) -> Result<u8, RynkError>;

‎rmk/src/host/context.rs‎

Lines changed: 8 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,4 @@
1-
//! Shared façade for host-facing services (Vial today, Rynk next).
2-
//!
3-
//! Bundles every keymap mutation with its flash persistence so callers don't
4-
//! repeat `keymap.X(); FLASH_CHANNEL.send(FlashOperationMessage::Y).await`
5-
//! by hand, and exposes synchronous reads of live keyboard state (LED,
6-
//! battery, connection, active layer) that are otherwise scattered across
7-
//! module-private statics.
8-
//!
9-
//! The context does not subscribe to events — the underlying statics it
10-
//! reads from are kept in sync by the relevant event handlers
11-
//! (`BatteryProcessor::commit`, `state.rs::update_status`,
12-
//! `keyboard::run_led_reader`).
1+
//! Shared context for host-facing services (Vial today, Rynk next).
132
143
use core::sync::atomic::{AtomicBool, AtomicU16, Ordering};
154

@@ -29,16 +18,7 @@ use crate::keymap::KeyMap;
2918
#[cfg(feature = "storage")]
3019
use crate::{channel::FLASH_CHANNEL, storage::FlashOperationMessage};
3120

32-
/// Façade shared between Vial and Rynk host services.
33-
///
34-
/// `keymap` is intentionally `pub`: callers like `VialLock` that only need a
35-
/// raw `&KeyMap` keep their existing parameter and read it through
36-
/// `ctx.keymap` at the construction site.
37-
///
38-
/// `latest_wpm` / `latest_sleep` cache pure event-stream topics that have no
39-
/// producer-side store. The Rynk topic subscriber loop calls `cache_wpm` /
40-
/// `cache_sleep` when each event fires; `Get*` handlers read via `wpm()` /
41-
/// `sleep_state()`. Pre-first-event readers see `0` / `false`.
21+
/// Context shared between Vial and Rynk host services.
4222
pub struct KeyboardContext<'a> {
4323
pub keymap: &'a KeyMap<'a>,
4424
latest_wpm: AtomicU16,
@@ -95,7 +75,11 @@ impl<'a> KeyboardContext<'a> {
9575
self.keymap.set_action_by_flat_index(index, action);
9676
#[cfg(feature = "storage")]
9777
{
98-
let (row, col, layer) = position_from_flat_index(index, rows, cols);
78+
let layer_size = rows * cols;
79+
let layer = index / layer_size;
80+
let layer_offset = index % layer_size;
81+
let row = layer_offset / cols;
82+
let col = layer_offset % cols;
9983
if FLASH_CHANNEL
10084
.try_send(FlashOperationMessage::KeymapKey {
10185
layer: layer as u8,
@@ -205,7 +189,7 @@ impl<'a> KeyboardContext<'a> {
205189
let _ = config;
206190
}
207191

208-
// ── Morses (Vial: tap-dance) ─────────────────────────────────────────
192+
// ── Morses ─────────────────────────────────────────
209193

210194
pub fn get_morse(&self, idx: u8) -> Option<Morse> {
211195
self.keymap.get_morse(idx as usize)
@@ -395,14 +379,3 @@ impl<'a> KeyboardContext<'a> {
395379
self.keymap.read_matrix_state(target);
396380
}
397381
}
398-
399-
/// Map a flat keymap index back to `(row, col, layer)`.
400-
///
401-
/// Layout: `index = layer * (rows * cols) + row * cols + col`.
402-
fn position_from_flat_index(index: usize, rows: usize, cols: usize) -> (usize, usize, usize) {
403-
let layer = index / (cols * rows);
404-
let layer_offset = index % (cols * rows);
405-
let row = layer_offset / cols;
406-
let col = layer_offset % cols;
407-
(row, col, layer)
408-
}

‎rmk/src/host/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,6 @@ pub use rynk::RYNK_BLE_CHUNK_SIZE;
1616
#[cfg(all(feature = "rynk", not(feature = "_no_usb")))]
1717
pub use rynk::{RYNK_USB_MAX_PACKET_SIZE, RynkUsbTransport};
1818
#[cfg(feature = "rynk")]
19-
pub use rynk::RynkService;
19+
pub use rynk::{RynkService, write_error_response};
2020
#[cfg(feature = "vial")]
2121
pub use via::VialService as HostService;

‎rmk/src/host/rynk/mod.rs‎

Lines changed: 67 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -3,20 +3,17 @@
33
//! `RynkService` is the transport-agnostic core. It holds a
44
//! [`KeyboardContext`](super::context::KeyboardContext) and exposes:
55
//!
6-
//! - [`dispatch`](RynkService::dispatch) — turn one inbound message into
7-
//! one outbound message, in-place on the same buffer.
6+
//! - [`dispatch`](RynkService::dispatch) — process inbound message in-place
87
//! - [`write_topic`](RynkService::write_topic) — fill a buffer with one
98
//! topic message for the per-transport publisher task.
10-
//!
11-
//! Per-transport adapters (`UsbTransport`, `BleTransport`) live under
12-
//! [`transport`] and call `dispatch` inside their own RX/TX loop.
139
1410
mod handlers;
1511
mod topics;
1612
pub mod transport;
1713
pub(crate) mod wire;
1814

1915
use rmk_types::protocol::rynk::{Cmd, RYNK_HEADER_SIZE, RYNK_MIN_BUFFER_SIZE, RynkError, RynkMessage};
16+
// TODO: implement and remove this comment
2017
// Re-exports used by macro-generated entry code (Phase 6). The `unused`
2118
// lint can flip on for feature combos where the macro path isn't compiled —
2219
// keep them at the module surface so manually-driven examples still see
@@ -38,126 +35,102 @@ const _: () = assert!(
3835
floor",
3936
);
4037

41-
/// Max packet size to use for the Rynk USB BULK IN/OUT endpoints. 64 B is
42-
/// the full-speed maximum and works on every embassy-usb driver; HS-only
43-
/// devices can override at the call site for higher throughput.
38+
/// Max packet size to use for the Rynk USB BULK IN/OUT endpoints.
4439
#[cfg(not(feature = "_no_usb"))]
4540
pub const RYNK_USB_MAX_PACKET_SIZE: u16 = 64;
4641

47-
/// Maximum BLE chunk size that fits in a single GATT write — matches the
48-
/// `output_data` characteristic's value-array length in `ble_server.rs`
49-
/// (≈ MTU − 3 for the typical 247-byte negotiated MTU).
42+
/// Maximum BLE chunk size that fits in a single GATT write
5043
#[cfg(feature = "_ble")]
5144
pub const RYNK_BLE_CHUNK_SIZE: usize = 244;
5245

53-
/// Transport-agnostic Rynk dispatch core.
54-
///
55-
/// Construct via [`RynkService::new`] with a borrowed `KeyMap`. Hand it
56-
/// to one or both transports' `run` futures and join the futures into
57-
/// the existing `::rmk::run_all!(…)` chain (same pattern Vial uses for
58-
/// `host_service.run()`).
46+
/// Transport-agnostic Rynk service.
5947
pub struct RynkService<'a> {
6048
pub(super) ctx: KeyboardContext<'a>,
6149
}
6250

6351
impl<'a> RynkService<'a> {
64-
/// Build a service over a `&KeyMap`. The keymap outlives every
65-
/// transport future, so the borrow is sound.
6652
pub fn new(keymap: &'a KeyMap<'a>) -> Self {
6753
Self {
6854
ctx: KeyboardContext::new(keymap),
6955
}
7056
}
7157

72-
/// Process one inbound message in place: route on `cmd`, let the
73-
/// matching handler write the response payload into the same
74-
/// buffer, then patch the `LEN` field.
58+
/// Process one inbound message in place.
7559
///
7660
/// `cmd` and `seq` are echoed verbatim from request to response —
77-
/// they're never re-written. The bytes after `RYNK_HEADER_SIZE +
78-
/// msg.payload_len()` are unspecified after this returns.
79-
///
80-
/// On a malformed header or a topic CMD sent as a request, returns
81-
/// `Err(RynkError::InvalidRequest)` and the transport drops the
82-
/// message. Handler-level errors are encoded into the payload as
83-
/// `Err::<(), RynkError>(e)` and reported as `Ok(())` — every
84-
/// legitimate request gets a reply.
85-
///
86-
/// Adding a `Cmd`:
87-
/// 1. Append the variant in `rmk-types/src/protocol/rynk/cmd.rs`.
88-
/// 2. Add the match arm here.
89-
/// 3. Add the matching `handle_xxx` method in the relevant
90-
/// `handlers/*.rs` file.
61+
/// they're never re-written.
62+
/// The bytes after `RYNK_HEADER_SIZE + msg.payload_len()` are
63+
/// unspecified after this returns.
9164
pub async fn dispatch(&self, msg: &mut [u8]) -> Result<(), RynkError> {
9265
let cmd = msg.cmd()?;
9366

94-
let handler_result: Result<usize, RynkError> = match cmd {
67+
let payload_len: usize = match cmd {
9568
// ── System ──
96-
Cmd::GetVersion => self.handle_get_version(msg.payload_mut()?).await,
97-
Cmd::GetCapabilities => self.handle_get_capabilities(msg.payload_mut()?).await,
98-
Cmd::Reboot => self.handle_reboot(msg.payload_mut()?).await,
99-
Cmd::BootloaderJump => self.handle_bootloader_jump(msg.payload_mut()?).await,
100-
Cmd::StorageReset => self.handle_storage_reset(msg.payload_mut()?).await,
69+
Cmd::GetVersion => self.handle_get_version(msg.payload_mut()?).await?,
70+
Cmd::GetCapabilities => self.handle_get_capabilities(msg.payload_mut()?).await?,
71+
Cmd::Reboot => self.handle_reboot(msg.payload_mut()?).await?,
72+
Cmd::BootloaderJump => self.handle_bootloader_jump(msg.payload_mut()?).await?,
73+
Cmd::StorageReset => self.handle_storage_reset(msg.payload_mut()?).await?,
10174

10275
// ── Keymap (incl. encoder) ──
103-
Cmd::GetKeyAction => self.handle_get_key_action(msg.payload_mut()?).await,
104-
Cmd::SetKeyAction => self.handle_set_key_action(msg.payload_mut()?).await,
105-
Cmd::GetDefaultLayer => self.handle_get_default_layer(msg.payload_mut()?).await,
106-
Cmd::SetDefaultLayer => self.handle_set_default_layer(msg.payload_mut()?).await,
107-
Cmd::GetEncoderAction => self.handle_get_encoder_action(msg.payload_mut()?).await,
108-
Cmd::SetEncoderAction => self.handle_set_encoder_action(msg.payload_mut()?).await,
76+
Cmd::GetKeyAction => self.handle_get_key_action(msg.payload_mut()?).await?,
77+
Cmd::SetKeyAction => self.handle_set_key_action(msg.payload_mut()?).await?,
78+
Cmd::GetDefaultLayer => self.handle_get_default_layer(msg.payload_mut()?).await?,
79+
Cmd::SetDefaultLayer => self.handle_set_default_layer(msg.payload_mut()?).await?,
80+
Cmd::GetEncoderAction => self.handle_get_encoder_action(msg.payload_mut()?).await?,
81+
Cmd::SetEncoderAction => self.handle_set_encoder_action(msg.payload_mut()?).await?,
10982
#[cfg(feature = "bulk_transfer")]
110-
Cmd::GetKeymapBulk => self.handle_get_keymap_bulk(msg.payload_mut()?).await,
83+
Cmd::GetKeymapBulk => self.handle_get_keymap_bulk(msg.payload_mut()?).await?,
11184
#[cfg(feature = "bulk_transfer")]
112-
Cmd::SetKeymapBulk => self.handle_set_keymap_bulk(msg.payload_mut()?).await,
85+
Cmd::SetKeymapBulk => self.handle_set_keymap_bulk(msg.payload_mut()?).await?,
11386

11487
// ── Macro ──
115-
Cmd::GetMacro => self.handle_get_macro(msg.payload_mut()?).await,
116-
Cmd::SetMacro => self.handle_set_macro(msg.payload_mut()?).await,
88+
Cmd::GetMacro => self.handle_get_macro(msg.payload_mut()?).await?,
89+
Cmd::SetMacro => self.handle_set_macro(msg.payload_mut()?).await?,
11790

11891
// ── Combo ──
119-
Cmd::GetCombo => self.handle_get_combo(msg.payload_mut()?).await,
120-
Cmd::SetCombo => self.handle_set_combo(msg.payload_mut()?).await,
92+
Cmd::GetCombo => self.handle_get_combo(msg.payload_mut()?).await?,
93+
Cmd::SetCombo => self.handle_set_combo(msg.payload_mut()?).await?,
12194
#[cfg(feature = "bulk_transfer")]
122-
Cmd::GetComboBulk => self.handle_get_combo_bulk(msg.payload_mut()?).await,
95+
Cmd::GetComboBulk => self.handle_get_combo_bulk(msg.payload_mut()?).await?,
12396
#[cfg(feature = "bulk_transfer")]
124-
Cmd::SetComboBulk => self.handle_set_combo_bulk(msg.payload_mut()?).await,
97+
Cmd::SetComboBulk => self.handle_set_combo_bulk(msg.payload_mut()?).await?,
12598

12699
// ── Morse ──
127-
Cmd::GetMorse => self.handle_get_morse(msg.payload_mut()?).await,
128-
Cmd::SetMorse => self.handle_set_morse(msg.payload_mut()?).await,
100+
Cmd::GetMorse => self.handle_get_morse(msg.payload_mut()?).await?,
101+
Cmd::SetMorse => self.handle_set_morse(msg.payload_mut()?).await?,
129102
#[cfg(feature = "bulk_transfer")]
130-
Cmd::GetMorseBulk => self.handle_get_morse_bulk(msg.payload_mut()?).await,
103+
Cmd::GetMorseBulk => self.handle_get_morse_bulk(msg.payload_mut()?).await?,
131104
#[cfg(feature = "bulk_transfer")]
132-
Cmd::SetMorseBulk => self.handle_set_morse_bulk(msg.payload_mut()?).await,
105+
Cmd::SetMorseBulk => self.handle_set_morse_bulk(msg.payload_mut()?).await?,
133106

134107
// ── Fork ──
135-
Cmd::GetFork => self.handle_get_fork(msg.payload_mut()?).await,
136-
Cmd::SetFork => self.handle_set_fork(msg.payload_mut()?).await,
108+
Cmd::GetFork => self.handle_get_fork(msg.payload_mut()?).await?,
109+
Cmd::SetFork => self.handle_set_fork(msg.payload_mut()?).await?,
137110

138111
// ── Behavior ──
139-
Cmd::GetBehaviorConfig => self.handle_get_behavior_config(msg.payload_mut()?).await,
140-
Cmd::SetBehaviorConfig => self.handle_set_behavior_config(msg.payload_mut()?).await,
112+
Cmd::GetBehaviorConfig => self.handle_get_behavior_config(msg.payload_mut()?).await?,
113+
Cmd::SetBehaviorConfig => self.handle_set_behavior_config(msg.payload_mut()?).await?,
141114

142115
// ── Connection ──
143-
Cmd::GetConnectionType => self.handle_get_connection_type(msg.payload_mut()?).await,
116+
Cmd::GetConnectionType => self.handle_get_connection_type(msg.payload_mut()?).await?,
144117
#[cfg(feature = "_ble")]
145-
Cmd::GetBleStatus => self.handle_get_ble_status(msg.payload_mut()?).await,
118+
Cmd::GetBleStatus => self.handle_get_ble_status(msg.payload_mut()?).await?,
146119
#[cfg(feature = "_ble")]
147-
Cmd::SwitchBleProfile => self.handle_switch_ble_profile(msg.payload_mut()?).await,
120+
Cmd::SwitchBleProfile => self.handle_switch_ble_profile(msg.payload_mut()?).await?,
148121
#[cfg(feature = "_ble")]
149-
Cmd::ClearBleProfile => self.handle_clear_ble_profile(msg.payload_mut()?).await,
122+
Cmd::ClearBleProfile => self.handle_clear_ble_profile(msg.payload_mut()?).await?,
150123

151124
// ── Status ──
152-
Cmd::GetCurrentLayer => self.handle_get_current_layer(msg.payload_mut()?).await,
153-
Cmd::GetMatrixState => self.handle_get_matrix_state(msg.payload_mut()?).await,
125+
Cmd::GetCurrentLayer => self.handle_get_current_layer(msg.payload_mut()?).await?,
126+
Cmd::GetMatrixState => self.handle_get_matrix_state(msg.payload_mut()?).await?,
154127
#[cfg(feature = "_ble")]
155-
Cmd::GetBatteryStatus => self.handle_get_battery_status(msg.payload_mut()?).await,
128+
Cmd::GetBatteryStatus => self.handle_get_battery_status(msg.payload_mut()?).await?,
156129
#[cfg(all(feature = "_ble", feature = "split"))]
157-
Cmd::GetPeripheralStatus => self.handle_get_peripheral_status(msg.payload_mut()?).await,
158-
Cmd::GetWpm => self.handle_get_wpm(msg.payload_mut()?).await,
159-
Cmd::GetSleepState => self.handle_get_sleep_state(msg.payload_mut()?).await,
160-
Cmd::GetLedIndicator => self.handle_get_led_indicator(msg.payload_mut()?).await,
130+
Cmd::GetPeripheralStatus => self.handle_get_peripheral_status(msg.payload_mut()?).await?,
131+
Cmd::GetWpm => self.handle_get_wpm(msg.payload_mut()?).await?,
132+
Cmd::GetSleepState => self.handle_get_sleep_state(msg.payload_mut()?).await?,
133+
Cmd::GetLedIndicator => self.handle_get_led_indicator(msg.payload_mut()?).await?,
161134

162135
// Topic CMDs — host shouldn't be sending these.
163136
Cmd::LayerChange | Cmd::WpmUpdate | Cmd::ConnectionChange | Cmd::SleepState | Cmd::LedIndicator => {
@@ -169,15 +142,6 @@ impl<'a> RynkService<'a> {
169142
}
170143
};
171144

172-
let payload_len = match handler_result {
173-
Ok(n) => n,
174-
Err(e) => {
175-
let envelope: Result<(), RynkError> = Err(e);
176-
postcard::to_slice(&envelope, msg.payload_mut()?)
177-
.map(|s| s.len())
178-
.unwrap_or(0)
179-
}
180-
};
181145
msg.set_payload_len(payload_len as u16)?;
182146
Ok(())
183147
}
@@ -199,3 +163,22 @@ impl<'a> RynkService<'a> {
199163
Ok(())
200164
}
201165
}
166+
167+
/// Encode a `RynkError` as the response payload (`Result<(), RynkError>::Err(e)`
168+
/// envelope) and patch the header LEN. Transports call this when
169+
/// [`RynkService::dispatch`] returns `Err`, so the host always receives a
170+
/// `Result<T, RynkError>` envelope reply instead of a dropped message.
171+
///
172+
/// `cmd` and `seq` in the header are left untouched — the host correlates
173+
/// the reply with its outgoing request by `seq`, regardless of the error.
174+
/// Returns `Err(InvalidRequest)` if `msg` itself is shorter than
175+
/// `RYNK_HEADER_SIZE` (i.e. there isn't even room for the header) —
176+
/// transports always pass `RYNK_BUFFER_SIZE` buffers, so this is dead in
177+
/// practice.
178+
pub fn write_error_response(msg: &mut [u8], err: RynkError) -> Result<(), RynkError> {
179+
let envelope: Result<(), RynkError> = Err(err);
180+
let n = postcard::to_slice(&envelope, msg.payload_mut()?)
181+
.map(|s| s.len())
182+
.unwrap_or(0);
183+
msg.set_payload_len(n as u16)
184+
}

0 commit comments

Comments
 (0)