Task::delay is not public

[electretmike]

I have a class that inherits from Task. This works very well. Thank you for these nice wrappers! Except that this class calls methods in other classes, which need to delay the current task. Those classes don't have access to Task::delay, because it is protected. For me, it would help if it was public.
Is there any objection against that?

[jonenz]

I think the original idea here was to try and enforce the idea that any call to delay() should originate from the task's taskFunction().

This helps prevent someone from calling delay() from outside of a task loop. This is easy to get around, so I'm not sure if it's a good justification. For example:

void MyTask::somePublicFunction() {
  delay();
}

MyTask task;

void nonTaskFunction() {
  task.somePublicFunction();
}

Another way to look at this is that delay() doesn't specify the handle of the task being delayed, it just delays the current thread. I thought that might be confusing for an object oriented approach to tasks. For example:

class MyTask : public FreeRTOS::Task {
 public:
  void taskFunction() final { /* Do some work */};
};

class MyOtherTask : public FreeRTOS::Task {
 public:
  void taskFunction() final;
};

MyTask task1;

void MyOtherTask::taskFunction() {
  task1.delay(); // Which task is blocked here?
}

Here MyOtherTask is delayed even though you're calling delay() from an instance of MyTask. I thought this had the potential to be confusing since the implication would be that you're delaying the task instance which you call delay from.

By this same logic Task::notifyWait should also be protected.

[electretmike]

I understand. Indeed, delaying another task is confusing.

What about a different approach: create a new class this_task, like so:

class this_task {
  static void delay() {
     vTaskDelay()
  }
};

That way, the api makes it clearer thay you are delaying the current task, and there is no relation with a specific task.
The same could be used for yield, notifyWait and maybe others.

(admitted: this is inspired by https://github.com/IntergatedCircuits/freertos-mcpp/blob/master/include/freertos/thread.h#L443)

[jonenz]

That is similar to what I do for other functions that are part of the FreeRTOS::Kernel namespace. I'd consider moving delay out of the task class and into that namespace, but that would break the pattern a little bit.

This is a new library, so I'm not sure how people will use it yet. Are you wanting to use this functionality in many classes or just this one? Would it makes sense to declare your class a friend of your task class?

[electretmike]

I am using the delay in a device driver that is to be used by multiple applications. So making that class a friend won't work, as the class is in a separate library.

Moving delay to FreeRTOS::Kernel would work. I am not sure on how nice that solution is.

But I also just starting using your library, so I am still looking for the best way to use it.

[jonenz]

My initial thought/question would be why does your device driver need to delay the task? If someone else were to use the class would it unexpectedly be delaying the thread?

A high-level guideline I would give for the library would be only delay the task from the task class (which is why it's protected). Construct the driver interface in way that doesn't "hide" thread delays from the application.

If that doesn't work, I'd suggest making your driver it's own FreeRTOS::Task, and provide a public API that uses notifications, queues, etc... so the application task can communicate with the driver task.

[electretmike]

I'm writing a library for my own use, not a public library. For me, the design of drivers or other library classes delaying the task work. I just make sure to document it. My MCU doesn't have enough memory to have many tasks.

I can always add my own 'ThisTask' class ofcourse, if that doesn't fit in your design.

[jonenz]

I would say do that for now. If it's just a static method I would throw it in a namespace. Something like:
TaskControl.hpp

namespace FreeRTOS {
namespace TaskControl {
  inline static void delay(const TickType_t ticksToDelay = 0) {
    vTaskDelay(ticksToDelay);
  }
} // namespace TaskControl
} // namespace FreeRTOS

The design is intended to stop you from doing exactly what you're trying to do, so I don't want to change it yet. If others also find this too restrictive then I'll consider changing it in the future.

[lukasnee]

@jonenz @electretmike , hey guys, I find this topic relevant and really important. I want to share my take.

I think it is good to imply good usage pattern but it should not be forced. Why should a FreeRTOS C++ wrapper restrict me from using FreeRTOS the way it can be used with the original C API? I think this is outside wrapper's responsibility to do so, otherwise it is more than just a wrapper. I think it is a design choice of the FreeRTOS itself that delay does not require a task handle and it implies a delay for the current task. Anyone who uses FreeRTOS already knows it. I think making delay() a static method of FreeRTOS::Task is what a wrapper should do.

Moreover - why can't I use delay() outside class members that inherits FreeRTOS::Task but I am free to create FreeRTOS::Queue, FreeRTOS::Semaphore instances anywhere and use them in any scope? Doesn't that break the integrity of the design idea? I also found it strange that delayUntil is tied to a FreeRTOS::Task and its state (delayUntil() method uses previousWakeTime member variable that is private to TaskBase). I feel that "delay until" should actually be a separate class.

Also, if impact and reach of this project is of any concern, I think that restrictiveness of methods like delay() and delayUntil() will discourage many users from using the library. Especially to ones that are not experienced RTOS developers. I find it awkward that I have to think so much to use a simple delay in my code. I think that this library shouldn't require you to significantly refactor and redesign your application if you decide to switch away from original FreeRTOS C API and use these wrappers.

However, don't get me wrong, @jonenz idea to make a safer FreeRTOS API is interesting. Maybe it should be a separate optional layer that goes on top of a basic wrapper? Just an idea.

[jonenz]

@lukasnee sorry for the delay on getting to this.

As far as the structure of the Task base class design vs Queue, Semaphore, etc.. I just thought it would be cleaner to implement the tasks by inheriting the base class and filling their class with specific task functionality. For the other containers, usually all they need is a template parameter on instantiation and they're good to go, but they could be inherited also if you had a reason to.

I understand that delay as protected function is restrictive and annoying for porting existing code. When I was creating this I was thinking about new projects and it seemed like a good way to force the task code to be more contained/safer. Since multiple users have this problem, I'll change delay from a protected member function to a public static function so it can be used more freely. I can also add a static option for delayUntil.

[jonenz]

See #20