How to combine TMR0H + TMR0L into a 16 bit variable using XC8

[DTJ]

Hoping for some help with a basic PIC XC8 question:

I'm counting pulses using TMR0, converting the resulting count to ASCII & displaying it on an LCD.
To get the value in TMR0 into the counter variable I do it the long way as shown below, it works ok.

Q) I imagine I can do it much simpler. How to I get it to just concatenate the two TMR0 registers?

Thanks.




unsigned int counts = 0;   //counts variable

counts = TMR0H*256+TMR0L; // load current TMR0 counter value

itoa(buffer,counts,10); // convert counts to ASCII string & place them in buffer array

// now write buffer to LCD

[Mechatrommer]

Code: [Select]

counts = (TMR0H << 8) | TMR0L;


Code: [Select]

unsigned int *ptr = &count + 3;
*ptr = TMR0L; ptr--;
*ptr = TMR0H;
// it depends on the endianess


[helius]

C does not have any concatenate operator. The standard library has string catenating functions, but they do not work on binary values.
Either use counts = (TMR0H<<8 ) ^TMR0L;
or

Code: [Select]

union {unsigned v; struct {unsigned char h, l;} s;} counts;
counts.s.h = TMR0H; counts.s.l = TMR0L;
itoa(buffer, counts.v, 10);

[Ian.M]

If you are using Timer 0 in 16 bit mode, you must be using a PIC18 as all other families supported by XC8 have an 8 bit Timer 0.

The PIC18 Timer 0 has a latch for the high byte, which is used both for read and write operations.  TMR0H is the latch, not the actual timer.   To write the timer you must first write TMR0H to load the latch then write TMR0L to transfer the latch to the timer high byte, while simultaneously writing the low byte.   To read the timer you must first read TMR0L which also updates the latch from the high byte of the timer, then read TMR0H to get the previously latched value.

Although the XC8 headers include a definition for a 16 bit TMR0, It should NOT be used as you are relying on undefined behaviour, namely the order of byte reads or writes to a 16 bit volatile unsigned short.   

Even your:

Code: [Select]

counts = TMR0H*256+TMR0L; // load current TMR0 counter valueis unsafe as the compiler is free to rearrange operations between sequence points and the RHS doesn't contain any.

To guarantee a correct read, even if you upgrade the compiler or change optimisation level, you *must* use:

Code: [Select]

counts = TMR0L;
counts|=TMR0H*256U; // load current TMR0 counter value
or a similar sequence to ensure you get the high byte that was latched most recently.

[DTJ]

Many thanks for the replies gentlemen. Based on Ian's comments ( & my lack of reading the data sheet), I was also using the incorrect method to write to the Timer.

I've found the ReadTimer0() and WriteTimer0() functions also. Is it preferable to use these? I imagine they should take into account subtleties you have pointed out.


I also noted the 'U' suffix on the 256 in:

counts|=TMR0H*256U; // load current TMR0 counter value

Is this necessary or just good practice? I did some reading and it seems to indicate that the constant (256) is unsigned. I thought that would be implicit anyway.
Also I imagine using the 8 left shifts would be faster than multiplying by 256 - is that correct?


Thanks for your help guys, really appreciated.

[Xenoamor]

Code: [Select]

union {unsigned v; struct {unsigned char h, l;} s;} counts;
counts.s.h = TMR0H; counts.s.l = TMR0L;
itoa(buffer, counts.v, 10);

+1

[dannyf]

The fastest approach would be to use a 16-bit pointer and point to TMR0L - in cases where TMR0H and TMR0L are consecutive. This approach suffers from the same flaw as the struct approach in that they are endian-dependent.

In general, it is safer to take a look at the datasheet and see precautions on operating 16-bit types on an 8-bit mcu -> atomocity isn't always assured if yo don't follow the right sequence of operations.

[Ian.M]

@Helius, Xenoamor: You are wrong. TMR0L *MUST* be read first. See any PIC18 datasheet.  Even if you fix it, the type-punning union approach is also generally regarded as bad practice, and is non-portable.

@DannyF: Its an 8 bit CPU with only three index registers, each split into two parts.  Pointers are *SLOW*.

@DTJ,
Its good practice to use unsigned constants when doing unsigned maths.  Under ANSI C's usual arithmetic conversions (C89 #3.2.1.5) the unsigned type of TMR0H dominates so in this case it doesn't really matter.   Efficiency relies on the compiler recognising the idiom as a byte shift so simply using MOV instructions, which it will also do for unsigned shifts that are multiples of 8 bits, but some non-ANSI compliant PIC compilers (including Microchip C18) don't do standard integer promotion for chars so the result of TMR0H<<8 will be 0.

The functions are actually macros defined in pic18.h so can be used without a performance penalty.  As the compiler writers know the current 16 bit byte read order, they will always be correct for the current compiler version and may be faster than the alternative explicit method.  However they are non-portable.

[dannyf]

I fed the four approaches (1=using a struct, 2=using a pointer, 3=left shift, 4=multiplication) through XC8 and here is the output:

Code: [Select]

33:                    myTMR0_struct->W+=1; //increment the word
  1FB2    4AD6     INFSNZ 0xfd6, F, ACCESS
  1FB4    2AD7     INCF 0xfd7, F, ACCESS
34:                    tmp = myTMR0_struct->W; //the struct approach
  1FB6    CFD6     MOVFF 0xfd6, 0x1
  1FB8    F001     NOP
  1FBA    CFD7     MOVFF 0xfd7, 0x2
  1FBC    F002     NOP
35:                    tmp = myTMR0_ptr; //the pointer approach
  1FBE    C003     MOVFF 0x3, 0xfd9
  1FC0    FFD9     NOP
  1FC2    C004     MOVFF 0x4, 0xfda
  1FC4    FFDA     NOP
  1FC6    CFDE     MOVFF 0xfde, 0x1
  1FC8    F001     NOP
  1FCA    CFDD     MOVFF 0xfdd, 0x2
  1FCC    F002     NOP
36:                    tmp = myTMR0_shift; //the left-shifting approach
  1FCE    50D6     MOVF 0xfd6, W, ACCESS
  1FD0    CFD7     MOVFF 0xfd7, 0x5
  1FD2    F005     NOP
  1FD4    6A06     CLRF 0x6, ACCESS
  1FD6    C005     MOVFF 0x5, 0x6
  1FD8    F006     NOP
  1FDA    6A05     CLRF 0x5, ACCESS
  1FDC    1005     IORWF 0x5, W, ACCESS
  1FDE    6E01     MOVWF 0x1, ACCESS
  1FE0    5006     MOVF 0x6, W, ACCESS
  1FE2    6E02     MOVWF 0x2, ACCESS
37:                    tmp = myTMR0_mlt; //the multiplication approach
  1FE4    50D6     MOVF 0xfd6, W, ACCESS
  1FE6    CFD7     MOVFF 0xfd7, 0x5
  1FE8    F005     NOP
  1FEA    6A06     CLRF 0x6, ACCESS
  1FEC    C005     MOVFF 0x5, 0x6
  1FEE    F006     NOP
  1FF0    6A05     CLRF 0x5, ACCESS
  1FF2    1005     IORWF 0x5, W, ACCESS
  1FF4    6E01     MOVWF 0x1, ACCESS
  1FF6    5006     MOVF 0x6, W, ACCESS
  1FF8    6E02     MOVWF 0x2, ACCESS

Compiler in free mode.

The compiler is pretty smart, isn't it? Especially being free,

[Bruce Abbott]

pic18f458.h

Code: [Select]

// Register: TMR0
extern volatile unsigned short          TMR0                @ 0xFD6;
main.c

Code: [Select]

unsigned short counts = 0;

counts = TMR0;
18f458.as

Code: [Select]

;main.c: 12: counts = TMR0;
movff (c:4054),(c:_counts) ;volatile
movff (c:4054+1),(c:_counts+1) ;volatile

[Ian.M]

That's what the ReadTimer0() macro generates, a direct 16 bit read of TMR0.  If they ever reorder it, the macro will be updated to match.

[Mechatrommer]

18f458.as

Code: [Select]

;main.c: 12: counts = TMR0;
movff (c:4054),(c:_counts) ;volatile
movff (c:4054+1),(c:_counts+1) ;volatile

you should look how the TMR0 get assigned then...

[dannyf]

That's what the ReadTimer0() macro generates, a direct 16 bit read of TMR0.  If they ever reorder it, the macro will be updated to match.

Have you tried to tell Micorchip engineers that those macros they wrote violated your directive?

Although the XC8 headers include a definition for a 16 bit TMR0, It should NOT be used ...

[Ian.M]

Because the macros are provided by the compiler development team, they can make sure that the access order for each macro is correct for the current compiler version.  e.g. the WRITETIMER0()  macro is currently quite complex because of this:

Code: [Select]

#define WRITETIMER0(x) ((void)(TMR0H=((x)>>8),TMR0L=((x)&0xFF)))If you use TMR0 directly and a new compiler version breaks your code, that's your problem.
A fatal mistake would be to try to alter the period of the timer in its ISR by:

Code: [Select]

TMR0+=CyclesToSkipthe same way you would in 8 bit mode using TMR0L.

Its therefore best to avoid using the 16 bit TMR0 definition directly in your code, and if you need to 'bump' the timer like that, use:

Code: [Select]

{
   unsigned t;
   t=READTIMER0();
   t+=CyclesToSkip;
   WRITETIMER0(t);
}

n.b.

Code: [Select]

WRITETIMER0(READTIMER0()+CyclesToSkip);is also a disastrous mistake, because WRITETIMER0(x) evaluates x twice so would read the timer twice, with the possibility of TMR0L rollover between the two reads.

[Brutte]

Code: [Select]

main.c: 12: counts = TMR0;I'd also add:

Code: [Select]

assert(no IRQs accessing TMR0L are enabled);just before that.

[Bruce Abbott]

If you use TMR0 directly and a new compiler version breaks your code, that's your problem.

Wonderful. You write programs in C because it's portable and less error-prone, but spend half your time dealing with compiler issues. Then when you try to move the program to another chip you discover what 'portability' really means...
   

[Ian.M]

*DON'T* remind me.  Code written in CCS C using its idiosyncratic library functions is a bear to port to any other compiler.  As soon as you make heavy use of non-ANSI libraries, you've locked yourself in to that compiler vendor.   At least if you hit the registers directly, the differences in register bit access syntax can usually be accommodated by simple search and replace.

Porting up and down within one PIC range (e.g. PIC18) isn't usually so troublesome as long as the new device has the peripherals you need and they support the operating modes you have actually used.

[dannyf]

Because the macros are provided by the compiler development team, they can make sure that the access order for each macro is correct for the current compiler version. 

Yeah right.

e.g. the WRITETIMER0()  macro is currently quite complex because of this:

Or maybe not.

[hamster_nz]

unsigned int counts = 0;   //counts variable

counts = TMR0H*256+TMR0L; // load current TMR0 counter value

itoa(buffer,counts,10); // convert counts to ASCII string & place them in buffer array

// now write buffer to LCD

Isn't this code borked anyway?

There is a race condition in that TMR0H may change between when it is read and TMR0L is read, causing the count to be off by 256.

If TMR0H is read first, when the timer is 00:FF and then TMR0L is read when it is 01:00 then you end up with 00:00.

or If TMR0L is read first, when the timer is 00:FF and then TMR0H is read when it is 01:00 then you end up with 01:FF.

Read the high byte first, then the low byte, then the high byte again and check that it hasn't changed. If it has, then rinse and repeat.
     

Code: [Select]

unsigned short this_high, low, last_high;

this_high = TMR0H;
do {
   last_high = this_high;
   low = TMR0H;
   this_high = TMR0H;
} while (last_high != this_high);

count = (this_high << 8) | low;


[mikerj]

That's what the ReadTimer0() macro generates, a direct 16 bit read of TMR0.  If they ever reorder it, the macro will be updated to match.

Have you tried to tell Micorchip engineers that those macros they wrote violated your directive?

Although the XC8 headers include a definition for a 16 bit TMR0, It should NOT be used ...

You are trying to be a too clever, and failed.  There is a 16 bit TMR0 definition which is a literal 16 bit pointer to the TMR0 registers, so write ordering in this case is effectively undefined and could change with compiler revisions (though unlikely).  This is was Ian was referring to.

There is also a pair of macros that explicitly define the order of the reads and writes.

[dannyf]

Read the high byte first, then the low byte, then the high byte again and check that it hasn't changed. If it has, then rinse and repeat.

Only if the high byte is unbuffered. It is not necessary in this case as the high byte is buffered.

[Bruce Abbott]

or If TMR0L is read first, when the timer is 00:FF and then TMR0H is read when it is 01:00 then you end up with 01:FF.

No. TMR0H is not the timer high byte, it is a buffer. When you read TMR0L it copies the timer high byte to TMR0H. That is why you must read TMR0L first! (if you read TMR0H first it will have whatever it had on the previous write to TMR0H or read of TMR0 - whichever was the last operation).  Similarly you should write to TMR0H first, then writing to TMR0L updates both the high and low bytes together.

Note: not all PICs have this feature!

[dannyf]

Also your code can be simplified:

Code: [Select]

  do {
    msb = TMR0H;
    lsb = TMR0L;
   } while (msb == TMR0H);
  tmr0=(msb<<8) | lsb;


[westfw]

Although the XC8 headers include a definition for a 16 bit TMR0, It should NOT be used as you are relying on undefined behaviour, namely the order of byte reads or writes to a 16 bit volatile unsigned short.

Presumably the combination of the (Microchip-provided) compiler and the (Microchip-provided) access definition combine to provide "defined" behavior that you can complain about if it breaks.  Perhaps not very potable to other compilers, though, which is one of the reasons to use a HLL in the first place.

The compiler is pretty smart, isn't it?

Um.  WTF are those NOPs doing there, besides doubling the size of the code?
avr-gcc is disappointingly not smart enough to realize that it doesn't need to zero the low byte in "a = a<<8 + PORTB", nor does it recognize that it could INPUT the byte directly to the low register of (16bit) a.   Sigh.  Although it otherwise produces much better code for the shift case.

[Ian.M]

Although the XC8 headers include a definition for a 16 bit TMR0, It should NOT be used as you are relying on undefined behaviour, namely the order of byte reads or writes to a 16 bit volatile unsigned short.

Presumably the combination of the (Microchip-provided) compiler and the (Microchip-provided) access definition combine to provide "defined" behavior that you can complain about if it breaks.  Perhaps not very potable to other compilers, though, which is one of the reasons to use a HLL in the first place.

If you use the Microchip provided macros you get implementation defined behaviour - namely correctly ordered access to timers in 16 bit latched mode - that you can complain about if they break it.

If you use TMR0 (or the other 16 bit Timer word access SFR variables while the timer is in latched mode) directly, currently reads work but writes fail with the previous value of the latch being used.   You may not even notice if you are reloading it in an ISR with a constant, but I hnow of a few users doing more complex things with the timers who have already wasted days or weeks debugging it.   It doesn't help that the hardware debuggers use code running on the PIC so also interact with the latch, and the simulator has historically had fairly broken and untrustworthy timer handling.

[Bruce Abbott]

Getting slightly off topic because DTJ only wanted to read Timer0 in a much simpler way, but here is what WRITETIMER0(counts) becomes:-

 

Code: [Select]

TMR0H=counts>>8,TMR0L=counts&0xFF;

  3FA4    5006     MOVF 0x6, W, ACCESS
  3FA6    6ED7     MOVWF 0xfd7, ACCESS
  3FA8    C005     MOVFF 0x5, 0xfd6
Not quite as compact as handcrafted assembler, but pretty close!

[Ian.M]

What XC8 compiler version, mode and optimisation level produced that?

[Bruce Abbott]

XC8 V1.35 in 'free' mode, via MPLAB IDE V8.92