Skip to content

Asoragna/cleanup#49

Merged
irobot-ros merged 4 commits intoirobot/add-events-executorfrom
asoragna/cleanup
Feb 23, 2021
Merged

Asoragna/cleanup#49
irobot-ros merged 4 commits intoirobot/add-events-executorfrom
asoragna/cleanup

Conversation

@irobot-ros
Copy link
Copy Markdown
Collaborator

@irobot-ros irobot-ros commented Feb 22, 2021

This PR does some cleanup and small fixes to events executor and timers manager.|

I suggest to first review by commit

  • 191f47a

    • improve doxygen documentation in events executor by removing references to waitset and direct comparisons with other executors;
    • remove using EventQueue as ambiguous due to the presence of EventsQueue class
    • remove consume_all_events function and just move the code there into the only location where the function was called
    • rename get_all_events to pop_all_events to emphatise that the events queue will be empty after that
  • 6a99496

    • rework constructor of events executor by: checking that the events queue is not a null pointer and initialize events_queue_ member variable before the entities collector as otherwise there will be a race condition if an event arrives.
  • c99d764

    • improve doxygen in timers manager, for example not mention heap in public APIs since it's an implementation detail
    • execute_ready_timers now returns void since its return value is not needed anymore (and it was an optimization hack)
    • add friend declaration to timers heap to allow validate_and_lock to access underlying container rather than exposing a public push_back API
  • b72d124

    • The previous implementation had an issue because in run_timers it was computing the next timeout at the end of the while loop. This was done because in that location it was possible to directly access the locked heap structure.
      This works only if the next iteration of the while loop is immediately executed, but it will be incorrect if someone else acquires the mutex (e.g. for adding/removing timers).
      The reason is that the next timeout may be 1 second, but maybe the mutex has been held by someone else for 0.8 seconds: the real timeout is now 0.2 seconds, but the previous implementation would have still waited for 1 second.
      The solution is to compute the timeout at the beginning of the while loop, i.e. after the mutex has been acquired.
      In order to do that efficiently I created a new implementation of get_head_timeout_unsafe that if no timers have been removed can directly return the timeout without need to iterate over the all vector.

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com>
Signed-off-by: Soragna, Alberto <alberto.soragna@gmail.com>
@mauropasse
Copy link
Copy Markdown
Collaborator

IMOLGTM

@irobot-ros irobot-ros merged commit 00666c1 into irobot/add-events-executor Feb 23, 2021
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.

3 participants