Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/blocking uxr create session #351

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Ryanf55
Copy link

@Ryanf55 Ryanf55 commented Mar 4, 2023

Here's my fix to the blocking nature of uxr_create_session.

  1. Add asserts for developing that will prevent poor behavior if the user's uxr_millis() function is not working correctly, or the time source is frozen, stuck at zero, or jumps backwards.
  2. Increase const correctness to the internal implementation
  3. Help prevent misconfiguration through cmake of the cmake defines
  4. Add asserts during the narrowing conversion
  5. Improve documentation in the public header.

This closes #350

In CMakeCache, one can see when NDEBUG is set, which would remove the asserts.

ryan@ryan-m93p:~/Documents/ardu_ws/src/ardupilot/modules/Micro-XRCE-DDS-Client$ cat build/microxrcedds_client/CMakeCache.txt | grep NDEBUG
CMAKE_CXX_FLAGS_MINSIZEREL:STRING=-Os -DNDEBUG
CMAKE_CXX_FLAGS_RELEASE:STRING=-O3 -DNDEBUG
CMAKE_CXX_FLAGS_RELWITHDEBINFO:STRING=-O2 -g -DNDEBUG
CMAKE_C_FLAGS_MINSIZEREL:STRING=-Os -DNDEBUG
CMAKE_C_FLAGS_RELEASE:STRING=-O3 -DNDEBUG
CMAKE_C_FLAGS_RELWITHDEBINFO:STRING=-O2 -g -DNDEBUG

Since asserts aren't used yet in the library, it would be nice to add some documentation for library implementers to ensure they are disabled in production releases, but can be on during development.

…cking in uxr_create_session

* Added asserts that protect against time going backwards or staying static
* Added information to header on a call being blocking and for how long
* Increased const-correctecness of time handling
* Add assert for handling narrowing conversion of time
* Add CMake protection for incorrectly configuring options that can block for unreasonable amount of time
* CMake should theoretically remove the asserts in release mode, so there should be no overhead
* Reference: https://stackoverflow.com/questions/34302265/does-cmake-build-type-release-imply-dndebug
* Closes eProsima#350

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>
@Ryanf55 Ryanf55 changed the base branch from master to develop March 4, 2023 21:55
@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Windows Build Status

@Ryanf55
Copy link
Author

Ryanf55 commented Mar 4, 2023

Build status:

  • Linux Build Status
  • Windows Build Status

I can't see any reason the build is failing. Let me know what I am missing. Searched for errors in the logs, all tests pass. Thanks!

Since CI only builds in release mode by default, if you want me to add tests for the new lines of code, the gtest tests will need to be run in debug mode and check for assertion failures when you supply bad time or misconfiguration to that function.

Copy link
Member

@pablogs9 pablogs9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is preferred to not use assert(), could you refactor the logic to avoid it?

int remaining_time = UXR_CONFIG_MIN_SESSION_CONNECTION_INTERVAL;
const int64_t start_timestamp = uxr_millis();
int64_t remaining_time = UXR_CONFIG_MIN_SESSION_CONNECTION_INTERVAL;
assert(remaining_time > 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not using assert in this project's code base, this can lead to portability issues.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I thought this library was C99 complaint.

Are the users with incompatible C support able to set NDEBUG all the time then?

If not, can you give an example of the desired error propagation method?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is preferred to check for the underflow and return false as far as wait_session_status was not successful

@pablogs9
Copy link
Member

pablogs9 commented Mar 8, 2023

I can't see any reason the build is failing. Let me know what I am missing. Searched for errors in the logs, all tests pass. Thanks!

Probably uncrustify is failing, try:

curl -l https://raw.githubusercontent.com/eProsima/cpp-style/master/uncrustify.cfg -o uncrustify.cfg
find src test include examples \( -name "*.cpp" -o -name "*.c" -o -name "*.h" -o -name "*.hpp" \) -exec uncrustify -c uncrustify.cfg --check {} +

@Ryanf55 Ryanf55 marked this pull request as draft November 3, 2023 07:40
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.

None yet

3 participants