Skip to content
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

Unify three bluetooth de-init tasks and allow exit on shutdown request #245

Merged
merged 5 commits into from
Apr 4, 2024

Conversation

laurensvalk
Copy link
Member

@laurensvalk laurensvalk commented Apr 4, 2024

We have previously implemented the MicroPython VM abort to exit MicroPython under all circumstances, but we have recently added a few pbio tasks that were started after this, and they are not cancellable. This prevents shutdown.

This unifies awaiting on the deinitialization of the bluetooth peripheral, bluetooth broadcasting/observing and bluetooth stdout.

It also adds a break for a shutdown request so the hub can shut down even if a pbio task is stuck (e.g. pybricks/support#1419).

This also finishes implementing the remote disconnect task, which on some platforms was only started but not awaited inside the task. This could be used for a dedicated awaitable remote disconnect method in a future release, which has been requested several times.

This is largely untested, which is a work in progress.

@laurensvalk
Copy link
Member Author

Also added pybricks.pupdevices.Remote.disconnect method. This was a good way to test part of this PR, but also fixes pybricks/support#802

I called it disconnect instead of close since it is an (optionally) awaitable method. If it should still be called close, that's fine by me.

@@ -1051,17 +1061,16 @@ static PT_THREAD(peripheral_disconnect_task(struct pt *pt, pbio_task_t *task)) {
if (peri->con_handle != NO_CONNECTION) {
PT_WAIT_WHILE(pt, write_xfer_size);
GAP_TerminateLinkReq(peri->con_handle, 0x13);
PT_WAIT_UNTIL(pt, hci_command_status);
PT_WAIT_UNTIL(pt, peri->con_handle == NO_CONNECTION);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep the waiting for command status. The disconnect could happen for a different reason other than the request made here, so the program flow would continue while the command is still being processed on the BT chip.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So (in nominal conditions) are we guaranteed to get two events and always in that order?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. and even if the disconnect event happens first due to non-nominal conditions peri->con_handle == NO_CONNECTION is still going to be true after the status event.

#endif // PYBRICKS_PY_PUPDEVICES

#if PYBRICKS_PY_COMMON_BLE && PYBRICKS_PY_PUPDEVICES
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be || instead of &&?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'll probably just have to be specific and add PYBRICKS_PY_PUPDEVICES_REMOTE to account for the virtualhub which has pupdevices but no remote, and then we can use ||.

This was started as a task on some platforms, and waiting for completion was handled differently for each platform.

This uses a task so we can cleanly finalize this task along with broadcasting clean up in the following commits.

Now we could also implement an awaitable disconnect method on the Remote.
While MicroPython has a vm abort, several pbio tasks are started after this. If they get stuck, the hub cannot shut down.

This completes the unification of the de-init tasks and adds a break for a shutdown request.
The virtualhub has pupdevices but no Bluetooth.
@laurensvalk laurensvalk merged commit 1d0f987 into master Apr 4, 2024
34 checks passed
@dlech dlech deleted the bluetooth-stop branch April 4, 2024 18:44
@coveralls
Copy link

coveralls commented Apr 5, 2024

Coverage Status

coverage: 56.572% (+0.5%) from 56.071%
when pulling 1d0f987 on bluetooth-stop
into a0d063b on master.

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