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

pbio/sys/program_stop: Drop hack to stop Bluetooth. #234

Closed
wants to merge 1 commit into from

Conversation

laurensvalk
Copy link
Member

We have an old hack that shuts down the bluetooth process. When starting a new task after that in a cleanup routine such as added in 04fd2e7, the hub will never finish the task, which prevents the user program from stopping.

We have several proper fixes in place now, and we are about to merge some updates that will make all Bluetooth task objects static, so we should remove this hack.

Fixes pybricks/support#1438

We have an old hack that shuts down the bluetooth process. When starting a new task after that in a cleanup routine such as added in 04fd2e7, the hub will never finish the task, which prevents the user program from stopping.

We have several proper fixes in place now, and we are about to merge some updates that will make all Bluetooth task objects static, so we should remove this hack.

Fixes pybricks/support#1438
@coveralls
Copy link

Coverage Status

coverage: 56.508%. remained the same
when pulling a38c89e on fix-shutdown
into 68828ac on master.

@dlech
Copy link
Member

dlech commented Feb 11, 2024

This change looks good.

But I suspect there is still more to do to ensure the hub can always be shut down without having to remove the batteries...

Are we sure we have fixed all places where tasks aren't cancellable?

If we use static memory for tasks now and force a program to stop while Bluetooth is still using a task, then start a new program that tries to reuse a statically allocated task that is still in use, it is going to likely cause a crash or other undefined behavior.

@laurensvalk
Copy link
Member Author

One step at a time 😄

This change affects only forced stop on shutdown, so program restart should not be affected.

Thanks for the review. This can get merged for V3.4.0b2.

@laurensvalk
Copy link
Member Author

laurensvalk commented Feb 16, 2024

Merged via 0945da5 and 87ccb14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants