Improve Embedded Readiness, Runtime Portability, and Postcard-Ready Serialization#875
Open
jsmnbom wants to merge 13 commits intobuttplugio:devfrom
Open
Improve Embedded Readiness, Runtime Portability, and Postcard-Ready Serialization#875jsmnbom wants to merge 13 commits intobuttplugio:devfrom
jsmnbom wants to merge 13 commits intobuttplugio:devfrom
Conversation
…ices On BlueZ, calling peripheral.disconnect() on a device that has already disconnected can hang indefinitely waiting for a D-Bus reply that will never arrive. Fix by checking is_connected() first and skipping the disconnect call if the device is already gone. A 5-second tokio::time::timeout is still applied around the actual disconnect in case the device drops between the check and the call.
When build_device_handle() fails (e.g. protocol identification error, hardware init failure), the error was only logged server-side. The client received no notification and would be left waiting indefinitely. Fix by constructing an ErrorV0 from the ButtplugError and broadcasting it via server_sender, mirroring how other server-side errors are reported.
tokio::time::sleep_until is tokio-specific and does not compile for non-tokio runtimes (e.g. WASM). Replace with async_manager::sleep, computing the remaining duration via saturating_duration_since, which is consistent with how other sleeps in this crate are expressed.
Some Lovense devices treat battery reporting as a togglable subscription rather than a request/response: they do not promptly reply to a Battery; query but instead start streaming battery level at an interval, or do not reply at all. Without a timeout, handle_battery_level_cmd hangs indefinitely in those cases. See: buttplugio#865 Refactor the response loop from 'while let Ok' to 'loop { select! { biased; ... } }', racing the notification stream against a LOVENSE_COMMAND_TIMEOUT_MS sleep. On timeout, return a ProtocolSpecificError. Also handles Err(_) from recv() (channel closed) in the same arm as explicit Disconnected events. A fixed timeout is a known-bad workaround, but strictly better than hanging forever.
The patterns used in the Lovense and VibCrafter protocol impls do not require Unicode support, so the full regex crate is unnecessary here. Note: regex is still pulled in transitively via jsonschema -> fancy-regex -> regex in most common builds, so this does not eliminate the dependency from the binary today. However: 1. Embedded builds that avoid jsonschema code paths will no longer pay for regex as a direct dependency. 2. Once Stranger6667/jsonschema#470 is resolved (jsonschema drops fancy-regex), removing this direct dep will actually shrink the binary.
The old API was caller.update_with_configuration(config) where caller was the defaults and config was the device-specific entry. The semantics were inverted: 'update with configuration' implied the arg was the update source, but the receiver was actually the fallback. Rename to with_defaults(defaults) and invert: self is now the device-specific config (takes ownership), defaults is the optional protocol-level fallback (borrowed). Optional fields in self take priority; defaults fill in None slots via or_else. Call site in load_main_config simplifies: the if/else on whether defaults exist collapses to config.clone().with_defaults(default.as_ref()).into() since with_defaults(None) is a no-op.
…ition> Each ServerDeviceDefinition stored in the base map is now heap-allocated once and reference-counted. Benefits: - DeviceConfigurationManagerBuilder::finish() no longer clones every definition — it does cheap Arc::clone() instead. - load_main_config() creates one Arc<ServerDeviceDefinition> per config entry and shares it across all identifiers for that entry (e.g. multiple BLE advertisement names for the same device model). - ServerDeviceDefinitionBuilder::from_base() in device_definition() reads through the Arc without cloning the value. API changes: - DeviceConfigurationManagerBuilder::base_device_definition() now takes Arc<ServerDeviceDefinition> directly. - base_device_definitions() getter is now pub (was pub(crate)). - Add DeviceConfigurationManager::new() for constructing directly from pre-built maps (bypasses builder validation — see doc comment).
ServerDeviceFeature::new now takes String and owned SmallVecEnumMap values instead of &str and borrowed maps. This makes ownership explicit at the call site and removes the hidden clones inside the constructor. For ConfigBaseDeviceFeature -> ServerDeviceFeature, the fields can now move directly into the new value. For ConfigUserDeviceFeature::with_base_feature(), only the reused base_feature fields are cloned, which makes the remaining cloning costs visible and local to the actual reuse case.
Endpoint now serializes as lowercase strings for human-readable formats and as repr(u8) values for binary formats. This preserves the existing JSON/device-config representation while making Endpoint compatible with postcard-style binary serialization without string allocation.
ServerDeviceDefinition now derives Serialize and Deserialize. This makes device definitions serializable for future config caching and postcard-compatible transport work.
BaseDeviceIdentifier now exposes public getters while keeping mutation crate-private. This allows external code to inspect identifiers returned by public config-manager APIs.
ServerDeviceFeature and its output property types now serialize all fields needed for binary round trips. This keeps postcard-style serialization from losing empty, false, or zero-valued fields that must remain present in non-self-describing formats.
ClientDeviceOutputCommand now derives Debug and Clone. This matches the command-value helper type and makes the client output command easier to inspect and pass through higher-level client code.
jsmnbom
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR is a grouped set of changes that makes buttplug significantly more embedded-ready in practice, while also improving stability and runtime portability for general upstream users.
High-level goals:
This PR, together with already merged PRs #839 and #852, is intended to replace #833 with a cleaner, better-scoped set of changes.
It also makes buttplug work well for my use-case in https://github.com/jsmnbom/esp-butt, with only minor downstream fork deltas still needed:
What Changed
Reliability Fixes
Runtime Portability and Dependency Footprint
Device Config Quality and Clone Reduction
Postcard-Compatible Serialization Groundwork
Why This Replaces PR #833
Compared to #833, this series is split into smaller, logical commits and is easier to review and validate incrementally.
It also aligns the upstream story around postcard-compatible serde support rather than embedded-only changes, while keeping the fork-only items isolated.
Scope and Non-Goals
Included:
Not included:
Validation
Commit Groups
Reliability fixes:
Runtime portability and dependency footprint:
Device config cleanup and clone reduction:
Postcard-compatible serialization groundwork:
Follow-up Needed
We still need a proper memory-focused test pass similar to concerns raised in #833.