Skip to content

Conversation

@arkadiuszwojcik
Copy link
Contributor

In commit 68b5ad1 rom.memcpy was replaced by @memcpy by @patryk4815 but one more rom.memcpy left.

@patryk4815
Copy link
Contributor

I think this one rom.memcpy need be left, maybe you want add comment?
Or new zig0.14.0 fixed @memcpy?
On zig0.13.0 @memcpy crash there

image

@arkadiuszwojcik
Copy link
Contributor Author

Hmm intresting. @patryk4815 can you tell me if USB CDC example from fresh git clone of microzig and zig build works for you on RP2040? Because for me it doesn't but witch that change from this commit it does. Very bizarre.

@patryk4815
Copy link
Contributor

I have only rp2350, soo I will test this today or tomorrow

@arkadiuszwojcik arkadiuszwojcik marked this pull request as draft April 6, 2025 19:45
@Grazfather
Copy link
Collaborator

Why do we want to do this? Don't we think that the rom version will be optimized for the device?

@arkadiuszwojcik
Copy link
Contributor Author

Why do we want to do this? Don't we think that the rom version will be optimized for the device?

In other place it was already replaced from rom.memcpy to @memcpy, so this change is to keep consistency. But, probably better solution would be to utilze comptime and use rom.memcpy for rp2040 and @memcpy for rp2350

@Grazfather
Copy link
Collaborator

Oh I did not know that they are not in the rp2350 rom, though I just checked (they are there but not exported)

@KaroLaunonen
Copy link

At least I needed this change for RP2040 usb-hid example on zig 0.14.0. It would crash without the change.

@Grazfather
Copy link
Collaborator

I tried the CDC example, and it seems to work with this change. Without it I couldn't get the device to enumerate, but when connecting with jtag I don't see anything crashing.

❯ sudo cu -l /dev/tty.usbmodem1101 -s 115200
Connected.
This is very very long text sent from RP Pico by USB CDC to your device: 70
This is very very long text sent from RP Pico by USB CDC to your device: 71
Your message to me was: h
Your message to me was: e
Your message to me was: l
This is very very long text sent from RP Pico by USB CDC to your device: 72
Your message to me was: l
Your message to me was: o
This is very very long text sent from RP Pico by USB CDC to your device: 73
This is very very long text sent from RP Pico by USB CDC to your device: 74
Your message to me was:
~This is very very long text sent from RP Pico by USB CDC to your device: 75
.
Disconnected.

@Grazfather
Copy link
Collaborator

Can someone test this on rp2350? If it works, we can make rom.memcpy for rp2350 make a compile error or just @memset.

I don't think we should have rom.<anything> secretly call builtins, I'd rather have these functions in the HAL, and have those functions determine whether they call a builtin or a rom function.

@arkadiuszwojcik
Copy link
Contributor Author

arkadiuszwojcik commented Apr 16, 2025

@Grazfather I am very confused right now. Without this change RP2350 is working and RP2040 is not, but with this change it is other way around: RP2350 stops working and RP2040 starts working :D Just tested on both devices. This is very odd issue.
@patryk4815 any news from your side?

@Grazfather
Copy link
Collaborator

If we move the memcpy to some chip.mempy, we can have a dispatch there that does @memcpy on rp2040 and
std.mem.copyForwards on rp2350. This could be done in rom but that would be misleading.

@arkadiuszwojcik
Copy link
Contributor Author

@Grazfather I changed solution to use std.mem.copyForwards and created issue for this problem, leaving that in code comment. Tested on both platforms RP2040 and RP2350 - it works.

@arkadiuszwojcik arkadiuszwojcik marked this pull request as ready for review April 17, 2025 15:45
@Grazfather Grazfather merged commit cf14329 into ZigEmbeddedGroup:main Apr 17, 2025
35 checks passed
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.

4 participants