Fix too small return type for `uxTaskGetStackHighWaterMark()`

[feilipu]

Issue #27 raised by @Floessie.
uxTaskGetStackHighWaterMark() returns UBaseType_t which for AVR is set to uint8_t.

Repaired with a backward compatibility option in Arduino_FreeRTOS_Library Release 10.0.0-7.
Some additional fixes (for MPU, not relevant for Arduino) here.

[RichardBarry]

I don't think changing to a uint16_t is the right thing to do, as in V10 the type used to specify a stack size is user definable using the configSTACK_DEPTH_TYPE constant (which defaults to its old value if left undefined to ensure backward compatibility).

In my local copy prvTaskCheckFreeStackSpace() has been updated to return configSTACK_DEPTH_TYPE in place of uint16_t, but this has not been tested or checked in yet. prvTaskCheckFreeStackSpace() is called by uxTaskGetStackHighWaterMark(), but changing uxTaskGetStackHighWaterMark() itself has wider implications and the impact needs to be studied to see if it effects any kernel plug-ins that call the function.

As it is, on an 8-bit device, you would need a high water mark above 255 bytes to have a problem, which is quite a lot on a tiny device. Perhaps simply reducing the size of the stack allocated to the task that is giving an issue would be a solution for you?

[feilipu]

Yes, I've noted that configSTACK_DEPTH_TYPE is your new preferred way to do this.

With ATmega2560, using external RAM, or ATmega1284p (16kB RAM) devices there are no problems with larger stack sizes, so it would make sense to have your generalised solution working for AVR ATmega too.

@Floessie and I prefer to await your tested patch version to appear.

[RichardBarry]

Perhaps having uxTaskGetStackHighWaterMark() also return configSTACK_DEPTH_TYPE would be a solution as backward compatibility could be maintained by having it set to the same value as UBaseType_t - but really that would just move the problem to a different set of users rather than fix it.

I understand you can have a larger stack with external memory - but the reducing the stack such that the high water mark is at or below 255 is still a valid work around for you if this issue is a blocker. Note this has been open here https://sourceforge.net/p/freertos/bugs/120/ for a long time, which means there is no easy solution unless we just break backward compatibility and change the return type.

[Floessie]

@RichardBarry

Note this has been open here https://sourceforge.net/p/freertos/bugs/120/ for a long time, which means there is no easy solution unless we just break backward compatibility and change the return type.

Richard, I absolutely understand your concerns.

I understand you can have a larger stack with external memory - but the reducing the stack such that the high water mark is at or below 255 is still a valid work around for you if this issue is a blocker.

Hm, how am I supposed to test for the 255? My naive approach would be to generously provide let's say 1k (of my 16k) and let the program run. uxTaskGetStackHighWaterMark() would truncate the uint16_t returned by prvTaskCheckFreeStackSpace() to eight bits, which will most likely yield a number between 0 and 254, and I'll be unable to determine, if I'm overprovisioning by uxTaskGetStackHighWaterMark() or by x * 256 + uxTaskGetStackHighWaterMark(). It'll take serveral iterations to test, if I'm in the right ballpark...

Best,
Flössie

[Floessie]

After thinking a bit about it, here are two maybe viable solutions:

1. Do it like other OSes and add a corrected version of uxTaskGetStackHighWaterMark() which returns configSTACK_DEPTH_TYPE. Make the old uxTaskGetStackHighWaterMark() call the new one and truncate the result.

• Pros:
  • The only "correct" solution
  • Backward compatible
• Cons:
  • Code size increase (might be undone by LTO)

2. Make the return value clip instead of truncating it. Something like uxReturn = std::min(std::numeric_limits<UBaseType_t>::max(), prvTaskCheckFreeStackSpace(pucEndOfStack)) in C++ (I'm not a C expert...).

• Pros:
  • Usable solution
  • Backward compatible with minimal code size increase
• Cons:
  • Still requires some iterations to find the optimal stack size

Best,
Flössie

[feilipu]

Just thinking about this too. At least for us ATmega people, we could use the existing compatibility define (that I have turned off for the Arduino_FreeRTOS repo) to conditionally compile. With the corrected version being the default, being my preference. See below comment reference.

[RichardBarry]

Hmm, I think there are two viable options there then, Floessie's first suggestion of basically having two functions being wrapped, and the second is to use configENABLE_BACKWARD_COMPATIBILITY.

It is my preference to make a change (whatever that ends up being) as the smaller data type is really a hang over from very early versions of FreeRTOS, but I'm sure you understand the need to ensure we don't break our users code too. I've already changed the internal function called by uxTaskGetStackWaterMark(), it is just the public API function we need to consider the best approach to changing.

[feilipu]

I've implemented the changes in Arduino_FreeRTOS_Library with a compatibility option via configENABLE_BACKWARD_COMPATIBILITY.

This shows the diff in task.h, tasks.c, and timers.c.
Please, ignore the noise from port.c and friends.

Also some additional mpu related changes in mpu_prototypes.h and mpu_wrappers.c that aren't in the Arduino_FreeRTOS_Library repository.

Edited top comment to reflect this situation.

[feilipu]

@RichardBarry bump.
Thoughts?

[RichardBarry]

A change (made at the time of the original post in this thread) was pushed to the FreeRTOS kernel's public SVN repo just today - but I've not changed the public API yet.

Edit: Should have said the change allows the with of the variable used to hold the high water mark to be set when calling the vTaskGetInfo() function.

[feilipu]

I'll keep the change to your public API wrapped by configENABLE_BACKWARD_COMPATIBILITY in the Arduino_FreeRTOS_Library Pull #29 repository. I prefer correctness over legacy, and I'm prepared to keep the diff straight as you update in the public release.

I think this can be closed now.

[feilipu]

New 10.1.0 didn't consequently resolve this issue, so the fixes have been retained in Arduino_FreeRTOS_Library.

I'll submit a PR when amazon-freertos moves to 10.1.0.

[RichardBarry]

The kernel version 10.1.0 broke Segger's FreeRTOS kernel aware plug-in, which I am just looking into, so there is likely to be a V10.1.1 patch release soon to fix that, and into which we could potentially resolve your issue too - so please let me know asap what else needs to change for your issue to be resolved.

[feilipu]

The v10.1.0 has modified the ulStackDepth type to be configSTACK_DEPTH_TYPE in some places, but not all. Also sometimes it was cast to uint32_t, but in other places it was not cast. It should be internally consistent.

This ArduinoFreeRTOSv10.1.0 diff attempts to capture all references to stack depth and pointers to stack depth, and converts them to type configSTACK_DEPTH_TYPE.

But, to the heart of the issue. The function uxTaskGetStackHighWaterMark() should be defined to return a configSTACK_DEPTH_TYPE value. It currently returns UBaseType_t which for 8-bit devices is the real problem. The diff leaves a backwards compatible version, but provides an improved version with the correct return type.

Thanks to @Floessie for identifying the root problem, and the first PR.