Repository navigation
Display error/reason to user, if a transport had to be restarted - #231
msirringhaus wants to merge 7 commits into
Conversation
b2209ab to
852e47b
Compare
iinuwa
left a comment
iinuwa
left a comment
There was a problem hiding this comment.
haven't run this yet, but a couple of questions
| } else { | ||
| // Pre-active: the QR was never consumed or the BLE channel | ||
| // failed before the phone responded. Reissue silently. | ||
| tracing::debug!(?err, "Hybrid pre-active error, reissuing QR silently"); |
There was a problem hiding this comment.
I'm reading this on the train, so just wanted to double-check: can this get us in a restart loop? Is there a point where we eventually stop retrying a transport (without failing the ceremony)?
There was a problem hiding this comment.
Yes, it could potentially end in a loop. USB has a somewhat similar problem, actually.
I actually have to think about this a bit more. If and how to display this to the user, for example (do we want something like "we removed hybrid from the list, because it failed too often" in the UI?), and how to structure the error handling to deal with this.
It's somewhat hard to distinguish between "this will never work"-errors and those who might be recoverable.
I need a clearer structure for stopping transports (e.g. probably makes sense to stop it on Internal-errors)
Or we never stop/remove a transport, but only throttle it with delays between retries?
| /// as invalid by `TryFrom`. | ||
| #[repr(u8)] | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum TransportRestartReason { |
There was a problem hiding this comment.
We're currently using PascalCase strings to communicate enums everywhere else in the API. I find that easier to read in D-Bus output while debugging, and the extra 10 or so bytes in the message doesn't hurt.
Do you have opinions either way? If not, I'd recommend to make this a string for consistency.
|
Ok, I added a way to break the restart loop now. Sorry, this has gotten really big. Please let me know, if you want me to somehow look into splitting this into multiple PRs (although I'm not yet sure, how to easily do that without breaking the functionality. But I'm sure, I can come up with something). This now basically does what we did for USB already for all transports: Count 5 restarts before user interaction and then stop the transport. For this, we send an update to the UI, telling it that a certain transport has become unavailable. In doing so, I also changed the initial detection of available transactions a bit. We had duplicates of this. I removed those, and now we have one source of truth for available transports (which now also deactivates NFC, instead of commenting out the handler). In |
|
This API is getting a little gnarly, and I think it's going to be too hard for other backend implementations to bite off. I think we need to try another direction. I wonder if we've been thinking about this backwards: what if we send the errors to the UI as normal, and then the UI can send a trigger to restart the request or not? Then a simpler UI implementation can just bail and leave the user to restart the request if they want, and a more complex UI can show the error with "try again"/"cancel" options, and if they select try again, trigger the restart and re-do the flow. I think that leaves us with flat errors while still giving the ability to give the user more context. I like:
Maybe we need to model the transports as a D-Bus property that can be subscribed to instead? |
2 participants
Note: Based on #230 , and needs to be rebased if/when this lands.
Only send events, if the device was actively used by the user (Hybrid: QR scanned, USB: initial touch done, etc.).
Extended the Restarting-events introduced in #230 with an error reason. Display it at the bottom of the start page. Removed those error codes that trigger restarts from the terminal error code list.
Last commit updates the lang-files.