feat(sensor): Add timestamp difference handling for IMU data processing#38
Open
feat(sensor): Add timestamp difference handling for IMU data processing#38
Conversation
演进概览该PR通过引入时间戳差值（ 变更详情
序列流程图sequenceDiagram
participant GPIO as GPIO中断处理
participant Driver as BMI088驱动<br/>(加速度计/陀螺仪)
participant SPI as 异步SPI通信
participant Serializer as 序列化器
participant Uplink as 上行数据<br/>(DataCallback)
GPIO->>GPIO: 捕获时间戳<br/>capture_timestamp_quarter_us
GPIO->>Driver: data_ready_callback<br/>(capture_timestamp_quarter_us)
Driver->>Driver: 存储待机时间戳
Driver->>SPI: read_async(传感器数据)
SPI-->>Driver: 异步完成回调<br/>(接收数据)
Driver->>Driver: 计算timestamp_diff<br/>(相对于上次成功)
Driver->>Serializer: 写入IMU数据<br/>+ timestamp_diff_quarter_us
Serializer-->>Driver: 序列化结果
alt 序列化成功
Driver->>Driver: 更新last_success_timestamp
end
Driver->>Uplink: 触发DataCallback<br/>含timestamp_diff字段
Uplink-->>Driver: 回调完成
Driver->>Driver: 清除待机时间戳
代码审查工作量评估🎯 4 (复杂) | ⏱️ ~60 分钟 可能相关的PR
庆祝诗
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
firmware/rmcs_board/src/gpio/gpio.cpp (1)
62-64: 建议添加魔数注释。
/ 6U这个除数假设 MCHTMR 时钟频率为 24MHz（24MHz / 6 = 4MHz = 0.25µs），建议添加注释说明这个计算依据，以便后续维护。💡 建议的改进
void gpio_bmi088_int_gyro_isr() { if (gpio_check_clear_interrupt_flag(HPM_GPIO0, GPIO_DI_GPIOB, kBmi088IntGyroPin)) { + // MCHTMR runs at 24MHz; divide by 6 to get quarter-microseconds (4MHz) const uint32_t capture_timestamp_quarter_us = static_cast<uint32_t>(mchtmr_get_count(HPM_MCHTMR) / 6U); spi::bmi088::gyroscope->data_ready_callback(capture_timestamp_quarter_us); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@firmware/rmcs_board/src/gpio/gpio.cpp` around lines 62 - 64, The division by 6U in the computation of capture_timestamp_quarter_us is a magic number; update the code around capture_timestamp_quarter_us (where mchtmr_get_count(HPM_MCHTMR) / 6U is used before calling spi::bmi088::gyroscope->data_ready_callback) to include a brief comment explaining that dividing by 6 assumes a 24MHz MCHTMR clock (24 MHz / 6 = 4 MHz -> 0.25µs per tick), and consider introducing a named constant (e.g., MCHTMR_DIVIDER_24MHZ_OR NOTE) if you prefer to make the intent explicit rather than only a comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@firmware/c_board/app/src/spi/bmi088/accel.hpp`:
- Line 57: kFirstFrameTimestampDiffQuarterUs is defined as a fixed
1/1600-derived quarter-microsecond value and stored in a uint16_t, which
mismatches supported DataRate values (e.g. k50Hz/k25Hz/k12Hz) and causes
overflow/clamping for lower ODRs; update the implementation so timestamp-diff
encoding can represent all DataRate periods by either (A) widening the field
type (replace uint16_t with a larger integer like uint32_t) or changing the unit
(e.g. use microseconds instead of quarter-µs) and recomputing
kFirstFrameTimestampDiff... constants for each DataRate, or (B)
restrict/validate DataRate to only those representable by the current
quarter-us/uint16_t range; apply this change consistently wherever
kFirstFrameTimestampDiffQuarterUs and its related constants (lines referenced
147-159) are used and ensure DataRate-to-timestamp conversion functions handle
the new unit/size.
In `@firmware/rmcs_board/src/spi/bmi088/gyro.hpp`:
- Line 49: The constant kFirstFrameTimestampDiffQuarterUs is hardcoded to 2000Hz
so the first-frame timestamp delta is wrong when the constructor is passed other
ODRs (k1000/k400/k200/k100); change the logic to compute and cache the initial
timestamp_diff_quarter_us in the gyro class constructor based on the selected
rate (use the same formula as the original constant but replace 2000 with the
actual odr_hz derived from the rate enum) and store it in a member field
(instead of the static kFirstFrameTimestampDiffQuarterUs) so
timestamp_diff_quarter_us reflects the configured rate for the first packet.
- Around line 104-109: data_ready_callback currently sets
pending_capture_timestamp_quarter_us_ and has_pending_capture_timestamp_ when
read_async(...) succeeds but there is no failure/abort path from Spi/spi_end_int
to clear that flag; update Spi to invoke the module callback on failed/aborted
transfers (e.g., call transmit_receive_async_callback with size==0) or add a
timeout/cleanup in data_ready_callback/transmit_receive_async_callback to clear
has_pending_capture_timestamp_ and pending_capture_timestamp_quarter_us_ on
failure so the flag cannot remain permanently set; also remove the hardcoded
kFirstFrameTimestampDiffQuarterUs constant and compute the first-frame timestamp
diff from the actual configured ODR in the Gyro constructor (use the runtime
rate parameter used for k2000And230/k1000And116 instead of 1'000'000/2000*4) so
the value reflects the chosen rate.
---
Nitpick comments:
In `@firmware/rmcs_board/src/gpio/gpio.cpp`:
- Around line 62-64: The division by 6U in the computation of
capture_timestamp_quarter_us is a magic number; update the code around
capture_timestamp_quarter_us (where mchtmr_get_count(HPM_MCHTMR) / 6U is used
before calling spi::bmi088::gyroscope->data_ready_callback) to include a brief
comment explaining that dividing by 6 assumes a 24MHz MCHTMR clock (24 MHz / 6 =
4 MHz -> 0.25µs per tick), and consider introducing a named constant (e.g.,
MCHTMR_DIVIDER_24MHZ_OR NOTE) if you prefer to make the intent explicit rather
than only a comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13e7c4b5-cd11-4b11-a006-9c46bb3c921c
📒 Files selected for processing (10)
core/include/librmcs/data/datas.hppcore/src/protocol/deserializer.cppcore/src/protocol/protocol.hppcore/src/protocol/serializer.hppfirmware/c_board/app/src/gpio/gpio.cppfirmware/c_board/app/src/spi/bmi088/accel.hppfirmware/c_board/app/src/spi/bmi088/gyro.hppfirmware/rmcs_board/src/gpio/gpio.cppfirmware/rmcs_board/src/spi/bmi088/accel.hppfirmware/rmcs_board/src/spi/bmi088/gyro.hpp
| public: | ||
| using Lazy = utility::Lazy<Accelerometer, Spi::Lazy*>; | ||
|
|
||
| static constexpr uint16_t kFirstFrameTimestampDiffQuarterUs = 1'000'000 / 1600 * 4; |
There was a problem hiding this comment.
可配置 ODR 与时间差编码范围不匹配。
kFirstFrameTimestampDiffQuarterUs 固定按 1600Hz 计算，所以任何非默认 data_rate 的首帧都会写错；另外 uint16_t quarter-us 的上限只有 16383.75us（约 61Hz），而 DataRate 还支持 k50Hz/k25Hz/k12Hz，这些模式的正常周期都会被长期压成 65535。要么把时间差字段改大/改单位，要么把可选 ODR 限制在可表示范围内。
Also applies to: 147-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/c_board/app/src/spi/bmi088/accel.hpp` at line 57,
kFirstFrameTimestampDiffQuarterUs is defined as a fixed 1/1600-derived
quarter-microsecond value and stored in a uint16_t, which mismatches supported
DataRate values (e.g. k50Hz/k25Hz/k12Hz) and causes overflow/clamping for lower
ODRs; update the implementation so timestamp-diff encoding can represent all
DataRate periods by either (A) widening the field type (replace uint16_t with a
larger integer like uint32_t) or changing the unit (e.g. use microseconds
instead of quarter-µs) and recomputing kFirstFrameTimestampDiff... constants for
each DataRate, or (B) restrict/validate DataRate to only those representable by
the current quarter-us/uint16_t range; apply this change consistently wherever
kFirstFrameTimestampDiffQuarterUs and its related constants (lines referenced
147-159) are used and ensure DataRate-to-timestamp conversion functions handle
the new unit/size.
| public: | ||
| using Lazy = utility::Lazy<Gyroscope, Spi::Lazy*, ChipSelectPin>; | ||
|
|
||
| static constexpr uint16_t kFirstFrameTimestampDiffQuarterUs = 1'000'000 / 2000 * 4; |
There was a problem hiding this comment.
首帧 delta 被写死成默认 2000Hz。
构造函数仍允许传入 k1000/k400/k200/k100，但首帧 timestamp_diff_quarter_us 始终按 2000Hz 的 500us 发送。只要实例化成非默认 ODR，第一包时间差就是错的。建议按当前 rate 计算并缓存这个初始值。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/rmcs_board/src/spi/bmi088/gyro.hpp` at line 49, The constant
kFirstFrameTimestampDiffQuarterUs is hardcoded to 2000Hz so the first-frame
timestamp delta is wrong when the constructor is passed other ODRs
(k1000/k400/k200/k100); change the logic to compute and cache the initial
timestamp_diff_quarter_us in the gyro class constructor based on the selected
rate (use the same formula as the original constant but replace 2000 with the
actual odr_hz derived from the rate enum) and store it in a member field
(instead of the static kFirstFrameTimestampDiffQuarterUs) so
timestamp_diff_quarter_us reflects the configured rate for the first packet.
Comment on lines
+104
to
+109
| void data_ready_callback(uint32_t capture_timestamp_quarter_us) { | ||
| if (read_async(RegisterAddress::kRateXLsb, 6)) { | ||
| pending_capture_timestamp_quarter_us_ = capture_timestamp_quarter_us; | ||
| has_pending_capture_timestamp_ = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find firmware/rmcs_board/src/spi -type f -name "*.cpp" -o -name "*.hpp" | head -20Repository: Alliance-Algorithm/librmcs
Length of output: 274
🏁 Script executed:
wc -l firmware/rmcs_board/src/spi/spi.cppRepository: Alliance-Algorithm/librmcs
Length of output: 108
🏁 Script executed:
cat -n firmware/rmcs_board/src/spi/spi.cppRepository: Alliance-Algorithm/librmcs
Length of output: 816
🏁 Script executed:
wc -l firmware/rmcs_board/src/spi/spi.hppRepository: Alliance-Algorithm/librmcs
Length of output: 109
🏁 Script executed:
cat -n firmware/rmcs_board/src/spi/spi.hppRepository: Alliance-Algorithm/librmcs
Length of output: 7423
🏁 Script executed:
wc -l firmware/rmcs_board/src/spi/bmi088/gyro.hppRepository: Alliance-Algorithm/librmcs
Length of output: 117
🏁 Script executed:
cat -n firmware/rmcs_board/src/spi/bmi088/gyro.hppRepository: Alliance-Algorithm/librmcs
Length of output: 7128
rmcs_board 的 SPI 缺失失败路径回调会导致 has_pending_capture_timestamp_ 无法清理。
data_ready_callback() 在 read_async() 成功时设置 has_pending_capture_timestamp_ = true（第 105-108 行），清理仅在 transmit_receive_async_callback() 中进行（第 118 行）。但该回调只在 spi_end_int 成功完成时触发，spi.cpp 和 spi.hpp 中均未实现失败/超时/中止路径的处理。若传输后续被中止或卡死，该标志将永久悬挂，直到下一次 data_ready_callback()（可能很久不会发生），导致设备状态不一致。
建议让 rmcs_board 的 Spi 在失败完成时也调用模块回调（可传 size == 0 表示失败），或在 data_ready_callback() 中添加超时保护机制。
另外，kFirstFrameTimestampDiffQuarterUs 的计算硬编码了 2000 Hz 的默认 ODR。
第 49 行常量值为 1'000'000 / 2000 * 4，对应构造函数默认速率 k2000And230。但构造函数允许传入其他速率参数（第 71 行）。若采用 k1000And116（1000 Hz）等非默认速率初始化，首帧时间戳差值将错误。建议使 kFirstFrameTimestampDiffQuarterUs 依赖实际设定的速率而非硬编码默认值。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/rmcs_board/src/spi/bmi088/gyro.hpp` around lines 104 - 109,
data_ready_callback currently sets pending_capture_timestamp_quarter_us_ and
has_pending_capture_timestamp_ when read_async(...) succeeds but there is no
failure/abort path from Spi/spi_end_int to clear that flag; update Spi to invoke
the module callback on failed/aborted transfers (e.g., call
transmit_receive_async_callback with size==0) or add a timeout/cleanup in
data_ready_callback/transmit_receive_async_callback to clear
has_pending_capture_timestamp_ and pending_capture_timestamp_quarter_us_ on
failure so the flag cannot remain permanently set; also remove the hardcoded
kFirstFrameTimestampDiffQuarterUs constant and compute the first-frame timestamp
diff from the actual configured ODR in the Gyro constructor (use the runtime
rate parameter used for k2000And230/k1000And116 instead of 1'000'000/2000*4) so
the value reflects the chosen rate.
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.
Pull Request 摘要
概述
该PR为IMU（惯性测量单元）数据处理添加了时间戳差值处理功能，使加速度计和陀螺仪能够报告相邻数据帧之间的时间差值（以四分之一微秒为单位），从而提供更精确的传感器数据时序信息。
主要改动
1. 数据结构扩展
AccelerometerDataView和GyroscopeDataView各新增uint16_t timestamp_diff_quarter_us字段，用于存储帧间时间差值2. 协议层更新
ImuAccelerometerPayload和ImuGyroscopePayload的Bitfield大小从6扩展至8，在比特偏移48处新增TimestampDiffQuarterUs字段（16位无符号整数）3. 序列化/反序列化
timestamp_diff_quarter_us写入有效负载4. GPIO中断时间戳捕获
HAL_GPIO_EXTI_Callback中通过timer::timer->timepoint().time_since_epoch().count()捕获中断时间戳mchtmr_get_count(HPM_MCHTMR) / 6U获取中断时间戳5. 传感器驱动更新（加速度计和陀螺仪）
data_ready_callback()签名改为接收uint32_t capture_timestamp_quarter_us参数pending_capture_timestamp_quarter_us_：待处理的捕获时间戳last_success_capture_timestamp_quarter_us_：上一次成功序列化的时间戳kFirstFrameTimestampDiffQuarterUs，用于首帧时间戳差值uint16_t最大值变更规模