Skip to content
This repository has been archived by the owner on Jun 27, 2022. It is now read-only.

LL-9033 Handle inner exception after pairing timeout #778

Merged
merged 1 commit into from
Feb 22, 2022
Merged

LL-9033 Handle inner exception after pairing timeout #778

merged 1 commit into from
Feb 22, 2022

Conversation

juan-cortes
Copy link
Contributor

🦒 Context (issues, jira)

https://ledgerhq.atlassian.net/browse/LL-9033

💻 Description

This is a bit of a pickle, for context the new fw 2.0.2 introduced some changes to the new characteristic, the one that brought the performance boost 13d63400-2c97-0004-0003-4c6564676572, which threw an error from inside the monitor characteristic call from ble-plx.

We have to note that this is only when we reach the 30s timeout when the pairing code is shown. You need to be pairing a new device, ignore the pairing prompt, and reach this crash. If you feel this change introduces a performance hit (I measured and looked heavily into the instanceof perf and it seems pretty light) please advice for a better solution. I banged my head for two days and this is the only solution I could find that doesn't totally silence the error or hinder the functionality.

As an explanation of why this is happening, my understanding is that when we create the observable outside of the transport instance or the open method, the exception thrown is unhandled and therefor breaks the application. By catching that specific error in the observable, channeling it into the inferMTU method, and rethrowing it from there, we get the handled error and the correct "Pairing Failed" screen instead of an app crash.

In other words, without this, the error is unhandled.

@juan-cortes juan-cortes requested a review from a team as a code owner January 27, 2022 20:14
@codecov
Copy link

codecov bot commented Jan 27, 2022

Codecov Report

Merging #778 (98de152) into master (84b9178) will decrease coverage by 0.07%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #778      +/-   ##
==========================================
- Coverage   45.17%   45.09%   -0.08%     
==========================================
  Files          83       83              
  Lines        5052     5060       +8     
  Branches      893      897       +4     
==========================================
  Hits         2282     2282              
- Misses       2570     2576       +6     
- Partials      200      202       +2     
Impacted Files Coverage Δ
.../react-native-hw-transport-ble/src/BleTransport.ts 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84b9178...98de152. Read the comment docs.

@juan-cortes juan-cortes requested a review from gre January 31, 2022 14:55
@juan-cortes
Copy link
Contributor Author

Think we can merge this until we figure out a cleaner solution @gre it addresses two bugs for 2.0.2
I don't think we can solve the typing issue without introducing checking overhead to make TS happy, we'd have to check that it's a Buffer and not an error every time.

https://ledgerhq.atlassian.net/browse/LIVE-1400
https://ledgerhq.atlassian.net/browse/LIVE-579

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants