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
MOVFFPOSTINC0, PREINC1
to save the first 19 bytes (at adresses 0 to 18), and then executes an additional
MOVFFINDF0, 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

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.

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

What hellish compiler is this?

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

Paul