MycroftAI / skill-volume

Mycroft AI official Volume Skill - control the volume of your Device
https://mycroft.ai/skills
Apache License 2.0
10 stars 30 forks source link

Setting volume to out of range integer makes Intent fail #24

Open KathyReid opened 6 years ago

KathyReid commented 6 years ago
>> Volume set to 10
 set volume to 12
 >> I'm not sure I understood you

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.

Wait, why is 10 working? shrug I don't have time to dig into this right now, flagging as #hacktoberfest.

Root cause thoughts

The regex defined at https://github.com/MycroftAI/skill-volume/blob/18.08/regex/en-us/volume.amount.rx is (?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.

preethamvishy commented 6 years ago

The regex(?P<Level>\d+) would match all sequences of digits from the input string and therefore, 10, 11 and 121 would all work. Isn't my understanding correct?

forslund commented 6 years ago

@preethamvishy yes you are correct. I think the issue here is that adapt has problems with the regex for some reason.

Changing it to to (?P<Level>\d+) makes it match correctly with @KathyReid's example sentence. (Currently the Level.voc is matched and that contains only numbers up to 11)

preethamvishy commented 6 years ago

But what happens when user wants to set the volume to 12% percent as opposed to an out-of-bounds Level 12?

forslund commented 6 years ago

The current implementation tries to interpret the utterance as a percentage if it's > 11. In the case of 12 it will result in a volume of 1.32 on the 11 step scale we use.

But there is no real check for a percent keyword so this is just software guessing.

preethamvishy commented 6 years ago

it will result in a volume of 1.32

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?

forslund commented 6 years ago

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.

KathyReid commented 6 years ago

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 percent added to the .voc file - my thinking is that this is a user interface behaviour.

Use cases:

Does this seem sensible?

forslund commented 6 years ago

Sounds sensible and reflects the current logic well. I believe looking over the regex would be all that's required for this.

thorstenMueller commented 4 years ago

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.

krisgesling commented 4 years ago

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%" The one change is that "10" and "11" are now both treated as max volume.

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.

thorstenMueller commented 4 years ago

I summarize what i think i understood. Three options are possible:

Option 1: Accept all values between 0 and 100

< 0: error
> 100: error
0 - 10: not treated as %, more as absolute values (0: mute, 10: max volume)
0% - 10%: Percentual value
5: 50%
5%: 5%
10: max volume
10%: 10%
100: max volume
100%: max volume

Option 2: Accept values 0-10 (not treated as percent)

< 0: error
> 10: error
0: mute
10: max volume
5: average volume (50%)

Option 3: Accept values 0-100 (treat every input as percent)

< 0: error
> 100: error
5: 5%
5%: 5%
0: mute
0%: mute
100: max volume
100%: max volume

I my opinion option 1 might be very confusing to users - why is „5“ (50%) louder than „12“ (12%). I’d prefer option 2 or 3.

Did i get this right?

krisgesling commented 4 years ago

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:

11: max volume
11%: 11%

where as

12: 12%
12%: 12%
13: 13%
13%: 13%
... and so on

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...

thorstenMueller commented 4 years ago

Hey @krisgesling . So 11 is significantly louder than 12. Though I doubt many people are really wanting their volume to be at precisely 11%. Hey @krisgesling .

Though I doubt many people are really wanting their volume to be at precisely 11%.

I think you're right on this. Did you ever thought on offering some sort of presets which can be controlled by the user under home.mycroft.ai?

Such as volume settings: key | value very quite | 10% quite | 25% normale | 50% loud | 75% TURN IT ON | 100%

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.

thorstenMueller commented 4 years ago

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? @KathyReid opened this issue two years ago having trouble with setting volume to > 10. I tested it and couldn't reproduce the behavior.

Within the past two years there has been some commits on this maximum volume issue. Is this issue right now still an issue?

krisgesling commented 4 years ago

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:

  1. "set the volume to 120" > reports current volume
  2. "set the volume to 120%" > throws an error

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 can't go above 100% volume, setting the volume to max.

I love the idea of creating some human understandable levels too!

krisgesling commented 4 years ago

Also increase and decrease I'm fairly sure make 10% jumps

thorstenMueller commented 4 years ago

Hey @krisgesling.

I'm checking code on 120% issue.

  • "set the volume to 120%" > throws an error

I don't get an error on picroft, but the request is silently ignored which may bring user to repeat request. After checking code these lines seems the reason for Mycroft ignoring percentage values > 100. https://github.com/MycroftAI/skill-volume/blob/5777b2e57928247cf24d96267e55c123c5aa651c/__init__.py#L100-L102

https://github.com/MycroftAI/skill-volume/blob/5777b2e57928247cf24d96267e55c123c5aa651c/__init__.py#L156-L158

Should Mycroft answer to all % (percent) values and anwer with a dialoge that percentage values are only allowed between 0 and 100?

krisgesling commented 4 years ago

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 Set.voc and Volume.voc. Any utterances that have a valid level or percentage should get a higher confidence match with the existing intents, so we could then use get_response to prompt for a new level. Something like:

User: "Hey Mycroft, set the volume to 120%" M: "How loud did you want it?"

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:

"My volume can be set from zero to ten, or using a percentage up to 100. How loud did you want it?"

forslund commented 4 years ago

We discussed this a bit in chat as well: https://chat.mycroft.ai/community/pl/rucgx3rgp38cifz91e9nk1e4fr