Hi Andy, Thanks for the comments. This is indeed a cut-out section of what I wanted to submit next. On Mon, 3 Aug 2020 at 18:35, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > > On Mon, Aug 3, 2020 at 6:17 PM Crt Mori <cmo@xxxxxxxxxxx> wrote: > > > > TAdut4 was calculated each iteration although it did not change. In light > > of near future additions of the Extended range DSP calculations, this > > function refactoring will help reduce unrelated changes in that series as > > well as reduce the number of new functions needed. > > Okay! > > > Also converted shifts in this function of signed integers to divisions as > > that is less implementation-defined behavior. > > This is what I'm wondering about. Why? > > ... The reason for this is that whenever something is wrong with the calculation I am looking into the shifts which are implementation-defined and might not keep the signed bit. Division however would. > > > - Ha_customer = ((s64)Ha * 1000000LL) >> 14ULL; > > - Hb_customer = ((s64)Hb * 100) >> 10ULL; > > + Ha_customer = div64_s64((s64)Ha * 1000000LL, 16384); > > + Hb_customer = div64_s64((s64)Hb * 100, 1024); > > Have you checked the code on 32-bit machines? > As far as I can see the div64_*64() do not have power of two divisor > optimizations. I bet it will generate a bulk of unneeded code. > > ... > > > - calcedKsTO = ((s64)((s64)Ga * (prev_object_temp - 25 * 1000LL) > > - * 1000LL)) >> 36LL; > > - calcedKsTA = ((s64)(Fb * (TAdut - 25 * 1000000LL))) >> 36LL; > > - Alpha_corr = div64_s64((((s64)(Fa * 10000000000LL) >> 46LL) > > - * Ha_customer), 1000LL); > > > + calcedKsTO = div64_s64((s64)((s64)Ga * (prev_object_temp - 25 * 1000LL) > > + * 1000LL), 68719476736); > > + calcedKsTA = div64_s64((s64)(Fb * (TAdut - 25 * 1000000LL)), 68719476736); > > + Alpha_corr = div64_s64(div64_s64((s64)(Fa * 10000000000LL), 70368744177664) > > + * Ha_customer, 1000LL); > > This is less readable and full of magic numbers in comparison to the > above (however, also full of magics, but at least gives better hint). > > ... These are coefficients so there is not much to unmagic. I can keep the shifts, if you think that is more readable or add comments after lines with 2^46 or something? > > > + TAdut4 = (div64_s64(TAdut, 10000LL) + 27315) * > > + (div64_s64(TAdut, 10000LL) + 27315) * > > + (div64_s64(TAdut, 10000LL) + 27315) * > > + (div64_s64(TAdut, 10000LL) + 27315); > > Shouldn't you switch to definitions from units.h? (perhaps as a separate change) > > -- > With Best Regards, > Andy Shevchenko
