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.

Sign up for GitHub

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

Jump to bottom

[voicerss] Add LRU cache #14561

Merged
merged 1 commit into from
Jul 12, 2023
Merged

[voicerss] Add LRU cache #14561

merged 1 commit into from
Jul 12, 2023

Conversation

lolodomo
Copy link
Contributor

@lolodomo lolodomo commented Mar 9, 2023

Signed-off-by: Laurent Garnier [email protected]

@lolodomo lolodomo added the enhancement An enhancement or new feature for an existing add-on label Mar 9, 2023
@lolodomo lolodomo requested a review from JochenHiller as a code owner March 9, 2023 20:22
@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 9, 2023

@dalgwen : here is a first implementation.
It is working well when the cache is enabled. But when I disable the cache, I see that my method synthesizeForCache is called but then I have no rendering (no sound). I am using the command say. I suppose there is a bug in the code of the cache when the cache is disabled.

@lolodomo lolodomo mentioned this pull request Mar 9, 2023
Closed
@lolodomo lolodomo added help wanted work in progress A PR that is not yet ready to be merged and removed help wanted labels Mar 9, 2023
@lolodomo lolodomo changed the title [voicerss] Use the common TTS cache mechanism from core framework [voicerss] Add LRU cache Mar 10, 2023
@J-N-K
Copy link
Member

J-N-K commented Jun 13, 2023

What is the state here?

@lolodomo
Copy link
Contributor Author

Just waiting for core audio PR to be merged. Then I will try again but it should then work properly.

PS: we have several PRs about TTS services waiting for this core PR to be merged.

@lolodomo lolodomo removed the work in progress A PR that is not yet ready to be merged label Jun 18, 2023
@lolodomo
Copy link
Contributor Author

This PR is now finished and properly tested. But it is preferable to first wait for an update of all audio sinks. If not, the case where the TTS cache is disabled will probably not work with most of audio sinks.

@lolodomo lolodomo mentioned this pull request Jun 18, 2023
Closed
7 tasks
@lolodomo lolodomo force-pushed the voicerss_cache branch 3 times, most recently from d0050b7 to 481fce3 Compare June 18, 2023 16:50
@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 1, 2023 •
edited
Loading

To be updated when openhab/openhab-core#3675 will be merged.

@lolodomo lolodomo added the awaiting other PR Depends on another PR label Jul 1, 2023
@lolodomo lolodomo removed the awaiting other PR Depends on another PR label Jul 7, 2023
@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 7, 2023

This PR is now ready for a review. It has been fully tested with Sonos as audio sink, either with cache enabled or disabled.

@lolodomo
[voicerss] Add LRU cache
50c10c2 
Signed-off-by: Laurent Garnier <[email protected]>
@lolodomo lolodomo marked this pull request as draft July 8, 2023 12:06
@lolodomo lolodomo marked this pull request as ready for review July 8, 2023 16:17
kaikreuzer
kaikreuzer approved these changes Jul 12, 2023
View reviewed changes
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@kaikreuzer kaikreuzer merged commit e58991c into openhab:main Jul 12, 2023
@kaikreuzer kaikreuzer added this to the 4.0 milestone Jul 12, 2023
@lolodomo
Copy link
Contributor Author

Excellent, thank you.

@lolodomo lolodomo deleted the voicerss_cache branch July 12, 2023 20:02
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
@lolodomo @austvik
[voicerss] Add LRU cache (openhab#14561)
f344881 
Signed-off-by: Laurent Garnier <[email protected]>
Signed-off-by: Jørgen Austvik <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaikreuzer kaikreuzer kaikreuzer approved these changes

@JochenHiller JochenHiller Awaiting requested review from JochenHiller

Assignees
No one assigned
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@lolodomo @J-N-K @kaikreuzer