Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stm32h7/linum-stm32h753bi: add tone support #15310

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

JorgeGzm
Copy link
Contributor

Summary

Add support to play sound using the tone library.

Impact

Play the sounds using the buzzer with tone library.

Testing

build the tone sample

$ ./tools/configure.sh linum-stm32h753bi:tone

@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Board: arm Size: M The size of the change in this PR is medium labels Dec 23, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 23, 2024

[Experimental Bot, please feedback here]

No, this PR description does not meet the NuttX requirements. It's far too brief and lacks crucial details. Here's why and how it can be improved:

Summary Issues:

  • Insufficient detail: "Add support to play sound using the tone library" is vague. Which sounds? How are they specified? Is this a new application, a driver change, or a modification to the tone library itself?
  • Missing context: Why is this change necessary? Is there a user need or is it fixing a bug?
  • No links: Are there related issues in the NuttX or NuttX Apps repositories?

Impact Issues:

  • Unclear descriptions: "Play the sounds using the buzzer with tone library" doesn't explain the impact. Does this introduce a new API? Does it change existing behavior?
  • Missing yes/no answers: The template requires explicit "YES" or "NO" answers for each impact category, followed by a description if "YES." This PR provides neither.

Testing Issues:

  • Insufficient host details: Simply stating the board configuration is not enough. Provide the host OS, CPU architecture, and compiler version used for the build.
  • Missing target details: Specify the simulator used if applicable. If a real board, provide more details about the configuration.
  • No logs: The template explicitly requests "before" and "after" logs demonstrating the change. This PR omits them entirely.

Here's an example of a better PR description:

Summary

This PR adds support for playing a predefined set of tones using the existing tone library. This addresses the need for simple audio feedback in applications without requiring complex audio codecs. The change introduces a new example application (apps/examples/tone_player) that demonstrates the functionality.

Related NuttX Issue: #1234 (Hypothetical issue requesting audio feedback)

Impact

  • Is new feature added? YES (New tone_player application and associated API functions)
  • Impact on user (will user need to adapt to change)? YES (Users can now incorporate audio feedback using the new API and example application.)
  • Impact on build (will build process change)? NO
  • Impact on hardware (will arch(s) / board(s) / driver(s) change)? NO (Assumes an existing buzzer driver; if a new driver is required, this section must be updated.)
  • Impact on documentation (is update required / provided)? YES (Documentation will be updated to describe the new API and example application.)
  • Impact on security (any sort of implications)? NO
  • Impact on compatibility (backward/forward/interoperability)? NO
  • Anything else to consider? None.

Testing

I confirm that changes are verified on local setup and works as intended:

  • Build Host(s): Linux (Ubuntu 22.04), x86_64, GCC 11.2.0
  • Target(s): sim:linum-stm32h753bi:tone

Testing logs before change:

(No existing functionality; application did not build)

Testing logs after change:

nsh> tone_player play 1
Playing tone 1 (C4)
nsh> tone_player play 2
Playing tone 2 (D4)

This improved description provides the necessary detail and context required for a proper review. Remember to replace the placeholder information with your actual details.

@xiaoxiang781216
Copy link
Contributor

@JorgeGzm could you rebase the last commit? the ci break fix just merged.

@xiaoxiang781216 xiaoxiang781216 merged commit 4d7be17 into apache:master Dec 23, 2024
26 checks passed
@JorgeGzm JorgeGzm deleted the add_tone branch December 23, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Documentation Improvements or additions to documentation Board: arm 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.

3 participants