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
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
