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

RP2040 tweaks #2723

Merged
merged 6 commits into from
Jul 19, 2024
Merged

RP2040 tweaks #2723

merged 6 commits into from
Jul 19, 2024

Conversation

liamfraser
Copy link
Collaborator

RP2040 tweaks:

  • Compiler warning fixes
  • allow pico-sdk LWIP to be used if TinyUSB one does not exist
  • Use custom memcpy in RP2040 code
  • Don't clear registers when not needed as hardware reset already does it

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look good, thank you

@hathach hathach merged commit e9f9d43 into hathach:master Jul 19, 2024
79 checks passed
@earlephilhower
Copy link

earlephilhower commented Jul 19, 2024

I know this has been merged, but replacing memcpy with a local implementation makes no sense to me.

memcpy, by definition, does not have any alignment restrictions (but of course you need memmove if there's potential overlap). I would suggests that any memcpy that crashes on a non-4-byte-aligned input or output is busted or should not be called memcpy because lots of other things are going to break, not just USB.

None of the man pages (ISO C spec is $$$) I've seen show an alignment requirement.

While the implementation here of unaligned_memcpy looks like it works (but doesn't return the almost always unused pointer that ISO memcpy does), it feels like it's going to be slower than whatever GCC or the ROM would implement...

edit There are also lots of memcpy calls in the class drivers as well. So, only a couple memcpy calls out of a whole bunch are being replaced w/this specific commit...

@hathach
Copy link
Owner

hathach commented Jul 22, 2024

@earlephilhower yeah, thank you for your feedback. You are rigtht, this is my thought as well initially. However, since it copy to/from a dual port USB RAM, it may have some extra hw limit, we have seen this in other ports e.g #1024, though that is a bit different since MCU is M4 and able to do unaligned access (hence memcpy is implemented in such as way), however the usb ram is limited to byte/aligned access only which could cause issue.

Update: It is confirmed that this does actually fixed an actual hw issue under certain testing condition.

@earlephilhower
Copy link

Gotcha. Thanks to all for looking into it.

@hathach
Copy link
Owner

hathach commented Jul 23, 2024

Gotcha. Thanks to all for looking into it.

no problems, thank you for providing feedback on the PR.

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

Successfully merging this pull request may close these issues.

4 participants