-
-
Notifications
You must be signed in to change notification settings - Fork 61
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
pybricks/util_pb/pb_task: Don't wait for cancel on shutdown. #129
Conversation
For reproduction, use the script in pybricks/support#567 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some WIP Bluetooth fixes that should help avoid the issues this is working around. But if we want to save that for a later release, this seems like an OK workaround for now.
pybricks/util_pb/pb_task.c
Outdated
@@ -41,7 +42,7 @@ void pb_wait_task(pbio_task_t *task, mp_int_t timeout) { | |||
} else { | |||
pbio_task_cancel(task); | |||
|
|||
while (task->status == PBIO_ERROR_AGAIN) { | |||
while (task->status == PBIO_ERROR_AGAIN && !pbsys_status_test(PBIO_PYBRICKS_STATUS_SHUTDOWN_REQUEST)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a HACK
comment here that explains that this is dangerous but is hopefully OK on shutdown. I'm a little concerned about USB hubs though since they don't actually power off on shutdown when the battery is charging, so this could cause hard crashes on those hubs. To avoid that, we would also need to empty any task queues so that we don't try to keep running zombie tasks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hack is only needed on technic / city hub right now, so documenting it as a hack and enabling it only on non-charging hubs could work for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against merging BT fixes by the way, we could talk about it tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable.
153d76e
to
a7d4e8b
Compare
Fixes pybricks/support#814 so the user can at least turn the hub off without removing the batteries.
a7d4e8b
to
01c5c73
Compare
Fixes pybricks/support#814. This also works around things like pybricks/support#567 so the user can at least turn the hub off without removing the batteries.