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

Add RTTTL volume control. #5968

Merged
merged 34 commits into from Feb 27, 2024
Merged

Conversation

nielsnl68
Copy link
Contributor

@nielsnl68 nielsnl68 commented Dec 19, 2023

What does this implement/fix?

Add Gain option to set the volume.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#3644

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040
  • BK72xx
  • RTL87xx

Example

   rtttl:
      speaker: my_speaker
      id: my_rtttl
      gain: 0.8

@probot-esphome
Copy link

Hey there @glmnet, mind taking a look at this pull request as it has been labeled with an integration (rtttl) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not tested this yet, but should this be a config option like volume between 0-10 etc that maps to bigger values?

@nielsnl68
Copy link
Contributor Author

I have not tested this yet, but should this be a config option like volume between 0-10 etc that maps to bigger values?

Yes, but i need to figure out what the high and low values should be.

@nielsnl68 nielsnl68 marked this pull request as draft December 20, 2023 00:07
@nielsnl68 nielsnl68 marked this pull request as ready for review December 20, 2023 00:07
@jesserockz
Copy link
Member

Yes, but i need to figure out what the high and low values should be.

True...haha

@edwardtfn
Copy link
Contributor

edwardtfn commented Dec 20, 2023

Could this be like a volume_gain argument on the rtttl settings?

Something like:

rtttl:
  output: rtttl_out
  id: my_rtttl
  gain: 2

Or:

rtttl:
  output: rtttl_out
  id: my_rtttl
  gain: 0.5

So you multiply 8192 by the gain, which will be float, with 1 as default or not used and limited by something like 0 to 10 (I haven't tested).

My point is more that naming gain makes more sense than volume in this case.

@nielsnl68
Copy link
Contributor Author

Could this be like a volume_gain argument on the rtttl settings?

Something like:

rtttl:
  output: rtttl_out
  id: my_rtttl
  gain: 2

Or:

rtttl:
  output: rtttl_out
  id: my_rtttl
  gain: 0.5

So you multiply 8192 by the gain, which will be float, with 1 as default or not used and limited by something like 0 to 10 (I haven't tested).

My point is more that naming gain makes more sense than volume in this case.

I think this is the way to go.

@probot-esphome probot-esphome bot removed the small-pr label Dec 25, 2023
@nielsnl68
Copy link
Contributor Author

Okay @edwardtfn, @jesserockz i have added the gain option it will be a value between 0.1 to 1. And will be default set to 0.5.

@nielsnl68 nielsnl68 marked this pull request as ready for review January 9, 2024 01:24
@esphome esphome bot requested a review from jesserockz January 9, 2024 01:24
@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.81%. Comparing base (4d8b5ed) to head (3c281b2).
Report is 30 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #5968      +/-   ##
==========================================
+ Coverage   53.70%   53.81%   +0.10%     
==========================================
  Files          50       50              
  Lines        9408     9438      +30     
  Branches     1654     1660       +6     
==========================================
+ Hits         5053     5079      +26     
+ Misses       4056     4052       -4     
- Partials      299      307       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of things below, and also can you please update the PR title and description and also add a docs PR for the new config?

Cheers
Jesse

@@ -254,7 +254,7 @@ void Rtttl::loop() {
// Add small silence gap between same note
this->output_freq_ = freq;

ESP_LOGVV(TAG, "playing note: %d for %dms", note, this->note_duration_);
ESP_LOGI(TAG, "playing note: %d for %dms", note, this->note_duration_);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this log level be reverted?

esphome/components/rtttl/__init__.py Show resolved Hide resolved
@esphome esphome bot marked this pull request as draft February 19, 2024 01:28
@nielsnl68 nielsnl68 changed the title try to set the volume higher. try to set the RTTTL volume higher. Feb 19, 2024
@nielsnl68
Copy link
Contributor Author

Thanks for the review @jesserockz. i resolved the issues you reported

@nielsnl68 nielsnl68 marked this pull request as ready for review February 20, 2024 01:17
@esphome esphome bot requested a review from jesserockz February 20, 2024 01:17
@nielsnl68 nielsnl68 changed the title try to set the RTTTL volume higher. Add RTTTL volume control. Feb 27, 2024
@nielsnl68
Copy link
Contributor Author

A couple of things below, and also can you please update the PR title and description and also add a docs PR for the new config?

Cheers Jesse

I have updated the documentation now.

esphome/components/rtttl/__init__.py Outdated Show resolved Hide resolved
@esphome esphome bot marked this pull request as draft February 27, 2024 19:11
@jesserockz jesserockz marked this pull request as ready for review February 27, 2024 20:43
@esphome esphome bot requested a review from jesserockz February 27, 2024 20:43
@jesserockz jesserockz merged commit c43c9ad into esphome:dev Feb 27, 2024
55 checks passed
@nielsnl68 nielsnl68 deleted the nvds-rttti-speaker-volume branch February 28, 2024 01:09
@github-actions github-actions bot locked and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants