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 voice pack managemenet capability #725

Merged
merged 14 commits into from
Mar 4, 2021

Conversation

depau
Copy link
Contributor

@depau depau commented Mar 1, 2021

Type of change

Type B:

  • New capability

Description (Type B)

Feature discussion thread: well...

As mentioned in Telegram, I don't think this way of implementing it makes it particularly complex (though slightly less user friendly).

I designed this as a single capability because, at least for the Viomi, separating the "apply local voice pack" and "download new voice pack" functionalities are very similar and creating two capabilities would increase duplication by a lot.

As far as I can tell all vacuums do not accept an upload, but they accept a URL from which they can download the pack. So I exposed it as such, uploading the pack to valetudo and having it serve it back to the vacuum software is quite messy and complex.

Functionality covers:

  • Retrieving locally-available voice packs + voice packs that the vacuum or capability can retrieve autonomously
  • Applying such voice packs
  • Retrieving current voice pack language
  • Optionally downloading a custom voice pack from URL
  • Retrieving operation progress (which can be checked for both the download of a custom pack and for the application of an available voice pack)

The reason why I did not split this in two is that on Viomi the process of applying a local or remote voice pack is exactly the same.

@Hypfer
Copy link
Owner

Hypfer commented Mar 1, 2021

So I exposed it as such, uploading the pack to valetudo and having it serve it back to the vacuum software is quite messy and complex.

I like that idea.

Can we ditch the local voicepack part and just allow the download and install of a remote URL though?
At least for roborock, there doesn't seem to be this concept of a local cache and I'd say that even if there were one, it wouldn't hurt to redownload every time.

So basically just have

  • downloadCustomVoicePack
    and
  • getVoicePackOperationProgress

@depau
Copy link
Contributor Author

depau commented Mar 1, 2021

So I exposed it as such, uploading the pack to valetudo and having it serve it back to the vacuum software is quite messy and complex.

I like that idea.

Can we ditch the local voicepack part and just allow the download and install of a remote URL though?
At least for roborock, there doesn't seem to be this concept of a local cache and I'd say that even if there were one, it wouldn't hurt to redownload every time.

So basically just have

* downloadCustomVoicePack
  and

* getVoicePackOperationProgress

Viomi doesn't cache either, it simply has 2 voice packs in the ROM. It only optionally downloads one voice pack at a time and it deletes it when you switch to a new one, including the stock ones in ROM.

I would not remove it because:

  • It would make it a lot more troublesome to set one of the stock voice packs, since you'd have to pack them yourself and upload them, which also means they take additional space on a device that already has super tiny flash storage
  • It opens up for future implementations that are able to transparently generate URLs to download the official packs from the vendor's cloud

@depau
Copy link
Contributor Author

depau commented Mar 2, 2021

Okay, the base capability + router are implemented, I will go ahead and add an implementation for Viomi.

Remarks:

  • I kept the built-in voice pack management methods for the reasons detailed above. I also noticed viomi won't let you set english/chinese from a downloaded URL anyway.
  • The download voice pack method now takes only presignedUrl as mandatory. Hash and language can optionally be provided. The capability will have to decide what to do if they are required and missing. As discussed in telegram, viomi won't let you set an invalid language code.
Viomi language codes

image

  • I provided a default implementation for the following methods:
    • getAvailableStockVoicePacks(): empty list
    • enableStockVoicePack(): Error("Not supported")
    • canDownloadCustomVoicePack(): false
    • downloadCustomVoicePack(): Error("Not supported")
    • getVoicePackOperationProgress(): null (== no operation in progress)

The REST API is defined as follows:

  • GET / => {"currentLanguage": "xx", "operationProgress": int|null}
  • GET /stockPack => [...] available built-in voice packs
  • PUT /stockPack body {"action": "enable", "language": "xx"}
  • GET /customPack => {"supported": bool}
  • PUT /customPack body {"action": "download", "presignedUrl": "http://...", "language": "optional", "hash": "optional"}

I updated the hoppscotch config with these requests.

@Hypfer
Copy link
Owner

Hypfer commented Mar 2, 2021

which also means they take additional space on a device that already has super tiny flash storage

If a user wants to revert to stock, he might as well just enable the raw command capability and use that.
There's no such thing as baked-in voicepacks on other robots so it doesn't make sense to build that into the capability.

It opens up for future implementations that are able to transparently generate URLs to download the official packs from the vendor's cloud

No. Under absolutely no circumstance will Valetudo ever talk to the vendor's cloud.

Error("Not supported")

The whole point of capabilities is that they're implemented fully so that the UI/REST Interface can rely on all features being there if the capability is there. Therefore, this doesn't make sense

@depau
Copy link
Contributor Author

depau commented Mar 2, 2021

Alright.

Thinking about it, since no checks are performed against the hash and URL, one could simply provide "local" for both the URL and hash to set a stock voice.

I'll remove that part.

depau added a commit to depau-forks/Valetudo that referenced this pull request Mar 2, 2021
@depau depau marked this pull request as ready for review March 2, 2021 21:06
@depau
Copy link
Contributor Author

depau commented Mar 2, 2021

New JS API:

  • getCurrentVoiceLanguage() abstract
  • downloadVoicePack(options) abstract
  • getVoicePackOperationProgress() default null

New REST API:

  • GET / => {"currentLanguage": "xx", "operationProgress": int|null}
  • PUT / body {"action": "download", "presignedUrl": "http://...", "language": "optional", "hash": "optional"}

On Viomi one can still send {"action": "download", "presignedUrl": "local", "hash": "local", "language": "en/zh"} to enable the stock packs.

@depau depau requested a review from Hypfer March 3, 2021 22:58
Copy link
Owner

@Hypfer Hypfer left a comment

Choose a reason for hiding this comment

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

Mostly bikeshedding at this point 😁

@depau
Copy link
Contributor Author

depau commented Mar 4, 2021

All done :)

@Hypfer Hypfer merged commit c953e2b into Hypfer:master Mar 4, 2021
@depau depau deleted the capabilities-voice-pack branch March 5, 2021 22:59
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants