Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TaskScheduler tasks with limited lifespans #597

Draft
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

schlimmchen
Copy link
Member

@tbnobody This might have information that could be helpful to the upstream project and I would appreciate your opinion on this matter.

This is an attempt to use TaskScheduler tasks with a particular interval. In general, this works. In this case, the BatteryClass MQTT publish task works as expected: It is executed in the expected interval.

I would really like to extend this approach to all classes that need to execute work regularly. In particular, this would allow for simplifications based on the knowledge that a particular function is executed at a particular interval. See the changes in the JkBms::Controller class, where a sender and receiver task are supposed to be used and where the receiver task is only enabled when needed, allowing for a better overall approach to tackle some of the problems that arise if the loop task was executed with high frequency.

Unfortunately, the JkBms::Controller tasks don't even execute once. I don't yet understand why that is. The difference here is that the task instances are constructed "dynamically", i.e., once the main loop is executing. The idea is that only the specific battery provider that is needed is constructed at all, and some may need no task, another needs one, another needs two.

What I do understand is that even though I made @tbnobody define _TASK_THREAD_SAFE for the TaskScheduler library, the whole library is really not at all thread-safe. This is something that I realized after I actually looked at the code. In particular, adding a task to the scheduler while a scheduler pass is active is simply unsupported. Adding does not lead to serious problems, I guess, but deleting a task whose instance then is freed definitely does: There is no guarantee whatsoever that TaskScheduler does not try to load/execute a task in the chain that was previously deleted. If the task was also freed, this leads to a memory access violation:

0x420418c7: Scheduler::execute() at /home/schlimmchen/Documents/OpenDTU-OnBattery/.pio/libdeps/opendtufusionv2/TaskScheduler/src/TaskScheduler.h:1401
 (inlined by) Scheduler::execute() at /home/schlimmchen/Documents/OpenDTU-OnBattery/.pio/libdeps/opendtufusionv2/TaskScheduler/src/TaskScheduler.h:1339
0x42041972: loop() at /home/schlimmchen/Documents/OpenDTU-OnBattery/src/main.cpp:191
0x42069ee5: loopTask(void*) at /home/schlimmchen/.platformio/packages/framework-arduinoespressif32/cores/esp32/main.cpp:50

I am confident that the whole TaskScheduler is not made to handle task objects with limited lifespan. And that is very disappointing.

Ideas:

  1. Find or create and maintain a fork of TaskScheduler that actually is thread-safe and which does support tasks with limited lifetimes.
  2. Create some sort of shim, like "DynamicTaskScheduler", for OpenDTU(-OnBattery) which allows class instances to die freely that somehow use a task, and which performs cleanup (removing the task from the TaskScheduler chain) outside of the scheduler pass once the task is no longer needed. This could use some sort of reference counting, like attaching the task instance to a std::shared_ptr and once there is only once reference (in the DynamicTaskScheduler), the task ist deleted from the chain and is freed.
  3. Create static instances of a task.

Assessment:

  1. I would love to go this route. The TaskScheduler has a lot of potential, but it needs some more love. Real thread-safety and allowing tasks to die would unlock some of the potential. However, I am unsure if it is doable with a reasonable amount of work and whether or not @helgeerbe and/or @tbnobody would even have interest in those features.
  2. This sounds doable, but also sounds like a lot of overhead.
  3. Not really an option. At least not an attractive one. Dying classes that use static tasks still need to make sure to disable the task in their constructor. They also would need to set a new static do-nothing callback method, simply because they do not know whether or not the task will be executed once more or not. At least not if the respective class instance dies in the context of another thread, like when a web app request changes the settings of the Battery interface.

Thank you very much if you made it through this wall of text 🙈 @helgeerbe What's your feeling about this? The changeset of this PR might give you an idea of the potential that I am talking about.

@schlimmchen schlimmchen force-pushed the development branch 2 times, most recently from 91cc2fc to 8ff94e7 Compare September 16, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant