Action goal state machine tests #311
Conversation
|
|
Attempt to fix MSVC compiler warnings:
|
jacobperron
added
in review
| }, | ||
| }; | ||
| static rcl_action_goal_event_handler | ||
| _goal_state_transition_map[GOAL_STATE_NUM_STATES][GOAL_EVENT_NUM_EVENTS]; |
There was a problem hiding this comment.
@jacobperron just out of curiosity, what was wrong about the previous direct initialization?
There was a problem hiding this comment.
When the direct initialization gets included in C++ (e.g. gtest), then gcc complains about a sparsely populated 2d array. But we might be able to use null pointers in the direct initialization instead of this init function.
There was a problem hiding this comment.
Moving the direct initialization to the C file got rid of the compiler warnings :)
| _goal_state_transition_map[GOAL_STATE_NUM_STATES][GOAL_EVENT_NUM_EVENTS]; | ||
|
|
||
| static inline void | ||
| rcl_action_goal_transition_map_init() |
There was a problem hiding this comment.
@jacobperron Why are we using static linkage for these? Why not defining them on goal_state_machine.c ?
There was a problem hiding this comment.
Yeah, agreed; for a large function like this, making it static inline is bloating the downstream users for not a lot of reason. This is probably better as a function inside a C file.
There was a problem hiding this comment.
In retrospect, I think all of these static functions and the transition map can be moved to a C file.
There was a problem hiding this comment.
+1, you should move as much out of the public interface as is possible/reasonable.
| { | ||
| // Only run this function once | ||
| static bool _is_goal_state_transition_map_init = false; | ||
| if (_is_goal_state_transition_map_init) { |
There was a problem hiding this comment.
If this could be called from multiple threads, there is a TOCTTOU bug here; thread 1 could have read the state of _is_goal_state_transition_map_init as false then been swapped out, then thread 2 could read the state of _is_goal_state_transition_map as false, and do the initialization. Then when thread 1 wakes back up, it still thinks that it needs to do the initialization, and does it again.
There was a problem hiding this comment.
Moving the initialization to the C file let me do a direct initialization, so I believe this issue goes away.
|
#315 should fix the unstable windows build. I'll rebase once it's merged. |
| } | ||
|
|
||
| // Transition map | ||
| rcl_action_goal_event_handler |
There was a problem hiding this comment.
@jacobperron nit: this one could be static.
Along with unit tests: - Fix buggy if-conditions in transition functions. - Populate transition map in function for compatibility with cpp code (e.g. gtest). - Move rcl_action_transition_goal_state implementation to .c file and build rcl_action library
jacobperron
force-pushed
the
action_goal_state_machine_tests
branch
from
b63275b to
6a8750e
Compare
|
Reviews addressed and rebased on master:
|
Connects to #306.
As a follow up, we might consider replacing the hard-coded transition map with the more general one in rcl_lifecycle. This would involve moving the general transition map to a common package like rcutils.