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

[rest] Added Voice / TTS API #1017

Merged
merged 3 commits into from
Sep 2, 2019
Merged

[rest] Added Voice / TTS API #1017

merged 3 commits into from
Sep 2, 2019

Conversation

lolodomo
Copy link
Contributor

Fix #927

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

Fix #927

Signed-off-by: Laurent Garnier <[email protected]>
@lolodomo lolodomo changed the title REST Voice API: new API for TTS {WIP] REST Voice API: new API for TTS Aug 31, 2019
@lolodomo lolodomo changed the title {WIP] REST Voice API: new API for TTS REST Voice API: new API for TTS Aug 31, 2019
@lolodomo
Copy link
Contributor Author

In addition to this enhancement, we need another REST API for audio to get the list of audio sinks. I will cover that in a separate PR.

Signed-off-by: Laurent Garnier <[email protected]>
@@ -67,6 +71,7 @@
UriInfo uriInfo;

private VoiceManager voiceManager;
private AudioManager audioManager;
Copy link
Member

Choose a reason for hiding this comment

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

You should imho do without that dependency - the VoiceManager offers methods to call without sinkIds and it handles the calls internally. The REST API should behave the same way as the Java API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, this was necessary just to make a pre-control and avoid returning status 200 when say will fail because the provided audio sink is wrong.
But I can suppress this test and so this dependency to AudoManager.

}

@POST
@Path("/say/{sinkid: [a-zA-Z_:0-9]*}")
Copy link
Member

Choose a reason for hiding this comment

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

The sinkId is not a member of an entity "say", so it should imho rather be a QueryParam instead of a PathParam here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense.
I think I do it like that because the interpret API just above was using this pattern.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but for the interpreters call, it is selecting an entity by id from the list of interpreters, so it is a different situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand the difference.
Here I want to select a voice amongst a list of voices, and a sink amongst a list of audio sinks.

Copy link
Member

Choose a reason for hiding this comment

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

Your uri is say/sinkid and not sinks/sinkid or voices/voiceid. Hence "sinkid" is not an entity of the "say" entities. See the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean now.

}

@POST
@Path("/say/{sinkid: [a-zA-Z_:0-9]*}/{voiceid: [a-zA-Z_:0-9]*}")
Copy link
Member

Choose a reason for hiding this comment

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

Same here: Make sinkid and voiceid to QueryParams (this will actually allow you to combine the three methods into one as two params can both be optional).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible to have QueryParam in a POST command ?

Copy link
Member

Choose a reason for hiding this comment

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

I would think/hope so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a doubt. I will check if we have such case in all our REST APIs (POST).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing a WEB search, it looks like this is possible.

@lolodomo
Copy link
Contributor Author

lolodomo commented Sep 2, 2019

Changes done. My tests are ok.

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.

Thanks, looks much cleaner, doesn't it?

@kaikreuzer kaikreuzer merged commit a1f3880 into openhab:master Sep 2, 2019
@lolodomo lolodomo deleted the rest_voice_tts branch September 2, 2019 21:16
@cweitkamp cweitkamp added this to the 2.5 milestone Sep 4, 2019
@cweitkamp cweitkamp changed the title REST Voice API: new API for TTS [rest] Added Voice / TTS API Dec 3, 2019
@cweitkamp cweitkamp added the enhancement An enhancement or new feature of the Core label Dec 3, 2019
Rosi2143 pushed a commit to Rosi2143/openhab-core that referenced this pull request Dec 26, 2020
…". If you do you will see this `warning: 2019-07-11 20:57:02.024 [WARN ] [el.core.internal.ModelRepositoryImpl] - Configuration model 'groundfloor.sitemap' has errors, therefore ignoring it: [31,91]: mismatched input '"20"' expecting RULE_INT` (openhab#1017)

Line 31 from groundfloor.sitemap:
```
Colorpicker item=cinema_led_strip_color label="Cinema LED Strip Color[]" sendFrequency="20"
```

Signed-off-by: Soren Thorsen <[email protected]>
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023
* REST Voice API: new API for TTS

Fix openhab#927

Signed-off-by: Laurent Garnier <[email protected]>
GitOrigin-RevId: a1f3880
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core REST/SSE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REST API] Endpoint for "Say" under "Voice" missing
3 participants