So this is the issue I wasn't able to solve without rewriting the handles. Do you have an idea on how we could make this thread safe?

This calls the controller update(), and if it coincides with the CM thread's write or read, then this is a data race, as both of those write to the same memory through a double ptr.

With the current version, I agree we will have this issue. WIth #1240 we might be able to avoid this issue with the getter and setters that it is going to provide

Actually I'm not sure even that will solve it, because if you take this code from the JTC for example:

    for (size_t index = 0; index < dof_; ++index)
    {
      trajectory_point_interface[index] = joint_interface[index].get().get_value(); // get_state() from #1240
    }

and you're in the async update cycle, while the HWIF from the CM thread does

    for (size_t index = 0; index < dof_; ++index)
    {
      hw_states_[index] = some_value // set_state() with the updated version
    }

at the same time, then it doesn't matter if the access itself is thread safe, the async JTC will do the assignment and calculations based on a mixture of old and new state interfaces.

For async HW, maybe it could be possible to disable plain getters / setters and instead use a grouped set_states and get_states function? That way you could wait until all values are set by the JTC, and update the states only after that! (e.g. lock until all states are read, or all states are set)

As a quick example something like this could easily be extended for multiple types:

std::map<const char*, double> interface_name_and_value = { {"Chocolate", 12.0}, {"Coffee", 14.0}, {"Cake", 15.0}};

template<typename T>
struct my_pair {
    my_pair() = delete;
    my_pair(const char* first, T second) : first_(first), second_(second) {}
    const char* first_;
    T second_;
};

template <typename T, typename ...Pairs>
void fun(const my_pair<T>& first_pair, const Pairs&... pairs) {

   interface_name_and_value[first_pair.first_] = first_pair.second_;
   if constexpr (sizeof...(pairs) > 0)
   {
     fun(pairs...);
   }
}

//    Usage:
//    const my_pair p1 = {"Chocolate", 18.0};
//    const my_pair p2 = {"Cake", 15.2};
//    const my_pair p3 = {"Coffee", 198.2};

//    lock();
//    fun(p1, p2, p3);
//    unlock()

Plain folds are not be enough if the data is stored in some data structure though, but I think this could work with index sequences / recursive templates.

What do you think about this? The code might not be very readable, but I think this could ensure data consistency later.

Regarding this, I agree, that there might be cases where the data might not be perfectly synchronized, this is the case we are more or less talking about in the other thread. For now, we can print the warning message as you mentioned and we shall try to fix it in future versions.

The batch reading and updating is a good idea, but I don't know if the Maintainers allow that level of changes at this point before Jazzy, we can try to do something like that in the future, so we could have some data consistency.

What do you say?

Is this a placeholder? I haven't seen this fn called anywhere other than the tests.

Well it is for testing purpose, but if can be used alongside before stopping the async thread. If I use this at the beginning of the stop_async_update, may be we won't be having the situation you were describing

Instead of having the controller manager act as the scheduler, would it make sense to trigger the async updates based on whether the hardware component whose command interfaces the controller is using has any data ready?

In the latter case, it might be possible to make async HW and controllers work together!

As an example, let's say I'd like to use an async controller combined with a KUKA-specific external interface, where the timing is always handled inside the internal motion stack instead of the CM, and therefore would work better as an async HW component.

I'm not yet sure if there's a legit use case where this would make sense, I think it's just interesting to think about if the design we choose should permit the combination of the two or not.

This design process would be cool. I'm not sure about triggering only when the data is ready.

For instance, having a chainable controller A -> B -> HW. A is async and running at a lower rate like 200 Hz and controller B is async and running at 500 Hz closing the loop locally with the actual values (So, we will need to trigger the controller B anyway as it needs to close the loop with the same reference values) and then command to the HW on each loop, if the HW is async then it gets the references on it's cycle (Some hardware like elmo respectively they expect a reference sent constantly at a certain frequency, in that case this mechanism would also be a problem right?

Right now, I couldn't think of a way to make both HW and controllers work together for both cases. It would be really cool as we have a data dependency

What do you think?

I see, that makes sense.

Also it's a difficult question because afaik there is no definition on what the behaviour of an async HW and async controller combination should be, so maybe at least a warning could be given if the user tries to use this combination, because otherwise there's no information if a certain configuration is invalid or not.

What do you think about that?

I think it is always nice to warn the user about the data synchronization issue between async controllers and async hardware combination. Where do you think is the right place to do it?. The resource manager at the time of configuring the hardware can print this message of possible discrepancies.

Also in most of the case, the controller manager runs at almost same frequency as the hardware even though it is run async, because mostly it doesn't make sense to compute the same command twice and being unable to send to the hardware.

Do you think std::future would be more idiomatic here, instead of a condition variable? As I see we're doing some calculations in the async thread, and returning it when it's ready. I think we'd also have one less flag to keep track of.

I initially thought of using future, but if there is a case that the update takes time and it doesn't have it's current update cycle's return state, then I think it makes sense to continue with its old return state without triggering another cycle, rather than waiting for the future result.

If this above situation occurs, then the controller manager will print a missing update cycle message, so the user knows that he has to lower the frequency of his controller as the computation is taking longer than what he initially estimated

That's fair.

Maybe it's possible to compute a timeout using the time & period for the future's wait_for() / wait_until() fn but probably it's needless complexity.

Yeah, I thought so, but I rather prefer that the conditional variable is not awake after a certain duration, because that's exactly what we want to avoid with proper triggering

With the current version, I agree we will have this issue. WIth #1240 we might be able to avoid this issue with the getter and setters that it is going to provide

Well it is for testing purpose, but if can be used alongside before stopping the async thread. If I use this at the beginning of the stop_async_update, may be we won't be having the situation you were describing

I initially thought of using future, but if there is a case that the update takes time and it doesn't have it's current update cycle's return state, then I think it makes sense to continue with its old return state without triggering another cycle, rather than waiting for the future result.

If this above situation occurs, then the controller manager will print a missing update cycle message, so the user knows that he has to lower the frequency of his controller as the computation is taking longer than what he initially estimated

This design process would be cool. I'm not sure about triggering only when the data is ready.

For instance, having a chainable controller A -> B -> HW. A is async and running at a lower rate like 200 Hz and controller B is async and running at 500 Hz closing the loop locally with the actual values (So, we will need to trigger the controller B anyway as it needs to close the loop with the same reference values) and then command to the HW on each loop, if the HW is async then it gets the references on it's cycle (Some hardware like elmo respectively they expect a reference sent constantly at a certain frequency, in that case this mechanism would also be a problem right?

Right now, I couldn't think of a way to make both HW and controllers work together for both cases. It would be really cool as we have a data dependency

What do you think?

That's fair.

Maybe it's possible to compute a timeout using the time & period for the future's wait_for() / wait_until() fn but probably it's needless complexity.

I see, that makes sense.

Also it's a difficult question because afaik there is no definition on what the behaviour of an async HW and async controller combination should be, so maybe at least a warning could be given if the user tries to use this combination, because otherwise there's no information if a certain configuration is invalid or not.

What do you think about that?

Actually I'm not sure even that will solve it, because if you take this code from the JTC for example:

    for (size_t index = 0; index < dof_; ++index)
    {
      trajectory_point_interface[index] = joint_interface[index].get().get_value(); // get_state() from #1240
    }

and you're in the async update cycle, while the HWIF from the CM thread does

    for (size_t index = 0; index < dof_; ++index)
    {
      hw_states_[index] = some_value // set_state() with the updated version
    }

at the same time, then it doesn't matter if the access itself is thread safe, the async JTC will do the assignment and calculations based on a mixture of old and new state interfaces.

For async HW, maybe it could be possible to disable plain getters / setters and instead use a grouped set_states and get_states function? That way you could wait until all values are set by the JTC, and update the states only after that! (e.g. lock until all states are read, or all states are set)

As a quick example something like this could easily be extended for multiple types:

std::map<const char*, double> interface_name_and_value = { {"Chocolate", 12.0}, {"Coffee", 14.0}, {"Cake", 15.0}};

template<typename T>
struct my_pair {
    my_pair() = delete;
    my_pair(const char* first, T second) : first_(first), second_(second) {}
    const char* first_;
    T second_;
};

template <typename T, typename ...Pairs>
void fun(const my_pair<T>& first_pair, const Pairs&... pairs) {

   interface_name_and_value[first_pair.first_] = first_pair.second_;
   if constexpr (sizeof...(pairs) > 0)
   {
     fun(pairs...);
   }
}

//    Usage:
//    const my_pair p1 = {"Chocolate", 18.0};
//    const my_pair p2 = {"Cake", 15.2};
//    const my_pair p3 = {"Coffee", 198.2};

//    lock();
//    fun(p1, p2, p3);
//    unlock()

Plain folds are not be enough if the data is stored in some data structure though, but I think this could work with index sequences / recursive templates.

What do you think about this? The code might not be very readable, but I think this could ensure data consistency later.

cannot this hang for an undetermined amount of time?

Not really, this is just until the current cycle is finished.

it might hang only when there is a crash or throw exception in another thread. That's why I added this one : ros-controls/realtime_tools#172

This is still a bit tricky, as it can easily disturb the update loop. for not is this fine. But I would love to see "async" deactivation of an async controller, or at least that we somehow earlier know what is happening.

The scenario I have in mind. My async controller is probably slow, so I would expect it to be blocking when writing for it to finish. which means that this would break the update loop here. I am not fully sure how to avoid this TBH, of how to manage it properly.

Well but this is inside the nonRT loop, it is inside the switch_controllers method, so waiting there shouldn't be a problem

What would be interesting is to skip the update trigger for the deactivated controllers to avoid it blocking

how often does this trigger in a realistic setup? if something is off by a bit this can massively spam the console... should it be THROTTLED?

It depends this can help you see, if your controller is missing the deadline, may be this can be throttled!

Yeah it would be nice if not only the log level but the throttling ratio was also controllable for logging

For the future: why not just to have this once using <depend> tag?

This is still a bit tricky, as it can easily disturb the update loop. for not is this fine. But I would love to see "async" deactivation of an async controller, or at least that we somehow earlier know what is happening.

The scenario I have in mind. My async controller is probably slow, so I would expect it to be blocking when writing for it to finish. which means that this would break the update loop here. I am not fully sure how to avoid this TBH, of how to manage it properly.