PickNikRobotics / generate_parameter_library

Declarative ROS 2 Parameters
BSD 3-Clause "New" or "Revised" License
240 stars 45 forks source link

Add Non-Blocking try_get_params Function for Real-Time Control Systems #205

Closed KentaKato closed 4 months ago

KentaKato commented 4 months ago

This pull request adds a new function, try_get_params(), to improve responsiveness especially in real-time systems by avoiding blocking operations. The existing is_old() function also risks blocking, so I incorporated it into try_get_params().

pac48 commented 4 months ago

@KentaKato Is there any reason to return a bool and take a reference argument as opposed to returning an optional? I think memory will be allocated in either case.

KentaKato commented 4 months ago

@pac48 Thank you for your review.

When returning an std::optional, users can interact with the results in the following ways:

Pattern 1: This incurs a copy cost.

if (const auto params_opt = try_get_params(); params_opt.has_value()) {
    params_local = params_opt.value();
}

Pattern 2: This avoids copy costs but is slightly more cumbersome.

if (const auto params_opt = try_get_params(); params_opt.has_value()) {
    params_local = std::move(params_opt.value());
}

Given that the approach to return a bool and take a reference argument excels in both cost efficiency and ease of use, I opted for this method.

However, if returning an optional is preferred, I am open to making that change. If we decide to return an optional, would the following logic be suitable? This allows users to detect when try_lock() fails:

  1. try_lock() == true && is_old == true
    • return params_
  2. try_lock() == true && is_old == false
    • return params_
  3. try_lock() == false
    • return std::nullopt

(Cases 1 and 2 can be combined, resulting in returning params_ whenever try_lock() == true.)

pac48 commented 4 months ago

@KentaKato I think the pattern is okay. Since in the context of real time control, it is probably better. Can you ad a simple test that calls this function, maybe here example/test/example_test_gtest.cpp?

KentaKato commented 4 months ago

@pac48 I've added the requested test in example/test/example_test_gtest.cpp. Please let me know if there's anything else that needs to be addressed. Thank you!