So, again I sent the next version hoping that it would be merged this time xP
But I was wrong and again received few more comments especially from Andy Shevchenko (Senior Linux Kernel Developer @ Intel) about the 64 bit heavy multiplications in the bme680_compensate_gas().
What I really love about kernel community is that the reviewers/hobbyist can come from any subsystem and review your code irrespective of their primary work. AFAIK Andy’s primary job at Intel is not involved with IIO and I have often seen him reviewing code ranging to several subsystems.
Anyways, let’s first take a look at code snippet of discussion:
static u32 bme680_compensate_gas(struct bme680_data *data, u16 gas_res_adc,
u8 gas_range)
{
struct bme680_calib *calib = &data->bme680;
s64 var1;
u64 var2;
s64 var3;
u32 calc_gas_res;
/* Look up table 1 for the possible gas range values */
u32 lookupTable1[16] = {2147483647u, 2147483647u, 2147483647u,
2147483647u, 2147483647u, 2126008810u,
2147483647u, 2130303777u, 2147483647u,
2147483647u, 2143188679u, 2136746228u,
2147483647u, 2126008810u, 2147483647u,
2147483647u};
/* Look up table 2 for the possible gas range values */
u32 lookupTable2[16] = {4096000000u, 2048000000u, 1024000000u,
512000000u, 255744255u, 127110228u, 64000000u,
32258064u, 16016016u, 8000000u, 4000000u,
2000000u, 1000000u, 500000u, 250000u, 125000u};
var1 = ((1340 + (5 * (s64) calib->range_sw_err)) *
((s64) lookupTable1[gas_range])) >> 16;
var2 = (((s64) ((s64) gas_res_adc < 15) - 16777216) + var1);
var3 = (((s64) lookupTable2[gas_range] * (s64) var1) >> 9);
calc_gas_res = (u32) ((var3 + ((s64) var2 >> 1)) / (s64) var2);
return calc_gas_res;
}
So, Andy writes:
>> > + /* Look up table 2 for the possible gas range values */
>> > + u32 lookupTable2[16] = {4096000000u, 2048000000u, 1024000000u,
>> > + 512000000u, 255744255u, 127110228u, 64000000u,
>> > + 32258064u, 16016016u, 8000000u, 4000000u,
>> > + 2000000u, 1000000u, 500000u, 250000u, 125000u};
…this one obviously just a not needed one. You may replace it with a one constant and simple calculation to get either value (index from value, or value from index).
The solution was to use near approximation of array members as clearly we can figure out that the members are indeed power of 2 * 125000. Conclusion was to use bitshifting technique and avoid 64 bit expensive multiplications as evident:
var3 = (((s64) lookupTable2[gas_range] * (s64) var1) >> 9);
This was replaced by:
var3 = ((125000 << (15 – gas_range)) * var1) >> 9;
Amazing! Isn’t it ?
Saves a lot CPU cycles and speeds up calculations 🙂
I tested this new shifting technique and there was no such odd behavior that was visible. But had to confirm since, it is a near approximation, and might be possible that we could get disasterous results! For instance,
125000 << 8 = 32000000
and not 32258064 which is near enough.
But in my opinion it doesn’t matter for gas sensor because the gas sensor simply outputs in Ωs, which is often used in calculating IAQ index. So, there will be an adjustment in scale only. For eg. if you previously got 90kΩ maybe now we get 100kΩ or maybe 80kΩ but doesn’t hurt anything other than adjusting IAQ reference scale.
What I observed while testing was that bad air quality is inversely proportional to the gas readings. Infact, when I sprayed my deodorant right above the sensor, the reading showed a major downfall instantaneously from 88kΩ to 1300Ω to further 530Ω for few seconds. Then again it slowly began to rise further while calibrating with the environment.
If it were the case for temperaure/pressure/humidity channel then certainly it was BIG NO! for approximating the members.
Anothering review comment was what kbuild test robot (Intel Open Source Technology Center) hit while building through the numerous configurations! This bot tests in about 300 different kernel configuration and very likely catches huge number of bugs. I have this testing service since November’17 and I test all patches through it before sending to the maintainers.
Special thanks to Fengguang Wu 🙂
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/iio/chemical/bme680_core.o: In function `bme680_compensate_gas’:
>> drivers/iio/chemical/bme680_core.c:450: undefined reference to `__divdi3′
vim +450 drivers/iio/chemical/bme680_core.c
> 450 calc_gas_res = (u32) ((var3 + ((s64) var2 >> 1)) / (s64) var2);
So, the build broke due 64 bit division on 32 bit arch(i386) if you want to exlore more, then read here. The solution was simply to use div64_s64() and there would be no build errors even on i386.
Last review comment from Jonathan since v3 was due to redundant casting in the compensate functions. Therefore, I removed the all those unnecessary casts carefully as the implicit conversion rules of C language are one of the scariest mindfuck if you don’t handle them with care!
linux_kernel_hacking 