Superfluous instruction in MPLAB port

Kernel
1

nobody wrote on Tuesday, July 18, 2006:

Hi,

I studied the context save and restore routines and noticed that 1 byte too much is saved for the MATH_DATA and .tmpdata sections. Also the stack initialisation is 1 byte too long.

If portCOMPILER_MANAGED_MEMORY_SIZE is 0x13 as in the default/demo, 19 bytes should be saved.
(I assume MATH_DATA plus .tmpdata sections are 19 bytes long)
The initialisation loop runs 20 times however (from 0 to (and including) 19).
The save_context routine uses
MOVFF    POSTINC0, PREINC1
to save the first 19 bytes (at adresses 0 to 18), and then executes an additional
MOVFF    INDF0, PREINC1
to store adress 19 without incrementing the FSR0 register.

This is potentially harmful because the byte at adress 19 could be a global variable which is supposed to be changed by an other task, but restoring the old task context overwrites the new value.

Fix: if MATH_DATA plus tmpdata sections are 19 bytes long, documentation should state clearly that 18 MOVFF POSTINC0,PREINC1 and
1 MOVFF INDF0,PREINC1 instructions are required to save the sections.
The stack initialisation ‘for loop’ should also be changed to loop one time less or documentation should state that portCOMPILER_MANAGED_MEMORY_SIZE should be initialised 1 less.

Paul

2

rtel wrote on Tuesday, July 18, 2006:

It seems that everything *in the code* is consistent.  When the stack is created one more byte than portCOMPILER_MANAGED_MEMORY_SIZE is placed on the initial stack.  When the context is saved and restored the same is true.  The comment above the save context macro states that this is what is going to happen "This macro stores from data address 0x00 to portCOMPILER_MANAGED_MEMORY_SIZE" (so one more byte is saved).  Provided you are aware of the way the constant is used then I think it is ok.

It seems the inconsistent and problematic thing is the documentation, which does less than make you aware of how the constant is used.

It is very unusual (in that I thought never) for my coding style to have a <= in a loop in this manner.  I think this stems from the original code which used a loop in the asm macro to save the global memory areas, and this was the most code space efficient way to do it.  Thinking this was too time inefficient I unrolled to loop creating a long list of push statements instead.

I just tried rebuilding v3.2.4 using my current installation of MPLAB and the first thing I note is that there are compiler warnings generated.  This must be due to the new MPLAB version.  The second thing I note is that the global memory size is 22.  The apparent size change from when the port was originally produced makes it hard to asses which is actually correct, the documentation or the code.

Further there is one byte in the vSerialTxISR_tmp and one byte in the vSerialRxISR_tmp sections that look suspicious.

An embedded compiler that silently does not produce reentrant code would seem to be an oddity.

Regards.

3

nobody wrote on Tuesday, July 18, 2006:

Hi,

I agree that the code is consistent and problem is mainly in the documentation.
About the comment above the save context macro: in dutch (my mother tongue) there is a very explicit difference between "to" and "to and including", which makes the comment (at least for some foreigners) ambiguous.

I compiled rtosdemo3 from 3.2.4. I got sizes 0x10 for math_data and 0x06 for tmpdata (usually this one is 0x08 for me, so it is also variable). I guess you got the same. I also got some unimportant warnings about context saves which are compiler managed nowadays. No errors.
The section at address 0x0016 (vSerialTxISR_tmp) is in danger of being overwritten. It consists of a variable named: __tmp_2 which is apparently used in serial.c (according to the map file)

The dissassembly listing reveales that this address is used to temporarily store the return value of the function in line:
215:                   if( xQueueReceiveFromISR( xCharsForTx, &cChar, &cTaskWoken ) == pdTRUE )

disassembles to:

  376C    50D9     MOVF 0xfd9, W, ACCESS
  376E    0F01     ADDLW 0x1
  3770    6EE6     MOVWF 0xfe6, ACCESS
  3772    0E00     MOVLW 0
  3774    20DA     ADDWFC 0xfda, W, ACCESS
  3776    6EE6     MOVWF 0xfe6, ACCESS
  3778    CFD9     MOVFF 0xfd9, 0xfe6
  377A    FFE6     NOP
  377C    CFDA     MOVFF 0xfda, 0xfe6
  377E    FFE6     NOP
  3780    C578     MOVFF 0x578, 0xfe6
  3782    FFE6     NOP
  3784    C579     MOVFF 0x579, 0xfe6
  3786    FFE6     NOP
  3788    ECB8     CALL 0x1f70, 0            --> call the function, return value will be in WREG
  378A    F00F     NOP
  378C    6E16     MOVWF 0x16, ACCESS            --> temporarily store the return value
  378E    0E06     MOVLW 0x6                --> Do something (I don’t know what)
  3790    5CE1     SUBWF 0xfe1, W, ACCESS
  3792    E202     BC 0x3798
  3794    6AE1     CLRF 0xfe1, ACCESS
  3796    52E5     MOVF 0xfe5, F, ACCESS
  3798    6EE1     MOVWF 0xfe1, ACCESS
  379A    5016     MOVF 0x16, W, ACCESS            --> copy back to WREG
  379C    0801     SUBLW 0x1                --> compare to ‘1’ (=true)
  379E    E103     BNZ 0x37a6                --> skip if not zero

There is a 7 instruction window where things can go wrong. This is obviously no problem here since it is inside an interrupt function, but if the same thing happens during normal program flow…

In my program it is a global variable DelayCounter1 which is used by the library delay routines. I disable interrupts before calling the delay (I wouldn’t want to risk a task switch there) but if I didn’t…

I don’t think the microchip library delay functions are reentrant, since they use 1 global variable. Multiple simultaneous calls from diffent tasks will certainly mess things up there.

Paul

4

nobody wrote on Tuesday, July 18, 2006:

What hellish compiler is this?

5

nobody wrote on Wednesday, July 19, 2006:

The microchip C18 compiler, with optimisations set to ‘debug’, it is not meant to be optimal.

Paul