add graph functions to support wait_for_service #57
Conversation
|
Do you think it would be possible to push more logic from rmw into rcl? |
|
In which functions? I don't have any ideas. |
|
Anything in the API that is left up to the rmw implementations means that the code has to be written and maintained |
|
Each rmw impl. needs to be able to provide its own impl. of these functions since some vendors have very efficient (non standardized) functions for that. If multiple rmw impl. end up using the same algo to collect the information based on other api that can easily be refactored into a single place. |
|
Yeah, unfortunately I think @dirk-thomas is right about the topic listing and pub/sub counting. It's also true for the service is ready call, which might be implemented different ways in each rmw impl. |
|
I pushed some new commits which add a function to create a |
| printf("waiting up to %lld nanoseconds for graph changes\n", time_to_sleep.count()); | ||
| ret = rcl_wait(&wait_set, time_to_sleep.count()); | ||
| if (ret == RCL_RET_TIMEOUT) { | ||
| printf("timeout!\n"); |
There was a problem hiding this comment.
Is there something more useful we can do here than printing? It's ambiguous to me if this expected behavior or not. Should we expect rcl_wait to spuriously time out in this case?
There was a problem hiding this comment.
Sorry, that was left over from debug. I removed it. There's an identical check one below.
Incidentally, I have a few more changes for this pr.
|
Ok, this pr is also ready for review. |
wjwwood
added
in review
|
In the most recent commits I added a function called |
this also addressed the segfault issues, but the proper fix was committed to rmw_opensplice_cpp while this ordering is not strictly necessary it is slightly more correct
it's too strict and will be relaxed in newer gcc's
| break; | ||
| } | ||
| } | ||
| ASSERT_TRUE(is_available); |
There was a problem hiding this comment.
One simple extension to this test would be to tear down the service and assert is_available goes back to false to make sure all state is cleared.
There was a problem hiding this comment.
Good idea, I updated this test to do that: 631898e
|
+1 the extra test case would be great if you have time, but is not necessary |
| * The returned handle is made invalid if the node is finialized or if | ||
| * rcl_shutdown() is called. | ||
| * | ||
| * The guard condition will be triggered anytime change to the ROS graph occurs. |
There was a problem hiding this comment.
anytime change -> anytime a change
* WIP specify logger severity by name * Add a fini to destroy severity map * wip cont * Add is_enabled_for concept * People won't override is_enabled_for * fini -> shutdown * Fallback to no configurability if map errors Can't stop the log calls from being called, so handle it gracefully * Hierarchy will require distinction between set and unset levels * More error handling * WIP logger hierarchy * Add a non-null-terminated get_severity to avoid strdup * lint * Add tests for effective threshold * Docblock update * Additional doc fixup * Replace global severity check in rcutils_log * Remove debug prints * get_severity_threshold -> get_global_severity_threshold * Revert "get_severity_threshold -> get_global_severity_threshold" This reverts commit 182b1338bd2233f97d0dcfcfe378af6e93cd5a9a. * Use RCUTILS_LOGGING_ROOT_LOGGER_NAME * Remove the notion of global severity threshold * Add initialize() where allocator's specified * safer logic * RCUTILS_LOGGING_ROOT_LOGGER_NAME wasn't really a root logger. It was the absence of a name; we don't prepend it to other names. * root severity threshold -> default severity threshold * Distinguish between nameless log calls and those with empty name * The map will allocate space for a new string regardless * Add more documentation * Add tests for empty children names and clearing map * Don't autoinit anywhere (affects the memory allocation adherance) * Fixups after rebasing master * Remove allocator from ENSURE_LARGE_ENOUGH_BUFFER macro * Refactor to use new rcutils_find_lastn * Add rcutils_findn for completeness (not used) * Don't go to extremes to avoid memory allocation * Return -1 on invalid severity get * Add return code for setting logger level * Add return code for logging init * Return value for logging shutdown * Add key_existsn for string_map * Add tests for findn and find_lastn * Rename allocator * fixup * Use EXPECT_FALSE to fix warning on linux * Move TODO outside of docblock * Always mark finalised severity map as invalid * Swap ordering of test macro args So it's (expected, actual) * Missed some macro arg swaps * Update severity values so unset is lowest Also match python logging severities * Documentation fixes * Clarify return value will be 0 if str is null * Add mapping of severity level names for reuse * Add test for unsetting severity threshold * Use new severity level name array * Remove 'unknown error' reference for key_exists return * logger_effective_threshold -> logger_effective_severity_threshold * is_enabled_for -> logger_is_enabled_for * spelling * Prevent uncrustify from making indents with LOGGIN_AUTOINIT * Equate the empty logger name to the default I allowed them to be independent as an arbitrary decision but equating them is needed to facilitate an empty-named root logger concept in wrappers * add comment * True -> true
Don't type-erase request header
Connects to ros2/ros2#215