[Proposition] Give user task pointer instead of ID

[Miangie]

ID require you to search through the list to find corresponding task. I propose to replace ID with pointer to Task struct which will remove unnecessary lookup.

If you want to prevent user from messing with internals, I also propose to declare structs in separate header and typedef them as void in public header. This way from user point it's only a void pointer and he cannot change internal stuff whereas HeliOS functions will be able to do so.

[MannyPeterson]

I think there are definite advantages to doing that. To your point, it eliminates the need to scan the TaskList to obtain the pointer from the ID. Can you give me an example here (in the issue) of how the change to the header would look? Just one example is sufficient. Thank you.

[Miangie]

Sure,
first off I would divide header into two: public and internal

definitions in public header:
typedef void Task;
ideally wrapped in #ifndef HELIOS_INTERNAL

and in internal header:
typedef struct Task {...};

now user writing Task *task will get in reality void *task whereas we got whole struct

[Miangie]

also, take a note all internal files will have to include helios_internal.h, which also includes helios.h but after defining HELIOS_INTERNAL so we won't get re-definition errors

[MannyPeterson]

Before proceeding with this change, I would like to do the following:

1. One advantage of using a task handle like ID is that function calls like xTaskGetInfo(int) can validate the existence of the task before taking any action by scanning the TaskList for the ID with TaskListSeek(int). I understand that it is impossible to completely protect the programmer from him/herself, but what risk does this introduce (if any).

2. I would like to revisit how FreeRTOS implements "task handles". Not because I want to copy everything FreeRTOS does, but just use it as a point of reference on how other embedded OSs implement task handles. As shown in other function calls like xTaskGetInfo(int), I am not opposed to passing pointers out into user land. However, I am trying to be principled in the approach used (passing copies of internal structures out to user land instead of pointers to the real internal structures). I think controlling visibility to the structure's members with "typedef void Task" would help mitigate some or all of that risk.

3. My last wish may not be possible yet because the project may not have enough critical mass. But, I would like to make a habit out of leaving changes to the API open for enough time to allow others to comment. My hope is for HeliOS to be more about the community of users and contributors than "my project".

Please excuse anything I have worded poorly. I had limited time to review my thoughts. :)

[Miangie]

1. As you said, it's impossible to protect programmer from himself. Let's say someone would create a void* ptr and pass it TaskListSeek((Task*)ptr). Cast is needed, it's easy to spot and compiler will most likely complain if you don't do it explicitly.
   Both methods have some drawbacks.
   IDs are slow, passing wrong value (i.e. typo) is hard to spot but it's safe so without logs you wouldn't even see why your program does nothing.
   Now opaque pointers are really fast, have explicit type requirements (You won't pass Message* to TaskX() without a cast) and if you pass wrong pointer program will most likely segfault. And that's a good thing actually, bug that fail so hard will require to fix it right away which makes code using this API more prone to user errors.
   So it depends which risk is worse to you, easy to do bug or segfault if someone really want's it

2. Copying is slow. Although it's easier to give whole struct and use that in userspace, consider functions like TaskInfoGetX(Task*). It can return value user specifically want without copying entire struct or letting user potentially mess with internals

I looked up FreeRTOS code, it's also defining something like TaskHandle_t as tskTaskControlBlock *. What I mean is we can define TaskHandle as Task * so user won't even get a chance to forgot * in variable declaration. I believe Win32 API does something similar but not sure about that one

EDIT: Forgot one line in ID vs ptr

[MannyPeterson]

I agree with everything you've said and I think we should proceed with your proposition. I think in the future I will want to give API changes time to get feedback from the community, but given my earlier point about "critical mass" - I just don't think the project is there yet. So in the meantime, let's not unnecessarily delay improving the code/API.

Would you plan on submitting a pull request for this? If so, please make sure you compare/rebase to develop. I am trying to run weekly sprints with a release on or around Saturday (U.S. Central Time). This week I plan to build a few more example Arduino sketches and I can update the existing ones for the changes to the API. Let me know your thoughts. And as always, I appreciate your contributions.

[Miangie]

I tried already but task.c and list.c both need to be de facto rewritten with this changes in mind and I simply don't have so much time this week. If you'll write base yourself I could join later on expanding it, just remember to commit it even unfinished

[MannyPeterson]

Let's do this. I have already committed to getting additional example Arduino sketches out. I will finish that work up and get it ready to release this weekend. I will then start digging in on refactoring the task handle as you proposed. It might take a week or two but I can get started. i will commit as I go. Thanks!

[MannyPeterson]

@Konidem you’ll notice in 0.2.5 now uses TaskId_t for the task handle. Not quite a pointer yet but headed in the right direction!

[JakubRakus]

@MannyPeterson Any progress on this?

[MannyPeterson]

@JakubRakus there is no more WAITINGTASKSIZE limitations. The scheduler now has private list pointers.

[MannyPeterson]

@Miangie @Miangie your wishes have been answered. Enjoy 0.3.0. :)