Skip to content

Conversation

@chenzihan0416
Copy link
Contributor

Summary

Added the ability to set sampling rate for goldfish

Impact

Now goldfish sensor can adjust the sampling rate

Testing

qemu uorb_listener

Added the ability to set sampling rate for goldfish

Signed-off-by: chenzihan1 <chenzihan1@xiaomi.com>
@github-actions github-actions bot added Area: Sensors Sensors issues Size: M The size of the change in this PR is medium labels Jan 3, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 3, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements as described. While it provides a summary and mentions testing, it lacks crucial details. Here's a breakdown of what's missing:

  • Summary: While it states what was added, it omits why this change is necessary. What problem does it solve? What use case does it enable? How does it work (implementation details)? Are there any related issues?
  • Impact: The description is very vague. It needs to be more specific. Does this impact only the goldfish sensor? Are there any user-facing changes (e.g., new configuration options)? Does this change the build process (new Kconfig options, etc.)? All "Impact" sections require explicit "YES" or "NO" answers, followed by details if "YES". The current response misses all of these details.
  • Testing: "qemu uorb_listener" is insufficient. What exactly was tested? What were the expected results? The requirement explicitly asks for logs before and after the change to demonstrate the impact and correctness of the modification. What host and target platforms were used for testing? Provide specifics about the OS, architecture, board, and configuration.

In short, the PR needs significantly more detail to meet the NuttX requirements. It needs to be more explicit and demonstrate a thorough understanding of the change's impact and provide evidence of proper testing.

@xiaoxiang781216
Copy link
Contributor

let's ignore ci error fixed by: apache/nuttx-apps#2925

@xiaoxiang781216 xiaoxiang781216 merged commit 724797e into apache:master Jan 9, 2025
24 of 27 checks passed
@chenzihan0416 chenzihan0416 deleted the your-topic branch December 31, 2025 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Sensors Sensors issues Size: M The size of the change in this PR is medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants