Re: [tlug] measureTemp_hot.c: release early, release often

[Jim <jep200404@example.com>]

On Thu, 27 Jul 2006 12:55:30 -0400 Jim <jep200404@example.com> wrote:

> measureTemp_hot.c

>    th=0.0;
>    product=1.0;
>    for (i=0;i<COEFFICIENT_TABLE_LEN;i++) {
>       th+=coefficient_table[i]*product;
>       product*=e;
>    }

We have the same kind of pattern repeated, so it begs for: 

/* ^^^ maybe polynomial belongs in a separate file to be shared */
double polynomial(double x,const double *coefficient_table[],n)
{
   double y=0.0;
   double product=1.0;
   int i;

   for (i=0;i<n;i++) {
      y+=coefficient_table[i]*product;
      product*=x;
   }

   return y;
}

which make the code easier to understand. (That's a good thing.)

> #define A0 (-0.176004136860E-01)
> #define A1 ( 0.389212049750E-01)
> #define A2 ( 0.185587700320E-04)
> #define A3 (-0.994575928740E-07)
> #define A4 ( 0.318409457190E-09)
> #define A5 (-0.560728448890E-12)
> #define A6 ( 0.560750590590E-15)
> #define A7 (-0.320207200030E-18)
> #define A8 ( 0.971511471520E-22)
> #define A9 (-0.121047212750E-25)

const double a_coefficient_table[]={
   -0.176004136860E-01,
    0.389212049750E-01,
    0.185587700320E-04,
   -0.994575928740E-07,
    0.318409457190E-09,
   -0.560728448890E-12,
    0.560750590590E-15,
   -0.320207200030E-18,
    0.971511471520E-22,
   -0.121047212750E-25
};

> /* ^^^ I'm uncomfortable with the following constant names being
> *  lower-case. (Making them variables would be a feeble workaround)
> *  Having such similar names with upper-case names is also uncomfortable.
> *  Good luck choosing better names */
> 
> #define a0 ( 0.118597600000E+00)
> #define a1 (-0.118343200000E-03)
> #define a2 ( 0.126968600000E+03)

/* ^^^ I'm uncomfortable with the following constant names being
*  lower-case. (Making them variables would be a feeble workaround)
*  Having such similar names with upper-case names is also uncomfortable.
*  Good luck choosing better names */
/* ^^^ Now that A0 through A9 have been eliminated by replacing with a table,
*  ^^^ I capitalize a0 through A2: */

#define A0 ( 0.118597600000E+00)
#define A1 (-0.118343200000E-03)
#define A2 ( 0.126968600000E+03)

> #define B0 (  0.000000E+00)
> #define B1 (  2.508355E+01)
> #define B2 (  7.860106E-02)
> #define B3 ( -2.503131E-01)
> #define B4 (  8.315270E-02)
> #define B5 ( -1.228034E-02)
> #define B6 (  9.804036E-04)
> #define B7 ( -4.413030E-05)
> #define B8 (  1.057734E-06)
> #define B9 ( -1.052755E-08)

const double b_coefficient_table[]={
    0.000000E+00,
    2.508355E+01,
    7.860106E-02,
   -2.503131E-01,
    8.315270E-02,
   -1.228034E-02,
    9.804036E-04,
   -4.413030E-05,
    1.057734E-06,
   -1.052755E-08
};

> #define C0 (-1.318058E+02)
> #define C1 ( 4.830222E+01)
> #define C2 (-1.646031E+00)
> #define C3 ( 5.464731E-02)
> #define C4 (-9.650715E-04)
> #define C5 ( 8.802193E-06)
> #define C6 (-3.110810E-08)
> #define C7 ( 0.000000E+00)
> #define C8 ( 0.000000E+00)
> #define C9 ( 0.000000E+00)

const double c_coefficient_table[]={
   -1.318058E+02,
    4.830222E+01,
   -1.646031E+00,
    5.464731E-02,
   -9.650715E-04,
    8.802193E-06,
   -3.110810E-08,
    0.000000E+00,
    0.000000E+00,
    0.000000E+00
};

>    struct timeval time_now;

   struct timeval now; /* ^^^ note simpler now name instead of time_now */

>    /* MERENI VZORKU */
> 
>    /* configure K706 */

comment is in wrong place, since it does not describe e2 calculation. */

>    e2 = A0
>    + A1*powf(Tc,1)
>    + A2*powf(Tc,2)
>    + A3*powf(Tc,3)
>    + A4*powf(Tc,4)
>    + A5*powf(Tc,5)
>    + A6*powf(Tc,6)
>    + A7*powf(Tc,7)
>    + A8*powf(Tc,8)
>    + A9*powf(Tc,9)
>    + a0*expf(a1 * powf(Tc-a2,2));

   e2=polynomial(tc,a_coefficient_table,ArrayLength(a_coefficient_table)
   + A0*exp(A1 * pow(tc-A2,2));

   /* ^^^ by expf, do you mean float version of exp()?
   * ^^^ if so, just use normal exp() that uses doubles.
   * ^^^ don't be too cheap by using floats.
   * ^^^ Doubles are NOT your time problem.  */

put the configure K706 comment _here_ 

>    ibwrtstr(K706Ud,"C10C13X");

>    if (e<=20.644)

#define MAGICAL_THRESHOLD (20.644)
   if (e<=MAGICAL_THRESHOLD)

>       th = B0
>       + B1*powf(e,1)
>       + B2*powf(e,2)
>       + B3*powf(e,3)
>       + B4*powf(e,4)
>       + B5*powf(e,5)
>       + B6*powf(e,6)
>       + B7*powf(e,7)
>       + B8*powf(e,8)
>       + B9*powf(e,9);

      th=polynomial(tc,b_coefficient_table,ArrayLength(b_coefficient_table)

>       th = C0
>       + C1*powf(e,1)
>       + C2*powf(e,2)
>       + C3*powf(e,3)
>       + C4*powf(e,4)
>       + C5*powf(e,5)
>       + C6*powf(e,6)
>       + C7*powf(e,7)
>       + C8*powf(e,8)
>       + C9*powf(e,9);

      th=polynomial(tc,c_coefficient_table,ArrayLength(c_coefficient_table)



> 
>    printf("t= %d.%06ld s \t T= %.2f C \n"
>    , (int)time_now.tv_sec,time_now.tv_usec,th);
>    /* ^^^ why case to an int? why not allow big values: */
>    printf("t= %ld.%06ld s \t T= %.2f C \n"
>    , time_now.tv_sec,time_now.tv_usec,th);

   printf("t= %ld.%06ld s \t T= %.2f C \n",now.tv_sec,now.tv_usec,th);

>    /* ^^^ also, beware of hard tabs. They are not portable. 
>    *  different programs and printers do different things with them */

Amen. 

The revised code in one shot is: 

************************************************************************************

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <stdint.h>
#include <string.h>
#include <getopt.h>
#include <gpib/ib.h>
#include <sys/timeb.h>
#include <time.h>
#include <math.h>
#include "lojza.h"

#define ArrayLength(x) (sizeof(x)/sizeof(*(x))) /* ^^^ put in header file */
/* ^^^ granted: my ArrayLength macro is dangerous because sizeof() reports
* ^^^ padded stored ^^^ I've become too comfortable with it,
* ^^^ because I've become to successful with it. */

/*^^^ maybe constants should go in a header file, perhaps kcoefficients_inverse.h */

/* data from http://srdata.nist.gov/its90/type_k/kcoefficients_inverse.html */
/* ^^^ nice reference! */

const double a_coefficient_table[]={
   -0.176004136860E-01,
    0.389212049750E-01,
    0.185587700320E-04,
   -0.994575928740E-07,
    0.318409457190E-09,
   -0.560728448890E-12,
    0.560750590590E-15,
   -0.320207200030E-18,
    0.971511471520E-22,
   -0.121047212750E-25
};

/* ^^^ Now that A0 through A9 have been eliminated by replacing with a table,
*  ^^^ I capitalize a0 through A2: */

#define A0 ( 0.118597600000E+00)
#define A1 (-0.118343200000E-03)
#define A2 ( 0.126968600000E+03)

#define MAGICAL_THRESHOLD (20.644) /*^^^ document the hell out of this */

const double b_coefficient_table[]={
    0.000000E+00,
    2.508355E+01,
    7.860106E-02,
   -2.503131E-01,
    8.315270E-02,
   -1.228034E-02,
    9.804036E-04,
   -4.413030E-05,
    1.057734E-06,
   -1.052755E-08
};

const double c_coefficient_table[]={
   -1.318058E+02,
    4.830222E+01,
   -1.646031E+00,
    5.464731E-02,
   -9.650715E-04,
    8.802193E-06,
   -3.110810E-08,
    0.000000E+00,
    0.000000E+00,
    0.000000E+00
};

/* ^^^ maybe polynomial belongs in a separate file to be shared */
double polynomial(double x,const double *coefficient_table[],n)
{
   double y=0.0;
   double product=1.0;
   int i;

   for (i=0;i<n;i++) {
      y+=coefficient_table[i]*product;
      product*=x;
   }

   return y;
}

double measureTemp_hot(double tc)
{
   char buffer[MAX_KEITHLEY_REPLY_LEN+1];

   /* ^^^ document these puppies */
   float e2;
   float e1; /* thermocouple voltage */
   float e;
   float th;

   struct timeval now; /* ^^^ note simpler now name instead of time_now */

   /* MERENI VZORKU */

   e2=polynomial(tc,a_coefficient_table,ArrayLength(a_coefficient_table)
   + A0*exp(A1 * pow(tc-A2,2));

   /* ^^^ by expf, do you mean float version of exp()?
   * ^^^ if so, just use normal exp() that uses doubles.
   * ^^^ don't be too cheap by using floats.
   * ^^^ Doubles are NOT your time problem.  */

   /* configure K706 */ /* ^^^ e2 calc was NOT part of config K706, so this comm
ent was moved */

   ibwrtstr(K706Ud,"C10C13X");
   ibwrtstr(K2182Ud,"*trg");

   ibwrtstr(K2182Ud,":sense:data:fresh?");
   ibrd(K2182Ud,buffer,MAX_KEITHLEY_REPLY_LEN);
   gettimeofday(&now,NULL);
   buffer[ibcnt]='\0';
   ibwrtstr(K706Ud,"N10N13X");
   e1 = 1000*atof(buffer);
   e = e1 + e2;

   if (e<=MAGICAL_THRESHOLD)
      th=polynomial(tc,b_coefficient_table,ArrayLength(b_coefficient_table)
   else
      th=polynomial(tc,c_coefficient_table,ArrayLength(c_coefficient_table)

   printf("t= %ld.%06ld s \t T= %.2f C \n",now.tv_sec,now.tv_usec,th);

   return th;
}

#if 0
don't arbitrarily restrict your constants to float.
You loose accuracy and speed, since all floats are converted to doubles
before expressions are evaluated.
avoid super long lines.
Keep in mind that the = part of <= in E<=20.644,
has little practical meaning because of the nature of
exact comparisons in floating point math.

If later on you are really concerned about execution speed,
consider eliminating the use of powf() function.
If it uses logarithms, it might be slow.
consider replacing the formulas that use powf() with a loop
that does simple (albeit repeated) multiplication.
Here's the incomplete ideas:

   const double c_coefficient_table[COEFFICIENT_TABLE_LEN]={
      -1.318058E+02,
       4.830222E+01,
      -1.646031E+00,
       5.464731E-02,
      -9.650715E-04,
       8.802193E-06,
      -3.110810E-08,
       0.000000E+00,
       0.000000E+00,
       0.000000E+00
   };

   if (e<=20.644)
      coefficient_table=b_coefficient_table;
   else
      coefficient_table=c_coefficient_table;

   or

   coefficient_table = (e<=20.644) ? b_coefficient_table : c_coefficient_table;

   th=0.0;
   product=1.0;
   for (i=0;i<COEFFICIENT_TABLE_LEN;i++) {
      th+=coefficient_table[i]*product;
      product*=e;
   }

   or

   double *p;

   p = (e<=20.644) ? b_coefficient_table : c_coefficient_table;

   th=0.0;
   product=1.0;
   for (i=0;i<COEFFICIENT_TABLE_LEN;i++) {
      th+=*p++*product;
      product*=e;
   }

In this case, the constants would be stored in variables,
so the names would be lower case.

Whatever you do, don't worry about speed too soon.
Make your code clear first.
Maximize clarity.

Notice that the polynomial function has clarified the code.
It is much much easier to understand now.
Don't be surprised if it also runs faster than your powf() fiasco,
but you should now worry about speed (yet).

#endif