Skip to content

Conversation

KentaKato
Copy link
Contributor

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().

@sjahr sjahr requested a review from pac48 June 13, 2024 15:38
@pac48
Copy link
Contributor

pac48 commented Jun 18, 2024

@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
Copy link
Contributor Author

@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
Copy link
Contributor

pac48 commented Jun 21, 2024

@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
Copy link
Contributor Author

@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!

@pac48 pac48 merged commit 138e501 into PickNikRobotics:main Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants