Close the param threads properly if close is called. #535
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Put a None object on the queue if closed and read this out. The lock was released but since the queue was emptied the run task would never return from queue.get() and then this thread could not be closed. Make sure to unblock it by sending None and then setting should_close variable
It was started on setting up the crazyflie, and if the crazflie is never closed the thread will keep running. It makes more sense to start it when opening and stop it when closing
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a thread closure issue by ensuring that a None object is sent via the queue to unblock threads waiting on queue.get() when the connection is closed and also defers parameter initialization until a link is open.
- In cflib/crazyflie/param.py, the _close and run methods are modified to set the _should_close flag and enqueue a None to unblock the run loop.
- In cflib/crazyflie/init.py, the Param object is now deferred (set to None) during initialization and later instantiated upon opening a link.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cflib/crazyflie/param.py | Added None enqueuing and _should_close flag updates to unstick thread waits. |
| cflib/crazyflie/init.py | Deferred Param instantiation until a link is opened. |
Comments suppressed due to low confidence (1)
cflib/crazyflie/param.py:589
- [nitpick] While using 'continue' to skip processing a None value works since the loop condition will eventually cause exit, replacing 'continue' with a 'break' may simplify the logic for immediately terminating the thread once closure is signaled.
if pk is None: # Put none to close the thread
| @@ -119,7 +119,7 @@ def __init__(self, link=None, ro_cache=None, rw_cache=None): | |||
| self.extpos = Extpos(self) | |||
| self.log = Log(self) | |||
| self.console = Console(self) | |||
| self.param = Param(self) | |||
| self.param = None | |||
There was a problem hiding this comment.
Setting self.param to None in the initializer may lead to potential null reference errors if other parts of the code access param before open_link is called. Consider adding a guard or clearly documenting this initialization sequence.
Suggested change
| self.param = None | |
| self.param = Param(self) |
Copilot uses AI. Check for mistakes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Put a None object on the queue if closed and read this out. The lock was released but since the queue was emptied the run task would never return from queue.get() and then this thread could not be closed. Make sure to unblock it by sending None and then setting should_close variable.
Note that this has been failing during system tests, so this is tested there.