-
Notifications
You must be signed in to change notification settings - Fork 30
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
Setting volume to out of range integer makes Intent fail #24
Comments
The regex |
@preethamvishy yes you are correct. I think the issue here is that adapt has problems with the regex for some reason. Changing it to |
But what happens when user wants to set the volume to 12% percent as opposed to an out-of-bounds Level 12? |
The current implementation tries to interpret the utterance as a percentage if it's But there is no real check for a percent keyword so this is just software guessing. |
Exactly. So limiting the bounds to [0-11] would mean that we only want to use Levels going forward instead of volume percentages. Is this the expected behavior? |
I think it's better if @KathyReid answers this. Personally, I could see a case for limiting the result to 0-11 and adding a separate handler for values in percent (if a "Percent" keyword is found. |
Great questions. This is really a voice user interface decision. In normal usage, it would be highly unlikely that the user would select a volume of 11% or less - this would be barely audible. But it is likely that if they spoke the Utterance "set volume to 40" or "set volume to 20" or "set volume to 60" that they are implying percent. I don't think we actually need the Use cases:
Does this seem sensible? |
Sounds sensible and reflects the current logic well. I believe looking over the regex would be all that's required for this. |
Due this issue is tagged with hacktoberfest i'd like to give it a try. In my opinion setting volume in % seems most natural. So i'd accept any value from 0 (mute) to 100 (max volume) and treat it as % value regardless if the %-percent is in utterance. Any thoughts on that? Does this make sense? I didn't check code till now so not sure if my idea is a good one. |
Hey Thorsten, We consider any of the low numbers to be a volume out of 10. It used to be out of 11 but I'm fairly sure this has been changed. So it's basically as Kathy outlined above. If a user says "set volume to 6", it is the equivalent of "set volume to 60%" I believe that all of these are implemented except the "out of bounds" piece. If someone asks for a volume of 120 I believe it just reports the current volume. |
I summarize what i think i understood. Three options are possible: Option 1: Accept all values between 0 and 100
Option 2: Accept values 0-10 (not treated as percent)
Option 3: Accept values 0-100 (treat every input as percent)
I my opinion option 1 might be very confusing to users - why is „5“ (50%) louder than „12“ (12%). Did i get this right? |
Hey, you did get it right, however it's option 1 that we have and I think it is workable. It's because some people think of volume out of 10 and others think of it more as a percentage. I actually use both forms at different times myself. This does mean that there will always be a strange change over point, and because the volume used to be from 0-11, we also have:
where as
So 11 is significantly louder than 12. Though I doubt many people are really wanting their volume to be at precisely 11%. I used to think this was a SpinalTap reference, but I'm assuming it was actually because there are 12 LEDs in each eye of the Mark 1, 0% volume keeps 1 LED on, and 100% volume is 11 more LEDs... |
Hey @krisgesling .
I think you're right on this. Such as volume settings: In this case the user doesn't need to struggle with any numeric value. User just speaks "set volume to quiet". Maybe combined with increase or decrease volume on few steps. |
As it's not about adding new features such as presets i tried to find out what the actual problem is or if there's still any problem? Within the past two years there has been some commits on this maximum volume issue. |
There's definitely been work on it so the functionality should be there for anything within the range of 0-100. What I think is still missing are handling phrases like:
Reporting the current volume instead is ok. I guess it gently gives the user some feedback on the current state of the system and they're likely to try again with different wording. But they might just assume that the device misunderstood and retry the same utterance. Throwing the error is clearly a bug and should not happen. So we should work out under what circumstances that can happen and handle them appropriately. I think we want to use this as an education opportunity for users by guiding them on the best ways to interact, but still doing what we assume they want, in this case a very high volume. Eg
I love the idea of creating some human understandable levels too! |
Also increase and decrease I'm fairly sure make 10% jumps |
Hey @krisgesling. I'm checking code on 120% issue.
I don't get an error on picroft, but the request is silently ignored which may bring user to repeat request. Lines 100 to 102 in 5777b2e
Lines 156 to 158 in 5777b2e
Should Mycroft answer to all % (percent) values and anwer with a dialoge that percentage values are only allowed between 0 and 100? |
Yeah I forgot that these were registered as their own vocab. It raises the question of how far do we go, 120%, 150%, 200%, 2000%... Perhaps a better way to go about this is to see if we can detect that someone wants to change the volume, but no valid level has been detected. For example a new intent that only requires
Could also keep track of how many times this has been triggered and if it gets triggered a couple of times provide a more detailed response like:
|
We discussed this a bit in chat as well: https://chat.mycroft.ai/community/pl/rucgx3rgp38cifz91e9nk1e4fr |
Object deviation description
The expected behaviour if the user sets the volume to an out of bounds integer (ie, 12 in this case) is for the Skill to detect that the number is out of bounds, set the volume to the closest in-bound integer (ie 10) and then notify the user. The actual behaviour observed is that an out of bounds integer causes the Intent to fail.
Root cause thoughts
The
regex
defined at https://github.com/MycroftAI/skill-volume/blob/18.08/regex/en-us/volume.amount.rxis
(?P<Level>\d+)
this matches integer values
0-9
, so 10 and above is out of bounds.A little bit of regex work would make this intent more flexible.
The text was updated successfully, but these errors were encountered: