Skip to content

Fix examples requesting thread closure from within same thread#567

Merged
gemenerik merged 1 commit intomasterfrom
rik/callback-thread-self-close
Sep 17, 2025
Merged

Fix examples requesting thread closure from within same thread#567
gemenerik merged 1 commit intomasterfrom
rik/callback-thread-self-close

Conversation

@gemenerik
Copy link
Member

@gemenerik gemenerik commented Sep 17, 2025

70e6f7d closes IncomingPacketHandlerThread upon link closure. This changed behavior for examples that close the link from within IncomingPacketHandlerThread callbacks - previously these threads remained open (leading to duplicates being created), now they close immediately. This PR updates the examples to use a flag-based approach to avoid issues with threads closing themselves.

Prevents "cannot join current thread" error by avoiding requests to close the IncomingPacketHandler thread from within its own callbacks. Uses flag-based approach instead.
@gemenerik gemenerik self-assigned this Sep 17, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a threading issue where examples were attempting to close the IncomingPacketHandlerThread from within its own callbacks, which became problematic after a recent change that closes this thread upon link closure. The solution implements a flag-based approach to defer the actual link closure until the main thread can handle it safely.

  • Replaces direct close_link() calls in callbacks with a flag-based mechanism
  • Adds polling logic in main loops to check the disconnect flag and close the link from the main thread
  • Simplifies the flash-memory.py example by removing the complex signal-based termination approach

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
examples/parameters/basicparam.py Adds disconnect flag and defers link closure to main thread
examples/memory/write-ow.py Adds disconnect flag and defers link closure to main thread
examples/memory/write-eeprom.py Adds disconnect flag and defers link closure to main thread
examples/memory/read-eeprom.py Adds disconnect flag and defers link closure to main thread
examples/memory/flash-memory.py Replaces signal-based termination with flag-based approach and removes unused import
examples/memory/erase-ow.py Adds disconnect flag and defers link closure to main thread

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

print('Apparently data could not be written to memory... :(')

flasher.disconnect()
else:
Copy link

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

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

The else clause on the for loop will execute when the loop completes without breaking, meaning the timeout occurred. However, if flasher.should_disconnect becomes True on the last iteration, the loop will break but flasher.disconnect() won't be called. Consider moving the timeout disconnect logic outside the loop or handling this case explicitly.

Suggested change
else:
if not flasher.should_disconnect:

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

10 seconds is very conservative. I am not sure it is necessary to add code for a potential race on the last 0.5 second.

Copy link
Member

@ataffanel ataffanel left a comment

Choose a reason for hiding this comment

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

Works for me! Fixes the threads problems.

@gemenerik gemenerik merged commit 4d8d41e into master Sep 17, 2025
1 check passed
@gemenerik gemenerik deleted the rik/callback-thread-self-close branch September 17, 2025 14:10
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