View Issue Details

IDProjectCategoryView StatusLast Update
0034721FPCCompilerpublic2018-12-28 09:38
ReporterChristo CrauseAssigned ToFlorian 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version3.3.1Product Build40588 
Target VersionFixed in Version3.3.1 
Summary0034721: AVR - 16 bit timer registers needs to be written high byte first
DescriptionWhen writing a 16 bit value to 16 bit registers of 16 bit timers the high byte should be written first, then the low byte. Currently the compiler only generate the write sequence low byte, high byte.

If not, incorrect values will end up being stored in the high byte location of the register.

The attached patch is an attempt to reverse the order of the writes to high byte first followed by low byte for unsigned 16 bit values.
Steps To ReproduceThe following assignment to ICR1:

  ICR1 := 7812;

Gets compiled to:
  ldi r18, 0x84
  ldi r19, 0x1E
  sts 0x0086, r18
  sts 0x0087, r19

After completion of this assignment inspection reveals that ICR1L contains 0x84 and ICR1H contains 0.
Additional InformationSee e.g. the atmega328p manual, 0000015.3 Accessing 16 bit registers:
"Accessing the low byte triggers the 16-bit read or write operation. When the low byte of a 16-bit register is written by the CPU, the high byte stored in the temporary register, and the low byte written are both copied into the 16-bit register in the same clock cycle."
TagsNo tags attached.
Fixed in Revision40678
FPCOldBugId
FPCTarget
Attached Files
  • cgcpu.pas.patch (2,223 bytes)
    Index: compiler/avr/cgcpu.pas
    ===================================================================
    --- compiler/avr/cgcpu.pas	(revision 40588)
    +++ compiler/avr/cgcpu.pas	(working copy)
    @@ -1345,21 +1345,36 @@
                end;
              if not conv_done then
                begin
    -             for i:=1 to tcgsize2size[fromsize] do
    +             // Write to 16 bit ioreg, first high byte then low byte
    +             // sequence required for 16 bit timer registers
    +             // See e.g. atmega328p manual para 15.3 Accessing 16 bit registers
    +             if (fromsize = OS_16) and QuickRef then
                    begin
    -                   if not(QuickRef) and (i<tcgsize2size[fromsize]) then
    -                     href.addressmode:=AM_POSTINCREMENT
    -                   else
    -                     href.addressmode:=AM_UNCHANGED;
    -
    +                 tmpreg:=GetNextReg(reg);
    +                 href.addressmode:=AM_UNCHANGED;
    +                 inc(href.offset);
    +                 list.concat(taicpu.op_ref_reg(GetStore(href),href,tmpreg));
    +                 dec(href.offset);
                      list.concat(taicpu.op_ref_reg(GetStore(href),href,reg));
    +               end
    +             else
    +               begin
    +                 for i:=1 to tcgsize2size[fromsize] do
    +                   begin
    +                       if not(QuickRef) and (i<tcgsize2size[fromsize]) then
    +                         href.addressmode:=AM_POSTINCREMENT
    +                       else
    +                         href.addressmode:=AM_UNCHANGED;
     
    -                 if QuickRef then
    -                   inc(href.offset);
    +                     list.concat(taicpu.op_ref_reg(GetStore(href),href,reg));
     
    -                 { check if we are not in the last iteration to avoid an internalerror in GetNextReg }
    -                 if i<tcgsize2size[fromsize] then
    -                   reg:=GetNextReg(reg);
    +                     if QuickRef then
    +                       inc(href.offset);
    +
    +                     { check if we are not in the last iteration to avoid an internalerror in GetNextReg }
    +                     if i<tcgsize2size[fromsize] then
    +                       reg:=GetNextReg(reg);
    +                   end;
                    end;
                end;
     
    
    cgcpu.pas.patch (2,223 bytes)

Activities

Christo Crause

2018-12-18 21:57

reporter  

cgcpu.pas.patch (2,223 bytes)
Index: compiler/avr/cgcpu.pas
===================================================================
--- compiler/avr/cgcpu.pas	(revision 40588)
+++ compiler/avr/cgcpu.pas	(working copy)
@@ -1345,21 +1345,36 @@
            end;
          if not conv_done then
            begin
-             for i:=1 to tcgsize2size[fromsize] do
+             // Write to 16 bit ioreg, first high byte then low byte
+             // sequence required for 16 bit timer registers
+             // See e.g. atmega328p manual para 15.3 Accessing 16 bit registers
+             if (fromsize = OS_16) and QuickRef then
                begin
-                   if not(QuickRef) and (i<tcgsize2size[fromsize]) then
-                     href.addressmode:=AM_POSTINCREMENT
-                   else
-                     href.addressmode:=AM_UNCHANGED;
-
+                 tmpreg:=GetNextReg(reg);
+                 href.addressmode:=AM_UNCHANGED;
+                 inc(href.offset);
+                 list.concat(taicpu.op_ref_reg(GetStore(href),href,tmpreg));
+                 dec(href.offset);
                  list.concat(taicpu.op_ref_reg(GetStore(href),href,reg));
+               end
+             else
+               begin
+                 for i:=1 to tcgsize2size[fromsize] do
+                   begin
+                       if not(QuickRef) and (i<tcgsize2size[fromsize]) then
+                         href.addressmode:=AM_POSTINCREMENT
+                       else
+                         href.addressmode:=AM_UNCHANGED;
 
-                 if QuickRef then
-                   inc(href.offset);
+                     list.concat(taicpu.op_ref_reg(GetStore(href),href,reg));
 
-                 { check if we are not in the last iteration to avoid an internalerror in GetNextReg }
-                 if i<tcgsize2size[fromsize] then
-                   reg:=GetNextReg(reg);
+                     if QuickRef then
+                       inc(href.offset);
+
+                     { check if we are not in the last iteration to avoid an internalerror in GetNextReg }
+                     if i<tcgsize2size[fromsize] then
+                       reg:=GetNextReg(reg);
+                   end;
                end;
            end;
 
cgcpu.pas.patch (2,223 bytes)

Christo Crause

2018-12-19 06:23

reporter   ~0112684

The first patch is an over simplified version which also affects other operations - so it requires more attention.

Florian

2018-12-27 22:43

administrator   ~0112923

Thanks, applied the version you posted on the mailing list.

Christo Crause

2018-12-28 09:38

reporter   ~0112939

Thanks.

Issue History

Date Modified Username Field Change
2018-12-18 21:57 Christo Crause New Issue
2018-12-18 21:57 Christo Crause File Added: cgcpu.pas.patch
2018-12-19 06:23 Christo Crause Note Added: 0112684
2018-12-27 22:43 Florian Fixed in Revision => 40678
2018-12-27 22:43 Florian Note Added: 0112923
2018-12-27 22:43 Florian Status new => resolved
2018-12-27 22:43 Florian Fixed in Version => 3.3.1
2018-12-27 22:43 Florian Resolution open => fixed
2018-12-27 22:43 Florian Assigned To => Florian
2018-12-28 09:38 Christo Crause Note Added: 0112939
2018-12-28 09:38 Christo Crause Status resolved => closed