On Wed, 31 Jul 2024 08:08:37 +0200, Страхиња Радић <sr_AT_strahinja.org> wrote:
> Дана 24/07/30 10:15PM, Joakim Sindholt написа:
> > -#define RSSI_TO_PERC(rssi) \
> > - rssi >= -50 ? 100 : \
> > - (rssi <= -100 ? 0 : \
> > - (2 * (rssi + 100)))
> > +static inline int
> > +RSSI_TO_PERC(int rssi)
> > +{
> > + static const int best = -20, worst = -85, delta = best-worst, square = delta*delta;
> > + int q = (100*square-(best-rssi)*(15*delta+62*(best-rssi)))/square;
> > + return q > 100 ? 100 : (q < 0 ? 0 : q);
> > +}
>
> 1. Why replace a macro with a function? The "inline" keyword is just a
> suggestion to the compiler. In my tests, even with `-O3 -std=c99`,
> GCC 11.2.0 still emits CALL instructions for inline functions, while
> Clang/LLVM 16.0.6 doesn't. Macros are unambiguous.
Because it's complicated enough to read as it is without turning it into
a single expression.
> 2. When replacing a macro with a function, its name should be lowercased, eg.
>
> rssi_to_perc(int rssi)
To avoid touching the rest of the file and potentially breaking other
people's patches/local changes.
> 3. While being "compressed" into three lines, the code is hard to read.
> Better:
>
> static inline int
> rssi_to_perc(const int rssi)
> {
> const int best = -20; /* enum { BEST, WORST }? */
> const int worst = -85;
> const int delta = best - worst;
> const int square = delta * delta;
> int q = (100 * square
> - (best - rssi) * (15 * delta + 62 * (best - rssi)))
> / square;
> return q > 100 ? 100 : (q < 0 ? 0 : q);
> }
If you prefer. I personally think it's easier to read the way I wrote it
but not my project, not my style guide.
> 4. While the formula in the proposed patch seems to be a quadratic
> approximation[1], it is worth checking out [2] and [3].
>
>
> * * *
>
> My opinion: Given the extra computing cost of non-constant
> multiplication, as well as the introduced code complexity, I think it
> would still be best to leave the linear approximation as default in the
> mainline slstatus, and add the quadratic approximation from this
> proposal as an (optional) patch.
>
> What might be worth exploring is the linear approximation with a
> steeper slope, represented in the last illustration at [2].
The linear conversion currently in use is taken from Microsoft. On this
one topic I think it's perfectly reasonable to leave it as-is since
there's (massive) precedent for it:
https://learn.microsoft.com/en-us/windows/win32/api/wlanapi/ns-wlanapi-wlan_association_attributes
With all that being said, I'm not going to get into a protracted
argument over what on my machine compiles to 3 imul instructions with
gcc -O2. It's neither costly nor slow, especially considering that the
rest of the function this is called from performs 6+ syscalls to obtain
one of the operands. If you don't want it by default then so be it.
https://uops.info/html-instr/IMUL_M32.html
Received on Wed Jul 31 2024 - 08:49:29 CEST