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

Automatically decode JSON payload in on_topic decorator #8

Open
koenvervloesem opened this issue Jun 7, 2020 · 8 comments
Open

Automatically decode JSON payload in on_topic decorator #8

koenvervloesem opened this issue Jun 7, 2020 · 8 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@koenvervloesem
Copy link
Member

From the forum:

It would be great if there were a parameter in the decorator to say the system “payload is json” and it doing the json-load and decode for you.

@koenvervloesem koenvervloesem added enhancement New feature or request good first issue Good for newcomers labels Jun 7, 2020
@JonahKr
Copy link
Contributor

JonahKr commented Aug 5, 2020

This would actually help issue #12 aswell. By decoding the payload before passing to the decorated method to a dictionary for example, you could access the slots by just payload[slots] if i understood it correctly.

@H3adcra5h
Copy link
Contributor

H3adcra5h commented Feb 2, 2021

But then this example "@app.on_topic("hermes/+/{site_id}/playBytes/#")" would not work anymore. In some of my cases I need the payload as bytes.
Maybe with a second parameter we can force decoding from Json and putting the result into TopicData and let payload untouched or empty.

@koenvervloesem
Copy link
Member Author

Yes, I would definitely make this optional. @H3adcra5h are you using the on_topic decorator in your projects? I haven't been using it, so these and other issues have been lingering, but I'm all for it and if someone creates a pull request I'll definitely merge it.

@H3adcra5h
Copy link
Contributor

Yes I do. That's why I implemented this. In my app(s) I do much more than intent handling. Some of my own sensors communicating over MQTT and I use this project too. Otherwise I would have to implement it twice.

On simple example where I use the bytes is publishing messages for all satellites if an important alert comes in. My master is running complete headless, no mic no speaker. For alerting if use hermes/tts/say on the master I copy the results from hermes/audioServer/master/playBytes to all satellites. It's faster than using "say" for all satellites.

@JonahKr
Copy link
Contributor

JonahKr commented Feb 2, 2021

Yes, I would definitely make this optional.

I would definitely make it opt out though. I think for most usecases a standard decoded format is the way to go.

@koenvervloesem
Copy link
Member Author

koenvervloesem commented Feb 2, 2021

It depends on the use case of course. I'm not sure which use cases are the most popular, because frankly I have no clue who is using rhasspy-hermes-app :-)

In SnipsKit I did decode the payload as JSON by default: https://snipskit.readthedocs.io/en/latest/API.html#module-snipskit.mqtt.decorators. If I remember correctly, I did this because then you wouldn't have to do anything special to be able to address the JSON content like a dict, e.g. payload[slots] and so on.

What other use cases are you seeing, @JonahKr?

@JonahKr
Copy link
Contributor

JonahKr commented Feb 2, 2021

I was more thinking about the users rather than the specific appliences.
It is simpler and more straightforeward if you have all the data you could need directly available instead of going the additional step of destructing it. Its just another feature lowering the bar for new users.
Additionally it wouldn't be a breaking change for existing apps.
@app.on_topic("app") would still work and binary data could be accessible like @app.on_topic("app", bin_payload=True)

@H3adcra5h
Copy link
Contributor

The intention for my implementation was the possibility to handle topics outside of the main scope "intent handling" in a flexible way. The decorator will definitely work with all topics.

Most of the topics in rhasspy are json based, that's true, but not all, e.g. playBytes or audioCaptured. If you want to make it as easy as possible, than all main topics should have it's own decorator.
I have no clue if this is useful for others, definitely not for me. In my eyes the flexibility is better way and I think content = json.loads(payload.decode('utf-8')) istn't too heavy to understand for beginners.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants