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.