Fix review issues#7
Merged
Merged
Conversation
PhantomData<fn() -> ()> is Send + Sync, so it did not prevent Bmi323Async from implementing Sync as the comment claimed. PhantomData<Cell<()>> is Send + !Sync, correctly preventing Sync.
All fields (u8, StatusWord, ErrorWord) implement Eq, but DeviceState was missing the Eq derive.
Add missing assertions for significant_motion (bit 6), gyro_data_ready (bit 12), and accel_data_ready (bit 13), and expand the false-assertion block for InterruptStatus(0).
The configure_interrupt_pin method uses matches! to map these enum variants to register bit values. Without explicit discriminants, reordering the variants would silently break the bit assignments.
Replace hard-coded addresses 0x0D, 0x0E, 0x0F in read_interrupt_status with named INT_STATUS_INT1, INT_STATUS_INT2, INT_STATUS_IBI constants matching the pattern used by every other register in the driver.
The transport layer's fixed-size stack buffer already catches oversized reads with a slice bounds panic. Using debug_assert! preserves detection in debug builds while avoiding redundant checks in release.
The STM32G030F6 example binaries were not being formatted or linted in CI. Added cargo fmt --check and cargo clippy steps. Fixed pre-existing formatting issues in the example source files.
AccelConfig and GyroConfig already implement Default. Adding it to FifoConfig with all data streams disabled provides a consistent starting configuration that matches the hardware reset state.
AccelConfig and GyroConfig already implement Default. Adding it to FifoConfig (derived since all fields are bool defaulting to false) provides a consistent starting configuration that matches the hardware reset state.
PhantomData<Cell<()>> prevented wrapping the driver in Mutex/RefCell for sharing across Embassy tasks, which is a common embedded pattern. The blocking Bmi323 had no such marker. The async driver has no invariant that requires !Sync, so the restriction is removed.
After a soft reset ACC_CONF and GYR_CONF read 0x0000, meaning the on-chip range bits are G2 and Dps125. The driver was caching G8/Dps2000 as the startup defaults, so as_g()/as_dps() calls made before any set_accel_config()/set_gyro_config() would apply the wrong scale factor. Change the initial cache values to G2/Dps125 to match the actual hardware POR state and update all associated documentation.
0x012C (feature engine config payload), 0x0001 for FEATURE_IO_STATUS sync trigger and FEATURE_CTRL enable, and the CMD opcodes SOFT_RESET/SELF_TEST now have documented named constants in registers.rs referencing the relevant datasheet sections.
InterruptSource::I3cSync existed and could be routed to an interrupt channel, but InterruptStatus had no corresponding bit-9 accessor. Added i3c_sync() and a matching test.
The BMI323 INT_STATUS register bit 10 is named err_status in the datasheet; it routes the error-engine status, not a generic feature-engine event. The previous name FeatureStatus was misleading. Renamed: - InterruptSource::FeatureStatus -> InterruptSource::ErrorStatus - InterruptStatus::feature_status() -> InterruptStatus::error_status()
The magic literal 64 appeared four times across transport.rs scratch buffers. Replaced with the public constant MAX_WORDS_PER_READ so the limit is named in one place. Also promoted debug_assert! to assert! in read_fifo_words: in release builds a slice over the 64-word scratch buffer would still panic via index bounds, so the assert! is not an extra cost—it just gives a clearer message earlier.
FIFO_WATERMARK is a 10-bit field (range 0..=1023) per BMI323 datasheet §6.11.17; the existing & 0x03FF mask is correct. FIFO_FILL_LEVEL is a separate 11-bit field (0..=2047), consistent with the & 0x07FF mask. Added doc comments on set_fifo_config explaining the truncation, and added a comment citing the datasheet section at the mask site.
Previously the loop slept 10 ms before the first status read. Self-test typically completes in ~30 ms; reading first avoids an unnecessary 10 ms delay when the result is already available early in the polling window.
BMI323 datasheet Table 3 specifies t_start = 2 ms typical after a soft reset. The 2 ms delay is correct; added a comment so the origin is clear to future readers.
…s errors On a fast SPI bus, back-to-back polls of FEATURE_IO1 could exhaust the 32-iteration window before the feature engine reaches a ready state (0x1 or 0x5), causing false FeatureEngineNotReady errors. Added a 200 µs delay between iterations, mirroring the pacing used in run_self_test_inner. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The BMI323 datasheet (§5.9) defines raw TEMP_DATA value 0x8000 as "invalid temperature", returned when no valid sample is available yet. Previously read_temperature_celsius silently converted it to ~−41 °C. Add Error::InvalidTemperature, extract a shared temperature_raw_to_celsius helper in driver.rs, and return the new error variant when the sentinel is detected. Covered by three new unit tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…types Every constant in registers.rs now carries a §6.1.2 / §6.2.2 inline comment pointing to its exact register entry in the BMI323 datasheet (BST-BMI323-DS000-12). Three existing references that cited non-existent section numbers (§6.11.37, §4.5, §6.11.25) are corrected. Every public type and enum in types.rs now has a §-prefixed reference in its doc comment linking it to the register field(s) and/or functional description section that defines its encoding. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…bit fields Add §-prefixed BST-BMI323-DS000-12 citations to all remaining code paths that were missing them: - driver.rs: fix temperature_raw_to_celsius doc (wrong §5.9 → §6.1.2 Register (0x09) temp_data; §5.6.3); cite formula source - types.rs: cite §5.6.1/§5.6.2 for the 32768 divisor in scale_g_per_lsb / scale_dps_per_lsb - blocking_driver.rs: fix §6.11.17 → §6.1.2 Register (0x35) for FIFO_WATERMARK; add §6.1.2 Register (0x15) for fill-level mask; add §5.7.4 for FIFO flush; add §6.1.2 Register (0x11) for FEATURE_IO1 self-test bits; add §6.2.2 Register (0x02) for EXT_GEN_SET_1 field layout; add §6.1.2 Register (0x10) bit-assignment table for FEATURE_IO0 - transport.rs: cite §7.2.3 for SPI read-bit encoding and dummy bytes; cite §7.2.4.2 for I2C dummy bytes (both blocking and async paths) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Merged src/async_driver.rs and src/blocking_driver.rs (~1800 lines, ~700 lines of which were mechanical async/await duplication) into a single src/driver_impl.rs (~975 lines) using the maybe-async 0.2.10 crate. Cargo feature flags select the mode: `blocking` (default, enables maybe-async/is_sync) or `async` (enables embedded-hal-async). Enabling both simultaneously is a compile error. The public API changes: Bmi323Async is removed; Bmi323 serves both modes. All examples, embassy-stm32g030f6-examples, and integration tests are updated accordingly. CI is also fixed: --all-features was a compile error with the new mutually exclusive feature flags. Each check/test step now runs explicitly with --features blocking and --no-default-features --features async. Coverage merges both runs before upload. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The self-test modified accel/gyro config and feature engine state and then restored them afterwards, adding 6 I2C reads and 6 writes around every test run. Instead, document that the sensor is in an undefined configuration after the self-test and the caller must reconfigure as needed. This aligns with how init() behaves (no implicit state assumptions). Also inlined run_self_test_inner into run_self_test since the wrapper was now a trivial single-call passthrough. Updated tests to match the new transaction sequence and removed the now-obsolete restore-error tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previously pin.wait_for_high() errors were silently dropped with .ok(), meaning callers could not distinguish a GPIO failure from a successful wait. The underlying GPIO error value cannot be preserved in Error<E> without a second type parameter, so a unit variant is used instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
0x1 and 0x5 are the BMI323 feature_io1 error_status values for "initialization successful" and "no error / normal running state". Named constants make the intent clear at the call site. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The BMI323 datasheet (§6.1.2, Register (0x11) feature_io1) defines 13 error_status values. Previously only 0x01 and 0x05 were named. Added the full set for reference. Also renamed FEATURE_ENGINE_STATUS_INIT_OK to FEATURE_ENGINE_STATUS_ACTIVATED to match the datasheet wording. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two remaining magic numbers: 0x0001 written to FIFO_CTRL to trigger a flush, and 13 used as the maximum interrupt hold-time exponent in three places. Replace with named constants for clarity. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The enum currently fits in 4 bits (values 0..=9) but the two nibble fields were packed without masking. A future variant above 15 would silently corrupt the adjacent nibble. Mask defensively with & 0x0F. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…onfig Writing to ACC_CONF or GYR_CONF returns the sensor from its alternate configuration when AltConfigControl::reset_on_user_config_write is set. This is non-obvious side-effect worth documenting on both methods. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Blocking example imported AccelMode, AccelRange, AverageSamples, Bandwidth, GyroMode, GyroRange but used none of them. Async example imported AccelRange, AverageSamples, Bandwidth but used none of them. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…atus constants The FEATURE_ENGINE_STATUS_* constants document all datasheet-defined FEATURE_IO1.error_status values (§6.1.2, Register 0x11). Most are not referenced in driver logic but are kept as documentation. Add per-constant #[allow(dead_code)] to silence the compiler warnings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…erConfig - ActiveLevel and OutputMode get #[derive(Default)] with Low/PushPull as defaults (matching BMI323 power-on reset state), which enables InterruptPinConfig to also derive Default (enabled: false, low, push-pull) - MotionAxes gets a manual Default returning XYZ — the all-false derive default would be useless for motion detection - StepCounterConfig gets Default delegating to disabled(), keeping the two in sync Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the four separate test files (blocking_i2c_transactions, async_i2c_transactions, blocking_spi_transactions, async_spi_transactions) with two shared source files (i2c_transactions.rs, spi_transactions.rs), each compiled twice — once per feature — via Cargo.toml [[test]] entries that point the same `path` under different names. The shared files use two small macros: - `run!(expr)` — evaluates directly in blocking mode, drives via block_on in async mode, replacing every block_on(...) call - `delay_tx!(ms/us, n)` — produces the right DelayTransaction variant for the active feature Additional improvements: - Switch from the loose `TestDelay` (no assertions) to `CheckedDelay` for all blocking tests that involve delay calls, giving them the same strict validation already present in the async tests. Error-path tests (init failures) now also validate the 2 ms soft-reset delay. - The enable_feature_engine success test (previously used NoopDelay in both variants) now validates the single 200 µs pre-poll delay. - Fill async SPI parity gaps: add spi_feature_engine_enable and spi_any_motion_configuration tests that were present only in blocking SPI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Allows users to use Error<E> with Box<dyn Error> and ? into anyhow/eyre without manual wrapping. core::error::Error is stable since Rust 1.81, within our MSRV of 1.85. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eatures cargo build --features async silently activates both blocking (default) and async, producing 49 cryptic type errors. The guards now emit a clear actionable message as the first error so users know exactly what to fix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-features
Async is now the implicit default — adding the crate with no feature flags
gives the async driver. Blocking users must opt out explicitly:
bmi323-driver = { version = "...", default-features = false, features = ["blocking"] }
This matches the modern embedded-rs convention where Embassy/async is the
primary target. The compile_error guards and CI commands are updated to
reflect the new default.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The feature flag design is now:
default = [] # async behaviour when nothing is specified
async = [] # explicit no-op marker; gates async examples/tests
blocking = [...] # opts into synchronous embedded-hal API
Library users:
bmi323-driver = "0.2" # async (default)
bmi323-driver = { ..., features = ["async"] } # async (explicit)
bmi323-driver = { ..., features = ["blocking"] } # sync — no
# default-features = false
# needed
embedded-hal-async is now a hard dependency (thin trait crate); blocking
users who want to exclude it can still use default-features = false.
All cfg(feature = "async") / cfg(not(feature = "async")) guards in src
and tests are replaced with cfg(not(feature = "blocking")) /
cfg(feature = "blocking") so correctness depends only on the blocking
feature rather than an explicit async opt-in.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Each [[test]] target now has its own thin wrapper file that include!s the shared implementation, giving Cargo a unique path per target. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… POR Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7 +/- ##
==========================================
- Coverage 99.13% 96.78% -2.35%
==========================================
Files 6 5 -1
Lines 1846 1245 -601
==========================================
- Hits 1830 1205 -625
- Misses 16 40 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
No description provided.