Code optimisation, reading temp from DS1820 (picc)

t06afre

Joined May 11, 2009
5,934
If you still need help I have some ideas to make your code much more compact. The whole style the program is written in is quite awkward. But that does not matter much. As I guess you are quite new in your learning curve. If you need more help can post a schematic.
 

Thread Starter

coldpenguin

Joined Apr 18, 2010
165
Your code is messy. Not easy to see what is going on. Next time if you ask for help. At least take the time to make proper comments.
[/CODE]
Thanks for the suggestions.
I know the code is messy, but it is the way that I think,
this is the reason why I only posted the routine in question in the first place, and made a little attempt at neatening it up before posting.

For some reason, the hitec delays are not working correctly, they were way too long. I don't have an oscilloscope, but I do have a logic analyser, and no reason to disbelieve the timings that this is giving. (doing a porta.1 up, down, up down gave a frequency of 1MHz using a 4MHZ chip, doing a _delay_us(1) between them, made the peaks 28us long, which is rediculous)
I believe that the Hitec-delay routines are intended for use with the 'pro' version of the compiler, and with the lite compiler they are not optimising correctly.
 

Thread Starter

coldpenguin

Joined Apr 18, 2010
165
If you aren't pushed for speed, you could try something real basic, such as:

[/code]That should be pretty easy on code space.
Speed isn't exactly an issue, using a 4MHz crystal on a really cheap PIC rather than the 48MHz on the PIC18 which I was programming before.
My code is currently as follows:
Rich (BB code):
    tres |=(temp[1]&0b00000111)<<8;
    tres |=temp[0];
unsigned char kres=0;
digits[2]=0;
    if(0x02!=sensornum){
kres=tres>>4;
digits[2]=((10*(0b00001111&tres))>>4);
    }else{
        kres=tres>>=1;
        if(temp[6]>12) tres--;
        digits[2]=10+(10*((12-temp[6])))>>4;
        while(digits[2]>9) digits[2]-=10;
    }
digits[0]=0;
while(kres>9){
digits[0]++;
kres-=10;
}
digits[1]=kres;
which is reasonably similar. Unfortunately I could not use digits as a temporary value, as when I tested, I believe I was over-ritting another value with the high-byte.
That calculates two normal and one decimal digit for every sensor now (as I have mixed s20 b20)
 

Thread Starter

coldpenguin

Joined Apr 18, 2010
165
If you still need help I have some ideas to make your code much more compact. The whole style the program is written in is quite awkward. But that does not matter much. As I guess you are quite new in your learning curve. If you need more help can post a schematic.
I am more used to programming computers than the PICs, these are a hobby that I have little time to do, as in my last project was a PIC18, over 2 years ago (I hadn't realised it had been so long, but it was on my old computer, so must have been).
On the computers that I program, it is often the case that either there is no such thing as multidimensional arrays, references or pointers (or good naming convention), or on others that using memory is more efficient than trying to keep code small.
Unfortunatley these two concepts make C on the PIC difficult.
I am not sure that a schematic has that much relevance to the issue that I was running out of space.
Hopefully the image should display

http://forum.allaboutcircuits.com/attachment.php?attachmentid=42507&d=1335961109
 

t06afre

Joined May 11, 2009
5,934
With a 4 MHz crystal each instruction will take 1 usec. The delay_functions are made in software so it is a lower limit for sure. Like setting up a function call and returning from the function. All this "steal" cpu cycles.
 

t06afre

Joined May 11, 2009
5,934
I made this yesterday before I found out you are using two types of the termprature sensors. I have tested it and it works in the simulator. It will format the the temprature reading from the DS18S20 sensor. At least it demonstrate the correct use of lookup tables stored in program memory. The code is less pompous than your current code
Rich (BB code):
#define _XTAL_FREQ 4000000 //12000000
#include <htc.h>
//#include "../delay.h"
//#include <stdlib.h>
unsigned char gete(unsigned char address);
#define sleep()        asm("sleep")
#define _D_MATCHROM_
#define _D_READROM_
#define D_PIN RA0
#define D_TRIS TRISA0
//#include "1wire.c"
//#include <stdlib.h>
__CONFIG( FOSC_HS & WDTE_OFF);
typedef union 
   { 
   signed short i16; 
   unsigned char u8[2]; 
   } DS1820_temp;
DS1820_temp temprature;
/* Example usage of temprature
 temprature.i16 = ?;
temprature.ru8[0]=ADRESL; 
temprature.u8[1]=ADRESH;*/
//The four LSB giving decimal temprature may take these values
//0.0000,0.0625,0.1250,0.1875,0.2500,0.3125,0.3750,0.4375,0.5000,0.5625,0.6250,0.6875,0.7500,0.8125,0.8750,0.9375
const char point_deg[16] ={0,1,1,2,2,3,4,4,5,6,6,7,8,8,9,9};//the vales are here rounded to one decimal point
const char seven_seg[] ={0x7E,0x30,0x6D,0x79,0x33,0x5B,0x5F,0x70,0x7F,0x7B};
unsigned char LED_data[3];
void main()
{   signed short temp_integer;
      signed short temp;
 temprature.u8[0]=0x08;
   temprature.u8[1]=0x0;
       temp_integer=(temprature.i16>>4);
       if (temp_integer<10)
  {
              LED_data[2]=0;
                   LED_data[1]=seven_seg[temp_integer];
                   LED_data[0]=seven_seg[point_deg[temprature.u8[0]&0x0f]];
             }
              else
    {
                           temp=temp_integer/10;
                           LED_data[2]=seven_seg[temp];
                           LED_data[1]=seven_seg[temp_integer-10*temp];
                           LED_data[0]=seven_seg[point_deg[temprature.u8[0]&0x0f]];
                          } 
}
 

Thread Starter

coldpenguin

Joined Apr 18, 2010
165
Thanks t06afre, I will digest your code and consider whether there is anything I should use.

I guess that your main issue with my code being pompous is mainly my routine gete?

Your arrays are certainly a nicer way of implementing this. When I was looking for eeprom read/write information, I did see that the Hitec could do this, I think I discounted it as other compilers may not be able to (as I was wondering whether a re-write to sdcc was the way to go). What I find interesting, is that your code attempts to store 26 bytes to eeprom, but the compiler somehow has compressed this to 22, and I cannot see visibly the data in the eeprom. I will have to look into what the compiler is actually doing.
The typedef union gives me something to think about as well, it isn't something I usually have to deal with, but the concept of mapping one variable type over the other certainly gave you an advantage.

Your code is 273 bytes compiled for setting an array suitable for outputting the one temperature.
My code, for temperature conversion of the two types, but without the segment lookup (as that is in the outer loops) is 233 bytes.
 

t06afre

Joined May 11, 2009
5,934
I posted the example most since it was done. And it also showed you how to make lookup tables. The union function is also quite handy. Just pick up what you feel for. and dump the rest. I was also toying with the posebilety to convert the 18s20 type result into a 18b20 type result. That could save you a lot of space.
 

t06afre

Joined May 11, 2009
5,934
Here is my attempt to convert a 18s20 to 18b20 type reading. So you can use one function to convert all the temp readings to LED seven segment. This program take 481 words of the program memory in lite mode(free). In PRO mode the result are 328 words. Again use the bits you need. But this way you have some more space to work with
Rich (BB code):
define _XTAL_FREQ 4000000 //12000000
#include <htc.h>
//#include "../delay.h"
//#include <stdlib.h>
unsigned char gete(unsigned char address);
#define sleep()        asm("sleep")
#define _D_MATCHROM_
#define _D_READROM_
#define D_PIN RA0
#define D_TRIS TRISA0
//#include "1wire.c"
//#include <stdlib.h>
__CONFIG( FOSC_HS & WDTE_OFF);
typedef union 
   { 
   signed short i16; 
   unsigned char u8[2]; 
   } DS1820_temp, *PTRDS1820_temp;
DS1820_temp temprature_s1;
DS1820_temp temprature_s2;
DS1820_temp temprature_s3;

/* Example usage of temprature
 temprature.i16 = ?;
temprature.ru8[0]=ADRESL; 
temprature.u8[1]=ADRESH;*/
//The four LSB giving decimal temprature may take these values
//0.0000,0.0625,0.1250,0.1875,0.2500,0.3125,0.3750,0.4375,0.5000,0.5625,0.6250,0.6875,0.7500,0.8125,0.8750,0.9375
const char point_deg[16] ={0,1,1,2,2,3,4,4,5,6,6,7,8,8,9,9};//the vales are here rounded to one decimal point
const char seven_seg[] ={0x7E,0x30,0x6D,0x79,0x33,0x5B,0x5F,0x70,0x7F,0x7B};
unsigned char LED_data_s1[3];
unsigned char LED_data_s2[3];
unsigned char LED_data_s3[3];
void temp_to_LED(DS1820_temp *temprature, unsigned char *LED_data)
{   signed short temp_integer;
      signed short temp;
  temp_integer=((temprature->i16)>>4);
      //the use of -> operator is described at http://pw1.netcom.com/~tjensen/ptr/ch5x.htm
       if (temp_integer<10)
  {
              LED_data[2]=0;
                   LED_data[1]=seven_seg[temp_integer];
                   
             }
              else
    {
                            temp=temp_integer/10;
                            LED_data[2]=seven_seg[temp];
                            LED_data[1]=seven_seg[temp_integer-10*temp];
                           } 
LED_data[0]=seven_seg[point_deg[temprature->u8[0]&0x0f]];
}
void convert_s20_to_b20(DS1820_temp *temprature, unsigned char* buffer)
{
      //this are the values using the formula subtract 0.25, then add (clock-counts - cycles remaining)/(clock-counts). cycles remaining
 //going from 0x00 to 0x0f (4 bit)
 //0.7500,0.6875,0.6250,0.5625,0.5000,0.4375,0.3750,0.3125,0.2500,0.1875,0.1250,0.0625,0.0000,-0.0625,-0.1250,-0.1875
      //This are the values for the 4 LSB in the result from 18b20
 //0.0000,0.0625,0.1250,0.1875,0.2500,0.3125,0.3750,0.4375,0.5000,0.5625,0.6250,0.6875,0.7500,0.8125,0.8750,0.9375
 //we have two cases  where cycles remaining must be mapped. cycles remaining<=12. And cycles remaining > 12. 
 //in both cases the mapping are quite easy by looking at the two latter tables
 unsigned char temp_var;
   temprature->u8[0]=buffer[0];
   temprature->u8[1]=buffer[1];
   temprature->i16=(temprature->i16>>1); //truncate the reading
    //COUNT REMAIN=buffer[6]  
       buffer[6]=buffer[6]&0x0f;
  if (buffer[6]>12)
   {
                  temprature->i16--;
                  temp_var=28-buffer[6];
              }
        else temp_var=12-buffer[6];
      temprature->i16=(temprature->i16<<4);//make room for four bytes with decimal point
 temprature->u8[0]=temprature->u8[0]+temp_var;
}

void main()
{     unsigned char  buffer[8];
      temprature_s1.u8[0]=0x08;
   temprature_s1.u8[1]=0x0;
      temp_to_LED(&temprature_s1,LED_data_s1);
     unsigned char  buffer[8];//={32,0x00,0,0,0,0x0,0,8};
     while(1)
    {
     convert_s20_to_b20(&temprature_s3,buffer);
    } 
}
 

Thread Starter

coldpenguin

Joined Apr 18, 2010
165
Thanks again t06afre,
From a readability point of view it is certainly better to convert one to the other and then deal with a consistent data item.
I have however hit an alternative snag in the form of the hall sensor that I obtained, assuming that they would all be similar in operation.
This at least will give me some time to look through the code and to watch the digits move.
 
Top