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

Transport device disconnected #15615

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Conversation

mroz22
Copy link
Contributor

@mroz22 mroz22 commented Nov 27, 2024

  • 1st commit - narrow error cases from all node-usb and web-usb and convert them to "device disconnected during action" in the same way legacy bridge does. This helps with proper error handling on the trezor-connect layer.
  • 2nd commit - do not release disconnected devices.
    • Trying to do this causes node bridge - another case of segfault 11 #15336 on my machine (mac-arm). I am unaware of anyone else who could reproduce it locally.
    • The legacy bridge is okay with it, and I haven't found out yet what the key difference is. But anyway, it doesn't feel right to me to release devices that are no longer present. The only possible problem this may cause could be when we false positively handle an error as a disconnected device and omit proper clean up.
    • Here is a little table where I tested the scenario explained in node bridge - another case of segfault 11 #15336 and it worked for me locally.
fixes #15336 legacy bridge node bridge webusb
enable labelling - button request - disconnect
get address - button request disconnect
  • 3rd commit - split the 'unacquired screen' into two 1] unacquired 2] device-used-elsewhere
    • now there is a common "unacquired" screen. Unacquired here means that features of the device are not known because some bug happened. There is a "Try again" button. image
    • then there is a special case of "used-elsewhere" screen that appears if we know device.transportSessionOwner. image
    • todo: maybe it could be shown also when device.used = true? This way this screen never renders for the legacy bridge since it never returns transportSessionOwner.
  • 4th commit - rework device.run error handling
    • in general, there are the following categories of errors:
      • recoverable -> show try again button
      • non-recoverable -> don't show button. only maybe reloading or reconnect could help
      • device disconnected -> do nothing. todo: maybe we should enumerate in this case to be sure that it really disconnected?
  • 5th commit - thanks to previous changes, unreadable device state is now better targeted and we don't need to show that many troubleshooting tips anymore.
    • unreadable non-hid device. this is not recoverable. This is typically the case of missing synchronization between apps for whatever reason.
    • image
    • webusb + unreadable hid device
    • image
    • unreadable hid device + suite desktop - allows to toggle legacy bridge
    • image

related issues:

@mroz22 mroz22 force-pushed the transport-device-disconnected branch 2 times, most recently from 0f0f58d to ddade07 Compare November 28, 2024 15:21
Copy link

github-actions bot commented Nov 28, 2024

🚀 Expo preview is ready!

  • Project → trezor-suite-preview
  • Platforms → android, ios
  • Scheme → trezorsuitelite
  • Runtime Version → 12
  • More info

Learn more about 𝝠 Expo Github Action

@mroz22 mroz22 mentioned this pull request Dec 2, 2024
@mroz22 mroz22 force-pushed the transport-device-disconnected branch 2 times, most recently from e024cd8 to 63e4b15 Compare December 2, 2024 15:22
@mroz22 mroz22 force-pushed the transport-device-disconnected branch from 63e4b15 to 4709f3b Compare December 2, 2024 17:23
@mroz22 mroz22 marked this pull request as ready for review December 2, 2024 17:23
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.

node bridge - another case of segfault 11
2 participants