[PATCH 2/4] convert internal frequencies to KHz

[Thomas Pedersen]

Hi Jouni,

Thanks for your feedback.

On 2021-02-26 14:08, Jouni Malinen wrote:
> On Wed, Jun 17, 2020 at 11:15:32PM -0700, Thomas Pedersen wrote:
>> In preparation for supporting (S1G) channels centered on
>> non-integer MHz, such as 902.5Mhz, convert nearly all
>> internal frequency representations into units of KHz.
> 
> This is a huge patch and very likely to cause conflicts with any other
> pending work.. That's one of the reasons why I did not really want to
> even consider this while there was quite a few pending patches in the
> queue. That situation is now better, so I can at least consider. Still,
> it would be nice if this patch could be split into smaller pieces that
> would go through interfaces in steps rather than everything at once.

OK. At the time most of the code seemed deeply intertwined, but I'll try 
doing a better job of splitting out the changes in the next version.

>> A conversion must be done when:
>> 
>> - parsing existing API, ie. control interface, dbus, etc.
>> - parsing config files
>> - sending driver commands
>> - receiving driver events
> 
> That sounds reasonable.
> 
>> When the user input is a string, we parse the input as a
>> MHz float, then convert to KHz. This will existing
>> commands to work on S1G channels unchanged.
> 
> But I don't really want to see floating point types for frequencies.
> Integers in kHz with helper functions for parsing might be OK, but
> ideally, I would not see even a single float or atof() in this patch..

atof() is needed for accepting eg. "scan_freq=902.5" or "scan_freq=902" 
with the same code.

What is the issue using atof() in parsing config files?

>> When generating events and log messages, we exploit the
>> "%g" format specifier which will drop the "." and
>> trailing zeroes if the input does not have a fractional
>> component. Similar to parsing user input this lets us
>> expose fractional MHz frequencies without disturbing
>> existing users.
> 
> Needing floating point numbers for printing out integer values looks
> undesired.. I guess "freq=902.5" might be more convenient to see in 
> many
> places instead of something like freq_khz=902500, though.. Anyway,
> generating that with "%u.%u" or "%u.%03u" for 902.500 might be simpler.

One problem is the various events are a form of ABI, so something like 
`freq_khz=902.500` is not acceptable because the consumer probably 
expects `freq_khz=%u`.

There is probably some macro magic that can be done to work around this, 
but "%g" sure is convenient :)

I'm curious why you don't want float in the codebase. Is this due to 
additional libc requirements + potentially trapping into a software 
float handler?

>> While dbus, driver_bsd, driver_atheros, driver_hostap,
>> driver_privsep, driver_wext, etc. have been converted
>> based on 'git grep freq ...', they have received no
>> testing :(. Suggestions, extra review, or help testing
>> these are welcome.
> 
> This is one of the major issues here. Alternative approach would be to
> introduce new frequency-in-kHz variables for the places that really 
> need
> this while leaving the existing -in-MHz variants available to avoid
> regressions.

Yes, this was my initial approach, but the code is very tightly coupled 
in most places (expecting a single int for frequency).

What do you think about redefining channels / frequencies in general as 
a "struct wpa_channel_info". Then the frequency / channel representation 
can change (again) without modifying the surrounding interfaces.

Maybe a simple `int freq_khz` where needed can be done, I need to 
revisit this after so long...

> And on the editorial front, I'd prefer correct spelling of the SI
> unit/prefix (kHz) instead of that Kelvin Herz thingie used here in most
> places.. ;-)

Noted :)

>> This commit should introduce no functional change.
> 
> Maybe so, but this is going to hugely inconvenient to review and show 
> to
> be correct.

Agreed.

-- 
thomas