I'm working on a Cortex-M4 MCU and using FreeRTOS.
One task:
uint32_t wait_period;
while(1) {
// make some things
vTaskDelay(pdMS_TO_TICKS(wait_period));
}
The following function can be called from another task:
void set_waiting_period(uint32_t new_period) {
wait_period = new_period;
}
In this case, is it needed to protect the access of the shared variable
wait_period with a mutex? I don't think, we are dealing with an integer
variable that should be accessed (reading and writing) atomically.
Cortex-M: share an int between two tasks
Started by ●March 13, 2020
Reply by ●March 13, 20202020-03-13
On 13/03/2020 15:38, pozz wrote:> I'm working on a Cortex-M4 MCU and using FreeRTOS. > > One task: > > uint32_t wait_period; > while(1) { > � // make some things > � vTaskDelay(pdMS_TO_TICKS(wait_period)); > } > > The following function can be called from another task: > > void set_waiting_period(uint32_t new_period) { > � wait_period = new_period; > } > > > In this case, is it needed to protect the access of the shared variable > wait_period with a mutex? I don't think, we are dealing with an integer > variable that should be accessed (reading and writing) atomically. >You don't need to use a mutex, as you won't have race conditions when accessing it (race conditions happen when at least one thread can be writing, and at least one thread reading, and the accesses are not atomic). However, you /do/ need to make the accesses volatile, or there will be no guarantee that the variable will be read or written in the spots where you have accessed it.
Reply by ●March 18, 20202020-03-18
Il 13/03/2020 17:49, David Brown ha scritto:> On 13/03/2020 15:38, pozz wrote: >> I'm working on a Cortex-M4 MCU and using FreeRTOS. >> >> One task: >> >> uint32_t wait_period; >> while(1) { >> � // make some things >> � vTaskDelay(pdMS_TO_TICKS(wait_period)); >> } >> >> The following function can be called from another task: >> >> void set_waiting_period(uint32_t new_period) { >> � wait_period = new_period; >> } >> >> >> In this case, is it needed to protect the access of the shared variable >> wait_period with a mutex? I don't think, we are dealing with an integer >> variable that should be accessed (reading and writing) atomically. >> > > You don't need to use a mutex, as you won't have race conditions when > accessing it (race conditions happen when at least one thread can be > writing, and at least one thread reading, and the accesses are not > atomic).Ok, I got the point.> However, you /do/ need to make the accesses volatile, or there > will be no guarantee that the variable will be read or written in the > spots where you have accessed it.Let me explain just to be sure I got it. If wait_period is not volatile, in the task endless loop, the compiler could make the assumption the variable doesn't ever change, so for example it could "cache" its value in a register. In this case, when set_waiting_period() is called from another task, the variable in RAM is changed, but the value in the register doesn't change, so the task will continue using the old value. I think the access could be volatile only in the loop and not in set_waiting_period: uint32_t wait_period; while(1) { // make some things volatile uint32_t *vwait_period = (volatile uint32_t *)&wait_period; vTaskDelay(pdMS_TO_TICKS(*vwait_period)); } void set_waiting_period(uint32_t new_period) { wait_period = new_period; }
Reply by ●March 18, 20202020-03-18
On 18/03/2020 09:09, pozz wrote:> Il 13/03/2020 17:49, David Brown ha scritto: >> On 13/03/2020 15:38, pozz wrote: >>> I'm working on a Cortex-M4 MCU and using FreeRTOS. >>> >>> One task: >>> >>> uint32_t wait_period; >>> while(1) { >>> �� // make some things >>> �� vTaskDelay(pdMS_TO_TICKS(wait_period)); >>> } >>> >>> The following function can be called from another task: >>> >>> void set_waiting_period(uint32_t new_period) { >>> �� wait_period = new_period; >>> } >>> >>> >>> In this case, is it needed to protect the access of the shared variable >>> wait_period with a mutex? I don't think, we are dealing with an integer >>> variable that should be accessed (reading and writing) atomically. >>> >> >> You don't need to use a mutex, as you won't have race conditions when >> accessing it (race conditions happen when at least one thread can be >> writing, and at least one thread reading, and the accesses are not >> atomic). > > Ok, I got the point. > > >> However, you /do/ need to make the accesses volatile, or there >> will be no guarantee that the variable will be read or written in the >> spots where you have accessed it. > > Let me explain just to be sure I got it. > > If wait_period is not volatile, in the task endless loop, the compiler > could make the assumption the variable doesn't ever change, so for > example it could "cache" its value in a register. In this case, when > set_waiting_period() is called from another task, the variable in RAM is > changed, but the value in the register doesn't change, so the task will > continue using the old value.Exactly right. (The same can work when writing variables in a loop - the compiler knows that it has full control of any normal variables it is using, so there is no need to write out the variable to ram until later, if that would give more efficient code.)> > I think the access could be volatile only in the loop and not in > set_waiting_period: > > uint32_t wait_period; > while(1) { > ��� // make some things > ��� volatile uint32_t *vwait_period = (volatile uint32_t *)&wait_period; > ��� vTaskDelay(pdMS_TO_TICKS(*vwait_period)); > �} > > void set_waiting_period(uint32_t new_period) { > �� wait_period = new_period; > }Bad idea - get in the habit of making such writes volatile too. If the "set_waiting_period" function is compiled and linked separately, then the compiler will have to generate the write instruction before the return. But if it has access to the definition (because it is in the same file as the one using it, or you have link-time optimisation), and you write something like: set_waiting_period(100); // more code set_waiting_period(1000); then the compiler can skip the first write to "wait_period" if it knows "more code" does not use it. Most problems with missing volatiles are seen from a failure to do volatile reads in a loop. The second most common failure (IME) is assuming that volatile accesses place an order on non-volatile accesses (they don't). But failure to use volatile on writes occurs too, and is often more subtle, with code working fine until you change something else that appears independent.
Reply by ●March 18, 20202020-03-18
Il 13/03/2020 15:38, pozz ha scritto:> I'm working on a Cortex-M4 MCU and using FreeRTOS. > > One task: > > uint32_t wait_period; > while(1) { > � // make some things > � vTaskDelay(pdMS_TO_TICKS(wait_period)); > } > > The following function can be called from another task: > > void set_waiting_period(uint32_t new_period) { > � wait_period = new_period; > } > > > In this case, is it needed to protect the access of the shared variable > wait_period with a mutex? I don't think, we are dealing with an integer > variable that should be accessed (reading and writing) atomically.Ok the example was extremely simple. If I have something a little more complex: --- volatile uint32_t wait_period; while(1) { // make some things if (wait_period > 0) { vTaskDelay(pdMS_TO_TICKS(wait_period)); } else { vTaskSuspend(myTask); } } void set_waiting_period(uint32_t new_period) { wait_period = new_period; if (wait_period > 0) { vTaskResume(myTask); } } --- I think this doesn't work in all the cases. For example, if wait_period=0 and set_waiting_period(3) is executed from another higher priority task immediately after if(wait_period > 0) // the if in the endless loop wait_period is changed to 3, but vTaskSuspended() is executed in the else branch, so leaving the task suspended with a not-null wait_period. Do I need a mutex in this case to protect wait_period access in both points? Pardon for the silly question, but I'm very new to multitasking and RTOS.
Reply by ●March 18, 20202020-03-18
On 18/03/2020 09:20, pozz wrote:> Il 13/03/2020 15:38, pozz ha scritto: >> I'm working on a Cortex-M4 MCU and using FreeRTOS. >> >> One task: >> >> uint32_t wait_period; >> while(1) { >> �� // make some things >> �� vTaskDelay(pdMS_TO_TICKS(wait_period)); >> } >> >> The following function can be called from another task: >> >> void set_waiting_period(uint32_t new_period) { >> �� wait_period = new_period; >> } >> >> >> In this case, is it needed to protect the access of the shared >> variable wait_period with a mutex? I don't think, we are dealing with >> an integer variable that should be accessed (reading and writing) >> atomically. > > Ok the example was extremely simple. If I have something a little more > complex: > > --- > volatile uint32_t wait_period; > > while(1) { > � // make some things > � if (wait_period > 0) { > ��� vTaskDelay(pdMS_TO_TICKS(wait_period)); > � } else { > ��� vTaskSuspend(myTask); > � } > } > > void set_waiting_period(uint32_t new_period) { > � wait_period = new_period; > � if (wait_period > 0) { > ��� vTaskResume(myTask); > � } > } > --- > > I think this doesn't work in all the cases. For example, if > wait_period=0 and set_waiting_period(3) is executed from another higher > priority task immediately after > > � if(wait_period > 0)�� // the if in the endless loop > > wait_period is changed to 3, but vTaskSuspended() is executed in the > else branch, so leaving the task suspended with a not-null wait_period. > > Do I need a mutex in this case to protect wait_period access in both > points? > > Pardon for the silly question, but I'm very new to multitasking and RTOS.When you have more complex examples, you start needing synchronisation of some sort (it can be a disabled interrupt block, a lock like a mutex, or message passing, or something similar). Sometimes it can be handled with lock-free methods. Generally, if you have one reader and one writer, volatile will be enough - but you have to think about exactly how you want to arrange the accesses. I find one of the best ways to get good software design is to think about it like hardware. You don't have one hardware signal controlled by various different signals scattered around the design - if you have an LED that can be controlled by different signals, you put these signals into a multiplexer or logic gate (or wired or / wired and). And you collect them all together in one place in the design to do that. The same applies to software. Don't change a variable from a dozen different places around the program. Don't access the same resource from different places. Put in queues, multiplexers, etc., so that you have control. In a case like this, the idea that you have your "myTask" being suspended from one thread and woken up by a different thread is your flaw. Fix the design - the suspension and resumption of "myTask" should happen from /one/ place in /one/ thread - and your problems of synchronisation go away.
Reply by ●March 18, 20202020-03-18
Il 18/03/2020 10:06, David Brown ha scritto:> On 18/03/2020 09:20, pozz wrote: >> Il 13/03/2020 15:38, pozz ha scritto: >>> I'm working on a Cortex-M4 MCU and using FreeRTOS. >>> >>> One task: >>> >>> uint32_t wait_period; >>> while(1) { >>> �� // make some things >>> �� vTaskDelay(pdMS_TO_TICKS(wait_period)); >>> } >>> >>> The following function can be called from another task: >>> >>> void set_waiting_period(uint32_t new_period) { >>> �� wait_period = new_period; >>> } >>> >>> >>> In this case, is it needed to protect the access of the shared >>> variable wait_period with a mutex? I don't think, we are dealing with >>> an integer variable that should be accessed (reading and writing) >>> atomically. >> >> Ok the example was extremely simple. If I have something a little more >> complex: >> >> --- >> volatile uint32_t wait_period; >> >> while(1) { >> �� // make some things >> �� if (wait_period > 0) { >> ���� vTaskDelay(pdMS_TO_TICKS(wait_period)); >> �� } else { >> ���� vTaskSuspend(myTask); >> �� } >> } >> >> void set_waiting_period(uint32_t new_period) { >> �� wait_period = new_period; >> �� if (wait_period > 0) { >> ���� vTaskResume(myTask); >> �� } >> } >> --- >> >> I think this doesn't work in all the cases. For example, if >> wait_period=0 and set_waiting_period(3) is executed from another >> higher priority task immediately after >> >> �� if(wait_period > 0)�� // the if in the endless loop >> >> wait_period is changed to 3, but vTaskSuspended() is executed in the >> else branch, so leaving the task suspended with a not-null wait_period. >> >> Do I need a mutex in this case to protect wait_period access in both >> points? >> >> Pardon for the silly question, but I'm very new to multitasking and RTOS. > > When you have more complex examples, you start needing synchronisation > of some sort (it can be a disabled interrupt block, a lock like a mutex, > or message passing, or something similar).� Sometimes it can be handled > with lock-free methods. > > Generally, if you have one reader and one writer, volatile will be > enough - but you have to think about exactly how you want to arrange the > accesses.Just to think of a more complex example. If you share a data structure with some members and they must be consistent (all members must be changed atomically avoiding reading only a part changed), volatile is not enough. You need mutex, right?> I find one of the best ways to get good software design is to think > about it like hardware.� You don't have one hardware signal controlled > by various different signals scattered around the design - if you have > an LED that can be controlled by different signals, you put these > signals into a multiplexer or logic gate (or wired or / wired and).� And > you collect them all together in one place in the design to do that. > > The same applies to software.� Don't change a variable from a dozen > different places around the program.� Don't access the same resource > from different places.� Put in queues, multiplexers, etc., so that you > have control.I see. I think the safest approach is async messages sent in a queue.> In a case like this, the idea that you have your "myTask" being > suspended from one thread and woken up by a different thread is your > flaw.� Fix the design - the suspension and resumption of "myTask" should > happen from /one/ place in /one/ thread - and your problems of > synchronisation go away.I have two tasks. Task A can be suspended and resumed. Because Task A can't resume itself from suspended state, only Task B should suspend and resume Task A. // Task A while(1) { // make some things vTaskDelay(pdMS_TO_TICKS(wait_period)); } However I want to suspend Task A, if needed, after "make some things" or during vTaskDelay(). I don't want to suspend Task A in the middle of "make some things". I think I need a semaphore: // Task A while(1) { xSemaphoreTake(...); // make some things xSemaphoreGive(...); vTaskDelay(pdMS_TO_TICKS(wait_period)); } // Task B if (taskA_must_be_suspended) { xSemaphoreTake(...); vTaskSuspend(taskA); xSemaphoreGive(...); } else { vTaskResume(taskA); } Or at least a simple boolean, but volatile, flag: volatile bool taskA_blocked; // Task A while(1) { // make some things taskA_blocked = true; vTaskDelay(pdMS_TO_TICKS(wait_period)); taskA_blocked = false; } // Task B if (taskA_must_be_suspended) { while(!taskA_blocked) ; vTaskSuspend(taskA); } else { vTaskResume(taskA); }
Reply by ●March 18, 20202020-03-18
On 18/03/2020 10:59, pozz wrote:> Il 18/03/2020 10:06, David Brown ha scritto: >> On 18/03/2020 09:20, pozz wrote: >>> Il 13/03/2020 15:38, pozz ha scritto: >>>> I'm working on a Cortex-M4 MCU and using FreeRTOS. >>>> >>>> One task: >>>> >>>> uint32_t wait_period; >>>> while(1) { >>>> �� // make some things >>>> �� vTaskDelay(pdMS_TO_TICKS(wait_period)); >>>> } >>>> >>>> The following function can be called from another task: >>>> >>>> void set_waiting_period(uint32_t new_period) { >>>> �� wait_period = new_period; >>>> } >>>> >>>> >>>> In this case, is it needed to protect the access of the shared >>>> variable wait_period with a mutex? I don't think, we are dealing >>>> with an integer variable that should be accessed (reading and >>>> writing) atomically. >>> >>> Ok the example was extremely simple. If I have something a little >>> more complex: >>> >>> --- >>> volatile uint32_t wait_period; >>> >>> while(1) { >>> �� // make some things >>> �� if (wait_period > 0) { >>> ���� vTaskDelay(pdMS_TO_TICKS(wait_period)); >>> �� } else { >>> ���� vTaskSuspend(myTask); >>> �� } >>> } >>> >>> void set_waiting_period(uint32_t new_period) { >>> �� wait_period = new_period; >>> �� if (wait_period > 0) { >>> ���� vTaskResume(myTask); >>> �� } >>> } >>> --- >>> >>> I think this doesn't work in all the cases. For example, if >>> wait_period=0 and set_waiting_period(3) is executed from another >>> higher priority task immediately after >>> >>> �� if(wait_period > 0)�� // the if in the endless loop >>> >>> wait_period is changed to 3, but vTaskSuspended() is executed in the >>> else branch, so leaving the task suspended with a not-null wait_period. >>> >>> Do I need a mutex in this case to protect wait_period access in both >>> points? >>> >>> Pardon for the silly question, but I'm very new to multitasking and >>> RTOS. >> >> When you have more complex examples, you start needing synchronisation >> of some sort (it can be a disabled interrupt block, a lock like a >> mutex, or message passing, or something similar).� Sometimes it can be >> handled with lock-free methods. >> >> Generally, if you have one reader and one writer, volatile will be >> enough - but you have to think about exactly how you want to arrange >> the accesses. > > Just to think of a more complex example. If you share a data structure > with some members and they must be consistent (all members must be > changed atomically avoiding reading only a part changed), volatile is > not enough. You need mutex, right? >You need something to make the accesses atomic, so that each reader or writer sees a consistent picture. (This applies also to data that is bigger than the processor can read or write at one time, and for read-modify-write accesses.) Mutexes are one way to make atomic access, but they are not the only way. With C11/C++11, atomic access is part of the language - but support in libraries can be poor or even wrong (a common implementation I have seen could lead to deadlock). Other locks and synchronisations will work, as will a simple interrupt-disable block. You can also use indicator flags that are atomic to mark when a block is being updated, or have more than one copy of the block and use an atomic pointer to the current block. Many of these techniques can be simpler and more efficient than mutexes, but are only correct if certain assumptions hold (like there only being one writer thread). If you can arrange for your data to be transferred between threads using message queues of some sort (most RTOS's support a few different types), then this is often the easiest and cleanest method of communicating between threads. But you are right that volatile is not enough!> >> I find one of the best ways to get good software design is to think >> about it like hardware.� You don't have one hardware signal controlled >> by various different signals scattered around the design - if you have >> an LED that can be controlled by different signals, you put these >> signals into a multiplexer or logic gate (or wired or / wired and). >> And you collect them all together in one place in the design to do that. >> >> The same applies to software.� Don't change a variable from a dozen >> different places around the program.� Don't access the same resource >> from different places.� Put in queues, multiplexers, etc., so that you >> have control. > > I see. I think the safest approach is async messages sent in a queue. >Yes, it is often good. It is often easier to see that the code is correct when you use message passing rather than mutexes.> >> In a case like this, the idea that you have your "myTask" being >> suspended from one thread and woken up by a different thread is your >> flaw.� Fix the design - the suspension and resumption of "myTask" >> should happen from /one/ place in /one/ thread - and your problems of >> synchronisation go away. > > I have two tasks. Task A can be suspended and resumed. Because Task A > can't resume itself from suspended state, only Task B should suspend and > resume Task A. > > // Task A > while(1) { > �� // make some things > �� vTaskDelay(pdMS_TO_TICKS(wait_period)); > } > > However I want to suspend Task A, if needed, after "make some things" or > during vTaskDelay(). I don't want to suspend Task A in the middle of > "make some things".If you are using "suspend" and "resume" directly, you are almost certainly doing it wrong - no matter which threads are doing what. Suspension of a task should be for a reason. If you are waiting for some time, use vTaskDelay (or similar functions). If you are waiting for data from another thread, wait for that - receiving from a queue, waiting on a semaphore, a condition variable, etc. Resumption of a task should be for a reason - send it a message in a queue, set the semaphore or condition variable, etc. "suspend" and "resume" are low-level "beat it with a hammer" approaches.> > I think I need a semaphore: > > // Task A > while(1) { > �� xSemaphoreTake(...); > �� // make some things > �� xSemaphoreGive(...); > �� vTaskDelay(pdMS_TO_TICKS(wait_period)); > } > > // Task B > if (taskA_must_be_suspended) { > � xSemaphoreTake(...); > � vTaskSuspend(taskA); > � xSemaphoreGive(...); > } else { > � vTaskResume(taskA); > } > > > Or at least a simple boolean, but volatile, flag: > > volatile bool taskA_blocked; > > // Task A > while(1) { > �� // make some things > �� taskA_blocked = true; > �� vTaskDelay(pdMS_TO_TICKS(wait_period)); > �� taskA_blocked = false; > } > > // Task B > if (taskA_must_be_suspended) { > � while(!taskA_blocked) ; > � vTaskSuspend(taskA); > } else { > � vTaskResume(taskA); > }
Reply by ●March 18, 20202020-03-18
pozz <pozzugno@gmail.com> wrote:> > Ok the example was extremely simple. If I have something a little more > complex: > > --- > volatile uint32_t wait_period; > > while(1) { > // make some things > if (wait_period > 0) { > vTaskDelay(pdMS_TO_TICKS(wait_period)); > } else { > vTaskSuspend(myTask); > } > } > > void set_waiting_period(uint32_t new_period) { > wait_period = new_period; > if (wait_period > 0) { > vTaskResume(myTask); > } > } > --- > > I think this doesn't work in all the cases. For example, if > wait_period=0 and set_waiting_period(3) is executed from another higher > priority task immediately after > > if(wait_period > 0) // the if in the endless loop > > wait_period is changed to 3, but vTaskSuspended() is executed in the > else branch, so leaving the task suspended with a not-null wait_period.To me the above looks like very natural anti-pattern. Namely, code testing 'wait_period' _and_ execution of 'vTaskSuspend' logically form a single ciritical section. AFAICS any placement of locks above leads to races. My solution to the above is have _whole thing_ in scheduler, so that testing flags and switching process is atomic. In your case messages look like step to solution, but you still need to think about messaging protocol... -- Waldek Hebisch
Reply by ●March 18, 20202020-03-18
Am 18.03.2020 um 09:09 schrieb pozz:> I think the access could be volatile only in the loop and not in > set_waiting_period:You could do that, but you absolutely should not. You make the _variable_ volatile, and that's that.> ��� volatile uint32_t *vwait_period = (volatile uint32_t *)&wait_period;Just looking at that line should tell you that this cannot possibly be the right way to do anything. The primary rule about explicitly casting pointers in C code is: don't do it. All the evidently safe casts are already performed implicitly by the compiler. The perceived need for less safe ones almost invariably results from an incorrect design decision a few steps earlier.
