probe-rs
probe-rs copied to clipboard
arm: retry WAIT errors at a lower level. Fixes nrf51 flashing.
the issue is this logic here https://github.com/probe-rs/probe-rs/blob/master/probe-rs/src/probe/arm_debug_interface.rs#L448-L449
Currently we assume for AP writes the result comes back in the next transfer for all errors. However this is not the case for WAIT errors. This causes WAIT errors to be incorrectly returned 1 transfer ahead (ie if transfer 5 failed with WAIT, it's returned on transfer 4), which causes the caller to retry transfer 4 onwards, duplicating transfer 4. This causes the word to be duplicated in the memory write.
Instead, retry below the perform_transfers
logic that handles the "delayed faults". This way WAITs are naturally not subject to that logic. It also makes the code a bit nicer, there's now 1 copy of the retry loop instead of 4.
Also, this PR removes retry on other errors (not WAIT or FAULT), for simplicity. https://github.com/probe-rs/probe-rs/blob/master/probe-rs/src/probe/arm_debug_interface.rs#L1269-L1275
- I think this was already wrong: if there's another error (like parity, no response..) there's no way to know whether the transfer actually executed or not. it might've been received and executed fine by the chip, but the response corrupted. if we retry, we might do it twice, which would be bad if it has side effects (like a DRW write)
- These retries weren't present everywhere, they were present only for the single-register transfers, not the block transfers.
- if you're getting these "other" errors your wiring is pretty fucked up anyway, so chances are things are already broken for you
TODO:
- [ ] the idle cycles behavior doesn't exactly match the previous one. It's hard to match since
perform_raw_transfers_retry
doesn't know the original purpose of the transfers so it can't use the right field from SwdSettings. Do we need this, or can we simplify SwdSettings a bit?
Tested working with nrf51422 and nrf52840.
I think your idle_cycles change should be okay. I'm hesitant about simplifying, though. Right now, in the worst case, it looks like WAIT-ed writes will get potentially slightly slower, but nothing else.
If you would rebase this PR, I think I'd be happy to a) test my RP2040 woes with this and b) get this merged.
closing in favor of #2415