feat(imu, gpio)!: Add Timestamped BMI088 telemetry & digital read support#54
Merged
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough为数据通路引入可选与每样本时间戳，扩展协议/序列化/反序列化与数据视图；将 BMI088 IMU 驱动改为中断锚定 + 延迟读以携带时间戳；新增高精度 Timer 并调整板级时钟与 ISR 集成；添加 BMI088 温度驱动与服务编排；主机与 Vendor/Agent 层新增温度回调占位。 Changes数据契约、协议与序列化/反序列化
BMI088 驱动、服务与上行流程
GPIO 输入采样与可选时间戳
高精度 Timer 与板级时钟调整
sequenceDiagram
autonumber
participant IRQ as BMI088 IRQ
participant Timer as Timer (MCHTMR)
participant Driver as BMI088 Driver
participant Service as service_pending_reads
participant SPI as SPI Controller
participant Serializer as USB Serializer
participant Host as Host/Deserializer
IRQ->>Timer: 读取 timestamp_quarter_us()
Timer-->>IRQ: 返回 timestamp
IRQ->>Driver: data_ready_callback(timestamp)
Driver->>Driver: 存储 pending timestamp (InterruptLockGuard)
IRQ->>Service: service_pending_reads()
Service->>Driver: service_pending_read()（按优先级）
Driver->>SPI: 发起异步 SPI 读取（锚定 launch timestamp）
SPI-->>Driver: transmit_receive_async_callback(rx_buf)
Driver->>Driver: 原子消费 active timestamp
Driver->>Serializer: handle_uplink(data, active_timestamp)
Serializer->>Host: 发送带 timestamp 的包（IMU/Temperature）
Host->>Host: 反序列化并调用相应回调（含 temperature）
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
firmware/c_board/app/src/gpio/gpio.hpp (1)
62-68: ⚡ Quick win统一 ASAP/周期采样的取时顺序。
这两条路径现在都是先读电平再取时，而中断路径是先取时再读电平。既然这次改动的重点是“采样时间戳”，这里最好统一成先取时，避免把额外的 HAL 调用延迟算进时间戳里。
✏️ 建议修改
if (data.asap) { const auto& hardware = channel_hardware(channel_index); - const bool high = - HAL_GPIO_ReadPin(hardware.gpio_port, hardware.gpio_pin) == GPIO_PIN_SET; const uint32_t timestamp_quarter_us = current_timestamp_quarter_us(); + const bool high = + HAL_GPIO_ReadPin(hardware.gpio_port, hardware.gpio_pin) == GPIO_PIN_SET; publish_digital_input_sample(channel_index, high, timestamp_quarter_us); } @@ const auto& hardware = channel_hardware(static_cast<uint8_t>(channel_index)); - const bool high = - HAL_GPIO_ReadPin(hardware.gpio_port, hardware.gpio_pin) == GPIO_PIN_SET; const uint32_t timestamp_quarter_us = current_timestamp_quarter_us(); + const bool high = + HAL_GPIO_ReadPin(hardware.gpio_port, hardware.gpio_pin) == GPIO_PIN_SET; publish_digital_input_sample( static_cast<uint8_t>(channel_index), high, timestamp_quarter_us);Also applies to: 81-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@firmware/c_board/app/src/gpio/gpio.hpp` around lines 62 - 68, ASAP and periodic sampling paths currently read the GPIO level before capturing the timestamp; change both paths (the block using channel_hardware(), HAL_GPIO_ReadPin(), current_timestamp_quarter_us(), and publish_digital_input_sample()) to capture timestamp first by calling current_timestamp_quarter_us() before HAL_GPIO_ReadPin(), then pass the timestamp and the read level into publish_digital_input_sample to match the interrupt path ordering and avoid counting HAL call latency in the sample timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/include/librmcs/spec/gpio.hpp`:
- Line 35: The enum bitmask expression containing GpioCapability::kPullUp |
GpioCapability::kPullDown | GpioCapability::kTimestampedDigitalRead exceeds the
100-column limit; split this expression across multiple lines (for example break
after kPullUp and/or kPullDown so each line is <100 cols) in the file around the
GpioCapability usage to satisfy the 100-column width rule and run
clang-format-check to ensure formatting compliance.
In `@firmware/c_board/app/src/spi/bmi088/temperature.hpp`:
- Around line 47-58: 在 service_pending_read() 中把用于 SPI 启动锚点的时间戳采样移动到调用
read_async(...) 之前：在调用 read_async(RegisterAddress::kTempMsb,
kTemperatureReadSizeBytes) 之前即时采样
timer::timer->timepoint().time_since_epoch().count() 并存入
active_probe_launch_timestamp_quarter_us_，然后按当前语义用
has_active_probe_launch_timestamp_.store(..., std::memory_order_release)
标记存在；随后调用 read_async 并按原来逻辑在成功后将 probe_pending_ 置为 false。保持现有的 memory_order
顺序与逻辑不变。
In `@firmware/rmcs_board/app/src/app.cpp`:
- Around line 44-48: 将 timer::timer.init() 提前到所有 BMI088 初始化之前：在启动序列中先调用
timer::timer.init()，然后再依次调用 spi::bmi088::accelerometer.init(),
spi::bmi088::gyroscope.init(), spi::bmi088::temperature.init()，以确保 IRQ
回调中读取时间戳时定时器已就绪；同时检查 timer::timer.init() 的返回/错误处理并保留现有 IMU 初始化顺序与逻辑。
In `@firmware/rmcs_board/app/src/spi/bmi088/temperature.hpp`:
- Around line 59-69: service_pending_read() currently calls read_async(...)
before recording the SPI launch timestamp, which makes the timestamp include
read_async()'s execution delay and contradicts the comment; move the lines that
set active_probe_launch_timestamp_quarter_us_.store(...) and
has_active_probe_launch_timestamp_.store(...) to just before the
read_async(RegisterAddress::kTempMsb, kTemperatureReadSizeBytes) call (keeping
the same timer::Timer::timestamp_quarter_us() and memory orders) so the
timestamp is captured at SPI launch time while still under the
utility::InterruptLockGuard.
In `@firmware/rmcs_board/app/src/timer/timer.hpp`:
- Around line 45-47: The function timestamp_quarter_us() uses reinterpret_cast
to treat HPM_MCHTMR->MTIME as a volatile uint32_t pointer which risks
strict-aliasing and endianness issues; instead read the register as its actual
type and then static_cast/truncate to uint32_t. Update timestamp_quarter_us() to
read the volatile 64-bit MTIME (use the symbol HPM_MCHTMR->MTIME) into a
volatile uint64_t or by accessing it as its defined type, then return
static_cast<uint32_t>(value) to produce the lower 32 bits, removing the
reinterpret_cast usage.
---
Nitpick comments:
In `@firmware/c_board/app/src/gpio/gpio.hpp`:
- Around line 62-68: ASAP and periodic sampling paths currently read the GPIO
level before capturing the timestamp; change both paths (the block using
channel_hardware(), HAL_GPIO_ReadPin(), current_timestamp_quarter_us(), and
publish_digital_input_sample()) to capture timestamp first by calling
current_timestamp_quarter_us() before HAL_GPIO_ReadPin(), then pass the
timestamp and the read level into publish_digital_input_sample to match the
interrupt path ordering and avoid counting HAL call latency in the sample
timestamp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f09c1e53-6ea9-464e-bf9e-1a5734b66bb0
📒 Files selected for processing (40)
core/include/librmcs/data/datas.hppcore/include/librmcs/spec/gpio.hppcore/src/protocol/deserializer.cppcore/src/protocol/deserializer.hppcore/src/protocol/protocol.hppcore/src/protocol/serializer.hppfirmware/c_board/app/src/app.cppfirmware/c_board/app/src/gpio/gpio.cppfirmware/c_board/app/src/gpio/gpio.hppfirmware/c_board/app/src/spi/bmi088/accel.hppfirmware/c_board/app/src/spi/bmi088/gyro.hppfirmware/c_board/app/src/spi/bmi088/service.hppfirmware/c_board/app/src/spi/bmi088/temperature.hppfirmware/c_board/app/src/usb/vendor.hppfirmware/rmcs_board/app/src/app.cppfirmware/rmcs_board/app/src/gpio/gpio.hppfirmware/rmcs_board/app/src/spi/bmi088/accel.cppfirmware/rmcs_board/app/src/spi/bmi088/accel.hppfirmware/rmcs_board/app/src/spi/bmi088/gyro.cppfirmware/rmcs_board/app/src/spi/bmi088/gyro.hppfirmware/rmcs_board/app/src/spi/bmi088/service.hppfirmware/rmcs_board/app/src/spi/bmi088/temperature.cppfirmware/rmcs_board/app/src/spi/bmi088/temperature.hppfirmware/rmcs_board/app/src/spi/spi.cppfirmware/rmcs_board/app/src/timer/tick.cppfirmware/rmcs_board/app/src/timer/tick.hppfirmware/rmcs_board/app/src/timer/timer.cppfirmware/rmcs_board/app/src/timer/timer.hppfirmware/rmcs_board/app/src/usb/vendor.hppfirmware/rmcs_board/boards/lite/app/board_app.cppfirmware/rmcs_board/boards/lite/app/board_app.hppfirmware/rmcs_board/boards/lite/board.cfirmware/rmcs_board/boards/pro/app/board_app.cppfirmware/rmcs_board/boards/pro/app/board_app.hppfirmware/rmcs_board/boards/pro/board.chost/include/librmcs/agent/c_board.hpphost/include/librmcs/agent/rmcs_board_lite.hpphost/include/librmcs/agent/rmcs_board_pro.hpphost/include/librmcs/protocol/handler.hpphost/src/protocol/handler.cpp
💤 Files with no reviewable changes (7)
- host/include/librmcs/protocol/handler.hpp
- firmware/rmcs_board/app/src/timer/tick.cpp
- firmware/rmcs_board/boards/lite/app/board_app.cpp
- firmware/rmcs_board/boards/lite/app/board_app.hpp
- firmware/rmcs_board/app/src/timer/tick.hpp
- firmware/rmcs_board/boards/pro/app/board_app.hpp
- firmware/rmcs_board/boards/pro/app/board_app.cpp
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
qzhhhi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@firmware/c_board/app/src/spi/bmi088/temperature.hpp`:
- Around line 74-80: The code currently only checks size != 0 and relies on
assert_debug, which doesn't prevent out-of-bounds reads from spi_.rx_buffer when
a short/invalid SPI frame arrives; before calling parse_raw_temperature or
indexing rx_buffer, add a runtime check that size >= the expected full frame
length (the number of bytes parse_raw_temperature/readers access, e.g.
kDummyBytes + 2 or whatever parse_raw_temperature requires) and return early if
the length is insufficient; replace the fragile assert_debug path with this
guard around the block that calls parse_raw_temperature and handle_uplink (same
change also applied to the analogous block around lines 88-94), referencing
parse_raw_temperature, spi_.rx_buffer, size, has_active_probe_launch_timestamp,
and handle_uplink to locate the code.
- Around line 36-45: In poll_pending_probe(), move the timestamp sample inside
the critical section so the computed now is consistent with the locked
check/update: acquire the utility::InterruptLockGuard first (i.e. construct
guard before sampling now), then call timer::timer->timepoint() and use that
value for the check timer::timer->check_reached(next_probe_deadline_) and for
setting next_probe_deadline_ = now + kProbePeriod; keep the existing
checks/assignments to probe_pending_ and next_probe_deadline_ but ensure now is
computed after the guard is taken to avoid ISR races.
In `@firmware/rmcs_board/app/src/spi/bmi088/temperature.cpp`:
- Line 28: The interrupt handler dereferences the global pointer
spi::bmi088::temperature without protection, which can HardFault if the
component isn't initialized; before calling timer_callback, copy
spi::bmi088::temperature to a local pointer, check it for null, and only then
invoke its timer_callback method (ensure the check-and-call happens atomically
from the ISR perspective to avoid TOCTOU races and that timer_callback is
ISR-safe).
In `@firmware/rmcs_board/app/src/timer/timer.hpp`:
- Line 51: timestamp64_quarter_us() reads the 64-bit MMIO HPM_MCHTMR->MTIME
directly which can tear on RV32; change it to either (A) implement a
read-compare-retry sequence that reads low32, high32, then re-reads low32 and
retries on mismatch to produce an atomic 64-bit value, or (B) remove/mark as
unsafe and have callers use timestamp_quarter_us() (which returns the safe
single 32-bit low half) instead; also ensure any SDK helper like
mchtmr_get_count() follows the same read-compare-retry pattern if a full 64-bit
count is required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75ccf36d-a04c-406a-ac71-32b57d89b9d7
📒 Files selected for processing (40)
core/include/librmcs/data/datas.hppcore/include/librmcs/spec/gpio.hppcore/src/protocol/deserializer.cppcore/src/protocol/deserializer.hppcore/src/protocol/protocol.hppcore/src/protocol/serializer.hppfirmware/c_board/app/src/app.cppfirmware/c_board/app/src/gpio/gpio.cppfirmware/c_board/app/src/gpio/gpio.hppfirmware/c_board/app/src/spi/bmi088/accel.hppfirmware/c_board/app/src/spi/bmi088/gyro.hppfirmware/c_board/app/src/spi/bmi088/service.hppfirmware/c_board/app/src/spi/bmi088/temperature.hppfirmware/c_board/app/src/usb/vendor.hppfirmware/rmcs_board/app/src/app.cppfirmware/rmcs_board/app/src/gpio/gpio.hppfirmware/rmcs_board/app/src/spi/bmi088/accel.cppfirmware/rmcs_board/app/src/spi/bmi088/accel.hppfirmware/rmcs_board/app/src/spi/bmi088/gyro.cppfirmware/rmcs_board/app/src/spi/bmi088/gyro.hppfirmware/rmcs_board/app/src/spi/bmi088/service.hppfirmware/rmcs_board/app/src/spi/bmi088/temperature.cppfirmware/rmcs_board/app/src/spi/bmi088/temperature.hppfirmware/rmcs_board/app/src/spi/spi.cppfirmware/rmcs_board/app/src/timer/tick.cppfirmware/rmcs_board/app/src/timer/tick.hppfirmware/rmcs_board/app/src/timer/timer.cppfirmware/rmcs_board/app/src/timer/timer.hppfirmware/rmcs_board/app/src/usb/vendor.hppfirmware/rmcs_board/boards/lite/app/board_app.cppfirmware/rmcs_board/boards/lite/app/board_app.hppfirmware/rmcs_board/boards/lite/board.cfirmware/rmcs_board/boards/pro/app/board_app.cppfirmware/rmcs_board/boards/pro/app/board_app.hppfirmware/rmcs_board/boards/pro/board.chost/include/librmcs/agent/c_board.hpphost/include/librmcs/agent/rmcs_board_lite.hpphost/include/librmcs/agent/rmcs_board_pro.hpphost/include/librmcs/protocol/handler.hpphost/src/protocol/handler.cpp
💤 Files with no reviewable changes (7)
- firmware/rmcs_board/boards/lite/app/board_app.cpp
- firmware/rmcs_board/app/src/timer/tick.cpp
- firmware/rmcs_board/boards/lite/app/board_app.hpp
- host/include/librmcs/protocol/handler.hpp
- firmware/rmcs_board/app/src/timer/tick.hpp
- firmware/rmcs_board/boards/pro/app/board_app.hpp
- firmware/rmcs_board/boards/pro/app/board_app.cpp
✅ Files skipped from review due to trivial changes (2)
- firmware/c_board/app/src/usb/vendor.hpp
- firmware/rmcs_board/boards/pro/board.c
🚧 Files skipped from review as they are similar to previous changes (22)
- host/include/librmcs/agent/c_board.hpp
- core/include/librmcs/spec/gpio.hpp
- firmware/rmcs_board/app/src/spi/bmi088/service.hpp
- firmware/rmcs_board/app/src/spi/bmi088/accel.cpp
- firmware/rmcs_board/boards/lite/board.c
- firmware/c_board/app/src/app.cpp
- firmware/c_board/app/src/spi/bmi088/service.hpp
- core/src/protocol/deserializer.hpp
- firmware/rmcs_board/app/src/spi/bmi088/accel.hpp
- firmware/rmcs_board/app/src/usb/vendor.hpp
- core/src/protocol/protocol.hpp
- firmware/c_board/app/src/gpio/gpio.cpp
- host/include/librmcs/agent/rmcs_board_pro.hpp
- firmware/rmcs_board/app/src/gpio/gpio.hpp
- host/src/protocol/handler.cpp
- core/src/protocol/deserializer.cpp
- firmware/rmcs_board/app/src/spi/bmi088/gyro.hpp
- firmware/rmcs_board/app/src/spi/bmi088/temperature.hpp
- core/include/librmcs/data/datas.hpp
- core/src/protocol/serializer.hpp
- firmware/c_board/app/src/spi/bmi088/accel.hpp
- firmware/c_board/app/src/gpio/gpio.hpp
…s_board - Replace the GPTMR1 tick helper with an MCHTMR-backed timer service that keeps the 1 kHz LED heartbeat while exposing quarter-microsecond timestamps with a retry-safe 64-bit MTIME read for later board events. - Move lite and pro board clock setup to a fixed 4 MHz mchtmr time base so timestamp_quarter_us() has a stable unit contract across both rmcs_board variants. - Remove board-specific GPTMR tick IRQ plumbing in favor of the shared machine-timer ISR path to prepare rmcs_board for timestamped sensor and GPIO reporting.
…IMU transmit APIs - Extend the shared IMU protocol and host callback pipeline with quarter-microsecond timestamps for accelerometer and gyroscope samples, and add a dedicated temperature payload. - Rework the c_board and rmcs_board BMI088 paths to latch capture-time timestamps, defer SPI reads through a shared pending-read service, and preserve board-side sampling time in uplink telemetry. - Add BMI088 temperature telemetry on both firmware boards, carrying the raw temperature register value plus board-side timestamp through the typed IMU pipeline. - Remove the host-side IMU transmit builders because IMU traffic is firmware-originated telemetry only and the existing host transmit API was both invalid and unused. BREAKING CHANGE: IMU accelerometer and gyroscope payloads now carry timestamps, a new temperature IMU payload is emitted, host callback implementations must handle temperature_receive_callback(), and the public host IMU transmit helpers have been removed.
Member
Author
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
- Extend GPIO read configuration with capture_timestamp and add an optional timestamp field to digital read results so host consumers can align GPIO events with board-side IMU telemetry. - Update the GPIO wire format to encode timestamped digital read requests and results, move pull configuration into the read payload, and widen period_ms storage to 16 bits. - Implement timestamp capture for ASAP, periodic, and interrupt-driven digital reads in both c_board and rmcs_board while keeping analog read and write paths explicitly non-timestamped. - GPIO periodic sampling now uses the board timer abstraction for safe 64-bit tick reads. BREAKING CHANGE: GPIO read-config packets use a new wire layout and timestamped digital read results may include an additional 32-bit timestamp payload.
Member
Author
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
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.
feat(firmware/rmcs_board): Add MCHTMR-based timestamp service for rmcs_board
feat(imu)!: Add Timestamped BMI088 telemetry and remove invalid host IMU transmit APIs
BREAKING CHANGE: IMU accelerometer and gyroscope payloads now carry timestamps, a new temperature IMU payload is emitted, host callback implementations must handle temperature_receive_callback(), and the public host IMU transmit helpers have been removed.
feat(gpio)!: Add Timestamped digital read support
BREAKING CHANGE: GPIO read-config packets use a new wire layout and timestamped digital read results may include an additional 32-bit timestamp payload.
PR 摘要：新增 BMI088 温度与时间戳支持、GPIO 时间戳与定时器重构（dev/imuheat → main）
概述
本 PR 为固件与主机协议引入四分之一微秒（quarter-µs）级时间戳支持，并新增 BMI088 温度传感器采集与上报。为保证一致性与高分辨率时间戳，替换与统一了板级定时器实现（MCHTMR 4 MHz 基准），并将 BMI088 驱动改为“中断捕获 → pending → service”延迟读取模式。协议、序列化/反序列化与数→据视图、主机回调链均相应扩展；若主机或固件未配套更新，将导致协议不兼容。
主要变更点
core（协议与数据结构）
固件（rmcs_board 与 c_board）
主机端（host）
破坏性更改与兼容性注意
建议审阅重点
影响范围（主要文件/组件）
（注：PR 评论区仅含重复的自动触发/暂停评审命令与 bot 回复，无人工代码审查评论。）