Low on program space with pic microcontroller

[mikeselectricstuff]

float voltage0;

there's your problem.

Embedded applications very rarely need floating point.
FP gives you range, not precision.
Real-world variables rarely need enough range to justify the overhead of floating point.
Fixed-point maths will usually be far more efficient as all the calculations are the same as for integers.

[T3sl4co1l]

Indeed, it's important to understand when you actually need a division.

For just scaling, it's never needed. You can multiply by any arbitrary constant, then bitshift. Note on bitshifting, simple architectures rarely have an instruction to shift by any arbitrary number of bits; for example, ATTiny CPUs can only shift by 1, so shift-by-3 becomes three instructions. But, shifting by multiples of 8 is always very efficient as that only means shuffling the whole bytes around, or just "ignoring" parts of the variables/registers.

In practice, this would mean something like reading a 12-bit voltage measurement into uint32, multiplying it with a constant of 65536 equaling "1.0", then shift by 16 bits. Now you can adjust this large constant to provide finely stepped gain calibration. And if the each ADC step happens to be 1.2345mV, then no problem, just instead of 65536, multiply by 53087, and you get millivolts out directly! No division anywhere.

Note that C does arithmetic in int by default, so you often need to apply typecasts carefully, within expressions, to make sure the compiler doesn't discard part of your expression (e.g., the 53087 constant above, might be written as long (53087L or (uint32_t)53087) to force this.

Arithmetically speaking, addition adds one bit (the carry), and multiplication sums the number of bits (so, two 16s give a 32-bit result), but in C, these data are discarded and the result is always in the same type as the operands, silently causing overflow (which may also be undefined behavior and platform dependent; mostly, it's modulo-2^N because almost all CPUs use 2s-complement arithmetic).  So you need to anticipate and provide extra space for the result, and also tell the compiler that you expect all the bits from that result (by casting one or both operands to the larger size; a 32x16 multiplication gives a 32 bit result in C).


Also, with calibration values, no hokey magic-number VREFs are needed -- use whatever is handy, use VCC even (if it's suitable, electrically speaking -- say everything is ratiometric, e.g. DACs, potentiometers, thermistors..).  A better way to view the ADC result is as a dimensionless ratio of full scale.  You typically need just a few extra bits (on top) to handle these values, to deal with carries on intermediate results.  (The reality is, you still tend to use 32-bit data sizes, because 24 is poorly supported if at all, and as mentioned above, byte alignments are easier.  And you can use the extra low bits to keep quantization (rounding) noise down.)  And you can apply calibration values anywhere between ADC acquisition, and how ever the values get presented to the user (whether digitally accessible registers, or written to a com port or display).  In particular, writing back out to a DAC of the same scale (bits and VREF), the calibration is trivial!

Tim

[Ian.M]

XC8 supports 24 bit short long integer types and as C99 fixed width types int24_t and uint24_t, *EXCEPT* if you have the CCI^* compiler option enabled.    Useful when 16 bits isn't enough, but you don't want the code and data space overheads of 32 bit math.

* CCI: Microchip Common C Interface, a fundamentally broken attempt to unify implementation defined extended syntax, features and behavior across the whole XC compiler range by banning HiTech PIC C derived stuff in XC8 that isn't supported in the GCC based XC16 and XC32 without adequately providing equivalent functionality when compiling for 8 bit PICs! 

[errorprone]

An easier way to check what is using program memory is to look at the .map file.  Not sure if it’s generated automatically but it is a text file that shows size and location of symbols.

[DavidAlfa]

Where's the problem for running multiple bitshifing instrutions?
Compare that to 100s when handling floats using software.
Fixed point is the way for simple values like these.

[brucehoult]

float voltage0;

there's your problem.

Embedded applications very rarely need floating point.
FP gives you range, not precision.
Real-world variables rarely need enough range to justify the overhead of floating point.
Fixed-point maths will usually be far more efficient as all the calculations are the same as for integers.

If you have something where you have to multiply two unknown (e.g. input) values then fixed-point is scarcely any faster than floating point, and you run a very real risk of underflow or overflow.

Sure, if you only ever add and subtract, or multiply by constants, then you can do the analysis and decide integer or fixed point will be enough.

But you have to actually do the analysis, and not everyone who is a programmer is competent to do that.

FP provides a safety net.

If I recall correctly, FP add/sub/mul on an ATMega at 16 MHz takes about 80 clock cycles i.e  5µs. That means you can do up to 200,000 of them a second. Even if you're doing closed-loop control at 100 Hz or even 1000 Hz that's quite a lot of FP calculation available in the time budget.

[westfw]

Hmm.  Are there any "modern" tools for analyzing binary size?
Linker maps are typically verbose and unfriendly.

A disassembly ... even moreso.

gcc's "nm -S " tool is very useful, but primitive (CLI input and output.)

I'm picturing something GUI with expanding trees of large contributors and the functions they alone require.
So if printf is the only function that uses a 32bit divide, the space used by the divide function would get blamed on printf(), but if there were other users, it wouldn't.  I *think* all the info needed is available and used for MAP/CREF file production, but not in a convenient fashion...

[DavidAlfa]

In STM32 IDE (Eclipse-based) you have a very nice build analyzer, showing the program/ram usage for every function and variable, also has a stack analyzer.
I've been searching for the same functionality on XC8 but found nothing.

[JPortici]

If I recall correctly, FP add/sub/mul on an ATMega at 16 MHz takes about 80 clock cycles i.e  5µs. That means you can do up to 200,000 of them a second. Even if you're doing closed-loop control at 100 Hz or even 1000 Hz that's quite a lot of FP calculation available in the time budget.

Precisely. If i have such slow control loops i don't even care anymore and just throw floats at the problem. Data for communication is sent in binary format (packed structure) so i let the PC deal with the conversion to human readable data

[jerryk]

I have modified the code attempting to follow some of the suggestions.  I have listed two versions below.

This is the full version that compiles and uploads to the mc fine.  It use 17% of program space which is great.  Unfortunately it does not work.  I can't tell if the error is an overflow issue or if it's hiding in the ADC_Read() function.

Code: [Select]

/**
  Included files
 */

#include "mcc_generated_files/mcc.h"
#include "lcd_jerry.h"
#include <stdio.h>
#include "app.h"



//*************************Declare Global Variables******************

char Buffer[20];

int ADC_Read(uint16_t channel);

uint32_t GET_Voltage(uint16_t resistorRatio, uint16_t convertedValue);

void LCD_Display(uint32_t cellVoltage, uint8_t row, uint8_t column);

void Lcd4_Clear(void);

void Lcd4_Init(void);


/*
                         Main application
 */
void main(void)
{
    //Initialize functions
    SYSTEM_Initialize();
    Lcd4_Init();
    ADCC_Initialize();
    APP_LED_Initialize();
     
    // Enable the Global Interrupts
    INTCONbits.GIE = 1;

    // Enable the Peripheral Interrupts
    INTCONbits.PEIE = 1;
   
    //Resistor Ratios
   
    /**
      The value of resistorRatioCelln calculated as follows:
      (1/(resistorTwoCelln / (resistorOneCelln + resistorTwoCelln))) * 100
    */
   
    uint16_t resistorRatioCell[4] = {496, 367, 245, 123};
   
    uint16_t cellVoltage[4] = {0,0,0,0};
   
    uint16_t voltage[4] = {0, 0, 0, 0};
       
    // LCD Greeting
   
    Lcd4_Clear();
    Lcd4_Set_Cursor(1, 1);
    Lcd4_Write_String("Battery");
    Lcd4_Set_Cursor(2, 1);
    Lcd4_Write_String("Simulation");
    led4_Toggle();
    __delay_ms(1500);
    led4_Toggle();
    Lcd4_Clear();
       
    // Turn on the ADC module
    ADCON0bits.ADON = 1;
   
    //Main while loop
    while (1)
    {
        APP_LED_Tasks();
        //Get Voltage at balance wires
        for (uint8_t i = 0; i < 4; i++)
        {
            voltage[i] = GET_Voltage(resistorRatioCell[i], ADC_Read(5 - i));
       
        }
        /* Used to confirm loops working
         *
        voltage[0] = GET_Voltage(resistorRatioCell[0], ADC_Read(5));
        voltage[1] = GET_Voltage(resistorRatioCell[1], ADC_Read(4));
        voltage[2] = GET_Voltage(resistorRatioCell[2], ADC_Read(3));
        voltage[3] = GET_Voltage(resistorRatioCell[3], ADC_Read(2));
         */
        //Converts readings at the balance wires to individual cell voltage
        for (uint8_t i = 0; i < 3; i++)
        {
        cellVoltage[i] = voltage[i] - voltage[i+1];
        }
       
        cellVoltage[3] = voltage[3];
       
        /*Used to confirm loops working
        *
        cellVoltage[0] = voltage[0] - voltage[1];
        cellVoltage[1] = voltage[1] - voltage[2];
        cellVoltage[2] = voltage[2] - voltage[3];
        cellVoltage[3] = voltage[3];
        */
        //Display the results
       
        LCD_Display(cellVoltage[0], 1, 0);
        LCD_Display(cellVoltage[1], 1, 8);
        LCD_Display(cellVoltage[2], 2, 0);
        LCD_Display(cellVoltage[3], 2, 8);
       
    }
}

//Function Definitions

int ADC_Read(uint8_t channel)
{
    uint16_t convertedValue = 0;
    // select the A/D channel
    ADPCH = channel;
    __delay_ms(10);
    // Start the conversion
    ADCON0bits.ADGO = 1;
    while (ADCON0bits.ADGO);
    convertedValue = ((adc_result_t) ((ADRESH << 8) + ADRESL));
    ADCON0bits.ADGO = 0;
    return convertedValue;
}

uint32_t GET_Voltage(uint16_t resistorRatio, uint16_t convertedValue)
{
    uint32_t voltage = 0;
    voltage = ((convertedValue) * 4) * resistorRatio;
    return voltage;

}

void LCD_Display(uint32_t cellVoltage, uint8_t row, uint8_t column)
{
    char volt[] = "1.11";
    char *volt_ptr = volt;
    volt_ptr[0] = look((cellVoltage / 100000) % 10);
    volt_ptr[2] = look((cellVoltage / 10000) % 10);
    volt_ptr[3] = look((cellVoltage / 1000) % 10);
    Lcd4_Set_Cursor(row, column);
    Lcd4_Write_String(volt);
    Lcd4_Write_String(" V");

}


/**
 End of File
*/

This is test code that I compiled with gcc and it works just fine.  I have to feed it the ADC values and have used a typical voltage read at the ADC pin of 2.6V or convertedValue = 650 using FVR of 4.096V.

Code: [Select]

/**
  Included files
 */

#include <stdio.h>
#include <stdint.h>



//*************************Declare Global Variables******************

char Buffer[20];

char look(int a);

uint32_t GET_Voltage(uint16_t resistorRatio, uint16_t convertedValue);

void LCD_Display(uint32_t cellVoltage);


/*
                         Main application
 */
int main(void)
{

    uint16_t resistorRatioCell[4] = {496, 367, 245, 123};

    uint16_t ADC_Values[4] = {650, 650, 650, 650};

    uint32_t cellVoltage[4] = {0,0,0,0};

    uint32_t voltage[4] = {0, 0, 0, 0};



    for (uint8_t i = 0; i < 4; i++)
    {
        voltage[i] = GET_Voltage(resistorRatioCell[i], ADC_Values[i]);
    }

    //Converts readings at the balance wires to individual cell voltage
    for (uint8_t i = 0; i < 3; i++)
    {
    cellVoltage[i] = voltage[i] - voltage[i + 1];
    printf("cellVoltage = %d\n", cellVoltage[i]);
    }

    cellVoltage[3] = voltage[3];

    //Display the results

    LCD_Display(cellVoltage[0]);
    LCD_Display(cellVoltage[1]);
    LCD_Display(cellVoltage[2]);
    LCD_Display(cellVoltage[3]);

}


//Function Definitions


uint32_t GET_Voltage(uint16_t resistorRatio, uint16_t convertedValue)
{
    int voltage = 0;
    printf("convertedValue = %d\n", convertedValue);
    printf("resistorRatio = %d\n", resistorRatio);
    voltage = ((convertedValue) * 4) * resistorRatio;
    return voltage;

}


void LCD_Display(uint32_t cellVoltage)
{
    char volt[] = "0.00";
    char *volt_ptr = volt;
    //volt_ptr[0] = look(cellVoltage / 10000000);
    //volt_ptr[1] = look((cellVoltage / 1000000) % 10);
    volt_ptr[0] = look((cellVoltage / 100000) % 10);
    printf("%c\n",volt[0] );

    volt_ptr[2] = look((cellVoltage / 10000) % 10);
    printf("%c\n",volt[2] );
    volt_ptr[3] = look((cellVoltage / 1000) % 10);
    printf("%c\n",volt[3] );
    //printf("%c\n",volt[6] );
    //volt_ptr[5] = look(cellVoltage / 100 % 10);
    //volt_ptr[6] = look(cellVoltage / 10 % 10);
    //volt_ptr[7] = look(cellVoltage / 1 % 10);
    printf("volt_ptr = %s\n",volt);

}

char look(int a)
{
   switch(a)
   {
       case 0:
              return '0';
       case 1:
              return '1';
       case 2:
              return '2';
       case 3:
              return '3';
       case 4:
              return '4';
       case 5:
              return '5';
       case 6:
              return '6';

I have checked the FVR related control register settings and they seem correct and are as follows.

Code: [Select]

ADREF = 0x03;
FVRCON = 0x83;

I do appreciate the help which has been amazing but if anyone can see how I might have stalled out feel free to give me a kick in the right direction.

Jerry

[DavidAlfa]

In ADC read function, ADRESH is 8 bit, so doing 8 bit shift might cause it to be always 0.

Code: [Select]

convertedValue = ((adc_result_t) ((ADRESH << 8) + ADRESL));
Try this:

Code: [Select]

convertedValue = ADRESH;
convertedValue = (convertedValue << 8) | ADRESL;

In GET voltage function, this will probably overflow:

Code: [Select]

voltage = ((convertedValue) * 4) * resistorRatio;
Convert convertedValue to uint32 before multiplying:

Code: [Select]

voltage = (uint32_t)convertedValue * 4 * resistorRatio;

Can't you debug it? Stepping the code would show the problem very easily.

How are the cells connected to the adc? What are the resistor values?
Remember that ADC sampling time is greatly affected by the input impedance.
Have you ensured it's correctly adjusted?

You say ADC reads 650 for 2.6V cell.
For cell 0, you do:
650*4*123 = 319800.

Then the display code makes 3.19V out of that.
So what's this result? Shouldn't it be 2.6V?

[jerryk]

You say ADC reads 650 for 2.6V cell.
For cell 0, you do:
650*4*123 = 319800.

Then the display code makes 3.198V out of that.
So what's this result? Shouldn't it be 2.6V?

It is a BMS for lifepo4 4S batteries and the controller on it is actually a pic16LF1824.  I'm stuck with the hardware that has very high impedance voltage dividers.  These dividers are stepped in a manner to present a uniform voltage below the 3.3Vdd for the low voltage pic.  A typical voltage seen by the 4 ADC inputs is around 2.6V if all cells balance at 3.2V nominal.  The reason for the display is to help me reverse engineer the existing BMS code.  To do that I want the display to accurately show cell voltage and not the divided voltage at the ADC input.  I chose to stick with the same high impedance dividers so I could learn how to adjust ADC acquisition times accordingly.

I did get the code to work as there was overflow issues from using uint16_t which you caught.

I hope my explanation makes sense.

Here is the new code that now functions and the program space is:

Memory Summary:
    Program space        used   5F7h (  1527) of  2000h words   ( 18.6%)
    Data space             used    B8h  (   184)  of   400h  bytes   ( 18.0%)
    EEPROM space        used     0h    (     0)  of   100h  bytes   (  0.0%)
    Data stack space     used     0h     (     0) of   32Ch  bytes   (  0.0%)
    Configuration bits    used     5h    (     5)  of     5h words    (100.0%)
    ID Location space    used     0h    (     0)  of     4h bytes      (  0.0%)

New code that works:

Code: [Select]

/**
  Included files
 */

#include "mcc_generated_files/mcc.h"
#include "lcd_jerry.h"
#include <stdio.h>
#include "app.h"



//*************************Declare Global Variables******************

char Buffer[20];

uint16_t ADC_Read(uint16_t channel);

uint32_t GET_Voltage(uint32_t resistorRatio, uint16_t convertedValue);

void LCD_Display(uint32_t cellVoltage, uint8_t row, uint8_t column);

void Lcd4_Clear(void);

void Lcd4_Init(void);


/*
                         Main application
 */
void main(void)
{
    //Initialize functions
    SYSTEM_Initialize();
    Lcd4_Init();
    ADCC_Initialize();
    APP_LED_Initialize();

    // Enable the Global Interrupts
    INTCONbits.GIE = 1;

    // Enable the Peripheral Interrupts
    INTCONbits.PEIE = 1;

    //Resistor Ratios

    /**
      The value of resistorRatioCelln calculated as follows:
      (1/(resistorTwoCelln / (resistorOneCelln + resistorTwoCelln))) * 100
    */

    uint32_t resistorRatioCell[4] = {496, 367, 245, 123};

    uint32_t cellVoltage[4] = {0,0,0,0};

    uint32_t voltage[4] = {0, 0, 0, 0};

    // LCD Greeting

    Lcd4_Clear();
    Lcd4_Set_Cursor(1, 1);
    Lcd4_Write_String("Battery");
    Lcd4_Set_Cursor(2, 1);
    Lcd4_Write_String("Simulation");
    led4_Toggle();
    __delay_ms(1500);
    led4_Toggle();
    Lcd4_Clear();

    // Turn on the ADC module
    ADCON0bits.ADON = 1;

    //Main while loop
    while (1)
    {
        APP_LED_Tasks();
        //Get Voltage at balance wires
        for (uint8_t i = 0; i < 4; i++)
        {
            voltage[i] = GET_Voltage(resistorRatioCell[i], ADC_Read(5 - i));

        }
       
        //Converts readings at the balance wires to individual cell voltage
        for (uint8_t i = 0; i < 3; i++)
        {
        cellVoltage[i] = voltage[i] - voltage[i+1];
        }

        cellVoltage[3] = voltage[3];


        LCD_Display(cellVoltage[0], 1, 0);
        LCD_Display(cellVoltage[1], 1, 8);
        LCD_Display(cellVoltage[2], 2, 0);
        LCD_Display(cellVoltage[3], 2, 8);

    }
}

//Function Definitions

uint16_t ADC_Read(uint16_t channel)
{
    uint16_t convertedValue = 0;
    // select the A/D channel
    ADPCH = channel;
    __delay_ms(10);
    // Start the conversion
    ADCON0bits.ADGO = 1;
    while (ADCON0bits.ADGO);
    convertedValue = ((adc_result_t) ((ADRESH << 8) + ADRESL));
    ADCON0bits.ADGO = 0;
    return convertedValue;
}

uint32_t GET_Voltage(uint32_t resistorRatio, uint16_t convertedValue)
{
    uint32_t voltage = 0;
    voltage = ((convertedValue) * 4) * resistorRatio;
    return voltage;

}

void LCD_Display(uint32_t cellVoltage, uint8_t row, uint8_t column)
{
    char volt[] = "1.11";
    char *volt_ptr = volt;
    volt_ptr[0] = look((cellVoltage / 100000) % 10);
    volt_ptr[2] = look((cellVoltage / 10000) % 10);
    volt_ptr[3] = look((cellVoltage / 1000) % 10);
    Lcd4_Set_Cursor(row, column);
    Lcd4_Write_String(volt);
    Lcd4_Write_String(" V");

}


/**
 End of File
*/

I'm still all ears on how to improve the efficiency of this code and will continue to digest all the suggestions thus far.

Jerry

[DavidAlfa]

Oh you meant 2.6V from the ADC input, not from the cell

No need to declare resistor ratios as uint32, it's wasting space. Just use uint32 for the calculation to avoid overflow.
You can re-use the same voltage array for computing the cell voltage.
What's char Buffer[20] for?

Initializing variables which you're going to write over is wasted time!
Mopst of them could be global, thus skipping unnecessary initialization on every call. Globals also makes debugging easier (Can be read at any time).
Also, you could replace look function by a simple constant table.
Finally, declare as constant the values you're not writing to (Ex. resistor ratios)
You need some timer to limit the refresh rate to 1-5Hz, faster than that you'll get unreadable dancing numbers when you get near a threashold (ex. 3.199-3.200)

Code: [Select]

/**
  Included files
 */

#include "mcc_generated_files/mcc.h"
#include "lcd_jerry.h"
#include <stdio.h>
#include "app.h"



//*************************Declare Global Variables******************

char Buffer[20];
uint32_t voltage[4];
char volt_str[] = "1.11 V";
const char digit[10] = { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9' };


/*
    Resistor Ratios
    The value of resistorRatioCelln calculated as follows:
    (1/(resistorTwoCelln / (resistorOneCelln + resistorTwoCelln))) * 100
*/
const uint16_t resistorRatioCell[4] = { 496, 367, 245, 123 };



uint16_t ADC_Read(uint16_t channel);
uint32_t GET_Voltage(uint32_t resistorRatio, uint16_t convertedValue);
void LCD_Display(uint32_t cellVoltage, uint8_t row, uint8_t column);
void Lcd4_Clear(void);
void Lcd4_Init(void);


   
/*
                         Main application
*/
void main(void)
{
    //Initialize functions
    SYSTEM_Initialize();
    Lcd4_Init();
    ADCC_Initialize();
    APP_LED_Initialize();

    // Enable the Global Interrupts
    INTCONbits.GIE = 1;

    // Enable the Peripheral Interrupts
    INTCONbits.PEIE = 1;

    // LCD Greeting

    Lcd4_Clear();
    Lcd4_Set_Cursor(1, 1);
    Lcd4_Write_String("Battery");
    Lcd4_Set_Cursor(2, 1);
    Lcd4_Write_String("Simulation");
    led4_Toggle();
    __delay_ms(1500);
    led4_Toggle();
    Lcd4_Clear();

    // Turn on the ADC module
    ADCON0bits.ADON = 1;

    //Main while loop
    while (1)
    {
        APP_LED_Tasks();
       
        //Get Voltage at balance wires
        for (uint8_t i = 0; i < 4; i++) {
            voltage[i] = GET_Voltage(resistorRatioCell[i], ADC_Read(5 - i));
        }
       
        //Converts readings at the balance wires to individual cell voltage
        for (uint8_t i = 0; i < 3; i++) {
            voltage[i] = voltage[i] - voltage[i+1];
        }

        LCD_Display(voltage[0], 1, 0);
        LCD_Display(voltage[1], 1, 8);
        LCD_Display(voltage[2], 2, 0);
        LCD_Display(voltage[3], 2, 8);

    }
}

//Function Definitions

uint16_t ADC_Read(uint16_t channel)
{
    // select the A/D channel
    ADPCH = channel;
    __delay_ms(10);
    // Start the conversion
    ADCON0bits.ADGO = 1;
    while (ADCON0bits.ADGO);
    return ( ((uint16_t)ADRESH << 8) | ADRESL );
}

uint32_t GET_Voltage(uint16_t resistorRatio, uint16_t convertedValue) {
    return ( (uint32_t)convertedValue * 4 * resistorRatio );
}

void LCD_Display(uint32_t cellVoltage, uint8_t row, uint8_t column) {
    volt_str[0] = digit[ (cellVoltage / 100000) % 10) ];
    volt_str[2] = digit[ (cellVoltage / 10000) % 10) ];
    volt_str[3] = digit[ (cellVoltage / 1000) % 10) ];
    Lcd4_Set_Cursor(row, column);
    Lcd4_Write_String(volt_str);
}


/**
 End of File
*/


[Picuino]

I find it faster and more efficient to perform the conversion from binary to decimal with this routine (for 32bits):

Code: (c) [Select]

#define BINARY_DIGITS   32
#define DECIMAL_DIGITS  10


void bin32_to_str(char *str, unsigned long bin) {
   char maxdig, digcnt, bitcnt;
   static char *p, carry;
 
   // Clear string
   p = str;
   digcnt = DECIMAL_DIGITS + 1;
   do { *p++ = 0; } while (--digcnt);
 
   // Transform binary to BCD
   bitcnt = BINARY_DIGITS;
   maxdig = (DECIMAL_DIGITS - 1);
   str += (DECIMAL_DIGITS - 1);
   do {
      // Shift left binary number with carry
      carry = 0;
      if (bin & (1L<<(BINARY_DIGITS-1)))
         carry |= 1;
      bin <<= 1;

      // Shift left decimal number with carry
      p = str;
      digcnt = DECIMAL_DIGITS - maxdig;
      do {
         carry = (*p<<1) + carry;
         if (carry >= 10) {
            *p-- = carry - 10;
            carry = 1;
            if (digcnt == 1) {
               maxdig--;
               digcnt++;
            }
         }
         else {
            *p-- = carry;
            carry = 0;
         }
      } while(--digcnt);
   } while(--bitcnt);

   // Transform BCD to ASCII
   digcnt = DECIMAL_DIGITS;
   do *str-- += '0'; while (--digcnt);
}


void setup() {
   Serial.begin(9600);
}


void loop() {
   unsigned long bin;
   char strnum[DECIMAL_DIGITS + 1];
   strnum[DECIMAL_DIGITS] = 0; // End of char

   for(bin=0; bin<100000; bin++) {
      bin32_to_str(strnum, bin);
      Serial.println(strnum);
   }
}
It's Arduino-ready, but it's easy to port it to the PIC.


Another one for 16bits:

Code: (c) [Select]

#define BINARY_DIGITS   16
#define DECIMAL_DIGITS  5


void bin16_to_str(char *str, unsigned int bin) {
   char maxdig, digcnt, bitcnt;
   static char *p, carry;
 
   // Clear string
   p = str;
   digcnt = DECIMAL_DIGITS + 1;
   do { *p++ = 0; } while (--digcnt);
 
   // Transform binary to BCD
   bitcnt = BINARY_DIGITS;
   maxdig = (DECIMAL_DIGITS - 1);
   str += (DECIMAL_DIGITS - 1);
   do {
      // Shift left binary number with carry
      carry = 0;
      if (bin & (1L<<(BINARY_DIGITS-1)))
         carry |= 1;
      bin <<= 1;

      // Shift left decimal number with carry
      p = str;
      digcnt = DECIMAL_DIGITS - maxdig;
      do {
         carry = (*p<<1) + carry;
         if (carry >= 10) {
            *p-- = carry - 10;
            carry = 1;
            if (digcnt == 1) {
               maxdig--;
               digcnt++;
            }
         }
         else {
            *p-- = carry;
            carry = 0;
         }
      } while(--digcnt);
   } while(--bitcnt);

   // Transform BCD to ASCII
   digcnt = DECIMAL_DIGITS;
   do *str-- += '0'; while (--digcnt);
}


void setup() {
   Serial.begin(9600);
}


void loop() {
   unsigned int bin;
   char strnum[DECIMAL_DIGITS + 1];
   strnum[DECIMAL_DIGITS] = 0; // End of char

   for(bin=0; bin<50000; bin++) {
      bin16_to_str(strnum, bin);
      Serial.println(strnum);
   }
}


[jerryk]

You can re-use the same voltage array for computing the cell voltage.

I had to stare at that one for a minute to understand how that worked

What's char Buffer[20] for?

It was a remnant from the sprintf() I originally posted.  I deleted it and added your improvements that dropped the program space to 16.7%.

You need some timer to limit the refresh rate to 1-5Hz, faster than that you'll get unreadable dancing numbers when you get near a threashold (ex. 3.199-3.200)

You hit that one on the head.  I have dancing numbers for sure.  That is the next task and any hints are more than welcome.  I do have timer1 running already to set the blink time for the led.  It's triggers an interrupt every ms, counts to 500 and toggles the led.  I'm not sure if that is the concept you are talking about when you mentioned "timer".  It seems like it should work.

Earlier you asked about the voltage divider values.  They are in the original code but summarize here:

             R1          R2
Cell 1 = 982       248.2
Cell 2 = 663       248.3
Cell 3 = 361       248.2
Cell 4 = 56.14    247.8

All in K ohms! All are fed direct to the 4 ADC inputs after division with no external capacitance or opamp buffering.

If you take a balanced 12V 4S battery at 3V per cell this translates to 2.44V (approx) at the ADC.
This is how the BMS is designed that I'm working on.  I can't change the design.  FYI these are expensive $400 - $1000 LFP batteries that I repair/salvage parts from.

Again - appreciate the help.

Jerry

[DavidAlfa]

Adding a 100pf capacitor to each ADC pin should provide more stable readings, reducing the high impedance issues.

What's the timer input frequency?
Ideally it should be free-running with a pre/postscaler so it increases every 1,10,100ms, to whatever resolution you need.

Then every function needs a static variable to store last timer value and compare it when entering.
If the delay is over, the static var is updated and the function executed.

Since the timer will eventually overflow, check that the new value isn't lower than before.
So if greater, it's simple as (new-old), but if lower, it's (65536-old) + new.
Assuming it's in 16-bit mode.

However timer1 capabilities are very limited.
Timer0 on ithe other hand, has much better options, 1-32768 prescaler and 1-16 postscaler.
If it's for leds and refreshing the lcd (No critical timing) you don't really need very precise timing.
Refreshing the lcd every 100 or 102ms won't make any differece, same for the leds.
So if you're running at 32MHz, you can use 32768 prescaler and get 1.024ms timing steps, overflowing every 65.5 seconds, which is ok.

Don't forget that the 16F18875 ADC has interesting computation options, the low-pass filter would greatly reduce noise.

[DavidAlfa]

This is a more advanced example showing how to program avoiding blocking using timers and state machines:

https://pastecode.io/s/o4q28qmn

The ADC never blocks the code execution.
When it's time to sample a channel, it will simply check if the time has passed, return or proceed to the next step.
When all channels are samples, it waits the specified update time.
Same for led and lcd.

The static variables might confuse you:
They're like local variables, but they hold their values, so when the function is called again, these variables hold their last values.

Might be some dumb errors!

[jerryk]

The clock frequency HFINTOSC set to 16Mhz and timer1 is FOSC/4, prescaler 1:1.

I'm looking over the code you posted. It is much easier to follow than the distributed source files in MCC.  I'm in no position to be constructively critical of it, I just find it easier to understand as you presented.  It will take a moment to firm up my grasp of what you posted.

Thanks! - Jerry