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

arm: retry WAIT errors at a lower level. Fixes nrf51 flashing. #2119

Closed
wants to merge 2 commits into from

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Jan 31, 2024

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.

@bugadani
Copy link
Contributor

bugadani commented Apr 29, 2024

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.

@Dirbaio
Copy link
Member Author

Dirbaio commented May 16, 2024

closing in favor of #2415

@Dirbaio Dirbaio closed this May 16, 2024
github-merge-queue bot pushed a commit that referenced this pull request May 16, 2024
Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
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.

None yet

2 participants