arkhipenko / TaskScheduler

Cooperative multitasking for Arduino, ESPx, STM32, nRF and other microcontrollers
http://playground.arduino.cc/Code/TaskScheduler
BSD 3-Clause "New" or "Revised" License
1.33k stars 243 forks source link

C++ compiler warning: compound assignment with 'volatile'-qualified left operand is deprecated #188

Closed GitMoDu closed 5 months ago

GitMoDu commented 5 months ago

C++20 deprecated compound assignment on volatiles. As Arduino cores update their C++ version, this warning will be emited on every compilation.

The Raspberry PI core already has addressed this issue: https://github.com/raspberrypi/pico-sdk/issues/1017

Current compilation warning on ESP32 latest core.

taskscheduler.h: In member function void Task::reset() taskscheduler.h: 521:24: warning: using value of simple assignment with 'volatile'-qualified left operand is deprecated [-Wvolatile] 521 | iInterval = iDelay = 0 taskscheduler.h: In member function void Task::set(long unsigned int, long int) taskscheduler.h: 597:34: warning: using value of simple assignment with 'volatile'-qualified left operand is deprecated [-Wvolatile] 597 | iSetIterations = iIterations = aIterations taskscheduler.h: In member function void Task::setIterations(long int) taskscheduler.h: 613:34: warning: using value of simple assignment with 'volatile'-qualified left operand is deprecated [-Wvolatile] 613 | iSetIterations = iIterations = aIterations taskscheduler.h: In member function bool Task::enable() taskscheduler.h: 703:59: warning: using value of simple assignment with 'volatile'-qualified left operand is deprecated [-Wvolatile] 703 | iPreviousMillis = _TASK_TIME_FUNCTION() - (iDelay = iInterval) taskscheduler.h: In member function void Task::adjust(long int) taskscheduler.h: 833:23: warning: compound assignment with 'volatile'-qualified left operand is deprecated [-Wvolatile] 833 | iPreviousMillis += aInterval taskscheduler.h: 836:14: warning: compound assignment with 'volatile'-qualified left operand is deprecated [-Wvolatile] 836 | iDelay += aInterval; \ we have to adjust delay because adjusting iPreviousMillis might push taskscheduler.h: In member function void Task::forceNextIteration() taskscheduler.h: 854:55: warning: using value of simple assignment with 'volatile'-qualified left operand is deprecated [-Wvolatile] taskscheduler.h: In member function bool Scheduler::execute() taskscheduler.h: 1387:60: warning: '--' expression of 'volatile'-qualified type is deprecated [-Wvolatile] 1387 | if ( iCurrent->iIterations > 0 ) iCurrent->iIterations--; \ do not decrement (-1) being a signal of never-ending task taskscheduler.h: 1410:43: warning: compound assignment with 'volatile'-qualified left operand is deprecated [-Wvolatile] 1410 | iCurrent->iPreviousMillis += iCurrent->iDelay

Edit: posted warning message.

You can safely ignore the warning for now, but eventually, such operations will be deprecated in C++.

TD-er commented 5 months ago

You should now use std::atomic<int> instead of volatile int.

GitMoDu commented 5 months ago

The typical migration path I've seen is about re-structuring the operations, not to add the std:: dependency.

volatile a;
a += b;

turns into

volatile a;
a = a + b;
TD-er commented 5 months ago

See also discussions like these: https://stackoverflow.com/questions/59223814/why-is-volatile-deprecated-in-c20

vortigont commented 5 months ago

think it would be easier to just remove volatile at all from this lib. Any way this poor-man's locks does not work as expected on, i.e. esp32 when concurrently adding/destructing task objects from Scheduler in a non-thread-safe manner because working with linked-list operations here is also not thread-safe.

arkhipenko commented 5 months ago

think it would be easier to just remove volatile at all from this lib. Any way this poor-man's locks does not work as expected on, i.e. esp32 when concurrently adding/destructing task objects from Scheduler in a non-thread-safe manner because working with linked-list operations here is also not thread-safe.

The whole point of the library is to be single-threaded cooperative multi-tasking. If you are multi-threading then RTOS is the answer.

arkhipenko commented 5 months ago

Am I also reading it correctly that "This has been de-deprecated in C++23"? So nothing to do really?

arkhipenko commented 5 months ago

v3.8.5: (testing branch) 2024-06-17:

updated volatile compound statements after C++20 deprecated compound assignment on volatiles

TD-er commented 5 months ago

Am I also reading it correctly that "This has been de-deprecated in C++23"? So nothing to do really?

Hmm that's new to me... Got to read a bit more about this then as I wonder what the reasoning is (apart from "causing lots of work on older libraries/code") @arkhipenko Do you have some links?

vortigont commented 5 months ago

The whole point of the library is to be single-threaded cooperative multi-tasking. If you are multi-threading then RTOS is the answer.

true, scheduling and executing is single-threaded here, but other operations, like adding/removing/changing Tasks could be done from other threads, which is unsafe and _TASK_THREAD_SAFE does not always works as expected but only adds confusion. This is what I wanted to say here. Usually I had to add guarded mutex in such cases, but it is beyond the scope of this lib.

GitMoDu commented 5 months ago

Am I also reading it correctly that "This has been de-deprecated in C++23"? So nothing to do really?

I think it was un-deprecated (volatile keyword will stay), but not for the cases of volatile left-hand operators.

arkhipenko commented 5 months ago

I updated the library, so we should be all good either way!Sent from a mobile device.Apologies for autocorrect.On Jun 18, 2024 7:21 AM, André @.***> wrote:

Am I also reading it correctly that "This has been de-deprecated in C++23"? So nothing to do really?

I think it was un-deprecated (volatile keyword will stay), but not for the cases of volatile left-hand operators.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

arkhipenko commented 5 months ago

You are right. _TASK_THREAD_SAFE was an attempt of thread safety for execution methods only (when enable or restart is executed in a separate thread, ts.execute() is running in another) I did not protect task creating and deleting as I very rarely do it actually. Usually tasks stay forever. Sent from a mobile device.Apologies for autocorrect.On Jun 18, 2024 2:19 AM, vortigont @.***> wrote:

The whole point of the library is to be single-threaded cooperative multi-tasking. If you are multi-threading then RTOS is the answer.

true, scheduling and executing is single-threaded here, but other operations, like adding/removing/changing Tasks could be done from other threads, which is unsafe and _TASK_THREAD_SAFE does not always works as expected but only adds confusion. This is what I wanted to say here. Usually I had to add guarded mutex in such cases, but it is beyond the scope of this lib.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

vortigont commented 5 months ago

@arkhipenko got it. THREAD_SAFE is just confusing define :) I'm on the contrary use your scheduler to launch one-time or short living tasks. It's more convenient to spawn class instances with on heap allocation than defining/creating RTOS tasks and do stack size management .

arkhipenko commented 5 months ago

Yes, much simpler! Don't need to worry about concurrency within the thread, and stack size is a killer, especially on esp32. I have very limited success with stack less than 2k! Sent from a mobile device.Apologies for autocorrect.On Jun 18, 2024 8:04 AM, vortigont @.***> wrote: @arkhipenko got it. THREAD_SAFE is just confusing define :) I'm on the contrary use your scheduler to launch one-time or short living tasks. It's more convenient to spawn class instances with on heap allocation than defining/creating RTOS tasks and do stack size management .

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>