Fix too small return type for `uxTaskGetStackHighWaterMark()` about amazon-freertos HOT 21 CLOSED

Head revision of the kernel now includes xTaskGetStackHighWaterMark2() - which is the same as xTaskGetStackHighWaterMark() other than the return type. https://sourceforge.net/p/freertos/code/HEAD/tree/trunk/FreeRTOS/Source/tasks.c

from amazon-freertos.

Currently this is 'just' in SVN - it is not in a formal release - so we can play around with the best method until such time as the next official release.

As it is today, the code is conditionally compiled, but using INCLUDE_uxTaskGetStackHighWaterMark2, so the user can have the old version only, the new version only, both versions together, or neither of the versions, depending on the INCLUDE_nnn settings.

I'm not keen on having an INCLUDE_nnn setting that can be anything other than 0 or 1 because it creates an exception from the [currently] simple rule of set to 1 to include, or set to 0 to exclude, although I have to admit the code you posted would be the simplest to code - especially the ability to have the >= 1 as you point out - so I have not discounted it as an option.

If the desire is to keep the function name the same for both versions of the function (i.e. not have another version with the '2' tacked onto the end, which I have to admit I did think about for a long time), then I make it such that if you define configSTACK_DEPTH_TYPE you get the new version, and if you leave configSTACK_DEPTH_TYPE undefined (so it defaults to uint16_t) then you get the old version. That would keep things backward compatible as you would have to take a deliberate action to change the behaviour (that is, define configSTACK_DEPTH_TYPE).

from amazon-freertos.

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.

from amazon-freertos.

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.

from amazon-freertos.

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.

from amazon-freertos.

@RichardBarry no problem. As you've noted there were two outcomes in the patch. They're both still valid.

1. The first was trying to consistently use the configuration value for ALL stack depth variables. For example, there are places where stack depth is cast to uint32_t so that it can be stored in a variable of that size, and other places where it is stored in a variable of configSTACK_DEPTH_TYPE size. This is really only for code prettiness, although, in the ATmega case, manipulating 32 bit variables where 8 bits would be sufficient is very inefficient. Your code typically uses configured variable sizes throughout, so this non-configurable stack size variable issue is a bit of an anomaly.

2. Secondly, converting the uxTaskGetStackHighWaterMark() function to return configSTACK_DEPTH_TYPE was the actual urgent fix needed. Previously, I'd wrapped the corrected function in the backwards compatibility defines, but finally for the v10.1.1-1 Arduino_FreeRTOS release I just removed the old broken function. Keeping a broken thing alive forever is well... end of story.

I hope they'll be considered useful.

from amazon-freertos.

Noted. Thanks.

But wouldn't it have made sense to do a conditional compile, rather than introduce a new function name?

#if ( INCLUDE_uxTaskGetStackHighWaterMark == 2 )
    configSTACK_DEPTH_TYPE uxTaskGetStackHighWaterMark( TaskHandle_t xTask );
#endif
#if ( INCLUDE_uxTaskGetStackHighWaterMark == 1 )
    UBaseType_t uxTaskGetStackHighWaterMark( TaskHandle_t xTask );
#endif

And then just change other conditional compilation defines from INCLUDE_uxTaskGetStackHighWaterMark == 1 to be >= 1.

from amazon-freertos.

[funny there is such a long thread over such a small thing :o) ]

from amazon-freertos.

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?

from amazon-freertos.

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.

from amazon-freertos.

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.

from amazon-freertos.

@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

from amazon-freertos.

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

from amazon-freertos.

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.

from amazon-freertos.

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.

from amazon-freertos.

@RichardBarry bump.
Thoughts?

from amazon-freertos.

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.

from amazon-freertos.

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.

from amazon-freertos.

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.

from amazon-freertos.

Thanks for the diff - as V10.1.1 of the kernel was just released without addressing your patch I wanted you to know that I understand this is causing a genuine issue for you, and as I recall from previous posts, the workaround of using the 'get system state' function is not suitable for you. At this time I think the 'get stack high water mark' function is the main issue as the other functions in the patch such as vPortStoreTaskMPUSettings() will not effect 16-bit MCUs, as the xTaskCreateStatic() doesn't suffer the same problem as it uses a 32-bit type (it is a much newer function). Anyway - I didn't want to use the backward compatibility constant if I can avoid it as that is a bit of a binary thing - it doesn't allow people to have all the latest updates but still have the longer return type when checking the stack size - that said - it may be the only options barring introducing a completely new API function.

from amazon-freertos.

@RichardBarry

[funny there is such a long thread over such a small thing :o) ]

Another example that obeys the Pareto principle. 😄

from amazon-freertos.