Skip to content

Conversation

@BazookaJoe1900
Copy link
Contributor

This PR expend the fix done under 'stm32h7/stm32_i2c: fix sending large data over i2c #12093' to all stm32 chips

To trigger TC interrupt NBYTES needs to be set before RELOAD is disabled similar to previous commit done on stm32h7

Testing

I could test it on stm32f7 board that required to send large data. But don't have other boards to test it too.

To trigger TC interrupt NBYTES needs to be set before RELOAD is disabled
similar to previous commitdone on stm32h7 510b622
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: S The size of the change in this PR is small labels Dec 19, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 19, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and testing information, it lacks crucial details.

Here's a breakdown of what's missing:

  • Summary: While it explains the "why," it's vague on the "what" and "how." It mentions NBYTES and RELOAD but doesn't explain what those are or precisely how they're being modified. The reference to stm32h7/stm32_i2c: fix sending large data over i2c #12093 is helpful but insufficient; the PR should stand on its own.

  • Impact: This section is almost entirely empty. This is a critical part of the review process. At a minimum, the author needs to assess and document the impact on hardware (since it affects all STM32 chips) and compatibility. Even if the answer is "NO" for most categories, it needs to be explicitly stated.

  • Testing: While testing logs are provided, they lack context. What test was run? What constitutes success/failure? More importantly, testing on a single STM32F7 board is insufficient given the claim that the change affects all STM32 chips. A broader range of testing is needed or, at least, a justification for why it wasn't possible. The author acknowledges this limitation, but needs to address it more directly (e.g., propose a plan for wider testing or request help from maintainers with access to other hardware).

In short, the PR needs more detail and more thorough testing before it can be properly reviewed.

@xiaoxiang781216
Copy link
Contributor

let's ignore the ci error:

https://github.com/apache/nuttx/actions/runs/12410625523/job/34646742111?pr=15288

which is fixed by: #15290

@xiaoxiang781216 xiaoxiang781216 merged commit a7869bb into apache:master Dec 19, 2024
13 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: arm Issues related to ARM (32-bit) architecture Size: S The size of the change in this PR is small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants