Synchronization

[Stapelzeiger]

Setters and getters are not synchronized with the PID control function. How should we handle synchronization of such functions?

One solution would be to restrict the code to processor architectures where float read & writes are atomic. This would reduce the problem to a few places where multiple floats must absolutely be written/read atomically. But after the discussion about stdints the general attitude seems to be in favor of portability.

[antoinealb]

Bah, synchronization issues never happen, do they ? ;)

We could add a mutex to the PID structure to lock access to the PID instance. Or use the ATOMIC block currently discussed.

[froj]

What about

void pid_set_gains(pid_filter_t *pid, float kp, float ki, float kd)
{
#ifndef ATOMIC_FLOAT_RW
    CIRTICAL_SECTION()
#endif
    {
        pid->kp = kp;
        pid->ki = ki;
        pid->kd = kd;
    }
}

[antoinealb]

I don't like #ifdef. They tend to be quite an anti-pattern and make the code pretty difficult to maintain. In addition, they make the code harder to test. The main problem with locking interrupts on a platform where it is not needed is increased interrupt latency. I think reducing this latency is not worth adding such complexity to the code. So I would suggest either :

void pid_set_gains(pid_filter_t *pid, float kp, float ki, float kd)
{
    CRITICAL_SECTION()
    {
        pid->kp = kp;
        pid->ki = ki;
        pid->kd = kd;
    }
}

or the second option, which is cleaner IMHO :

void pid_set_gains(pid_filter_t *pid, float kp, float ki, float kd)
{
    os_mutex_take(pid->lock);
    pid->kp = kp;
    pid->ki = ki;
    pid->kd = kd;
    os_mutex_release(pid->lock);
}

[Stapelzeiger]

I agree with not using #ifdefs. The problem I see with mutexes is overhead and most importantly the possibility that you block the (high priority) control loop by a low priority PID config. Edit: Ok, uC OS II has priority inheritance on mutexes so the blocking should be less of a problem (still some contexts switches though)

[antoinealb]

If you lock interrupts, you also block the control loop...

[Stapelzeiger]

Yes, but only for ~10 instructions. BTW also interrupts block the control loop

[antoinealb]

I think we should avoid prematures optimization like this. I feel like they are responsible for a lot of the code rot we had in the previous codebase. Let's stick to correctness and implement a fast version if we realize the correct version is not fast enough.

[pierluca]

agreed with Antoine -> KISS (keep it simple)

it's a very short snippet of code, from an aesthetic point of view I also prefer the mutex BUT I have no clue about mutex/critical_section performance.

On Sat, Jun 14, 2014 at 1:21 PM, Antoine Albertelli < notifications@github.com> wrote:

I think we should avoid prematures optimization like this. I feel like they are responsible for a lot of the code rot we had in the previous codebase. Let's stick to correctness and implement a fast version if we realize the correct version is not fast enough.

— Reply to this email directly or view it on GitHub https://github.com/cvra/pid/issues/8#issuecomment-46085233.

[Stapelzeiger]

avoid prematures optimization like this.

If are you referring to #ifdefs, then yes.

For mutex vs critcal section I prefer critical sections, since they have less overhead and achieve exactly the same thing. The induced interrupt latency is negligible. Edit: my preference for critical sections is only in places where we simply copy a few values. for more complex operations I would use mutexes.

[msplr]

Mutexes are internally implemented with a critical section anyway. So why not just use a critical section directly?

[31415us]

just to be clear about terminology: by 'critical section' you all mean simply masking interrupts?

[antoinealb]

@31415us yes

One reason I see to use mutex would be that using this kind of high level constructs, you can more easily test your code. Plus, we will probably use a mutex around pid_process anyway.

[msplr]

I think this should be a general decision, whether to use critical sections or mutexes for short operations.

Personally, I'm against using locks, where a short critical section is possible. Mostly because of Performance and mermory reasons. (the mutex would probably use more memory than the pid struct)

[Stapelzeiger]

Plus, we will probably use a mutex around pid_process anyway.

why that?

[antoinealb]

We have to decide how we would implement the lock around pid_process. We have two options :

1. We copy the struct and do the computation on the local copy before writing back the results. Initial read and write are performed in a critical section.
2. We use a mutex to lock the parameters when we do the computation.

[31415us]

I'd rather not see masked interrupts in application code, seems terribly improper,

[Stapelzeiger]

I think it would be option 1 if we use critical sections and option 2 if we use locks for setting parameters. Option 2 doesn't help if you use critical sections for setters.

[antoinealb]

According to uc/OS-III doc :

µC/OS-III defines one macro for entering a critical section and two macros for leaving:

    OS_CRITICAL_ENTER(), 
    OS_CRITICAL_EXIT() and 
    OS_CRITICAL_EXIT_NO_SCHED()

These macros are internal to µC/OS-III and must not be invoked by the application code. However, if you need to access critical sections in your application code, consult Resource Management.

Also, about disabling interrupts :

It is highly recommended to not use this method as it impacts interrupt latency.

[Stapelzeiger]

And what does it say in Resource Management?

[antoinealb]

When access to shared resource is very quick (reading from or writing to few variables) and access is faster than µC/OS-III’s interrupt disable time.

It is highly recommended to not use this method as it impacts interrupt latency.

[antoinealb]

The real question is : how long does it take to acquire a mutex ? We will probably be taking between 2000 and 3000 mutex per second (control loop running at 1kHz). For the memory occupation it should not be a problem as a mutex is only a few bytes.

[Stapelzeiger]

Worst case the task that has the mutex has to be scheduled, finish it's operation and reschedule the other task waiting to acquire the mutex. So 2x context switch + time to finish "critical section" of the task that has the mutex. (This is assuming the task in possession of the lock doesn't itself wait on another resource, but this should be avoided anyway so I'm not considering this option.)

[pierluca]

can't we just make a simple test with old hardware? 100 lines of code and you'll have your answer. it's probably simpler than all of us arguing here :-)

On Sat, Jun 14, 2014 at 2:07 PM, Antoine Albertelli < notifications@github.com> wrote:

The real question is : how long does it take to acquire a mutex ? We will probably be taking between 2000 and 3000 mutex per second (control loop running at 1kHz). For the memory occupation it should not be a problem as a mutex is only a few bytes.

— Reply to this email directly or view it on GitHub https://github.com/cvra/pid/issues/8#issuecomment-46086144.

[antoinealb]

The problem with synchronization implementation 1 I described above is : it does not protect you from race conditions. Suppose the following order of events :

1. You copy all fields in an atomic operation.
2. Another task calls pid_reset_integral
3. You write back the new integrator value which is not correct anymore. If the task who called pid_reset_integral was a security task, you now broke something.

The only way around this is to make entire pid_process a critical section (uh-oh interrupt latency) or to use a mutex.

edit : agree that we should maybe prototype something.

[Stapelzeiger]

I agree, you have to make the integrator read-modify-write atomic. (but not necessarily the entire function)

[antoinealb]

Now we are moving more and more code into a critical section... this will lead to bugs. I don't like bugs.

It is just so much easier and less prone to error to lock at the begining of function and unlock at the end.

[Stapelzeiger]

The code is the same using mutexes or "critical sections".

[froj]

I think we should listen to the uC/OS-III user manual and not use interrupt masking in application code. This leaves us with

• Locking the scheduler (not recommended either)
• Semaphores
• Mutexes (slower than semaphores, but no unbounded priority inversions)

IMO if we don't have enough CPU time to use mutexes, we should reconsider our choice of MCU/RTOS.

[31415us]

by mutex do you mean a reentrant lock?

[antoinealb]

By mutex we mean a binary semaphore with priority inheritance. I don't know what a re-entrant lock is.

IMO if we don't have enough CPU time to use mutexes, we should reconsider our choice of MCU/RTOS.

I would rephrase as : if we don't have enough CPU time to use mutexes, we don't have enough CPU time.

[Stapelzeiger]

Semaphores are not an option because they don't have priority inheritance. Locking only the scheduler would be better than locking interrupts.

[Stapelzeiger]

by mutex do you mean a reentrant lock?

I don't think uC/OS-III supports reentrant mutexes

[froj]

Locking only the scheduler would be better than locking interrupts.

Yes, but you'd make the uC/OS devs sad:

It is recommended not to use this method since it defeats the purpose of using μC/OS-III.

[pierluca]

I don't think uC/OS-III supports reentrant mutexes

It does.

µC/OS-III enables the user to nest ownership of mutexes. If a task owns a mutex, it can own the same mutex up to 250 times. The owner must release the mutex an equivalent number of times. In several cases, an application may not be immediately aware that it called OSMutexPend() multiple times, especially if the mutex is acquired again by calling a function as shown in Listing 13-12.

[antoinealb]

Plus, they already tell us what the best is :

This (mutex) is the preferred method for accessing shared resources, especially if the tasks that need to access a shared resource have deadlines.

[antoinealb]

I agree, you have to make the integrator read-modify-write atomic. (but not necessarily the entire function)

Read-Modify-Write of a float on a platform without FPU is slower than uc/OS-III interrupt latency.

[Stapelzeiger]

Plus, they already tell us what the best is :

So what are we still discussing?

[Stapelzeiger]

Interrupt latency is optimization and overrated. Only the highest priority interrupt has the guaranteed latency. For every lower priority interrupt the execution time of all higher priority interrupts add to their latency.

But I agree that in the PID example we have probably reached a critical point, where the latency gets too big.

[Stapelzeiger]

We still need to decide how we handle simpler cases, where we really only copy a few bytes.

[Stapelzeiger]

An option for the PID that I see, if mutexes turn out to have too much overhead, is using message passing for setters & getters.

[antoinealb]

I think if the only operation performed is a copy, then we can use critical sections. However, as soon as we start doing computations on those shared elements, we should switch to mutexes or whatever.

The idea of message passing is good, but since there is a lot of added complexity, I suggest we use it only in case of throughput problems.

[31415us]

Please don't mask interrupts in application code the overhead for using the correct higher level abstraction can't be that bad, can it?

[Stapelzeiger]

I think if the only operation performed is a copy, then we can use critical sections. However, as soon as we start doing computations on those shared elements, we should switch to mutexes or whatever.

agreed. So let's use mutexes for the PID controller.

[antoinealb]

@31415us It's not really bad, it's just that when you do it a few thousands times per second, it starts to add.

Preparing a patch for sync.

[Stapelzeiger]

Please don't mask interrupts in application code the overhead for using the correct higher level abstraction can't be that bad, can it?

what makes you think that masking interrupts is not "correct"? And in certain cases the overhead can get too big.

[antoinealb]

It would make writing tests for the sync code much easier if #9 gets merged (I would be able to inspect mutex state without a getter for the mutex).

[Stapelzeiger]

I thought about the synchronization of PID parameters. I think synchronization should not be part of the PID module. Instead we could have the the control loop check if any of the pid parameters must be changed before each control-loop execution. There we could use message passing or locks like we use now. The advantage is, that the PID-code is free of synchronization stuff. We used this approach for the nastya control loop. Because like I see it, we will have exactly the same problem with the filter module, and all the other "configurable" functions.

What do you think about moving synchronization out of "math" functions to the caller for all parameter changes?

[antoinealb]

That's probably the best way to do it. But how would you do it ? message box ?

[Stapelzeiger]

What we did on nasty was disabling interrupts to check if the param-value has changed (check of a bool) and if it changed, update the PID parameters from the control loop thread-context. (we did this using the "param" module) Message boxes would be an option too. The check for changes in the control loop was quite messy in terms of LOC because we checked for each parameter separately.

[antoinealb]

This seems pretty messy. Message box would be much cleaner I think :)