feat(i2c): Add end-to-end I2C support for RmcsBoard and CBoard#43
Open
gqsdjhh wants to merge 8 commits intoAlliance-Algorithm:mainfrom
Open
feat(i2c): Add end-to-end I2C support for RmcsBoard and CBoard#43gqsdjhh wants to merge 8 commits intoAlliance-Algorithm:mainfrom
gqsdjhh wants to merge 8 commits intoAlliance-Algorithm:mainfrom
Conversation
Walkthrough本PR在系统中添加I2C通信协议支持。包括在核心库中定义I2C数据结构和回调接口，在协议层实现I2C序列化和反序列化处理，在两个固件板中集成I2C硬件驱动，以及在主机端添加I2C命令构建和事件接收接口。 Changes
Sequence DiagramsequenceDiagram
participant Host as 主机应用
participant UsbDeserializer as USB反序列化器
participant Vendor as Vendor(路由)
participant I2cDriver as I2C驱动
participant Hardware as STM32 HAL/DMA
participant UsbSerializer as USB序列化器
Host->>UsbDeserializer: 接收I2C写/读命令
UsbDeserializer->>Vendor: i2c_write_deserialized_callback<br/>或i2c_read_config_deserialized_callback
Vendor->>I2cDriver: handle_downlink_write()<br/>或handle_downlink_read_config()
I2cDriver->>Hardware: 启动DMA写或读传输
Hardware->>Hardware: 执行I2C主模式通信
Hardware->>I2cDriver: HAL回调(tx/rx完成或错误)
I2cDriver->>I2cDriver: 封装结果到待发送队列
I2cDriver->>UsbSerializer: update()触发序列化
UsbSerializer->>UsbSerializer: write_i2c_read_result()<br/>或write_i2c_error()
UsbSerializer->>Host: 通过USB返回读结果/错误
Host->>Host: i2c0_receive_callback()<br/>或i2c0_error_callback()处理
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 涉及多个异构文件和层（协议、硬件、驱动、应用、主机），包括复杂的状态机（I2C驱动中的空闲/写/读状态）、中断安全缓冲、DMA处理和多个回调链。尽管各部分内部逻辑清晰，但跨层交互的复杂性和硬件细节需要仔细审查。 Possibly related PRs
Poem
🚥 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 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: 6
🧹 Nitpick comments (2)
host/include/librmcs/agent/c_board.hpp (1)
144-156: 给新的回调覆写补上override。这里是在覆写
data::DataCallback，按仓库约定应显式写override，不要只保留final。Based on learnings: "In the librmcs codebase, when overriding virtual functions from a base class in C++, declare overrides with the override specifier and do not repeat virtual on the overriding function. The virtual keyword should only be used on the topmost declaration of a virtual function. This aligns with clang-tidy's modernize-use-override check. Apply this pattern to header files under host/include/librmcs (e.g., host/include/librmcs/**/*.hpp) and ensure all overrides use 'override' without 'virtual'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@host/include/librmcs/agent/c_board.hpp` around lines 144 - 156, Add the missing override specifier to the two callback overrides: update the declarations of i2c_receive_callback and i2c_error_callback to include override in addition to final (e.g., change the signature from "... final {" to "... override final {" or "... final override {"), making sure you do not add the virtual keyword; this makes the overrides explicit per the project's style and clang-tidy rules.host/include/librmcs/agent/rmcs_board.hpp (1)
151-163: 给新的DataCallback重写补上override。这里两个重写点目前只有
final，会漏掉仓库里已经统一的modernize-use-override约束。建议直接写成override final，避免头文件风格继续分叉。♻️ 建议修改
- bool i2c_receive_callback(data::DataId id, const data::I2cDataView& data) final { + bool i2c_receive_callback(data::DataId id, const data::I2cDataView& data) override final { switch (id) { case data::DataId::kI2c0: i2c0_receive_callback(data); return true; default: return false; } } - void i2c_error_callback(data::DataId id, uint8_t slave_address) final { + void i2c_error_callback(data::DataId id, uint8_t slave_address) override final { switch (id) { case data::DataId::kI2c0: i2c0_error_callback(slave_address); break; default: break; } }Based on learnings "In the librmcs codebase, when overriding virtual functions from a base class in C++, declare overrides with the override specifier and do not repeat virtual on the overriding function. The virtual keyword should only be used on the topmost declaration of a virtual function. This aligns with clang-tidy's modernize-use-override check. Apply this pattern to header files under host/include/librmcs (e.g., host/include/librmcs/**/*.hpp) and ensure all overrides use 'override' without 'virtual'."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@host/include/librmcs/agent/rmcs_board.hpp` around lines 151 - 163, The two methods i2c_receive_callback(data::DataId, const data::I2cDataView&) and i2c_error_callback(data::DataId, uint8_t) currently use only the final specifier; change their declarations to include the override specifier as well (i.e., use override final) to conform with the codebase's modernize-use-override rule for DataCallback overrides so the signatures explicitly mark they override a base-class virtual.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/include/librmcs/data/datas.hpp`:
- Around line 127-130: The i2c_error_callback currently only forwards
slave_address and drops new error context; change its signature in Data::
(datas.hpp) from virtual void i2c_error_callback(DataId id, uint8_t
slave_address) to virtual void i2c_error_callback(DataId id, const I2cErrorView&
error) and propagate that type through the call chain (update protocol handler
code that invokes i2c_error_callback in host/src/protocol/handler.cpp to pass an
I2cErrorView, and update the receiving declarations in
host/include/librmcs/agent/c_board.hpp and
host/include/librmcs/agent/rmcs_board.hpp to match), ensuring callers use the
fields (is_read, reg_address, data_length, slave_address) from I2cErrorView
instead of the lone uint8_t.
In `@core/src/protocol/serializer.hpp`:
- Around line 570-595: required_i2c_size currently allows an I2C ReadRequest
with data_len == 0; update the function to reject kReadRequest when data_len is
zero by returning 0 (or otherwise failing the same way other bad cases do).
Inside required_i2c_size, add a guard that when payload ==
I2cHeader::PayloadEnum::kReadRequest && data_len == 0 it returns 0 (or triggers
the same LIBRMCS_VERIFY_LIKELY failure used elsewhere), ensuring the check
occurs before computing sizes; reference the symbols required_i2c_size,
I2cHeader::PayloadEnum::kReadRequest and data_len to locate the change.
In `@firmware/c_board/app/src/i2c/i2c.hpp`:
- Around line 40-69: Both handle_downlink_write and handle_downlink_read_config
currently validate lengths but don't reject invalid 8-bit slave addresses; add a
guard at the start of each function that checks if data.slave_address > 0x7F and
if so calls publish_error(make_error_view(data)) and returns (do not call
to_hal_slave_address or enqueue requests). This prevents malformed downlinks
from being converted/shifted and sending useless HAL bus transactions; reuse the
same error-reporting pattern already used elsewhere
(publish_error/make_error_view) so behavior is consistent.
- Around line 80-99: The completion callbacks use only
core::utility::assert_debug to validate transfer_state_, which is stripped in
release builds and allows stale DMA interrupts to proceed against a cleared
active_request_; update tx_complete_callback() and rx_complete_callback() to
perform a runtime check of transfer_state_ (using transfer_state_.load(...)) and
immediately return when the state is not the expected TransferState (kWrite for
tx, kRead for rx); in rx_complete_callback() additionally verify active_request_
is still valid (and request.data_length > 0) before doing memcpy,
finish_active_request(), publish_read_result(), etc., so late interrupts cannot
process a default/cleared request.
In `@firmware/rmcs_board/app/src/i2c/i2c.hpp`:
- Around line 34-87: 在 handle_downlink_write 和 handle_downlink_read_config 中补充对
7-bit I2C 地址的校验（slave_address 必须 <= 0x7F）；在发现非法地址时直接调用
publish_error(make_error_view(data)) 并返回，避免继续调用 i2c_master_write/i2c_master_read
系列函数。参照已有长度校验逻辑（kMaxDataLength）将地址校验放在最前面以尽早拒绝畸形包，保证对 has_register 分支都生效。
In `@host/include/librmcs/agent/rmcs_board.hpp`:
- Around line 158-167: 当前回调只透传 slave_address，丢失了操作类型/寄存器/长度等错误详情；把
i2c_error_callback(data::DataId, uint8_t)
的传递链改为透传完整错误视图（data::I2cErrorView）并把虚方法签名改为 virtual void
i2c0_error_callback(const librmcs::data::I2cErrorView& error) ，同时在
i2c_error_callback 的 switch 分支中调用 i2c0_error_callback(error)（或等价命名），确保新增的 I2C
错误字段一路传到应用层以便判断具体失败事务。
---
Nitpick comments:
In `@host/include/librmcs/agent/c_board.hpp`:
- Around line 144-156: Add the missing override specifier to the two callback
overrides: update the declarations of i2c_receive_callback and
i2c_error_callback to include override in addition to final (e.g., change the
signature from "... final {" to "... override final {" or "... final override
{"), making sure you do not add the virtual keyword; this makes the overrides
explicit per the project's style and clang-tidy rules.
In `@host/include/librmcs/agent/rmcs_board.hpp`:
- Around line 151-163: The two methods i2c_receive_callback(data::DataId, const
data::I2cDataView&) and i2c_error_callback(data::DataId, uint8_t) currently use
only the final specifier; change their declarations to include the override
specifier as well (i.e., use override final) to conform with the codebase's
modernize-use-override rule for DataCallback overrides so the signatures
explicitly mark they override a base-class virtual.
🪄 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: 4ba7c26d-b3b7-489a-b5f8-80d766acbe19
📒 Files selected for processing (29)
.codexcore/include/librmcs/data/datas.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/i2c/i2c.cppfirmware/c_board/app/src/i2c/i2c.hppfirmware/c_board/app/src/usb/interrupt_safe_buffer.hppfirmware/c_board/app/src/usb/vendor.hppfirmware/c_board/bsp/cubemx/Core/Inc/i2c.hfirmware/c_board/bsp/cubemx/Core/Inc/stm32f4xx_hal_conf.hfirmware/c_board/bsp/cubemx/Core/Inc/stm32f4xx_it.hfirmware/c_board/bsp/cubemx/Core/Src/dma.cfirmware/c_board/bsp/cubemx/Core/Src/gpio.cfirmware/c_board/bsp/cubemx/Core/Src/i2c.cfirmware/c_board/bsp/cubemx/Core/Src/main.cfirmware/c_board/bsp/cubemx/Core/Src/stm32f4xx_it.cfirmware/c_board/bsp/cubemx/cmake/stm32cubemx/CMakeLists.txtfirmware/c_board/bsp/cubemx/rmcs_slave.iocfirmware/rmcs_board/app/src/app.cppfirmware/rmcs_board/app/src/i2c/i2c.hppfirmware/rmcs_board/app/src/usb/interrupt_safe_buffer.hppfirmware/rmcs_board/app/src/usb/vendor.hpphost/include/librmcs/agent/c_board.hpphost/include/librmcs/agent/rmcs_board.hpphost/include/librmcs/protocol/handler.hpphost/src/protocol/handler.cpp
Comment on lines
+127
to
+130
|
|
||
| virtual bool i2c_receive_callback(DataId id, const I2cDataView& data) = 0; | ||
|
|
||
| virtual void i2c_error_callback(DataId id, uint8_t slave_address) = 0; |
There was a problem hiding this comment.
i2c_error_callback 现在把新增错误上下文全丢掉了。
协议层和反序列化层已经有 I2cErrorView，其中包含 is_read、reg_address 和 data_length，但这里仍只把 slave_address 往上抛。这样从 host/src/protocol/handler.cpp:98-107 到 host/include/librmcs/agent/c_board.hpp:151-160 / host/include/librmcs/agent/rmcs_board.hpp:151-163 这条链路都会丢失新增信息，SDK 使用者拿不到 PR 里补充的错误上下文。建议把回调签名改成传 const I2cErrorView&。
建议修改
- virtual void i2c_error_callback(DataId id, uint8_t slave_address) = 0;
+ virtual void i2c_error_callback(DataId id, const I2cErrorView& data) = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/include/librmcs/data/datas.hpp` around lines 127 - 130, The
i2c_error_callback currently only forwards slave_address and drops new error
context; change its signature in Data:: (datas.hpp) from virtual void
i2c_error_callback(DataId id, uint8_t slave_address) to virtual void
i2c_error_callback(DataId id, const I2cErrorView& error) and propagate that type
through the call chain (update protocol handler code that invokes
i2c_error_callback in host/src/protocol/handler.cpp to pass an I2cErrorView, and
update the receiving declarations in host/include/librmcs/agent/c_board.hpp and
host/include/librmcs/agent/rmcs_board.hpp to match), ensuring callers use the
fields (is_read, reg_address, data_length, slave_address) from I2cErrorView
instead of the lone uint8_t.
Comment on lines
+570
to
+595
| static std::size_t required_i2c_size( | ||
| FieldId field_id, I2cHeader::PayloadEnum payload, std::size_t data_len, | ||
| bool has_register) noexcept { | ||
| LIBRMCS_VERIFY_LIKELY(is_i2c_field_id(field_id), 0); | ||
| LIBRMCS_VERIFY_LIKELY(data_len <= ((1U << 9) - 1U), 0); | ||
| switch (payload) { | ||
| case I2cHeader::PayloadEnum::kWrite: | ||
| case I2cHeader::PayloadEnum::kReadRequest: | ||
| case I2cHeader::PayloadEnum::kReadResult: | ||
| case I2cHeader::PayloadEnum::kError: break; | ||
| default: return 0; | ||
| } | ||
|
|
||
| const std::size_t field_header_bytes = required_field_header_size(field_id); | ||
| const std::size_t i2c_header_bytes = sizeof(I2cHeader); | ||
| const std::size_t register_bytes = has_register ? 1 : 0; | ||
| const std::size_t payload_bytes = (payload == I2cHeader::PayloadEnum::kWrite | ||
| || payload == I2cHeader::PayloadEnum::kReadResult) | ||
| ? data_len | ||
| : 0; | ||
| const std::size_t total = | ||
| (field_header_bytes + i2c_header_bytes - 1) + register_bytes + payload_bytes; | ||
| LIBRMCS_VERIFY_LIKELY(total <= kProtocolBufferSize, 0); | ||
|
|
||
| return total; | ||
| } |
There was a problem hiding this comment.
在协议层也拒绝零长度的 I2C 读请求。
这里会接受 payload == kReadRequest && data_len == 0。上层 PacketBuilder::i2c0_read() 已经把这种输入视为非法，但直接用 Serializer 仍能编码它，导致协议约束只靠更高层包装维持。建议在这里把 kReadRequest 的 data_len == 0 一并拦掉。
建议修改
static std::size_t required_i2c_size(
FieldId field_id, I2cHeader::PayloadEnum payload, std::size_t data_len,
bool has_register) noexcept {
LIBRMCS_VERIFY_LIKELY(is_i2c_field_id(field_id), 0);
LIBRMCS_VERIFY_LIKELY(data_len <= ((1U << 9) - 1U), 0);
switch (payload) {
case I2cHeader::PayloadEnum::kWrite:
case I2cHeader::PayloadEnum::kReadRequest:
case I2cHeader::PayloadEnum::kReadResult:
case I2cHeader::PayloadEnum::kError: break;
default: return 0;
}
+ LIBRMCS_VERIFY_LIKELY(
+ payload != I2cHeader::PayloadEnum::kReadRequest || data_len != 0, 0);
const std::size_t field_header_bytes = required_field_header_size(field_id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static std::size_t required_i2c_size( | |
| FieldId field_id, I2cHeader::PayloadEnum payload, std::size_t data_len, | |
| bool has_register) noexcept { | |
| LIBRMCS_VERIFY_LIKELY(is_i2c_field_id(field_id), 0); | |
| LIBRMCS_VERIFY_LIKELY(data_len <= ((1U << 9) - 1U), 0); | |
| switch (payload) { | |
| case I2cHeader::PayloadEnum::kWrite: | |
| case I2cHeader::PayloadEnum::kReadRequest: | |
| case I2cHeader::PayloadEnum::kReadResult: | |
| case I2cHeader::PayloadEnum::kError: break; | |
| default: return 0; | |
| } | |
| const std::size_t field_header_bytes = required_field_header_size(field_id); | |
| const std::size_t i2c_header_bytes = sizeof(I2cHeader); | |
| const std::size_t register_bytes = has_register ? 1 : 0; | |
| const std::size_t payload_bytes = (payload == I2cHeader::PayloadEnum::kWrite | |
| || payload == I2cHeader::PayloadEnum::kReadResult) | |
| ? data_len | |
| : 0; | |
| const std::size_t total = | |
| (field_header_bytes + i2c_header_bytes - 1) + register_bytes + payload_bytes; | |
| LIBRMCS_VERIFY_LIKELY(total <= kProtocolBufferSize, 0); | |
| return total; | |
| } | |
| static std::size_t required_i2c_size( | |
| FieldId field_id, I2cHeader::PayloadEnum payload, std::size_t data_len, | |
| bool has_register) noexcept { | |
| LIBRMCS_VERIFY_LIKELY(is_i2c_field_id(field_id), 0); | |
| LIBRMCS_VERIFY_LIKELY(data_len <= ((1U << 9) - 1U), 0); | |
| switch (payload) { | |
| case I2cHeader::PayloadEnum::kWrite: | |
| case I2cHeader::PayloadEnum::kReadRequest: | |
| case I2cHeader::PayloadEnum::kReadResult: | |
| case I2cHeader::PayloadEnum::kError: break; | |
| default: return 0; | |
| } | |
| LIBRMCS_VERIFY_LIKELY( | |
| payload != I2cHeader::PayloadEnum::kReadRequest || data_len != 0, 0); | |
| const std::size_t field_header_bytes = required_field_header_size(field_id); | |
| const std::size_t i2c_header_bytes = sizeof(I2cHeader); | |
| const std::size_t register_bytes = has_register ? 1 : 0; | |
| const std::size_t payload_bytes = (payload == I2cHeader::PayloadEnum::kWrite | |
| || payload == I2cHeader::PayloadEnum::kReadResult) | |
| ? data_len | |
| : 0; | |
| const std::size_t total = | |
| (field_header_bytes + i2c_header_bytes - 1) + register_bytes + payload_bytes; | |
| LIBRMCS_VERIFY_LIKELY(total <= kProtocolBufferSize, 0); | |
| return total; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/protocol/serializer.hpp` around lines 570 - 595, required_i2c_size
currently allows an I2C ReadRequest with data_len == 0; update the function to
reject kReadRequest when data_len is zero by returning 0 (or otherwise failing
the same way other bad cases do). Inside required_i2c_size, add a guard that
when payload == I2cHeader::PayloadEnum::kReadRequest && data_len == 0 it returns
0 (or triggers the same LIBRMCS_VERIFY_LIKELY failure used elsewhere), ensuring
the check occurs before computing sizes; reference the symbols
required_i2c_size, I2cHeader::PayloadEnum::kReadRequest and data_len to locate
the change.
Comment on lines
+40
to
+69
| void handle_downlink_write(const data::I2cDataView& data) { | ||
| if (data.payload.empty()) | ||
| return; | ||
|
|
||
| if (data.payload.size() > kMaxDataLength) { | ||
| publish_error(make_error_view(data)); | ||
| return; | ||
| } | ||
|
|
||
| if (!enqueue_write_request(data)) { | ||
| led::led->downlink_buffer_full(); | ||
| publish_error(make_error_view(data)); | ||
| } else { | ||
| try_start_next_request(); | ||
| } | ||
| } | ||
|
|
||
| void handle_downlink_read_config(const data::I2cReadConfigView& data) { | ||
| if (data.read_length == 0 || data.read_length > kMaxDataLength) { | ||
| publish_error(make_error_view(data)); | ||
| return; | ||
| } | ||
|
|
||
| if (!enqueue_read_request(data)) { | ||
| led::led->downlink_buffer_full(); | ||
| publish_error(make_error_view(data)); | ||
| } else { | ||
| try_start_next_request(); | ||
| } | ||
| } |
There was a problem hiding this comment.
固件入口也要拦住非法从机地址。
这里校验了长度，但没有校验 slave_address <= 0x7F。如果收到畸形下行包，to_hal_slave_address() 会把 8-bit 值直接左移后送进 HAL，最终还是会对总线发起一次无意义访问。建议在 handle_downlink_write() 和 handle_downlink_read_config() 入口统一拒绝非法地址并上报错误。
🛡️ 建议修改
+ static bool is_valid_slave_address(uint8_t slave_address) {
+ return slave_address <= 0x7F;
+ }
+
void handle_downlink_write(const data::I2cDataView& data) {
if (data.payload.empty())
return;
- if (data.payload.size() > kMaxDataLength) {
+ if (!is_valid_slave_address(data.slave_address) || data.payload.size() > kMaxDataLength) {
publish_error(make_error_view(data));
return;
}
@@
void handle_downlink_read_config(const data::I2cReadConfigView& data) {
- if (data.read_length == 0 || data.read_length > kMaxDataLength) {
+ if (!is_valid_slave_address(data.slave_address) || data.read_length == 0
+ || data.read_length > kMaxDataLength) {
publish_error(make_error_view(data));
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/c_board/app/src/i2c/i2c.hpp` around lines 40 - 69, Both
handle_downlink_write and handle_downlink_read_config currently validate lengths
but don't reject invalid 8-bit slave addresses; add a guard at the start of each
function that checks if data.slave_address > 0x7F and if so calls
publish_error(make_error_view(data)) and returns (do not call
to_hal_slave_address or enqueue requests). This prevents malformed downlinks
from being converted/shifted and sending useless HAL bus transactions; reuse the
same error-reporting pattern already used elsewhere
(publish_error/make_error_view) so behavior is consistent.
Comment on lines
+80
to
+99
| void tx_complete_callback() { | ||
| const auto state = transfer_state_.load(std::memory_order::acquire); | ||
| core::utility::assert_debug(state == TransferState::kWrite); | ||
|
|
||
| finish_active_request(); | ||
| try_start_next_request(); | ||
| } | ||
|
|
||
| void rx_complete_callback() { | ||
| const auto state = transfer_state_.load(std::memory_order::acquire); | ||
| core::utility::assert_debug(state == TransferState::kRead); | ||
|
|
||
| const auto request = active_request_; | ||
| std::memcpy(read_result_buffer_.data(), read_dma_buffer_.data(), request.data_length); | ||
|
|
||
| finish_active_request(); | ||
| try_start_next_request(); | ||
| publish_read_result( | ||
| request, {read_result_buffer_.data(), static_cast<std::size_t>(request.data_length)}); | ||
| } |
There was a problem hiding this comment.
超时恢复后，迟到的完成回调会继续处理“空请求”。
tx_complete_callback() 和 rx_complete_callback() 现在只靠 assert_debug 检查状态；release 构建里如果 recover_active_request_locked() 先把 active_request_ 清空了，随后又来了旧 DMA 完成中断，rx_complete_callback() 仍会继续走完并上报一个默认构造请求的读结果。这里需要在状态不匹配时直接 return，不能只做 debug 断言。
🐛 建议修改
void tx_complete_callback() {
const auto state = transfer_state_.load(std::memory_order::acquire);
- core::utility::assert_debug(state == TransferState::kWrite);
+ if (state != TransferState::kWrite) {
+ core::utility::assert_debug(false);
+ return;
+ }
finish_active_request();
try_start_next_request();
}
void rx_complete_callback() {
const auto state = transfer_state_.load(std::memory_order::acquire);
- core::utility::assert_debug(state == TransferState::kRead);
+ if (state != TransferState::kRead) {
+ core::utility::assert_debug(false);
+ return;
+ }
const auto request = active_request_;
std::memcpy(read_result_buffer_.data(), read_dma_buffer_.data(), request.data_length);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/c_board/app/src/i2c/i2c.hpp` around lines 80 - 99, The completion
callbacks use only core::utility::assert_debug to validate transfer_state_,
which is stripped in release builds and allows stale DMA interrupts to proceed
against a cleared active_request_; update tx_complete_callback() and
rx_complete_callback() to perform a runtime check of transfer_state_ (using
transfer_state_.load(...)) and immediately return when the state is not the
expected TransferState (kWrite for tx, kRead for rx); in rx_complete_callback()
additionally verify active_request_ is still valid (and request.data_length > 0)
before doing memcpy, finish_active_request(), publish_read_result(), etc., so
late interrupts cannot process a default/cleared request.
Comment on lines
+34
to
+87
| void handle_downlink_write(const data::I2cDataView& data) { | ||
| if (data.payload.empty()) | ||
| return; | ||
|
|
||
| if (data.payload.size() > kMaxDataLength) { | ||
| publish_error(make_error_view(data)); | ||
| return; | ||
| } | ||
|
|
||
| auto* payload = reinterpret_cast<uint8_t*>(write_buffer_.data()); | ||
| std::memcpy(payload, data.payload.data(), data.payload.size()); | ||
| hpm_stat_t status = status_fail; | ||
| if (data.has_register) { | ||
| uint8_t reg_address = data.reg_address; | ||
| status = i2c_master_address_write( | ||
| i2c_base_, data.slave_address, ®_address, sizeof(reg_address), payload, | ||
| static_cast<uint32_t>(data.payload.size())); | ||
| } else { | ||
| status = i2c_master_write( | ||
| i2c_base_, data.slave_address, payload, static_cast<uint32_t>(data.payload.size())); | ||
| } | ||
|
|
||
| if (status != status_success) { | ||
| publish_error(make_error_view(data)); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| void handle_downlink_read_config(const data::I2cReadConfigView& data) { | ||
| if (data.read_length == 0 || data.read_length > kMaxDataLength) { | ||
| publish_error(make_error_view(data)); | ||
| return; | ||
| } | ||
|
|
||
| auto* payload = reinterpret_cast<uint8_t*>(read_buffer_.data()); | ||
| hpm_stat_t status = status_fail; | ||
| if (data.has_register) { | ||
| uint8_t reg_address = data.reg_address; | ||
| status = i2c_master_address_read( | ||
| i2c_base_, data.slave_address, ®_address, sizeof(reg_address), payload, | ||
| data.read_length); | ||
| } else { | ||
| status = i2c_master_read(i2c_base_, data.slave_address, payload, data.read_length); | ||
| } | ||
|
|
||
| if (status != status_success) { | ||
| publish_error(make_error_view(data)); | ||
| return; | ||
| } | ||
|
|
||
| publish_read_result( | ||
| data.slave_address, {read_buffer_.data(), data.read_length}, data.has_register, | ||
| data.reg_address); | ||
| } |
There was a problem hiding this comment.
RMCS 固件这边也缺了 7-bit 地址校验。
这两个入口只限制了长度，没有限制 slave_address <= 0x7F。现在 host SDK 会做一次校验，但协议层/固件层仍然需要自己兜底；否则畸形包会直接落到 i2c_master_write/read*()。建议在写和读配置入口都先拒绝非法地址，再发布对应错误。
🛡️ 建议修改
+ static bool is_valid_slave_address(uint8_t slave_address) {
+ return slave_address <= 0x7F;
+ }
+
void handle_downlink_write(const data::I2cDataView& data) {
if (data.payload.empty())
return;
- if (data.payload.size() > kMaxDataLength) {
+ if (!is_valid_slave_address(data.slave_address) || data.payload.size() > kMaxDataLength) {
publish_error(make_error_view(data));
return;
}
@@
void handle_downlink_read_config(const data::I2cReadConfigView& data) {
- if (data.read_length == 0 || data.read_length > kMaxDataLength) {
+ if (!is_valid_slave_address(data.slave_address) || data.read_length == 0
+ || data.read_length > kMaxDataLength) {
publish_error(make_error_view(data));
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void handle_downlink_write(const data::I2cDataView& data) { | |
| if (data.payload.empty()) | |
| return; | |
| if (data.payload.size() > kMaxDataLength) { | |
| publish_error(make_error_view(data)); | |
| return; | |
| } | |
| auto* payload = reinterpret_cast<uint8_t*>(write_buffer_.data()); | |
| std::memcpy(payload, data.payload.data(), data.payload.size()); | |
| hpm_stat_t status = status_fail; | |
| if (data.has_register) { | |
| uint8_t reg_address = data.reg_address; | |
| status = i2c_master_address_write( | |
| i2c_base_, data.slave_address, ®_address, sizeof(reg_address), payload, | |
| static_cast<uint32_t>(data.payload.size())); | |
| } else { | |
| status = i2c_master_write( | |
| i2c_base_, data.slave_address, payload, static_cast<uint32_t>(data.payload.size())); | |
| } | |
| if (status != status_success) { | |
| publish_error(make_error_view(data)); | |
| return; | |
| } | |
| } | |
| void handle_downlink_read_config(const data::I2cReadConfigView& data) { | |
| if (data.read_length == 0 || data.read_length > kMaxDataLength) { | |
| publish_error(make_error_view(data)); | |
| return; | |
| } | |
| auto* payload = reinterpret_cast<uint8_t*>(read_buffer_.data()); | |
| hpm_stat_t status = status_fail; | |
| if (data.has_register) { | |
| uint8_t reg_address = data.reg_address; | |
| status = i2c_master_address_read( | |
| i2c_base_, data.slave_address, ®_address, sizeof(reg_address), payload, | |
| data.read_length); | |
| } else { | |
| status = i2c_master_read(i2c_base_, data.slave_address, payload, data.read_length); | |
| } | |
| if (status != status_success) { | |
| publish_error(make_error_view(data)); | |
| return; | |
| } | |
| publish_read_result( | |
| data.slave_address, {read_buffer_.data(), data.read_length}, data.has_register, | |
| data.reg_address); | |
| } | |
| static bool is_valid_slave_address(uint8_t slave_address) { | |
| return slave_address <= 0x7F; | |
| } | |
| void handle_downlink_write(const data::I2cDataView& data) { | |
| if (data.payload.empty()) | |
| return; | |
| if (!is_valid_slave_address(data.slave_address) || data.payload.size() > kMaxDataLength) { | |
| publish_error(make_error_view(data)); | |
| return; | |
| } | |
| auto* payload = reinterpret_cast<uint8_t*>(write_buffer_.data()); | |
| std::memcpy(payload, data.payload.data(), data.payload.size()); | |
| hpm_stat_t status = status_fail; | |
| if (data.has_register) { | |
| uint8_t reg_address = data.reg_address; | |
| status = i2c_master_address_write( | |
| i2c_base_, data.slave_address, ®_address, sizeof(reg_address), payload, | |
| static_cast<uint32_t>(data.payload.size())); | |
| } else { | |
| status = i2c_master_write( | |
| i2c_base_, data.slave_address, payload, static_cast<uint32_t>(data.payload.size())); | |
| } | |
| if (status != status_success) { | |
| publish_error(make_error_view(data)); | |
| return; | |
| } | |
| } | |
| void handle_downlink_read_config(const data::I2cReadConfigView& data) { | |
| if (!is_valid_slave_address(data.slave_address) || data.read_length == 0 | |
| || data.read_length > kMaxDataLength) { | |
| publish_error(make_error_view(data)); | |
| return; | |
| } | |
| auto* payload = reinterpret_cast<uint8_t*>(read_buffer_.data()); | |
| hpm_stat_t status = status_fail; | |
| if (data.has_register) { | |
| uint8_t reg_address = data.reg_address; | |
| status = i2c_master_address_read( | |
| i2c_base_, data.slave_address, ®_address, sizeof(reg_address), payload, | |
| data.read_length); | |
| } else { | |
| status = i2c_master_read(i2c_base_, data.slave_address, payload, data.read_length); | |
| } | |
| if (status != status_success) { | |
| publish_error(make_error_view(data)); | |
| return; | |
| } | |
| publish_read_result( | |
| data.slave_address, {read_buffer_.data(), data.read_length}, data.has_register, | |
| data.reg_address); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@firmware/rmcs_board/app/src/i2c/i2c.hpp` around lines 34 - 87, 在
handle_downlink_write 和 handle_downlink_read_config 中补充对 7-bit I2C
地址的校验（slave_address 必须 <= 0x7F）；在发现非法地址时直接调用
publish_error(make_error_view(data)) 并返回，避免继续调用 i2c_master_write/i2c_master_read
系列函数。参照已有长度校验逻辑（kMaxDataLength）将地址校验放在最前面以尽早拒绝畸形包，保证对 has_register 分支都生效。
Comment on lines
+158
to
+167
| void i2c_error_callback(data::DataId id, uint8_t slave_address) final { | ||
| switch (id) { | ||
| case data::DataId::kI2c0: i2c0_error_callback(slave_address); break; | ||
| default: break; | ||
| } | ||
| } | ||
|
|
||
| virtual void i2c0_receive_callback(const librmcs::data::I2cDataView& data) { (void)data; } | ||
|
|
||
| virtual void i2c0_error_callback(uint8_t slave_address) { (void)slave_address; } |
There was a problem hiding this comment.
这里把 I2C 错误详情截断没了。
当前 host 回调链最终只把 slave_address 透传给应用层，但这次 PR 明确新增了操作类型、寄存器地址和请求长度等错误信息。到 i2c0_error_callback(uint8_t) 这一层后，这些字段已经全部丢失，应用层无法判断到底是哪一种 I2C 事务失败了。建议把这里的回调面改成接收 data::I2cErrorView（或等价结构）并一路透传。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@host/include/librmcs/agent/rmcs_board.hpp` around lines 158 - 167, 当前回调只透传
slave_address，丢失了操作类型/寄存器/长度等错误详情；把 i2c_error_callback(data::DataId, uint8_t)
的传递链改为透传完整错误视图（data::I2cErrorView）并把虚方法签名改为 virtual void
i2c0_error_callback(const librmcs::data::I2cErrorView& error) ，同时在
i2c_error_callback 的 switch 分支中调用 i2c0_error_callback(error)（或等价命名），确保新增的 I2C
错误字段一路传到应用层以便判断具体失败事务。
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 introduces end-to-end I2C support across the protocol layer, host SDK, and board firmware, and follows up with the
reliability fixes needed to make the implementation stable on real hardware.
Changes
RmcsBoardand expose the corresponding host APIsCBoardtransport support on the physicalI2C2bus while keeping the logicalI2C0protocol mappingCBoardtransfer handling and add DMA timeout recovery to avoid stuck transfersCBoardI2C interrupt priority to prevent USB interferencehpm_sdkto restorehpm5300evkboard definitions required byrmcs_boardbring-up