Counting down is the easy part, just do
if(SystemTime_Secs > 0) SystemTime_Secs--;
in the interrupt routine.

Is this correct ?

unsigned int SystemTime_Secs;

/******* Interrupt Service Routine ************************/
void interrupt() {
                                    // Used for Display Muxing.
  if (TMR0IE_bit)
  if (TMR0IF_bit)
  {
      TMR0IF_bit = 0;               // Ack IRQ
      TMR0 = TMR0Set;               // Reset Timer0
      //Display Multiplex Code goes here
    if(LEDflash_timer==0)           // if LED flash timer ran out
     {
       LEDflash_timer = LED_MUXflash_set;// reset timer and.. invert LEDs under mask
       DigitsBuf[dLEDs] ^= LEDflash_mask;
     }
     else
       LEDflash_timer--;

      PORTD = DigitsBuf[dLEDs];     // 'mulitiplex' the LED display - only discrete LEDs so far
                                    // This is where the 9 digits get selected/decoded
      MUXtheDisplay();              // Run the multiplexer.  When happy, move the code inline to here to save stack
  }
  if (CCP1IE_bit)                   // System Tik (dont specify Time here, it may change then this will be confusing)
  if (CCP1IF_bit)                   // Interrupt on Timer1 = CCP
  {
      CCP1IF_bit = 0;               // Ack IRQ
                                    // Decrement derived timers
  if (SYStik_timer_A) SYStik_timer_A--;

     SecsTimerPS--;                 // dec Seconds timer prescaler
  if(SecsTimerPS == 0)              // if one second passed..
    {
      SecsTimerPS = SecsTimerPSset; // reload the prescaler
      Sec_Elapsed = 1;              // signal main program
   //   if(SettingTimeOut)SettingTimeOut--;// A 1 count/s incrementing counter used in T,V & I Settings
      if(SystemTime_Secs>0)SystemTime_Secs--;
    }
  }
}

The red highlights are added like you said.

Am I on the right track ?

 

Am I on the right track ?

. Yup. But I guess that before, the timer was NOT in the interrupt routine yet?

So.. now it is and that's better (less for the main routine to do) BUT introduces the problem of reading/writing a multibyte value during a possible interrupt. You can't do that without ensuring that the read/write won't be interrupted which will foul things up (why?).

Here's how to fix that:

//******************** MAINTAIN SYSTEM TIME  ******************************
// Set and read SystemTime_Secs. Interrupt is disabled during multi-byte access.
// These expect that CCP1IE is enabled when called as they unconditionally
// re-enable the interrupt after accessing the timer.

// Set system time to passed value respecting interrupts
 void SET_SystemTime_Secs(unsigned int secs)
 {
      CCP1IE_bit = 0; // disable interrupt
      SystemTime_Secs = secs;
      CCP1IE_bit = 1; // reenable interrupt
 }

// Read and return system time respecting interrupts
 unsigned int READ_SystemTime_Secs(void)
 {
    unsigned int temp;
      CCP1IE_bit = 0; // disable interrupt
      temp = SystemTime_Secs;   // read the value
      CCP1IE_bit = 1; // reenable interrupt
      return(temp);   // return the value read
 }

Note how the CCP1 interrupt is disabled during the reading of the multibyte value.

Use the set/read like this:

// Example use:
 unsigned int timeNow;
 
  SET_SystemTime_Secs(3600);    // set timer to 1 hour


  if (READ_SystemTime_Secs() > 1800)  // more than 1/2 hour to go?
    do something(); 
    
  if ( (timeNow = READ_SystemTime_Secs())  > 0) // load timeNow, test if timer running
      Relay = 1;        // dummy operations for example
  else
      Relay = 0;

  Int2Time(timeNow,dTime,SegDP); // display time

Note that this is just bogus code as an example.
BTW: now that SystemTime_Secs is maintained by IRQ, be sure to remove all direct references to it (++, = etc) and use only SET and READ to manage it. Nice.

You might want to consider renaming SystemTime_Secs to something more meaningful/easier to type. Plus if you do it now, the compiler will flag all instances of it under the old name (less searching). I probably would typedef it to a more meaningful name as well.

Finally, next time everything is working, consider inlining the display multiplexer function i.e. remove the function header and containing braces and move the actual code to the interrupt routine in place of the call. That will save you 1 stack level - not a bad thing when you only have 8.

 

Another alternative for real time keeping is to use three separate variables;
hours
mins
secs
and every time a second is added, check/fix overflows.

Or, if you want something sneaky, just keep your 2 hour countdown timer as a simple integer; 200-000 (ie; 2:00 to 0:00), and every minute subtract 1 from it.

I said "sneaky" because you need to deal with the base-60 issue... Before subtracting 1 minute each time you can check if the minutes are "00", and if so, subtract 41 instead;
102 - 1
=101 (1:01)

100 - 41
= 59 (0:59)

you can tell if the last two digits are "00" like this;
if((timer % 100)==0) timer-= 41;
else timer-=1;

Sorry to complicate the ideas mix.

 

Sorry to complicate the ideas mix.

Not at all. Its good stuff and well suited I would think for many applications.

While I've used similar approaches, eventually I decided on always using scalar types (in this case an unsigned int of seconds) and only converting for display when req'd. If you have to do any math, assignments or pass the time to a function for example, it gets more complicated - at least more than I like (I'm kind of lazy ). Revisions are easier as well. We just went from increment the time to decrement. Instead of changing modular math, it was change ++ to --. The modular math we have to do the conversion from int -> time is unchanged. I like that.

But its always instructive to contemplate the various ways of doing things. I know you know all this stuff, Roman. Just thought I'd give the OP some more food for thought. Anyway, that's the reason I picked a raw seconds register.

Cheers!

 

Need some time to see if I can do this.

But I think I will still have an issue.
You see the time is not preloaded as I thought I would at first but since I can change the timer value it will be loaded prior to charging by the encoder. Say for example if I set to 1.00, meaning I hour, it should count down from 1 Hour.

Is there a way to save the value I set to uC and start from tht value.

I think the value is saved in Time integer after I set it, cause I can see the value unchanged if I do not reset the PIC.

what would be easier to implement. Having fixed count downs like, 30min, 1Hr, 1Hr30min and 2 hrs. I think it is better this way, isn't it ?

 

But I think I will still have an issue.
You see the time is not preloaded as I thought I would at first but since I can change the timer value it will be loaded prior to charging by the encoder. Say for example if I set to 1.00, meaning I hour, it should count down from 1 Hour.

Is there a way to save the value I set to uC and start from tht value.

I think the value is saved in Time integer after I set it, cause I can see the value unchanged if I do not reset the PIC.

what would be easier to implement. Having fixed count downs like, 30min, 1Hr, 1Hr30min and 2 hrs. I think it is better this way, isn't it ?

No problem. Here are my thoughts on how to do it.

You need 2 time registers, the starting charge time (setpoint) and the current (cycle remaining) time.

Each time the charger is started, the setpoint is copied into the countdown time.

If the charger is not running, changes to the time affect the setpoint.
If the charger IS running, changes to the time affect the countdown time - AKA trimming the cycle.

Setpoint Management:
The setpoint should be non-volatile i.e. its shows up after power off or reset. The best way to do this is to save it in EEPROM. You can (probably) specify a starting value that gets programmed when you blast the chip. You SHOULD also have a default value in ROM that is used if the EEPROM is corrupted. In that case, the default ROM value is copied to the EEPROM upon detection of the corrupt value and any changes proceed normally from there.

Cycle Management:
Its not unusual to save a copy of the current cycle time (and any other run-time generated parameters) in EEPROM as well so that the cycle can resume (instead of restart) after a power interruption. You should update this record periodically keeping in mind that the EEPROM has limitations on how many times it can be updated. On the '887 is 100K times min, 1M times typ. See sec 10 in the databook for more info.

If this sounds like what you need, we can discuss the particulars - like how do you know the EEPROM is corrupt..

 

I don't think you understood what I said.

But leave it for now. I will see what I can do with time and get back with a much clear explanation.

3 hrs later .......
I tried a lot but does not seem to get my head thru this.

Here is what I already have.
In a prior post you can see my settings routine.
I can cycle thru Time, Volts and Amps.

I can set Volts to 4.2 or 8.4 Volts which is fine
I can set current ( amps ) to 0 to 5.00 which I believe will reflect the same value when read from ADC. This will be dealt later.

The main issue is Time.
I have written another routine just for time as suggested called "Int2Time" which only deals with times settings.
The Volts and Amps will use the old "Int2Seg" routine.

I get stuck as how to roll over from 0.59 to 1.00 and 1.59 to 2.00.
As both you guys know this is very very hard for me. I am trying to figure out but am getting lost.
The Time incrementing part needs to roll over at 0.59 not 0.99 which it is doing now.
I understand it is due to the fact that 8 bit goes form 0 to 255 and since "time <200" is there it is not going above 200.

The part I could not figure is the 0.59 to 1.00. If this part is done I think counting down to 0 won't be an issue.

Sorry guys but I just could not get it done. Been at it for hours.

I know this is not what you had in mind

/******* Real time Conversion routine *********************/
// This routine is used to convert the Time Display only
void Int2Time(unsigned int i, unsigned short ofs, SegBits DPmask)
{
    unsigned short x, Hrs,minH,minL,Hours,min;
    Hrs = i/3600;
    min = (i%3600)/60;
    minH = min/10;
    minL = min%10;
    
    Hrs = i/100;
    DigitsBuf[ofs] = SegTable[Hrs] | DPmask;

    minH = (i/10)%10;
    DigitsBuf[ofs+1] = SegTable[minH];

    x = (i%10);
    DigitsBuf[ofs+2] = SegTable[x];
}

 

void Settings() {
      EncoderSwitch();                     // Read Encoder Selector Sw.
// Sets Top up time
    if(Set_Time == 1)                      // If Time flag is set.
   {
      Enc_New = (PORTB & 0xC0);            // Keep the Last too Bits of PortB.
    if(Enc_New != Enc_Old)                 // If Encoder moved,
   {
    if(Enc_New.RB7 == Enc_Old.RB6)         // Look for direction.
    {                                      // If CW,
     if(Time<200)Time++;
     if(Time == 60)Time = 100;
     if(Time == 160)Time = 200;

    }
    else
    {
     if(Time>0)Time--;
     if(Time == 199)Time = 159;
     if(Time == 99)Time = 59;
    }
     Enc_Old = Enc_New;
     Int2Segs(Time,dTime,SegDP);
   }
  }

I am even surprised this code even works.
Note tht I am still using the old Int2Segs.

It works perfectly....0.00 -> 0.59, 1.00 -> 1.59 and to 2.00
and goes down perfectly too.

Is it too good to be true or is there a catch. Please say I can use this...! Pretty please.

I just had brain farts. cause I thought of this on my own this time. My head hurts
I am going to bed...G'night

 

.. because apparently I suck at time/modular arithmetic. Anyway, here is an actual tested routine (tested..I mean, what a concept-eh?) that does what I was describing i.e. convert an integer's worth of seconds to h:mm to 7-segments on your display.

//******************** TIME TO SEGS  ***********************************
// Converts secs to h:mm in the display at ofs. Appends DPmask
void Time2Segs(unsigned int secs, unsigned short ofs, SegBits DPmask)
{
  unsigned short hours, mintemp, minH, minL;
  //unsigned int mintemp;

  hours =  (secs / 3600) ;    //results in a single digit for hours
  mintemp = (secs %3600)/60;  //returns minutes part 0-59 min
  minH = mintemp/10;          // returns 10's digit of 0-59 minutes
  minL = mintemp%10;          // returns 1's digit
  
  DigitsBuf[ofs] = SegTable[hours] | DPmask;
  DigitsBuf[ofs+1] = SegTable[minH];
  DigitsBuf[ofs+2] = SegTable[minL];
 }

As you can see, I still advocate keeping the system time as a scalar value. And although your work-around was clever and inspired I think secs is the way to go... YMMV

So...
Looking at your code, it seems that the encoder is still embedded in the code-flow (instead of a black-box approach). OK for now if it works. Black box is better.


Where is SystemTime_Secs now? If you have it in the interrupt routine go back and declare it as
volatile unsigned int SystemTime_Secs;
..and use the SET/READ routines exclusively to access it. If not, don't worry about it.

 

Hi, nice work on this;

    {
     if(Time>0)Time--;
     if(Time == 199)Time = 159;
     if(Time == 99)Time = 59;
    }

Using a "brute force post-correction" is normally bad form but you made it work very well, because there are only two fault conditions you need to check for, and post-correct.

Those two if() tests are much faster than my suggested workaround;

if((timer % 100)==0) timer-= 41;
else timer-=1;

Because the modulus operator % is very slow and involves division math (which you don't see because the compiler does it behind the scenes).

BUT I think what you have at the moment is not a code issue it's a DESIGN issue. You need to decide what form you are going to keep your time data in.

John suggested keeping seconds only, and then calculating to HH:MM:SS for display. That method has benefits because you can very easily calculate offsets between different points in the time; like how long the constant current cycle will take. You just subtract two different seconds variables.

That system gives you good options for setting different times for CC, CV and topup cycles. Or even just for displaying how long each cycle takes, you might want to show total time remaining AND remaining time for the CC cycle which is in progress.

And it also makes it easy to set the time, you just use this code to subtract 1 minute (-60 secs);

    {
     if(Time >= 60) Time -= 60;
    }

 

I tried all but suggestion does not worked for me.
May be I am missing something but tht is OK, I have worked around it.

My friends, you are missing something. That is the timer is just a count down used ONLY in CV mode. It starts to count down from the value I set in time during settings . Nothing more. When the time reaches 0, everything stops tht's how I want to do. No measurements of charging or top up cycle or anything. Just start to count down when it enters CV, tht's it.

And guess what I have made it to work using John's examples before

void Settings() {    
   EncoderSwitch();                     // Read Encoder Selector Sw. 
// Sets Top up time   
  if(Set_Time == 1)                      // If Time flag is set.    
{       Enc_New = (PORTB & 0xC0);            // Keep the Last too Bits of PortB.   
  if(Enc_New != Enc_Old)                 // If Encoder moved,  
  {     if(Enc_New.RB7 == Enc_Old.RB6)         // Look for direction.   
  {                                      // If CW,    
  if(Time<200)Time++;     
 if(Time == 60)Time = 100;    
  if(Time == 160)Time = 200;    
  }    
 else    
 {     
 if(Time>0)Time--;     
 if(Time == 199)Time = 159;     
 if(Time == 99)Time = 59;    
 }      
Enc_Old = Enc_New;     
 Int2Segs(Time,dTime,SegDP);  
  }  
 }

I used this to set the time and
wrote a test routine
to see if it counts down

    while(SHT_Sw == 1)
    {
       if(Sec_Elapsed)      // maintain multibyte timers
     {
        Sec_Elapsed = 0;      // ack flag


     if(Time)Time--;
     if(Time == 199)Time = 159;
     if(Time == 99)Time = 59;

        if (SystemTime_Secs & 0x0001)  // one way to flash the decimal if its running
           Int2Segs(Time,dTime,SegDP);// update display
        else
           Int2Segs(Time,dTime,0);// update display - DP off
        } // one second elapsed
   
     }

The above code works as I want.

When I set say 2.00 in Time display and enter the routine by pressing a button( SHT_Sw) it starts to count down and goes to zero.
That is what I want it do, nothing more.

Sorry guys, You spent a lot on my behalf but I think it was my mistake I did not explain tht part.

I believe this will do.
Though I do need a 60 sec to decrement 1 count.
Lesse if I can do it

 

Now working with a 60 second per count

   while(SHT_Sw == 1)
    {
      if(Sec_Elapsed)
     {
        Sec_Elapsed = 0;
        Min_Elapsed++;
      if(Min_Elapsed == 59)
       {
        Min_Elapsed = 0;
        if(Time)Time--;
        if(Time == 199)Time = 159;
        if(Time == 99)Time = 59;
       }
       if (SystemTime_Secs & 0x0001)
        Int2Segs(Time,dTime,SegDP);
       else
        Int2Segs(Time,dTime,0);
     }

    }

see the red highlight, should it be 59 or 60 ? I think 59

Now I have set it up against a stopwatch for 2 hour count down.
5 minutes past...so far so good.

 

see the red highlight, should it be 59 or 60 ? I think 59

I'm thinking 60. It counts from 0-59 but resets immediately at 59 meaning it really only counts 59 intervals instead of 60.

How's your timing?

 

Timing is OK for the application. But off around 30 secs after 30 minutes. So I think it is 60 too
I don't think I will be using 2 hours. Probably less.
The Timer is just a precaution. As topping up for too long is likely to damage the cell.

 

Timing is OK for the application. But off around 30 secs after 30 minutes. So I think it is 60 too
I don't think I will be using 2 hours. Probably less.
The Timer is just a precaution. As topping up for too long is likely to damage the cell.

I'd fix it anyway. Its not performing as the design expects. Pencil-whipping problems seldom leads to favorable outcomes.

 

I will check for count down accuracy after loading 60 to min_elapsed and get back.

 

Timing is dead on at Min_elapsed = 60

At 59 it was loosing a sec each minute. So for 30 mins there was a 30 second difference. You were right as usual.

I have a question
Refer to below;

while ((Ready !=0) && (Bat_in !=0))
{
blah;
}

I know the while remains in loop if
Ready is not 0 and Bat_in is not 0. If any one is false while exits.
Am I right ?

If I am write is it possible to use more than two conditions.
as in
While ((Blah !=0) && (WTF !=0) && (sue_me != 1) && (hell_ya == 0))
{
blah;
}

Is the opening and closing brackets () enough or should there be more ?

and this one

While(Sec_Elapsed)
{
}

How actually does tht apply ? This does confuse me.

 

If any one is false while exits.
Am I right ?
If I am right is it possible to use more than two conditions.
as in
while ((Blah !=0) && (WTF !=0) && (sue_me != 1) && (hell_ya == 0))
{
blah;
}

Yep. Each (condition) is evaluated for true/false independently. Internally, C uses 0 to indicate FALSE and any non-zero value (!FALSE) for TRUE. One or more false ones is AND'ing something with 0, the result is false.

You can make the expression as complicated as you want within reason i.e.
while( (Blah !=0) && (WTF !=0) && (sue_me != 1) && (hell_ya == 0)) || ((bypass == 1) && (bypass == ENABLED)) && !(time > cocktail_time)) ) etc etc. (I didn't count the parentheses so don't try this at home). The point is that the expression can be complicated - within compiler limitations - as long as it eventually evaluates to true or false.

If the whole thing evaluates to TRUE, the single statement or compound statement in braces will be executed in a loop until the condition is no longer TRUE or you execute a break within the loop. Then it exits the loop.

Is the opening and closing brackets?? () enough or should there be more ?

Each individual test should be in parentheses i.e. (Blah != 0) to avoid unexpected issues with operator precedence and association. But then you need to put the whole expression in parenthesis to satisfy the 'while' syntax. So:

while( 1 ) leads to
while ( (Blah!=0) && (WTF !=0) )

Note that during the evaluation of the various things that make up the loop, the compiler may stop evaluating as soon as it can make a decision. Its up to the compiler. This can create problems if you get cagy and do something like:

while ((timer > 0) && (++count < 10)){

}

As soon as timer > 0, it may stop evaluating. If you expected count to be the number of times through the loop after it exited, you could be one short since it would not necessarily evaluate the ++ if the timer test was false. Whether it evaluates everything even after FALSE, and the order it evaluates is up to the compiler and the ++count side-effect should not be relied on.

and this one
while(Sec_Elapsed)
{
}
How actually does that apply ? This does confuse me.

Its shorthand for (Sec_Elapsed != 0) Here, it reads Sec_Elapsed and calls it TRUE if its not 0, FALSE when it IS 0. That's why while(1) is an endless loop, 1 is always true - so is while(176) and while(15330059) but convention says to use (1). In the ridiculous test above, each of the tests in parentheses evaluates to some logical value (true or false i.e. NZ or Z) which is combined with the others by logical operations to see if the net result is non-zero or 0.

Back in the day, things like while (Sec_Elapsed) generated less code than while (Sec_Elapsed != 0) but any modern compiler will recognize the construct and use the simple test. Opinions vary as to which is the better construct.