Skip to content

detect_test: refine the activation threshold to fix trigger issue - #3116

Merged
lgirdwood merged 1 commit into
thesofproject:masterfrom
keyonjie:kwd_fix
Jul 6, 2020
Merged

lgirdwood merged 1 commit into
thesofproject:masterfrom
keyonjie:kwd_fix

Conversation

@keyonjie

Copy link
Copy Markdown
Contributor

This is submitted to fix the issue #3112

Refine to use the consistent activation threshold for different formats,
use a unified divider 0.05, and update the threshold at .params() stage.

These refinement will fix the KWD trigger volume inconsistent for
different sample format issue.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

@ClarexZhou ClarexZhou left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

verified, issue #3112 can be fixed by this PR.

@lgirdwood lgirdwood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@singalsu do these value make sense for given types and gain ?

@singalsu

Copy link
Copy Markdown
Collaborator

@keyonjie @lgirdwood Yes, the thresholds need to be scaled for their PCM format. But those multiplier values are not dividers, I'd call 0.05 as ACTIVATION_DEFAULT_THRESHOLD instead. Also since it's a float it could be converted this way. But it's a matter of taste what you feel is the best.

#define ACTIVATION_DEFAULT_THRESHOLD_S32 Q_CONVERT_FLOAT(ACTIVATION_DEFAULT_THRESHOLD, 31) /* Q1.31 */
#define ACTIVATION_DEFAULT_THRESHOLD_S24 Q_CONVERT_FLOAT(ACTIVATION_DEFAULT_THRESHOLD, 23) /* Q1.23 */
#define ACTIVATION_DEFAULT_THRESHOLD_S16 Q_CONVERT_FLOAT(ACTIVATION_DEFAULT_THRESHOLD, 15) /* Q1.15 */

@keyonjie

keyonjie commented Jul 1, 2020

Copy link
Copy Markdown
Contributor Author

Yes @singalsu the naming 'divider' here is somewhat, it should be something like coefficient or multiplier for calculating the threshold. But since it is fixing issue here I would prefer to keep the historic naming and leave the float converting to future refinement, for better bisect also.

@lgirdwood

Copy link
Copy Markdown
Member

@keyonjie I'd prefer we only change this once as threshold will change slightly everytime we make this change so lets get it right fist time. Please align with @singalsu method (which is using ints and not floats in the runtime code - i.e. float is converted to int by the macros above).

@keyonjie

keyonjie commented Jul 1, 2020

Copy link
Copy Markdown
Contributor Author

@keyonjie I'd prefer we only change this once as threshold will change slightly everytime we make this change so lets get it right fist time. Please align with @singalsu method (which is using ints and not floats in the runtime code - i.e. float is converted to int by the macros above).

yes I can convert to use those macros to make it unified, though actually both method are using ints and not floats in the runtime code, float is already converted to int with today's code.

Refine to use the consistent activation threshold for different formats,
use a unified coefficient 0.05, and update the threshold at .params().

These refinement will fix the KWD trigger volume inconsistent for
different sample format issue.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
@keyonjie
keyonjie requested a review from lgirdwood July 1, 2020 12:58
@lgirdwood
lgirdwood merged commit 2bd3026 into thesofproject:master Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants