[Sound-open-firmware] [PATCH 2/4] Tone: Add channel specific volume plus other fixes

[Seppo Ingalsuo]

On 22.06.2017 12:02, Takashi Iwai wrote:

> On Thu, 22 Jun 2017 10:47:54 +0200,
> Seppo Ingalsuo wrote:
>>
>>
>> On 22.06.2017 11:28, Takashi Iwai wrote:
>>> On Thu, 22 Jun 2017 10:04:27 +0200,
>>> Seppo Ingalsuo wrote:
>>>> On 22.06.2017 04:31, Keyon Jie wrote:
>>>>> On 2017年06月22日 01:12, Seppo Ingalsuo wrote:
>>>>>> The mono tone level can be controlled per channel. A similar channel map
>>>>>> feature as in the volume component was added into volume command of
>>>>>> tone.
>>>>>>
>>>>>> The default tone frequency of 997 Hz was updated to a proper Q16.16
>>>>>> value.
>>>>>>
>>>>>> Moved tone function pointer set to tone_new() since it is not modified
>>>>>> later.
>>>>>>
>>>>>> Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo at linux.intel.com>
>>>>>> ---
>>>>>>     src/audio/tone.c | 56
>>>>>> ++++++++++++++++++++++++++++++++++++++++++--------------
>>>>>>     1 file changed, 42 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/src/audio/tone.c b/src/audio/tone.c
>>>>>> index f45d8be..d1c7468 100644
>>>>>> --- a/src/audio/tone.c
>>>>>> +++ b/src/audio/tone.c
>>>>>> @@ -56,7 +56,8 @@
>>>>>>       #define TONE_NUM_FS            13       /* Table size for 8-192
>>>>>> kHz range */
>>>>>>     #define TONE_AMPLITUDE_DEFAULT 2147484  /* -60 dB from full scale */
>>>>>> -#define TONE_FREQUENCY_DEFAULT 16334848 /* 997 in Q18.14 */
>>>>>> +#define TONE_FREQUENCY_DEFAULT 65339392 /* 997 Hz in Q16.16 */
>>>>>> +#define TONE_VOLUME_DEFAULT 65536       /* 1.0 in Q16.16 */
>>>>> will they be more readable in hexadecimal?
>>>> Unfortunately the C preprocessor doesn't process float to fixed math
>>>> so I can't use any nice macro to create these. In hex it would look
>>>> like this and in my opinion it's not any better. The Q notation is
>>>> only sometimes by chance such that integer and decimal bits are not
>>>> mixed into same hex digit. For frequency Qx.y notation the used x is
>>>> sufficient to get to ultrasound frequencies and y to make precise
>>>> enough sweeps.
>>>>
>>>> #define TONE_AMPLITUDE_DEFAULT 0x20C49C  /* -60 dB from full scale */
>>>> #define TONE_FREQUENCY_DEFAULT 0xF940000 /* 997 Hz in Q18.14 */
>>>> #define TONE_FREQUENCY_DEFAULT 0x3E50000 /* 997 Hz in Q16.16 */
>>>> #define TONE_VOLUME_DEFAULT 0x10000       /* 1.0 in Q16.16 */
>>>>
>>>> To convert these with any calculator to floats when debugging code
>>>> (these are fractional numbers) there's extra effort needed with hex so
>>>> I definitely prefer decimal. Hex is good for bit masks but these are
>>>> not such.
>>> It's a fixed-point, so you can introduce a simple macro like
>>>
>>> #define TONE_FREQ_SHIFT		16
>>> #define TONE_FREQ(x, y)		((x) << TONE_FREQ_SHIFT | (y))
>>>
>>> then TONE_FREQUENCY_DEFAULT will be TONE_FREQ(997, 0), and so on.
>>> If you need to change the base shift, just change the TONE_FREQ_SHIFT
>>> and the rest remains as is.
>> Yes, that works for integers and it is readable but the needed
>> frequencies are often decimal numbers in Hz. E.g. C5 note is 523.251
>> Hz or if a sweep is started from 4th third octave band the frequency
>> would be 31.5 Hz.
> Yeah, the decimal point is tricky.
>
>> So I myself do with Octave that I use as pocket calculator
>>>> printf('%d\n',round(523.251*2^16))
>> 34291778
>>>> printf('%d\n',round(31.5*2^16))
>> 2064384
>>>> printf('%d\n',round(10^(-60/20)*2^31))
>> 2147484
> I thought C compiler (not preprocessor) should optimize the conversion
> and handle as an integer constant.
>
> #define TONE_FREQ(f)	(int)((f) * (2 << 16))	/* f = float */
>
> and
> 	int x = TONE_FREQ(523.251);
>
> should be converted to
>
> 	int x = 34291778;

Thanks, that works. At least with the compiler that I use 
(http://alsa-project.org/main/index.php/Firmware) the optimizing does 
the conversion to fixed without increase of the firmware image size. I 
also checked from disassembly that this part of the code remains 
identical. I did a minor fix and added rounding into the macro since 
compiler doesn't round the cast to int:

#define TONE_FREQ(f)    (int)((f) * (1 << 16))    /* f = float */

#define TONE_FREQ(f)    (((int)((f) * (1 << 17)) + 1) >> 1)  /* f = float */

If there's no concern from other people who know better the compilers 
I'm OK to make this change to increase readability and help avoiding 
mistakes with frequencies.

Seppo


>
>
> Takashi
>